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

Support accessing .zfs/snapshot via NFS #2797

Closed
wants to merge 7 commits into from
Closed

Support accessing .zfs/snapshot via NFS #2797

wants to merge 7 commits into from

Conversation

yshui
Copy link
Contributor

@yshui 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.

@behlendorf
Copy link
Contributor

@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.

@behlendorf behlendorf added the Type: Feature Feature request or new feature label Oct 15, 2014
@yshui
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.

@behlendorf
Copy link
Contributor

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
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
Copy link
Contributor

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
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
Copy link
Contributor

@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
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
Copy link
Contributor

@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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

@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.

Unverified

No user is associated with the committer email.
We could set MNT_SHRINKABLE when mouting snapshot mount instead of using
occasional post mount getattr.

Signed-off-by: Andrey Vesnovaty <[email protected]>
Signed-off-by: Yuxuan Shui <[email protected]>
Prevents NFS client from detection of different fileids of snapshot root dentry
before & after snapshot mount.

Signed-off-by: Andrey Vesnovaty <[email protected]>
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 <[email protected]>
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 <[email protected]>
We have to use long fids to include the dataset id.

Signed-off-by: Andrey Vesnovaty <[email protected]>
@yshui
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
Copy link
Contributor

@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
Copy link
Contributor

@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 and others added 2 commits February 16, 2015 03:09
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Yuxuan Shui <[email protected]>
@behlendorf behlendorf modified the milestones: 0.6.5, 0.6.4 Feb 24, 2015
@behlendorf behlendorf added this to the 0.7.0 milestone Jul 16, 2015
@behlendorf behlendorf removed this from the 0.6.5 milestone Jul 16, 2015
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 30, 2015
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 <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#2797
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 31, 2015
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 <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#2797
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 31, 2015
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 <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#2797
behlendorf added a commit to behlendorf/zfs that referenced this pull request Sep 3, 2015
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 <[email protected]>
Issue openzfs#2797
Issue openzfs#1655
Issue openzfs#616
@behlendorf
Copy link
Contributor

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
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 <[email protected]>
Issue openzfs#2797
Issue openzfs#1655
Issue openzfs#616
behlendorf added a commit to behlendorf/zfs that referenced this pull request Sep 4, 2015
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 <[email protected]>
Closes openzfs#2797
Closes openzfs#1655
Closes openzfs#616
tomgarcia pushed a commit to tomgarcia/zfs that referenced this pull request Sep 11, 2015
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 <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#2797
tomgarcia pushed a commit to tomgarcia/zfs that referenced this pull request Sep 11, 2015
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 <[email protected]>
Closes openzfs#2797
Closes openzfs#1655
Closes openzfs#616
ryao pushed a commit to ClusterHQ/zfs that referenced this pull request Sep 16, 2015
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 <[email protected]>
Closes openzfs#2797
Closes openzfs#1655
Closes openzfs#616
JKDingwall pushed a commit to JKDingwall/zfs that referenced this pull request Aug 11, 2016
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 <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#2797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Feature request or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants