| Message ID | 165708109258.1940.6581517569232462503.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: clean up locking. | expand |
On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote: > On non-error paths, nfsd_link() calls fh_unlock() twice. This is safe > because fh_unlock() records that the unlock has been done and doesn't > repeat it. > However it makes the code a little confusing and interferes with changes > that are planned for directory locking. > > So rearrange the code to ensure fh_unlock() is called exactly once if > fh_lock() was called. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/vfs.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 3f4579f5775c..4916c29af0fa 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1551,8 +1551,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > > dnew = lookup_one_len(name, ddir, len); > host_err = PTR_ERR(dnew); > - if (IS_ERR(dnew)) > - goto out_nfserr; > + if (IS_ERR(dnew)) { > + err = nfserrno(host_err); > + goto out_unlock; > + } > > dold = tfhp->fh_dentry; > > @@ -1571,17 +1573,17 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > else > err = nfserrno(host_err); > } > -out_dput: > dput(dnew); > -out_unlock: > - fh_unlock(ffhp); > +out_drop_write: > fh_drop_write(tfhp); > out: > return err; > > -out_nfserr: > - err = nfserrno(host_err); > - goto out_unlock; > +out_dput: > + dput(dnew); > +out_unlock: > + fh_unlock(ffhp); > + goto out_drop_write; > } > > static void > > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On Jul 6, 2022, at 12:18 AM, NeilBrown <neilb@suse.de> wrote: > > On non-error paths, nfsd_link() calls fh_unlock() twice. This is safe > because fh_unlock() records that the unlock has been done and doesn't > repeat it. > However it makes the code a little confusing and interferes with changes > that are planned for directory locking. > > So rearrange the code to ensure fh_unlock() is called exactly once if > fh_lock() was called. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/vfs.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 3f4579f5775c..4916c29af0fa 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1551,8 +1551,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > > dnew = lookup_one_len(name, ddir, len); > host_err = PTR_ERR(dnew); > - if (IS_ERR(dnew)) > - goto out_nfserr; > + if (IS_ERR(dnew)) { > + err = nfserrno(host_err); > + goto out_unlock; > + } Nit: Let's do it this way: dnew = lookup_one_len(name, ddir, len); if (IS_ERR(dnew)) { err = nfserrno(PTR_ERR(dnew); goto out_unlock; } > dold = tfhp->fh_dentry; > > @@ -1571,17 +1573,17 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > else > err = nfserrno(host_err); > } > -out_dput: > dput(dnew); > -out_unlock: > - fh_unlock(ffhp); > +out_drop_write: > fh_drop_write(tfhp); > out: > return err; > > -out_nfserr: > - err = nfserrno(host_err); > - goto out_unlock; > +out_dput: > + dput(dnew); > +out_unlock: > + fh_unlock(ffhp); > + goto out_drop_write; > } > > static void > > -- Chuck Lever
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 3f4579f5775c..4916c29af0fa 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1551,8 +1551,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, dnew = lookup_one_len(name, ddir, len); host_err = PTR_ERR(dnew); - if (IS_ERR(dnew)) - goto out_nfserr; + if (IS_ERR(dnew)) { + err = nfserrno(host_err); + goto out_unlock; + } dold = tfhp->fh_dentry; @@ -1571,17 +1573,17 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, else err = nfserrno(host_err); } -out_dput: dput(dnew); -out_unlock: - fh_unlock(ffhp); +out_drop_write: fh_drop_write(tfhp); out: return err; -out_nfserr: - err = nfserrno(host_err); - goto out_unlock; +out_dput: + dput(dnew); +out_unlock: + fh_unlock(ffhp); + goto out_drop_write; } static void
On non-error paths, nfsd_link() calls fh_unlock() twice. This is safe because fh_unlock() records that the unlock has been done and doesn't repeat it. However it makes the code a little confusing and interferes with changes that are planned for directory locking. So rearrange the code to ensure fh_unlock() is called exactly once if fh_lock() was called. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/vfs.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)