Re: [PATCH] nfsd: fix handling of readdir in v4root vs. mount upcall timeout

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2022-12-13 at 19:00 +0000, Chuck Lever III wrote:
> 
> > On Dec 13, 2022, at 1:08 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > If v4 READDIR operation hits a mountpoint and gets back an error,
> > then it will include that entry in the reply and set RDATTR_ERROR for it
> > to the error.
> > 
> > That's fine for "normal" exported filesystems, but on the v4root, we
> > need to be more careful to only expose the existence of dentries that
> > lead to exports.
> > 
> > If the mountd upcall times out while checking to see whether a
> > mountpoint on the v4root is exported, then we have no recourse other
> > than to fail the whole operation.
> 
> Thank you for chasing this down!
> 
> Failing the whole READDIR when mountd times out might be a bad idea.
> If the mountd upcall times out every time, the client can't make
> any progress and will continue to emit the failing READDIR request.
> 
> Would it be better to skip the unresolvable entry instead and let
> the READDIR succeed without that entry?
> 

Mounting doesn't usually require working READDIR. In that situation, a
readdir() might hang (until the client kills), but a lookup of other
dentries that aren't perpetually stalled should be ok in this situation.

If mountd is that hosed then I think it's unlikely that any progress
will be possible anyway.

> 
> > Cc: Steve Dickson <steved@redhat.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216777
> > Reported-by:  JianHong Yin <yin-jianhong@163.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/nfsd/nfs4xdr.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 2b4ae858c89b..984528ce8d68 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -3588,6 +3588,7 @@ nfsd4_encode_dirent(void *ccdv, const char *name, int namlen,
> > 	struct readdir_cd *ccd = ccdv;
> > 	struct nfsd4_readdir *cd = container_of(ccd, struct nfsd4_readdir, common);
> > 	struct xdr_stream *xdr = cd->xdr;
> > +	struct svc_export *exp = cd->rd_fhp->fh_export;
> > 	int start_offset = xdr->buf->len;
> > 	int cookie_offset;
> > 	u32 name_and_cookie;
> > @@ -3629,6 +3630,17 @@ nfsd4_encode_dirent(void *ccdv, const char *name, int namlen,
> > 	case nfserr_noent:
> > 		xdr_truncate_encode(xdr, start_offset);
> > 		goto skip_entry;
> > +	case nfserr_jukebox:
> > +		/*
> > +		 * The pseudoroot should only display dentries that lead to
> > +		 * exports. If we get EJUKEBOX here, then we can't tell whether
> > +		 * this entry should be included. Just fail the whole READDIR
> > +		 * with NFS4ERR_DELAY in that case, and hope that the situation
> > +		 * will resolve itself by the client's next attempt.
> > +		 */
> > +		if (exp->ex_flags & NFSEXP_V4ROOT)
> > +			goto fail;
> > +		fallthrough;
> > 	default:
> > 		/*
> > 		 * If the client requested the RDATTR_ERROR attribute,
> > -- 
> > 2.38.1
> > 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>