Re: [PATCH 10/12] nfsd: reduce locking in nfsd_lookup()

[Chuck Lever III <chuck.lever@oracle.com>]

> On Jun 13, 2022, at 7:18 PM, NeilBrown <neilb@suse.de> wrote:
> 
> nfsd_lookup() takes an exclusive lock on the parent inode, but many
> callers don't want the lock and may not need to lock at all if the
> result is in the dcache.
> 
> Also this is the only place where the fh_locked flag is needed, as
> nfsd_lookup() may or may not return with the inode locked, and the
> fh_locked flag tells which.
> 
> Change nfsd_lookup() to be passed a pointer to an int flag.
> If the pointer is NULL, don't take the lock.
> If not, record in the int whether the lock is held.
> Then in that one place that care, pass a pointer to an int, and be sure
> to unlock if necessary.

I feel it would be more helpful if the above paragraph were
instead part of a kdoc style documenting comment for
nfsd_lookup(). Can you make that change a part of this patch?

The new API for nfsd_lookup() is making me squirm:

@locked functions as both an input parameter (setting it to
NULL changes function behavior) and an output parameter. That's
always had a bit of a code smell to me. Generally I would create
a separate version of nfsd_lookup() to handle the NFSv4-specific
case, but that would amount to quite a bit of code duplication.

"locked" is a non-descript, perhaps overloaded, name.

Using a boolean would be a better choice for an output type
that can assume only two unique values, but "int" isn't
atypical in this code.

I don't really have any better suggestions, though, so:

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs3proc.c |    2 +-
> fs/nfsd/nfs4proc.c |   27 +++++++++++++--------------
> fs/nfsd/nfsproc.c  |    2 +-
> fs/nfsd/vfs.c      |   36 +++++++++++++++++++++++++-----------
> fs/nfsd/vfs.h      |    8 +++++---
> 5 files changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 0fdbb9504a87..d85b110d58dd 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -96,7 +96,7 @@ nfsd3_proc_lookup(struct svc_rqst *rqstp)
> 
> 	resp->status = nfsd_lookup(rqstp, &resp->dirfh,
> 				   argp->name, argp->len,
> -				   &resp->fh);
> +				   &resp->fh, NULL);
> 	return rpc_success;
> }
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71a4b8ef77f0..79434e29b63f 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -411,7 +411,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> }
> 
> static __be32
> -do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh)
> +do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> +	       struct nfsd4_open *open, struct svc_fh **resfh,
> +	       int *locked)
> {
> 	struct svc_fh *current_fh = &cstate->current_fh;
> 	int accmode;
> @@ -455,14 +457,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> 			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> 						FATTR4_WORD1_TIME_MODIFY);
> 	} else
> -		/*
> -		 * Note this may exit with the parent still locked.
> -		 * We will hold the lock until nfsd4_open's final
> -		 * lookup, to prevent renames or unlinks until we've had
> -		 * a chance to an acquire a delegation if appropriate.
> -		 */
> 		status = nfsd_lookup(rqstp, current_fh,
> -				     open->op_fname, open->op_fnamelen, *resfh);
> +				     open->op_fname, open->op_fnamelen, *resfh,
> +				     locked);
> 	if (status)
> 		goto out;
> 	status = nfsd_check_obj_isreg(*resfh);
> @@ -537,6 +534,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	struct net *net = SVC_NET(rqstp);
> 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> 	bool reclaim = false;
> +	int locked = 0;
> 
> 	dprintk("NFSD: nfsd4_open filename %.*s op_openowner %p\n",
> 		(int)open->op_fnamelen, open->op_fname,
> @@ -598,7 +596,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	switch (open->op_claim_type) {
> 	case NFS4_OPEN_CLAIM_DELEGATE_CUR:
> 	case NFS4_OPEN_CLAIM_NULL:
> -		status = do_open_lookup(rqstp, cstate, open, &resfh);
> +		status = do_open_lookup(rqstp, cstate, open, &resfh, &locked);
> 		if (status)
> 			goto out;
> 		break;
> @@ -636,6 +634,8 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 		fput(open->op_filp);
> 		open->op_filp = NULL;
> 	}
> +	if (locked)
> +		inode_unlock(cstate->current_fh.fh_dentry->d_inode);
> 	if (resfh && resfh != &cstate->current_fh) {
> 		fh_dup2(&cstate->current_fh, resfh);
> 		fh_put(resfh);
> @@ -920,7 +920,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> 		return nfserr_noent;
> 	}
> 	fh_put(&tmp_fh);
> -	return nfsd_lookup(rqstp, fh, "..", 2, fh);
> +	return nfsd_lookup(rqstp, fh, "..", 2, fh, NULL);
> }
> 
> static __be32
> @@ -936,7 +936,7 @@ nfsd4_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> {
> 	return nfsd_lookup(rqstp, &cstate->current_fh,
> 			   u->lookup.lo_name, u->lookup.lo_len,
> -			   &cstate->current_fh);
> +			   &cstate->current_fh, NULL);
> }
> 
> static __be32
> @@ -1078,11 +1078,10 @@ nfsd4_secinfo(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	if (err)
> 		return err;
> 	err = nfsd_lookup_dentry(rqstp, &cstate->current_fh,
> -				    secinfo->si_name, secinfo->si_namelen,
> -				    &exp, &dentry);
> +				 secinfo->si_name, secinfo->si_namelen,
> +				 &exp, &dentry, NULL);
> 	if (err)
> 		return err;
> -	fh_unlock(&cstate->current_fh);
> 	if (d_really_is_negative(dentry)) {
> 		exp_put(exp);
> 		err = nfserr_noent;
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 2dccf77634e8..465d70e053f6 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -133,7 +133,7 @@ nfsd_proc_lookup(struct svc_rqst *rqstp)
> 
> 	fh_init(&resp->fh, NFS_FHSIZE);
> 	resp->status = nfsd_lookup(rqstp, &argp->fh, argp->name, argp->len,
> -				   &resp->fh);
> +				   &resp->fh, NULL);
> 	fh_put(&argp->fh);
> 	if (resp->status != nfs_ok)
> 		goto out;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index b0df216ab3e4..4c2e431100ba 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -172,7 +172,8 @@ int nfsd_mountpoint(struct dentry *dentry, struct svc_export *exp)
> __be32
> nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 		   const char *name, unsigned int len,
> -		   struct svc_export **exp_ret, struct dentry **dentry_ret)
> +		   struct svc_export **exp_ret, struct dentry **dentry_ret,
> +		   int *locked)
> {
> 	struct svc_export	*exp;
> 	struct dentry		*dparent;
> @@ -184,6 +185,9 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	dparent = fhp->fh_dentry;
> 	exp = exp_get(fhp->fh_export);
> 
> +	if (locked)
> +		*locked = 0;
> +
> 	/* Lookup the name, but don't follow links */
> 	if (isdotent(name, len)) {
> 		if (len==1)
> @@ -199,13 +203,15 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 				goto out_nfserr;
> 		}
> 	} else {
> -		/*
> -		 * In the nfsd4_open() case, this may be held across
> -		 * subsequent open and delegation acquisition which may
> -		 * need to take the child's i_mutex:
> -		 */
> -		fh_lock_nested(fhp, I_MUTEX_PARENT);
> -		dentry = lookup_one_len(name, dparent, len);
> +		if (locked) {
> +			inode_lock_nested(dparent->d_inode, I_MUTEX_PARENT);
> +			dentry = lookup_one_len(name, dparent, len);
> +			if (IS_ERR(dentry))
> +				inode_unlock(dparent->d_inode);
> +			else
> +				*locked = 1;
> +		} else
> +			dentry = lookup_one_len_unlocked(name, dparent, len);
> 		host_err = PTR_ERR(dentry);
> 		if (IS_ERR(dentry))
> 			goto out_nfserr;
> @@ -218,7 +224,10 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 			 * something that we might be about to delegate,
> 			 * and a mountpoint won't be renamed:
> 			 */
> -			fh_unlock(fhp);
> +			if (locked && *locked) {
> +				inode_unlock(dparent->d_inode);
> +				*locked = 0;
> +			}
> 			if ((host_err = nfsd_cross_mnt(rqstp, &dentry, &exp))) {
> 				dput(dentry);
> 				goto out_nfserr;
> @@ -248,7 +257,8 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  */
> __be32
> nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> -				unsigned int len, struct svc_fh *resfh)
> +	    unsigned int len, struct svc_fh *resfh,
> +	    int *locked)
> {
> 	struct svc_export	*exp;
> 	struct dentry		*dentry;
> @@ -257,7 +267,7 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> 	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
> 	if (err)
> 		return err;
> -	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
> +	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry, locked);
> 	if (err)
> 		return err;
> 	err = check_nfsd_access(exp, rqstp);
> @@ -273,6 +283,10 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> out:
> 	dput(dentry);
> 	exp_put(exp);
> +	if (err && locked && *locked) {
> +		inode_unlock(fhp->fh_dentry->d_inode);
> +		*locked = 0;
> +	}
> 	return err;
> }
> 
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 26347d76f44a..b7d41b73dd79 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -45,10 +45,12 @@ typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
> int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
> 		                struct svc_export **expp);
> __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,
> -				const char *, unsigned int, struct svc_fh *);
> +			    const char *, unsigned int, struct svc_fh *,
> +			    int *);
> __be32		 nfsd_lookup_dentry(struct svc_rqst *, struct svc_fh *,
> -				const char *, unsigned int,
> -				struct svc_export **, struct dentry **);
> +				    const char *, unsigned int,
> +				    struct svc_export **, struct dentry **,
> +				    int *);
> __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
> 				struct iattr *, int, time64_t);
> int nfsd_mountpoint(struct dentry *, struct svc_export *);
> 
> 

--
Chuck Lever