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

From: Trond Myklebust <trondmy@hammerspace.com>
To: "bfields@fieldses.org" <bfields@fieldses.org>,
	"trondmy@kernel.org" <trondmy@kernel.org>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"bfields@redhat.com" <bfields@redhat.com>,
	"chuck.lever@oracle.com" <chuck.lever@oracle.com>
Subject: Re: [PATCH 5/6] nfsd: Fix up nfsd to ensure that timeout errors don't result in ESTALE
Date: Tue, 1 Dec 2020 00:39:11 +0000	[thread overview]
Message-ID: <03594580aeae8c7a94dcff16bcdb5a8049d654a7.camel@hammerspace.com> (raw)
In-Reply-To: <20201130230501.GC22446@fieldses.org>

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

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com