| Message ID | 20201130212455.254469-6-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Patches to support NFS re-exporting | expand |
On Mon, Nov 30, 2020 at 04:24:54PM -0500, trondmy@kernel.org wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > If the underlying filesystem times out, then we want knfsd to return > NFSERR_JUKEBOX/DELAY rather than NFSERR_STALE. Out of curiosity, what was causing ETIMEDOUT in practice? --b. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfsd/nfsfh.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index 0c2ee65e46f3..46c86f7bc429 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -268,12 +268,20 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > if (fileid_type == FILEID_ROOT) > dentry = dget(exp->ex_path.dentry); > else { > - dentry = exportfs_decode_fh(exp->ex_path.mnt, fid, > - data_left, fileid_type, > - nfsd_acceptable, exp); > - if (IS_ERR_OR_NULL(dentry)) > + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, > + data_left, fileid_type, > + nfsd_acceptable, exp); > + if (IS_ERR_OR_NULL(dentry)) { > trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, > dentry ? PTR_ERR(dentry) : -ESTALE); > + switch (PTR_ERR(dentry)) { > + case -ENOMEM: > + case -ETIMEDOUT: > + break; > + default: > + dentry = ERR_PTR(-ESTALE); > + } > + } > } > if (dentry == NULL) > goto out; > -- > 2.28.0
On Mon, 2020-11-30 at 18:05 -0500, J. Bruce Fields wrote: > On Mon, Nov 30, 2020 at 04:24:54PM -0500, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > If the underlying filesystem times out, then we want knfsd to > > return > > NFSERR_JUKEBOX/DELAY rather than NFSERR_STALE. > > Out of curiosity, what was causing ETIMEDOUT in practice? > If you're only re-exporting NFS from a single server, then it is OK to use hard mounts. However if you are exporting from multiple servers, or you have local filesystems that are also being exported by the same knfsd server, then you usually want to use softerr mounts for NFS so that operations that take an inordinate amount of time due to temporary server outages get converted into JUKEBOX/DELAY errors. Otherwise, it is really simple to cause all the nfsd threads to hang on that one delinquent server. > --b. > > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfsd/nfsfh.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > > index 0c2ee65e46f3..46c86f7bc429 100644 > > --- a/fs/nfsd/nfsfh.c > > +++ b/fs/nfsd/nfsfh.c > > @@ -268,12 +268,20 @@ static __be32 nfsd_set_fh_dentry(struct > > svc_rqst *rqstp, struct svc_fh *fhp) > > if (fileid_type == FILEID_ROOT) > > dentry = dget(exp->ex_path.dentry); > > else { > > - dentry = exportfs_decode_fh(exp->ex_path.mnt, fid, > > - data_left, fileid_type, > > - nfsd_acceptable, exp); > > - if (IS_ERR_OR_NULL(dentry)) > > + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, > > fid, > > + data_left, > > fileid_type, > > + nfsd_acceptable, > > exp); > > + if (IS_ERR_OR_NULL(dentry)) { > > trace_nfsd_set_fh_dentry_badhandle(rqstp, > > fhp, > > dentry ? PTR_ERR(dentry) : > > -ESTALE); > > + switch (PTR_ERR(dentry)) { > > + case -ENOMEM: > > + case -ETIMEDOUT: > > + break; > > + default: > > + dentry = ERR_PTR(-ESTALE); > > + } > > + } > > } > > if (dentry == NULL) > > goto out; > > -- > > 2.28.0
On Tue, Dec 01, 2020 at 12:39:11AM +0000, Trond Myklebust wrote: > On Mon, 2020-11-30 at 18:05 -0500, J. Bruce Fields wrote: > > On Mon, Nov 30, 2020 at 04:24:54PM -0500, trondmy@kernel.org wrote: > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > If the underlying filesystem times out, then we want knfsd to > > > return > > > NFSERR_JUKEBOX/DELAY rather than NFSERR_STALE. > > > > Out of curiosity, what was causing ETIMEDOUT in practice? > > > > If you're only re-exporting NFS from a single server, then it is OK to > use hard mounts. However if you are exporting from multiple servers, or > you have local filesystems that are also being exported by the same > knfsd server, then you usually want to use softerr mounts for NFS so > that operations that take an inordinate amount of time due to temporary > server outages get converted into JUKEBOX/DELAY errors. Otherwise, it > is really simple to cause all the nfsd threads to hang on that one > delinquent server. Makes sense, thanks. In theory the same thing could happen with block devices; longer term I wonder if it'd make sense to limit how many threads are waiting on a single backend. (ACK to the patch, though, that'd be a project for another day.) --b.
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index 0c2ee65e46f3..46c86f7bc429 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -268,12 +268,20 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) if (fileid_type == FILEID_ROOT) dentry = dget(exp->ex_path.dentry); else { - dentry = exportfs_decode_fh(exp->ex_path.mnt, fid, - data_left, fileid_type, - nfsd_acceptable, exp); - if (IS_ERR_OR_NULL(dentry)) + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, + data_left, fileid_type, + nfsd_acceptable, exp); + if (IS_ERR_OR_NULL(dentry)) { trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, dentry ? PTR_ERR(dentry) : -ESTALE); + switch (PTR_ERR(dentry)) { + case -ENOMEM: + case -ETIMEDOUT: + break; + default: + dentry = ERR_PTR(-ESTALE); + } + } } if (dentry == NULL) goto out;