diff mbox series

[5/6] nfsd: Fix up nfsd to ensure that timeout errors don't result in ESTALE

Message ID 20201130212455.254469-6-trondmy@kernel.org (mailing list archive)
State New
Headers show
Series Patches to support NFS re-exporting | expand

Commit Message

trondmy@kernel.org Nov. 30, 2020, 9:24 p.m. UTC
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.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfsd/nfsfh.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

J. Bruce Fields Nov. 30, 2020, 11:05 p.m. UTC | #1
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
Trond Myklebust Dec. 1, 2020, 12:39 a.m. UTC | #2
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
J. Bruce Fields Dec. 1, 2020, 2:30 a.m. UTC | #3
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 mbox series

Patch

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;