Hi,
Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
Debian bug http://bugs.debian.org/567546, but in short the problem seems
to be that sub-mounts (/a/b) take the top-level mount (/a) options
instead of their own, due to a bug in how mountd generates the crossmnt
subexports.
I checked that reverting the write_secinfo changes in commit
bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
only be a side effect, not the actual cause.
A short test:
- have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
/a/b)
- restart the mountd, clear exports, etc.
- try a mount on the client of /a/b, it gets readonly
- umount & remount, it's now r/w
- however, clearing the kernel export table (exportfs -f), makes the
next mount again get read-only
Disabling crossmnt fixes the issue completely, so I would venture to
guess that the subexports creation code has some issue, but I don't know
enough of this to be able to debug it.
thanks,
iustin
On 03/07/2010 03:07 PM, J. Bruce Fields wrote:
> This was obviously wrong, since path[strlen(path)] == '\0' should always
> be true.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> utils/mountd/cache.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index d63e10a..ff27bbf 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
> int l = strlen(path);
>
> return strcmp(subpath, path) == 0
> - || (strncmp(subpath, path, l) == 0 && path[l] == '/');
> + || (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
> }
>
> static int path_matches(nfs_export *exp, char *path)
Committed...
steved.
On 03/07/2010 03:08 PM, J. Bruce Fields wrote:
> Part of the reason for the previous bug was confusion between "subpath"
> and "path"; which is the shorter path, and which the longer?
>
> "child" and "parent" seem less ambiguous.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> utils/mountd/cache.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index ff27bbf..7dec468 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -614,12 +614,12 @@ static int dump_to_cache(FILE *f, char *domain, char *path, struct exportent *ex
> return qword_eol(f);
> }
>
> -static int is_subdirectory(char *subpath, char *path)
> +static int is_subdirectory(char *child, char *parent)
> {
> - int l = strlen(path);
> + int l = strlen(parent);
>
> - return strcmp(subpath, path) == 0
> - || (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
> + return strcmp(child, parent) == 0
> + || (strncmp(child, parent, l) == 0 && child[l] == '/');
> }
>
> static int path_matches(nfs_export *exp, char *path)
Committed...
steved
On 03/07/2010 03:06 PM, J. Bruce Fields wrote:
> On Sun, Mar 07, 2010 at 08:54:49PM +0100, Iustin Pop wrote:
>> Sorry for the delayed response. I can confirm this solves my test case. I get
>> the right mount options in both the "mount directly the sub-directory" case,
>> and in the "mount parent and access the sub-directory via the crossmnt feature"
>
> Excellent, thanks. Steve, could you apply the following patches?
>
> Also available from
>
> git pull git://linux-nfs.org/~bfields/nfs-utils.git crossmnt-fixes
I think I will wait on commit 72f6d0 until it hashed out...
steved.
On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> Debian bug http://bugs.debian.org/567546, but in short the problem seems
> to be that sub-mounts (/a/b) take the top-level mount (/a) options
> instead of their own, due to a bug in how mountd generates the crossmnt
> subexports.
>
> I checked that reverting the write_secinfo changes in commit
> bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> only be a side effect, not the actual cause.
>
> A short test:
> - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> /a/b)
> - restart the mountd, clear exports, etc.
> - try a mount on the client of /a/b, it gets readonly
> - umount & remount, it's now r/w
> - however, clearing the kernel export table (exportfs -f), makes the
> next mount again get read-only
>
> Disabling crossmnt fixes the issue completely, so I would venture to
> guess that the subexports creation code has some issue, but I don't know
> enough of this to be able to debug it.
Thanks for the report. What's the latest nfs-utils version you've
tested?
On a quick skim of the latest code I see one clear bug
(path[strlen(path)] will never be '/'!), but that should have caused
crossmnt to never get enforced at all rather than to override anything.
Could you retest the latest from the upstream git, with this patch
applied, and see if the problem is still present?
Then if it is I'll try to go reproduce it myself....
--b.
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d63e10a..ff27bbf 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
int l = strlen(path);
return strcmp(subpath, path) == 0
- || (strncmp(subpath, path, l) == 0 && path[l] == '/');
+ || (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
}
static int path_matches(nfs_export *exp, char *path)
On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > instead of their own, due to a bug in how mountd generates the crossmnt
> > subexports.
> >
> > I checked that reverting the write_secinfo changes in commit
> > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > only be a side effect, not the actual cause.
> >
> > A short test:
> > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > /a/b)
> > - restart the mountd, clear exports, etc.
> > - try a mount on the client of /a/b, it gets readonly
> > - umount & remount, it's now r/w
> > - however, clearing the kernel export table (exportfs -f), makes the
> > next mount again get read-only
> >
> > Disabling crossmnt fixes the issue completely, so I would venture to
> > guess that the subexports creation code has some issue, but I don't know
> > enough of this to be able to debug it.
>
> Thanks for the report. What's the latest nfs-utils version you've
> tested?
1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
commit which, again, I presume un-hides the problem.
Also there are reports of older versions being broken.
> On a quick skim of the latest code I see one clear bug
> (path[strlen(path)] will never be '/'!), but that should have caused
> crossmnt to never get enforced at all rather than to override anything.
>
> Could you retest the latest from the upstream git, with this patch
> applied, and see if the problem is still present?
I've tested right now with
git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
upstream repo) plus the patch - still happens, no change.
One thing that is interesting is that only the first mount gets the
wrong options, if the client unmounts and remounts (at this point the
exports are in the kernel's cache) the options are right. As long as the
kernel cache is populated, the options are right, if one clears the
cache, the first mount will be wrong. Maybe this helps, I find it an
interesting behaviour.
> Then if it is I'll try to go reproduce it myself....
For reference, here is the exports file I use for tests:
/srv/nfs client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
/srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
And here is the fstab entry on the client:
server:/srv/nfs/homes /home nfs noauto,hard,bg,sec=sys,proto=tcp 0 0
The rest of the tools on server and client are Debian's nfs-utils 1.2.1
(they have just a few, very trivial and unrelated, patches). The problem
manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.
Thanks!
iustin
On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > subexports.
> > >
> > > I checked that reverting the write_secinfo changes in commit
> > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > only be a side effect, not the actual cause.
> > >
> > > A short test:
> > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > > /a/b)
> > > - restart the mountd, clear exports, etc.
> > > - try a mount on the client of /a/b, it gets readonly
> > > - umount & remount, it's now r/w
> > > - however, clearing the kernel export table (exportfs -f), makes the
> > > next mount again get read-only
> > >
> > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > guess that the subexports creation code has some issue, but I don't know
> > > enough of this to be able to debug it.
> >
> > Thanks for the report. What's the latest nfs-utils version you've
> > tested?
>
> 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> commit which, again, I presume un-hides the problem.
>
> Also there are reports of older versions being broken.
>
> > On a quick skim of the latest code I see one clear bug
> > (path[strlen(path)] will never be '/'!), but that should have caused
> > crossmnt to never get enforced at all rather than to override anything.
> >
> > Could you retest the latest from the upstream git, with this patch
> > applied, and see if the problem is still present?
>
> I've tested right now with
> git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> upstream repo) plus the patch - still happens, no change.
>
> One thing that is interesting is that only the first mount gets the
> wrong options, if the client unmounts and remounts (at this point the
> exports are in the kernel's cache) the options are right. As long as the
> kernel cache is populated, the options are right, if one clears the
> cache, the first mount will be wrong. Maybe this helps, I find it an
> interesting behaviour.
>
> > Then if it is I'll try to go reproduce it myself....
>
> For reference, here is the exports file I use for tests:
> /srv/nfs client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
Is /srv/nfs/homes a mountpoint, or are both of these on the same
filesystem?
--b.
>
> And here is the fstab entry on the client:
> server:/srv/nfs/homes /home nfs noauto,hard,bg,sec=sys,proto=tcp 0 0
>
> The rest of the tools on server and client are Debian's nfs-utils 1.2.1
> (they have just a few, very trivial and unrelated, patches). The problem
> manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.
>
> Thanks!
> iustin
On Mon, Mar 01, 2010 at 04:39:52PM -0500, J. Bruce Fields wrote:
> On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> > On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > > subexports.
> > > >
> > > > I checked that reverting the write_secinfo changes in commit
> > > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > > only be a side effect, not the actual cause.
> > > >
> > > > A short test:
> > > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > > > /a/b)
> > > > - restart the mountd, clear exports, etc.
> > > > - try a mount on the client of /a/b, it gets readonly
> > > > - umount & remount, it's now r/w
> > > > - however, clearing the kernel export table (exportfs -f), makes the
> > > > next mount again get read-only
> > > >
> > > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > > guess that the subexports creation code has some issue, but I don't know
> > > > enough of this to be able to debug it.
> > >
> > > Thanks for the report. What's the latest nfs-utils version you've
> > > tested?
> >
> > 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> > commit which, again, I presume un-hides the problem.
> >
> > Also there are reports of older versions being broken.
> >
> > > On a quick skim of the latest code I see one clear bug
> > > (path[strlen(path)] will never be '/'!), but that should have caused
> > > crossmnt to never get enforced at all rather than to override anything.
> > >
> > > Could you retest the latest from the upstream git, with this patch
> > > applied, and see if the problem is still present?
> >
> > I've tested right now with
> > git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> > upstream repo) plus the patch - still happens, no change.
> >
> > One thing that is interesting is that only the first mount gets the
> > wrong options, if the client unmounts and remounts (at this point the
> > exports are in the kernel's cache) the options are right. As long as the
> > kernel cache is populated, the options are right, if one clears the
> > cache, the first mount will be wrong. Maybe this helps, I find it an
> > interesting behaviour.
> >
> > > Then if it is I'll try to go reproduce it myself....
> >
> > For reference, here is the exports file I use for tests:
> > /srv/nfs client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> > /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
>
> Is /srv/nfs/homes a mountpoint, or are both of these on the same
> filesystem?
It is a mountpoint, in all my tests. I'll try tomorrow to run a test with them
being on the same filesystem.
iustin
On Mon, Mar 01, 2010 at 10:41:16PM +0100, Iustin Pop wrote:
> On Mon, Mar 01, 2010 at 04:39:52PM -0500, J. Bruce Fields wrote:
> > On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> > > For reference, here is the exports file I use for tests:
> > > /srv/nfs client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> > > /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
> >
> > Is /srv/nfs/homes a mountpoint, or are both of these on the same
> > filesystem?
>
> It is a mountpoint, in all my tests. I'll try tomorrow to run a test with them
> being on the same filesystem.
No, no point, the no-mountpoint case is the case we already *know*
doesn't work.
So, OK, I'll try this here.
--b.
On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > subexports.
> > >
> > > I checked that reverting the write_secinfo changes in commit
> > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > only be a side effect, not the actual cause.
> > >
> > > A short test:
> > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > > /a/b)
> > > - restart the mountd, clear exports, etc.
> > > - try a mount on the client of /a/b, it gets readonly
> > > - umount & remount, it's now r/w
> > > - however, clearing the kernel export table (exportfs -f), makes the
> > > next mount again get read-only
> > >
> > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > guess that the subexports creation code has some issue, but I don't know
> > > enough of this to be able to debug it.
> >
> > Thanks for the report. What's the latest nfs-utils version you've
> > tested?
>
> 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> commit which, again, I presume un-hides the problem.
>
> Also there are reports of older versions being broken.
>
> > On a quick skim of the latest code I see one clear bug
> > (path[strlen(path)] will never be '/'!), but that should have caused
> > crossmnt to never get enforced at all rather than to override anything.
> >
> > Could you retest the latest from the upstream git, with this patch
> > applied, and see if the problem is still present?
>
> I've tested right now with
> git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> upstream repo) plus the patch - still happens, no change.
>
> One thing that is interesting is that only the first mount gets the
> wrong options, if the client unmounts and remounts (at this point the
> exports are in the kernel's cache) the options are right. As long as the
> kernel cache is populated, the options are right, if one clears the
> cache, the first mount will be wrong. Maybe this helps, I find it an
> interesting behaviour.
>
> > Then if it is I'll try to go reproduce it myself....
>
> For reference, here is the exports file I use for tests:
> /srv/nfs client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
>
> And here is the fstab entry on the client:
> server:/srv/nfs/homes /home nfs noauto,hard,bg,sec=sys,proto=tcp 0 0
>
> The rest of the tools on server and client are Debian's nfs-utils 1.2.1
> (they have just a few, very trivial and unrelated, patches). The problem
> manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.
The loop in utils/mountd/cache.c:cache_export_ent() looks extremely
suspicious: it seems to be populating the kernel's export cache with
parameters taken from the parent export. I can't see why that's
necessary at all--those exports can be looked up on demand later when
they're needed. Something as simple as the following might help?
(Totally untested!)
--b.
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d63e10a..7d87812 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
int l = strlen(path);
return strcmp(subpath, path) == 0
- || (strncmp(subpath, path, l) == 0 && path[l] == '/');
+ || (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
}
static int path_matches(nfs_export *exp, char *path)
@@ -817,46 +817,6 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
" fsid= required", exp->e_path);
}
- while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
- /* really an 'if', but we can break out of
- * a 'while' more easily */
- /* Look along 'path' for other filesystems
- * and export them with the same options
- */
- struct stat stb;
- int l = strlen(exp->e_path);
- int dev;
-
- if (strlen(path) <= l || path[l] != '/' ||
- strncmp(exp->e_path, path, l) != 0)
- break;
- if (stat(exp->e_path, &stb) != 0)
- break;
- dev = stb.st_dev;
- while(path[l] == '/') {
- char c;
- /* errors for submount should fail whole filesystem */
- int err2;
-
- l++;
- while (path[l] != '/' && path[l])
- l++;
- c = path[l];
- path[l] = 0;
- err2 = lstat(path, &stb);
- path[l] = c;
- if (err2 < 0)
- break;
- if (stb.st_dev == dev)
- continue;
- dev = stb.st_dev;
- path[l] = 0;
- dump_to_cache(f, domain, path, exp);
- path[l] = c;
- }
- break;
- }
-
fclose(f);
return err;
}
On Mon, Mar 01, 2010 at 10:20:02PM -0500, J. Bruce Fields wrote:
> On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> > On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > > subexports.
> > > >
> > > > I checked that reverting the write_secinfo changes in commit
> > > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > > only be a side effect, not the actual cause.
> > > >
> > > > A short test:
> > > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > > > /a/b)
> > > > - restart the mountd, clear exports, etc.
> > > > - try a mount on the client of /a/b, it gets readonly
> > > > - umount & remount, it's now r/w
> > > > - however, clearing the kernel export table (exportfs -f), makes the
> > > > next mount again get read-only
> > > >
> > > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > > guess that the subexports creation code has some issue, but I don't know
> > > > enough of this to be able to debug it.
> > >
> > > Thanks for the report. What's the latest nfs-utils version you've
> > > tested?
> >
> > 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> > commit which, again, I presume un-hides the problem.
> >
> > Also there are reports of older versions being broken.
> >
> > > On a quick skim of the latest code I see one clear bug
> > > (path[strlen(path)] will never be '/'!), but that should have caused
> > > crossmnt to never get enforced at all rather than to override anything.
> > >
> > > Could you retest the latest from the upstream git, with this patch
> > > applied, and see if the problem is still present?
> >
> > I've tested right now with
> > git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> > upstream repo) plus the patch - still happens, no change.
> >
> > One thing that is interesting is that only the first mount gets the
> > wrong options, if the client unmounts and remounts (at this point the
> > exports are in the kernel's cache) the options are right. As long as the
> > kernel cache is populated, the options are right, if one clears the
> > cache, the first mount will be wrong. Maybe this helps, I find it an
> > interesting behaviour.
> >
> > > Then if it is I'll try to go reproduce it myself....
> >
> > For reference, here is the exports file I use for tests:
> > /srv/nfs client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> > /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
> >
> > And here is the fstab entry on the client:
> > server:/srv/nfs/homes /home nfs noauto,hard,bg,sec=sys,proto=tcp 0 0
> >
> > The rest of the tools on server and client are Debian's nfs-utils 1.2.1
> > (they have just a few, very trivial and unrelated, patches). The problem
> > manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.
>
> The loop in utils/mountd/cache.c:cache_export_ent() looks extremely
> suspicious: it seems to be populating the kernel's export cache with
> parameters taken from the parent export. I can't see why that's
> necessary at all--those exports can be looked up on demand later when
> they're needed. Something as simple as the following might help?
> (Totally untested!)
Sorry for the delayed response. I can confirm this solves my test case. I get
the right mount options in both the "mount directly the sub-directory" case,
and in the "mount parent and access the sub-directory via the crossmnt feature"
case.
thanks!
iustin
On Sun, Mar 07, 2010 at 08:54:49PM +0100, Iustin Pop wrote:
> Sorry for the delayed response. I can confirm this solves my test case. I get
> the right mount options in both the "mount directly the sub-directory" case,
> and in the "mount parent and access the sub-directory via the crossmnt feature"
Excellent, thanks. Steve, could you apply the following patches?
Also available from
git pull git://linux-nfs.org/~bfields/nfs-utils.git crossmnt-fixes
--b.
This was obviously wrong, since path[strlen(path)] == '\0' should always
be true.
Signed-off-by: J. Bruce Fields <[email protected]>
---
utils/mountd/cache.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d63e10a..ff27bbf 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
int l = strlen(path);
return strcmp(subpath, path) == 0
- || (strncmp(subpath, path, l) == 0 && path[l] == '/');
+ || (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
}
static int path_matches(nfs_export *exp, char *path)
--
1.6.3.3
Part of the reason for the previous bug was confusion between "subpath"
and "path"; which is the shorter path, and which the longer?
"child" and "parent" seem less ambiguous.
Signed-off-by: J. Bruce Fields <[email protected]>
---
utils/mountd/cache.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ff27bbf..7dec468 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -614,12 +614,12 @@ static int dump_to_cache(FILE *f, char *domain, char *path, struct exportent *ex
return qword_eol(f);
}
-static int is_subdirectory(char *subpath, char *path)
+static int is_subdirectory(char *child, char *parent)
{
- int l = strlen(path);
+ int l = strlen(parent);
- return strcmp(subpath, path) == 0
- || (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
+ return strcmp(child, parent) == 0
+ || (strncmp(child, parent, l) == 0 && child[l] == '/');
}
static int path_matches(nfs_export *exp, char *path)
--
1.6.3.3
The extra cache entries added here all get the options of the parent
export. This is incorrect, since once of the children may be explicitly
exported with different options, and the parent crossmnt shouldn't
override the explicit child export.
What's more, this is unnecessary, since in the newcache case we'll
request these on demand when we need them.
Signed-off-by: J. Bruce Fields <[email protected]>
---
utils/mountd/cache.c | 40 ----------------------------------------
1 files changed, 0 insertions(+), 40 deletions(-)
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7dec468..200e179 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -817,46 +817,6 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
" fsid= required", exp->e_path);
}
- while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
- /* really an 'if', but we can break out of
- * a 'while' more easily */
- /* Look along 'path' for other filesystems
- * and export them with the same options
- */
- struct stat stb;
- int l = strlen(exp->e_path);
- int dev;
-
- if (strlen(path) <= l || path[l] != '/' ||
- strncmp(exp->e_path, path, l) != 0)
- break;
- if (stat(exp->e_path, &stb) != 0)
- break;
- dev = stb.st_dev;
- while(path[l] == '/') {
- char c;
- /* errors for submount should fail whole filesystem */
- int err2;
-
- l++;
- while (path[l] != '/' && path[l])
- l++;
- c = path[l];
- path[l] = 0;
- err2 = lstat(path, &stb);
- path[l] = c;
- if (err2 < 0)
- break;
- if (stb.st_dev == dev)
- continue;
- dev = stb.st_dev;
- path[l] = 0;
- dump_to_cache(f, domain, path, exp);
- path[l] = c;
- }
- break;
- }
-
fclose(f);
return err;
}
--
1.6.3.3
On Sun, 7 Mar 2010 15:08:01 -0500
"J. Bruce Fields" <[email protected]> wrote:
> The extra cache entries added here all get the options of the parent
> export. This is incorrect, since once of the children may be explicitly
> exported with different options, and the parent crossmnt shouldn't
> override the explicit child export.
>
> What's more, this is unnecessary, since in the newcache case we'll
> request these on demand when we need them.
Are you sure that removing this doesn't break something else?
It was explicitly added in commit 323d1c4d69b65ab36d, apparently for a good
reason.
Also, it shouldn't be causing the problem described as cache_export_ent
should only be called where 'exp' is the lowest (furthest from the root)
export for any directory in 'path'. So any child mounts that are found
should not be explicitly exported.
This code is needed to handle a case where you have
/a /a/b /a/b/c all mount points, /a is exported with crossmnt,
and a mount request comes in for /a/b/c (or /a/b).
mountd needs to get the filehandle for /a/b/c, so that filesystem must be
exported to the kernel. We cannot really on upcalls filling in that
information as doing so would cause mountd to deadlock - it asks the kernel
for a filehandle, the kernel asks it for export information, and mountd is
single-threaded...
Are you sure that removing this actually fixes a problem?
NeilBrown
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> utils/mountd/cache.c | 40 ----------------------------------------
> 1 files changed, 0 insertions(+), 40 deletions(-)
>
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 7dec468..200e179 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -817,46 +817,6 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
> " fsid= required", exp->e_path);
> }
>
> - while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
> - /* really an 'if', but we can break out of
> - * a 'while' more easily */
> - /* Look along 'path' for other filesystems
> - * and export them with the same options
> - */
> - struct stat stb;
> - int l = strlen(exp->e_path);
> - int dev;
> -
> - if (strlen(path) <= l || path[l] != '/' ||
> - strncmp(exp->e_path, path, l) != 0)
> - break;
> - if (stat(exp->e_path, &stb) != 0)
> - break;
> - dev = stb.st_dev;
> - while(path[l] == '/') {
> - char c;
> - /* errors for submount should fail whole filesystem */
> - int err2;
> -
> - l++;
> - while (path[l] != '/' && path[l])
> - l++;
> - c = path[l];
> - path[l] = 0;
> - err2 = lstat(path, &stb);
> - path[l] = c;
> - if (err2 < 0)
> - break;
> - if (stb.st_dev == dev)
> - continue;
> - dev = stb.st_dev;
> - path[l] = 0;
> - dump_to_cache(f, domain, path, exp);
> - path[l] = c;
> - }
> - break;
> - }
> -
> fclose(f);
> return err;
> }
On Mon, Mar 08, 2010 at 08:25:16AM +1100, Neil Brown wrote:
> On Sun, 7 Mar 2010 15:08:01 -0500
> "J. Bruce Fields" <[email protected]> wrote:
>
> > The extra cache entries added here all get the options of the parent
> > export. This is incorrect, since once of the children may be explicitly
> > exported with different options, and the parent crossmnt shouldn't
> > override the explicit child export.
> >
> > What's more, this is unnecessary, since in the newcache case we'll
> > request these on demand when we need them.
>
> Are you sure that removing this doesn't break something else?
Maybe so--thanks for looking at this.
> It was explicitly added in commit 323d1c4d69b65ab36d, apparently for a good
> reason.
>
> Also, it shouldn't be causing the problem described as cache_export_ent
> should only be called where 'exp' is the lowest (furthest from the root)
> export for any directory in 'path'.
Hm. Is nfsd_fh() following that rule?
> So any child mounts that are found should not be explicitly exported.
>
> This code is needed to handle a case where you have
> /a /a/b /a/b/c all mount points, /a is exported with crossmnt,
> and a mount request comes in for /a/b/c (or /a/b).
> mountd needs to get the filehandle for /a/b/c, so that filesystem must be
> exported to the kernel.
Yes, I didn't look carefully enough. I think I must have assumed the
first dump_to_cache did that. But wouldn't it be sufficient to just do:
dump_to_cache(f, domain, path, e_path);
(as in the below) instead of also running through all the parents?
> We cannot really on upcalls filling in that
> information as doing so would cause mountd to deadlock - it asks the kernel
> for a filehandle, the kernel asks it for export information, and mountd is
> single-threaded...
Don't we have this problem already, then? The export cache really is
just a cache, and we should be prepared for a given entry to be purged
at any time.
--b.
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7dec468..f682a9b 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -810,53 +810,13 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
if (!f)
return -1;
- err = dump_to_cache(f, domain, exp->e_path, exp);
+ err = dump_to_cache(f, domain, path, exp);
if (err) {
xlog(L_WARNING,
"Cannot export %s, possibly unsupported filesystem or"
" fsid= required", exp->e_path);
}
- while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
- /* really an 'if', but we can break out of
- * a 'while' more easily */
- /* Look along 'path' for other filesystems
- * and export them with the same options
- */
- struct stat stb;
- int l = strlen(exp->e_path);
- int dev;
-
- if (strlen(path) <= l || path[l] != '/' ||
- strncmp(exp->e_path, path, l) != 0)
- break;
- if (stat(exp->e_path, &stb) != 0)
- break;
- dev = stb.st_dev;
- while(path[l] == '/') {
- char c;
- /* errors for submount should fail whole filesystem */
- int err2;
-
- l++;
- while (path[l] != '/' && path[l])
- l++;
- c = path[l];
- path[l] = 0;
- err2 = lstat(path, &stb);
- path[l] = c;
- if (err2 < 0)
- break;
- if (stb.st_dev == dev)
- continue;
- dev = stb.st_dev;
- path[l] = 0;
- dump_to_cache(f, domain, path, exp);
- path[l] = c;
- }
- break;
- }
-
fclose(f);
return err;
}
On Sun, 7 Mar 2010 16:58:26 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Mon, Mar 08, 2010 at 08:25:16AM +1100, Neil Brown wrote:
> > On Sun, 7 Mar 2010 15:08:01 -0500
> > "J. Bruce Fields" <[email protected]> wrote:
> >
> > > The extra cache entries added here all get the options of the parent
> > > export. This is incorrect, since once of the children may be explicitly
> > > exported with different options, and the parent crossmnt shouldn't
> > > override the explicit child export.
> > >
> > > What's more, this is unnecessary, since in the newcache case we'll
> > > request these on demand when we need them.
> >
> > Are you sure that removing this doesn't break something else?
>
> Maybe so--thanks for looking at this.
>
> > It was explicitly added in commit 323d1c4d69b65ab36d, apparently for a good
> > reason.
> >
> > Also, it shouldn't be causing the problem described as cache_export_ent
> > should only be called where 'exp' is the lowest (furthest from the root)
> > export for any directory in 'path'.
>
> Hm. Is nfsd_fh() following that rule?
Hmmm... maybe not. However when it doesn't it should log a warning
"%s and %s have same filehandle for %s, using first"
Maybe we need better choice between options there. Maybe we should choose
the longest e_path??
>
> > So any child mounts that are found should not be explicitly exported.
> >
> > This code is needed to handle a case where you have
> > /a /a/b /a/b/c all mount points, /a is exported with crossmnt,
> > and a mount request comes in for /a/b/c (or /a/b).
> > mountd needs to get the filehandle for /a/b/c, so that filesystem must be
> > exported to the kernel.
>
> Yes, I didn't look carefully enough. I think I must have assumed the
> first dump_to_cache did that. But wouldn't it be sufficient to just do:
>
> dump_to_cache(f, domain, path, e_path);
s/e_path/exp/
>
> (as in the below) instead of also running through all the parents?
That sounds credible.. I wonder why I didn't think of that at the time.
My thoughts stray to lookups of ".." and whether they could use the wrong
export. I don't think that can in any sane configuration.
>
> > We cannot really on upcalls filling in that
> > information as doing so would cause mountd to deadlock - it asks the kernel
> > for a filehandle, the kernel asks it for export information, and mountd is
> > single-threaded...
>
> Don't we have this problem already, then? The export cache really is
> just a cache, and we should be prepared for a given entry to be purged
> at any time.
>
Yes, it is a cache, but things don't get pushed out when it is 'full', so you
can expect items to stay out their expiry time. So if you add something
with an expiry 30 minutes in the future, you can usually expect it to still
be there in 30 seconds.
If someone ran "exportfs -f" at exactly the wrong time you might be able to
deadlock mountd (I'd have to double-check to be sure), but that is very
different from mountd acting in a way which leads directly to a deadlock.
NeilBrown
> --b.
>
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 7dec468..f682a9b 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -810,53 +810,13 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
> if (!f)
> return -1;
>
> - err = dump_to_cache(f, domain, exp->e_path, exp);
> + err = dump_to_cache(f, domain, path, exp);
> if (err) {
> xlog(L_WARNING,
> "Cannot export %s, possibly unsupported filesystem or"
> " fsid= required", exp->e_path);
> }