| Message ID | 165516230202.21248.2917435222861526857.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow concurrent directory updates. | expand |
> 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
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 *);
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. 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(-)