Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix outstanding .zfs/snapshot issues #3718

Closed
wants to merge 2 commits into from

Conversation

behlendorf
Copy link
Contributor

No description provided.

@behlendorf
Copy link
Contributor Author

It's meant to be used instead of #3344, and it addresses most of the remaining issues I'm aware of.

@prometheanfire
Copy link
Contributor

seems to work for me

@Bronek
Copy link

Bronek commented Aug 31, 2015

I am testing this patch applied on tag 0.6.4.2 , one thing I noticed was that it does not apply cleanly due to change in function zfsctl_init(void). I had to modify the patch like this:

-         zfs_expire_taskq = taskq_create("z_unmount", 1, defclsyspri,
+         zfs_expire_taskq = taskq_create("z_unmount", 1, maxclsyspri,

EDIT: above change aside, this patch works well when applied on top of 0.6.4.2, on vanilla kernel 4.0.9 (ArchLinux, my own kernel build). I can enumerate and access snapshots, no problems when trying to access non-existing ones (as was the case in #3344), no problems explicitly unmounting them, no problems with automatic unmounting. The files look as expected inside mounted snapshots.

@behlendorf
Copy link
Contributor Author

@Bronek @prometheanfire thanks for the quick turn around on the testing.

There's no metadata to write to disk for ctldir inodes. So we check if
a inode belongs to the ctldir in zpl_commit_metadata, and returns
immediately if it is.

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#2797
@behlendorf
Copy link
Contributor Author

Refreshed against master, some minor compatibility code added for 2.6.37 and earlier kernels. Aside from that the patch remains the same and has been holding up well to stress testing.

@janlam7
Copy link

janlam7 commented Aug 31, 2015

Accessing files and the automatic (un)mounting works for me on a 4.0.5 kernel with the master branch of spl and the master + this patch of zfs. Thanks!

I did notice the following, but I'm not sure if it's related

  1. cd to /mnt/filesystem/.zfs/snapshot folder (and NOT enter a specific snapshot)
  2. find | grep specific_filename_sure_to_exist returns 0 files
  3. run the find again returns 1 file in the oldest snapshot
  4. run the find again returns 2 files in the 2 oldest snapshots
  5. repeat until all snapshots are automounted ;-)

Re-factor the .zfs/snapshot auto-mouting code to take in to account
changes made to the upstream kernels.  And to lay the groundwork for
enabling access to .zfs snapshots via NFS clients.  This patch makes
the following core improvements.

* All actively auto-mounted snapshots are now tracked in two global
trees which are indexed by snapshot name and objset id respectively.
This allows for fast lookups of any auto-mounted snapshot regardless
without needing access to the parent dataset.

* Snapshot entries are added to the tree in zfsctl_snapshot_mount().
However, they are now removed from the tree in the context of the
unmount process.  This eliminates the need complicated error logic
in zfsctl_snapshot_unmount() to handle unmount failures.

* References are now taken on the snapshot entries in the tree to
ensure they always remain valid while a task is outstanding.

* The MNT_SHRINKABLE flag is set on the snapshot vfsmount_t right
after the auto-mount succeeds.  This allows to kernel to unmount
idle auto-mounted snapshots if needed removing the need for the
zfsctl_unmount_snapshots() function.

* Snapshots in active use will not be automatically unmounted.  As
long as at least one dentry is revalidated every zfs_expire_snapshot/2
seconds the auto-unmount expiration timer will be extended.

* Commit torvalds/linux@bafc9b7 caused snapshots auto-mounted by ZFS
to be immediately unmounted when the dentry was revalidated.  This
was a consequence of ZFS invaliding all snapdir dentries to ensure that
negative dentries didn't mask new snapshots.  This patch modifies the
behavior such that only negative dentries are invalidated.  This solves
the issue and may result in a performance improvement.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#3589
Closes openzfs#3344
Closes openzfs#3295
Closes openzfs#3257
Closes openzfs#3243
Closes openzfs#3030
Closes openzfs#2841
argv[2] = kmem_asprintf(SET_MOUNT_CMD, full_name, full_path);
error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
strfree(argv[2]);
if (error && !(error & MOUNT_BUSY << 8)) {
printk("ZFS: Unable to automount %s at %s: %d\n",
full_name, full_path, error);
cmn_err(CE_WARN, "Unable to automount %s/%s: %d\n",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newline is superfluous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll remove it.

@taneli76
Copy link

This makes snapshots directory listing work on Ubuntu stock kernel 3.19.0-26-generic, yay! Previously, kernel panic with snapdir=visible always.

@behlendorf
Copy link
Contributor Author

@janlam7 I saw this in my testing as well but I haven't had a chance to look in to why only a single auto-mount is triggered by a find invocation. However, this isn't a new issue and it seems to exist in the current code so we can tackle it after this is merged. Other commands like /mnt/filesystem/.zfs/snapshot/* do seem to trigger all the auto-mounts.

@behlendorf
Copy link
Contributor Author

Testing looks good on a handful of kernels all the way back to 2.6.32. Thanks again for the prompt validation of the patch and feedback. I've merged it to master:

278bee9 Linux 3.18 compat: Snapshot auto-mounting
b23975c zfsctl: No need to sync ctldir inodes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants