| Message ID | 20200416221252.82102-7-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs-utils fixes | expand |
On Thu, Apr 16, 2020 at 06:12:51PM -0400, trondmy@kernel.org wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > In nfsd_fh(), if the error returned by the downcall is transient, > then we should ignore it. Only reject the export if the filesystem > path is truly not exportable. > This fixes a case where we can see spurious NFSERR_STALE errors > being returned by knfsd. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > utils/mountd/cache.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c > index 0f323226b12a..79d3ee085a90 100644 > --- a/utils/mountd/cache.c > +++ b/utils/mountd/cache.c > @@ -843,8 +843,14 @@ static void nfsd_fh(int f) > } > } > } > - if (found && > - found->e_mountpoint && > + > + if (!found) { > + /* The missing dev could be what we want, so just be > + * quiet rather than returning stale yet > + */ > + if (dev_missing) > + goto out; > + } else if (found->e_mountpoint && > !is_mountpoint(found->e_mountpoint[0]? > found->e_mountpoint: > found->e_path)) { > @@ -855,17 +861,12 @@ static void nfsd_fh(int f) > */ > /* FIXME we need to make sure we re-visit this later */ > goto out; > + } else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) { > + if (!path_lookup_error(errno)) > + goto out; > + /* The kernel is saying the path is unexportable */ > + found = NULL; > } > - if (!found && dev_missing) { > - /* The missing dev could be what we want, so just be > - * quite rather than returning stale yet > - */ > - goto out; > - } > - > - if (found) > - if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) > - found = 0; > > bp = buf; blen = sizeof(buf); > qword_add(&bp, &blen, dom); Is everybody else better at reading this kind of patch? Between the code reshuffling and the conditionals, it's almost totally opaque to me. I'd have split it up something like the below.--b. commit b0b623281017 Author: Trond Myklebust <trond.myklebust@hammerspace.com> Date: Thu Apr 16 18:12:51 2020 -0400 mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh() In nfsd_fh(), if the error returned by the downcall is transient, then we should ignore it. Only reject the export if the filesystem path is truly not exportable. This fixes a case where we can see spurious NFSERR_STALE errors being returned by knfsd. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c index bcd4b3ce5bb2..0426b171f040 100644 --- a/utils/mountd/cache.c +++ b/utils/mountd/cache.c @@ -826,8 +826,12 @@ static void nfsd_fh(int f) */ /* FIXME we need to make sure we re-visit this later */ goto out; - } else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) - found = 0; + } else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) { + if (!path_lookup_error(errno)) + goto out; + /* The kernel is saying the path is unexportable */ + found = NULL; + } bp = buf; blen = sizeof(buf); qword_add(&bp, &blen, dom); commit 931a2e77f954 Author: J. Bruce Fields <bfields@redhat.com> Date: Mon Apr 27 13:22:30 2020 -0400 rearrange logic; no change in behavior diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c index 8f54e37b7936..bcd4b3ce5bb2 100644 --- a/utils/mountd/cache.c +++ b/utils/mountd/cache.c @@ -808,8 +808,14 @@ static void nfsd_fh(int f) } } } - if (found && - found->e_mountpoint && + + if (!found) { + /* The missing dev could be what we want, so just be + * quiet rather than returning stale yet + */ + if (dev_missing) + goto out; + } else if (found->e_mountpoint && !is_mountpoint(found->e_mountpoint[0]? found->e_mountpoint: found->e_path)) { @@ -820,16 +826,7 @@ static void nfsd_fh(int f) */ /* FIXME we need to make sure we re-visit this later */ goto out; - } - if (!found && dev_missing) { - /* The missing dev could be what we want, so just be - * quite rather than returning stale yet - */ - goto out; - } - - if (found) - if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) + } else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) found = 0; bp = buf; blen = sizeof(buf);
Ack to the series, otherwise, looks good.--b.
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c index 0f323226b12a..79d3ee085a90 100644 --- a/utils/mountd/cache.c +++ b/utils/mountd/cache.c @@ -843,8 +843,14 @@ static void nfsd_fh(int f) } } } - if (found && - found->e_mountpoint && + + if (!found) { + /* The missing dev could be what we want, so just be + * quiet rather than returning stale yet + */ + if (dev_missing) + goto out; + } else if (found->e_mountpoint && !is_mountpoint(found->e_mountpoint[0]? found->e_mountpoint: found->e_path)) { @@ -855,17 +861,12 @@ static void nfsd_fh(int f) */ /* FIXME we need to make sure we re-visit this later */ goto out; + } else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) { + if (!path_lookup_error(errno)) + goto out; + /* The kernel is saying the path is unexportable */ + found = NULL; } - if (!found && dev_missing) { - /* The missing dev could be what we want, so just be - * quite rather than returning stale yet - */ - goto out; - } - - if (found) - if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) - found = 0; bp = buf; blen = sizeof(buf); qword_add(&bp, &blen, dom);