> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@xxxxxxx> wrote:
>
> When creating or unlinking a name in a directory use explicit
> inode_lock_nested() instead of fh_lock(), and explicit calls to
> fh_fill_pre_attrs() and fh_fill_post_attrs(). This is already done for
> renames.
IIUC, the antecedent of "This is already done" is only "explicit
calls to fh_fill_pre_attrs() and fh_fill_post_attrs()" ?
>
> Also move the 'fill' calls closer to the operation that might change the
> attributes. This way they are avoided on some error paths.
>
> For the v2-only code in nfsproc.c, drop the fill calls as they aren't
> needed.
This feels like 3 independent changes to me. At least the v2 change
should be moved to a separate patch. Relocating the "fill attrs" calls
seems like it could cause noticeable behavior changes, so maybe it
belongs also in a separate patch?
> Having the locking explicit will simplify proposed future changes to
^Having^Making ?
> locking for directories. It also makes it easily visible exactly where
> pre/post attributes are used - not all callers of fh_lock() actually
> need the pre/post attributes.
>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/nfs3proc.c | 6 ++++--
> fs/nfsd/nfs4proc.c | 6 ++++--
> fs/nfsd/nfsproc.c | 5 ++---
> fs/nfsd/vfs.c | 30 +++++++++++++++++++-----------
> 4 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 774e4a2ab9b1..c2f992b4387a 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -256,7 +256,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (host_err)
> return nfserrno(host_err);
>
> - fh_lock_nested(fhp, I_MUTEX_PARENT);
> + inode_lock_nested(inode, I_MUTEX_PARENT);
>
> child = lookup_one_len(argp->name, parent, argp->len);
> if (IS_ERR(child)) {
> @@ -314,11 +314,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (!IS_POSIXACL(inode))
> iap->ia_mode &= ~current_umask();
>
> + fh_fill_pre_attrs(fhp);
> host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true);
> if (host_err < 0) {
> status = nfserrno(host_err);
> goto out;
> }
> + fh_fill_post_attrs(fhp);
>
> /* A newly created file already has a file size of zero. */
> if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
> @@ -336,7 +338,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
>
> out:
> - fh_unlock(fhp);
> + inode_unlock(inode);
> if (child && !IS_ERR(child))
> dput(child);
> fh_drop_write(fhp);
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 48e4efb39a9c..90af82d49119 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -264,7 +264,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (is_create_with_attrs(open))
> nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
>
> - fh_lock_nested(fhp, I_MUTEX_PARENT);
> + inode_lock_nested(inode, I_MUTEX_PARENT);
>
> child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> if (IS_ERR(child)) {
> @@ -348,10 +348,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (!IS_POSIXACL(inode))
> iap->ia_mode &= ~current_umask();
>
> + fh_fill_pre_attrs(fhp);
> status = nfsd4_vfs_create(fhp, child, open);
> if (status != nfs_ok)
> goto out;
> open->op_created = true;
> + fh_fill_post_attrs(fhp);
>
> /* A newly created file already has a file size of zero. */
> if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
> @@ -373,7 +375,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (attrs.acl_failed)
> open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
> out:
> - fh_unlock(fhp);
> + inode_unlock(inode);
> nfsd_attrs_free(&attrs);
> if (child && !IS_ERR(child))
> dput(child);
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index d09d516188d2..4cff332f58bb 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -287,7 +287,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> goto done;
> }
>
> - fh_lock_nested(dirfhp, I_MUTEX_PARENT);
> + inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
> dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
> if (IS_ERR(dchild)) {
> resp->status = nfserrno(PTR_ERR(dchild));
> @@ -403,8 +403,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> }
>
> out_unlock:
> - /* We don't really need to unlock, as fh_put does it. */
> - fh_unlock(dirfhp);
> + inode_unlock(dirfhp->fh_dentry->d_inode);
> fh_drop_write(dirfhp);
> done:
> fh_put(dirfhp);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 8ebad4a99552..f2cb9b047766 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1369,7 +1369,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (host_err)
> return nfserrno(host_err);
>
> - fh_lock_nested(fhp, I_MUTEX_PARENT);
> + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> dchild = lookup_one_len(fname, dentry, flen);
> host_err = PTR_ERR(dchild);
> if (IS_ERR(dchild)) {
> @@ -1384,10 +1384,12 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> dput(dchild);
> if (err)
> goto out_unlock;
> + fh_fill_pre_attrs(fhp);
> err = nfsd_create_locked(rqstp, fhp, fname, flen, attrs, type,
> rdev, resfhp);
> + fh_fill_post_attrs(fhp);
> out_unlock:
> - fh_unlock(fhp);
> + inode_unlock(dentry->d_inode);
> return err;
> }
>
> @@ -1460,20 +1462,22 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> goto out;
> }
>
> - fh_lock(fhp);
> dentry = fhp->fh_dentry;
> + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> dnew = lookup_one_len(fname, dentry, flen);
> if (IS_ERR(dnew)) {
> err = nfserrno(PTR_ERR(dnew));
> - fh_unlock(fhp);
> + inode_unlock(dentry->d_inode);
> goto out_drop_write;
> }
> + fh_fill_pre_attrs(fhp);
> host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> err = nfserrno(host_err);
> cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> if (!err)
> nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> - fh_unlock(fhp);
> + fh_fill_post_attrs(fhp);
> + inode_unlock(dentry->d_inode);
> if (!err)
> err = nfserrno(commit_metadata(fhp));
> dput(dnew);
> @@ -1519,9 +1523,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> goto out;
> }
>
> - fh_lock_nested(ffhp, I_MUTEX_PARENT);
> ddir = ffhp->fh_dentry;
> dirp = d_inode(ddir);
> + inode_lock_nested(dirp, I_MUTEX_PARENT);
>
> dnew = lookup_one_len(name, ddir, len);
> if (IS_ERR(dnew)) {
> @@ -1534,8 +1538,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> err = nfserr_noent;
> if (d_really_is_negative(dold))
> goto out_dput;
> + fh_fill_pre_attrs(ffhp);
> host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL);
> - fh_unlock(ffhp);
> + fh_fill_post_attrs(ffhp);
> + inode_unlock(dirp);
> if (!host_err) {
> err = nfserrno(commit_metadata(ffhp));
> if (!err)
> @@ -1555,7 +1561,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> out_dput:
> dput(dnew);
> out_unlock:
> - fh_unlock(ffhp);
> + inode_unlock(dirp);
> goto out_drop_write;
> }
>
> @@ -1730,9 +1736,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> if (host_err)
> goto out_nfserr;
>
> - fh_lock_nested(fhp, I_MUTEX_PARENT);
> dentry = fhp->fh_dentry;
> dirp = d_inode(dentry);
> + inode_lock_nested(dirp, I_MUTEX_PARENT);
>
> rdentry = lookup_one_len(fname, dentry, flen);
> host_err = PTR_ERR(rdentry);
> @@ -1750,6 +1756,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> if (!type)
> type = d_inode(rdentry)->i_mode & S_IFMT;
>
> + fh_fill_pre_attrs(fhp);
> if (type != S_IFDIR) {
> if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK)
> nfsd_close_cached_files(rdentry);
> @@ -1757,8 +1764,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> } else {
> host_err = vfs_rmdir(&init_user_ns, dirp, rdentry);
> }
> + fh_fill_post_attrs(fhp);
>
> - fh_unlock(fhp);
> + inode_unlock(dirp);
> if (!host_err)
> host_err = commit_metadata(fhp);
> dput(rdentry);
> @@ -1781,7 +1789,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> out:
> return err;
> out_unlock:
> - fh_unlock(fhp);
> + inode_unlock(dirp);
> goto out_drop_write;
> }
>
>
>
--
Chuck Lever
