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

Support accessing .zfs/snapshot via NFS #2797

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@yshui
Copy link
Contributor

yshui commented Oct 14, 2014

Since @andrey-ve has been quiet recently, I decide to step up to address the remaining issues in his patchset (#1655).

I have re-organized the commits and re-phrased some of the commit log. For those commits I changed, I have appended my signed-off line to them.

Please review, thanks.

@yshui yshui force-pushed the yshui:fixsnap2 branch 4 times, most recently from 90d54e7 to 9534351 Oct 14, 2014

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Oct 15, 2014

@yshui Thanks for picking up the issue and helping push it forward. I'd love to get this functionality in place so lets start by sorting out the build failures and make sure all the automated testing passes.

I like the fact that this pull request is broken up in to many logical changes with good comments. It makes the patch review easier. But could you please restructure the patch stack so that it builds on itself. Patches earlier in the stack should add the infrastructure needed by patches later in the stack. This is important because when the buildbot tests the patch it will test every commit in the stack and we want all those tests to pass.

You can see the buildbot results in the details for this pull request.

@yshui

This comment has been minimized.

Copy link
Contributor Author

yshui commented Oct 16, 2014

@behlendorf

Patches earlier in the stack should add the infrastructure needed by patches later in the stack.

I was intended to do so, it seems I did it wrong...
I'll look into this and fix the build failures.

@yshui yshui force-pushed the yshui:fixsnap2 branch from 9534351 to 4db1a06 Oct 16, 2014

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Oct 16, 2014

One more thing to be aware of. The buildbot uses the --enable-debug option to maximize test coverage. This also causes build warnings like the one below to be fatal. You'll need to address this and the style issues which were identified.

/tmp/zfs-build--tzDcMt2i/BUILD/zfs-kmod-0.6.3/_kmod_build_3.2.0-70-generic/module/zfs/../../../zfs-0.6.3/module/zfs/zfs_ctldir.c:934:1: error: 'zfsctl_get_mnt_path' defined but not used [-Werror=unused-function]```

@yshui yshui force-pushed the yshui:fixsnap2 branch 2 times, most recently from 3eb4dab to 2238141 Oct 16, 2014

@yshui

This comment has been minimized.

Copy link
Contributor Author

yshui commented Oct 17, 2014

The build failure on ubuntu-14.04 buildbot seems to be a problem in the bot itself.

Soft lockup on buildbot ubuntu 13.10 is unrelated to this patch set.

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Oct 17, 2014

Right, don't worry about the 14.04 current builder results. It pretty closely tracks the latest kernel.org kernel and it appears they made an API change again. We'll need to sort that out.

Other than that things looked pretty good. I'll get this reviewed.

@yshui yshui force-pushed the yshui:fixsnap2 branch 2 times, most recently from 4ee8f4c to ebc91c9 Oct 17, 2014

@yshui

This comment has been minimized.

Copy link
Contributor Author

yshui commented Oct 26, 2014

I wrote some simple code that uses lookup_one_len, and that doesn't seem to work.

Don't know if it's because lookup_one_len doesn't trigger automount, or it's because I did something wrong.

I'll read the code to make sure.

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Oct 27, 2014

@yshui I've looked at the code and loojup_one_len() should trigger the automount. Anything which causes a lookup for that path component should.

@yshui

This comment has been minimized.

Copy link
Contributor Author

yshui commented Nov 4, 2014

@behlendorf lookup_one_len() uses __lookup_hash which doesn't seem to follow mounts.

@behlendorf behlendorf added this to the 0.6.4 milestone Nov 6, 2014

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Nov 6, 2014

@yshui Yes, I see what you're saying. OK, thanks for investigating this may then be our cleanest option. Hopefully we'll get this for the next tag.

@arturpzol

This comment has been minimized.

Copy link

arturpzol commented Dec 15, 2014

Is it possible that in 0.6.3 accessing .zfs/snapshot via NFS worked but in 0.6.4 not? My tests show that in 0.6.3 I had access to snapshot via NFS, after upgrade to 0.6.4 from repository I lost access.

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Dec 19, 2014

@arturpzol No, this has never worked. Although, we expect to have it implemented for the official 0.6.4 tag using this patch stack. One of the very few remaining hold ups is additional testing of this patch stack. If you could try it out and report back that would be a great data point.

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Feb 11, 2015

@yshui I was just looking this over again to see if we can get it in for 0.6.4 and noticed you have fixsnap2 and fixsnap3 branch. It looks to me like fixsnap3 was the exploratory branch for using lookup_one_len() which didn't pan out. Could you confirm that, and then rebase this pull request against master with that latest proposed patch stack.

andrey-ve added some commits Jul 30, 2013

Fix invalid fileid for snapshot root dentry
Prevents NFS client from detection of different fileids of snapshot root dentry
before & after snapshot mount.

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
Set MNT_SHRINKABLE on snapshot mount
We could set MNT_SHRINKABLE when mouting snapshot mount instead of using
occasional post mount getattr.

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
zfsctl: No need to sync ctldir inodes
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>
Ensure long fids is used for snapshot
We have to use long fids to include the dataset id.

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
Save snapshot root dentry on snapshot mount
When it's need to decode snapshot file handly it hard to get snapshot sb.
On the contrary on snapshot mount it's easy to get snapshot sb because
mount path available in the context of snapshot mount.

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>

@yshui yshui force-pushed the yshui:fixsnap2 branch from ebc91c9 to 0c4ff1a Feb 12, 2015

@yshui

This comment has been minimized.

Copy link
Contributor Author

yshui commented Feb 12, 2015

@behlendorf Yes, fixsnap3 is where I experimented with lookup_one_len()

I have rebased this pull request.

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Feb 12, 2015

@yshui Thanks, I'm looking forward to getting the buildbot results. Personally, I'd really like to get this in. I assume you've been using it successfully for quite some time now.

@behlendorf

This comment has been minimized.

Copy link

behlendorf commented on c9e1346 Feb 13, 2015

This looks like a nice bit of cleanup.

@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Feb 13, 2015

@yshui This looks like it's coming along nicely, I've made a few comments for issues I found while starting to test this.

@yshui yshui force-pushed the yshui:fixsnap2 branch from 0c4ff1a to ef88eb3 Feb 14, 2015

yshui and others added some commits Aug 15, 2013

Add helper function to get mount path of a dataset
This will be used in later commit which add support for decoding
snapshot fh. This is needed because in case the snapshot is not mounted
when decoding its fh, we have to trigger an automount. And to trigger a
snapshot automount, we have to know its path.

In this commit we add a helper function which get the mount path from
zpool and dataset properties (namely altroot and mountpoint
properties). This is practically the same method used by the userspace
zfs commandline tool. This method won't work for dataset with 'legacy'
mountpoint, but we have to do it this way because newer Linux
kernel hides the mount point information from file systems.

Theoretically the proper solution of the problem is to eliminate the
need to mount a snapshot, and just generate all snapshot related information
on access. However this will require a lot more work and probably
significant changes to how zfs handle snapshots. So I think we better
leave this for later.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Add encode and decode support for snapshot fh
The snapshot file handle should provide enough info in order to get the right
objsetid and right object in the objset defined by objsetid. Therefore
we encode the snapshot fh as objsetid + inode.

The objsetid should be encoded for every snapshot dentry including root dentry;
for snapshot root dentry zf_gen should be 0.

To decode a snapshot fh, we simply extract the objsetid and the inode,
then we try to retrive the super block stored in the snapentry. Zero matching
snapentry means the snapshot is not mounted. In this case we try to
trigger a automount of the snapshot by doing a path lookup on the full
snapshot path, and then try again.

If zf_gen is 0, we return the snapshot root dentry.

Also added a configure check for kern_path, since it is not available
in earlier kernels.

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

@yshui yshui force-pushed the yshui:fixsnap2 branch from ef88eb3 to 4bdaed0 Feb 16, 2015

@behlendorf behlendorf modified the milestones: 0.6.5, 0.6.4 Feb 24, 2015

@behlendorf behlendorf modified the milestones: 0.7.0, 0.6.5 Jul 16, 2015

behlendorf added a commit to behlendorf/zfs that referenced this pull request Aug 30, 2015

zfsctl: No need to sync ctldir inodes
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 zfsonlinux#2797

behlendorf added a commit to behlendorf/zfs that referenced this pull request Aug 31, 2015

zfsctl: No need to sync ctldir inodes
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 zfsonlinux#2797

behlendorf added a commit to behlendorf/zfs that referenced this pull request Aug 31, 2015

zfsctl: No need to sync ctldir inodes
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 zfsonlinux#2797

behlendorf added a commit to behlendorf/zfs that referenced this pull request Sep 3, 2015

Support accessing .zfs/snapshot via NFS
This patch is based on the previous work done by @andrey-ve and
@yshui.  It triggers the automount by using kern_path() to traverse
to the known snapshout mount point.  Once the snapshot is mounted
NFS can access the contents of the snapshot.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue zfsonlinux#2797
Issue zfsonlinux#1655
Issue zfsonlinux#616
@behlendorf

This comment has been minimized.

Copy link
Member

behlendorf commented Sep 3, 2015

Replaced by #3737.

@behlendorf behlendorf closed this Sep 3, 2015

behlendorf added a commit to behlendorf/zfs that referenced this pull request Sep 4, 2015

Support accessing .zfs/snapshot via NFS
This patch is based on the previous work done by @andrey-ve and
@yshui.  It triggers the automount by using kern_path() to traverse
to the known snapshout mount point.  Once the snapshot is mounted
NFS can access the contents of the snapshot.

Allowing NFS clients to access to the .zfs/snapshot directory would
normally mean that a root user on a client mounting an export with
'no_root_squash' would be able to use mkdir/rmdir/mv to manipulate
snapshots on the server.  To prevent configuration mistakes a
zfs_admin_snapshot module option was added which disables the
mkdir/rmdir/mv functionally.  System administators desiring this
functionally must explicitly enable it.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue zfsonlinux#2797
Issue zfsonlinux#1655
Issue zfsonlinux#616

behlendorf added a commit to behlendorf/zfs that referenced this pull request Sep 4, 2015

Support accessing .zfs/snapshot via NFS
This patch is based on the previous work done by @andrey-ve and
@yshui.  It triggers the automount by using kern_path() to traverse
to the known snapshout mount point.  Once the snapshot is mounted
NFS can access the contents of the snapshot.

Allowing NFS clients to access to the .zfs/snapshot directory would
normally mean that a root user on a client mounting an export with
'no_root_squash' would be able to use mkdir/rmdir/mv to manipulate
snapshots on the server.  To prevent configuration mistakes a
zfs_admin_snapshot module option was added which disables the
mkdir/rmdir/mv functionally.  System administators desiring this
functionally must explicitly enable it.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes zfsonlinux#2797
Closes zfsonlinux#1655
Closes zfsonlinux#616

tomgarcia added a commit to tomgarcia/zfs that referenced this pull request Sep 11, 2015

zfsctl: No need to sync ctldir inodes
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 zfsonlinux#2797

tomgarcia added a commit to tomgarcia/zfs that referenced this pull request Sep 11, 2015

Support accessing .zfs/snapshot via NFS
This patch is based on the previous work done by @andrey-ve and
@yshui.  It triggers the automount by using kern_path() to traverse
to the known snapshout mount point.  Once the snapshot is mounted
NFS can access the contents of the snapshot.

Allowing NFS clients to access to the .zfs/snapshot directory would
normally mean that a root user on a client mounting an export with
'no_root_squash' would be able to use mkdir/rmdir/mv to manipulate
snapshots on the server.  To prevent configuration mistakes a
zfs_admin_snapshot module option was added which disables the
mkdir/rmdir/mv functionally.  System administators desiring this
functionally must explicitly enable it.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes zfsonlinux#2797
Closes zfsonlinux#1655
Closes zfsonlinux#616

ryao added a commit to ClusterHQ/zfs that referenced this pull request Sep 16, 2015

Support accessing .zfs/snapshot via NFS
This patch is based on the previous work done by @andrey-ve and
@yshui.  It triggers the automount by using kern_path() to traverse
to the known snapshout mount point.  Once the snapshot is mounted
NFS can access the contents of the snapshot.

Allowing NFS clients to access to the .zfs/snapshot directory would
normally mean that a root user on a client mounting an export with
'no_root_squash' would be able to use mkdir/rmdir/mv to manipulate
snapshots on the server.  To prevent configuration mistakes a
zfs_admin_snapshot module option was added which disables the
mkdir/rmdir/mv functionally.  System administators desiring this
functionally must explicitly enable it.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes zfsonlinux#2797
Closes zfsonlinux#1655
Closes zfsonlinux#616

JKDingwall added a commit to JKDingwall/zfs that referenced this pull request Aug 11, 2016

zfsctl: No need to sync ctldir inodes
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 zfsonlinux#2797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment