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

From: Chuck Lever III
Date: Fri Jun 24 2022 - 10:46:36 EST

Next message: Chuck Lever III: "Re: [PATCH 11/12] nfsd: use (un)lock_inode instead of fh_(un)lock"
Previous message: Chuck Lever III: "Re: [PATCH 09/12] nfsd: support concurrent renames."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On Jun 13, 2022, at 7:18 PM, NeilBrown <neilb@xxxxxxx> 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@xxxxxxxxxx>

> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> 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

Next message: Chuck Lever III: "Re: [PATCH 11/12] nfsd: use (un)lock_inode instead of fh_(un)lock"
Previous message: Chuck Lever III: "Re: [PATCH 09/12] nfsd: support concurrent renames."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]