From: Jeff Layton <jlayton@kernel.org>
To: NeilBrown <neilb@suse.de>, Chuck Lever <chuck.lever@oracle.com>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH 6/8] NFSD: use explicit lock/unlock for directory ops
Date: Wed, 06 Jul 2022 10:05:05 -0400 [thread overview]
Message-ID: <77cbc2a5fc499066a2a7ee42c2ded2bb5e761080.camel@kernel.org> (raw)
In-Reply-To: <165708109259.1940.685583862810495747.stgit@noble.brown>
On Wed, 2022-07-06 at 14:18 +1000, NeilBrown 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.
>
> Also move the 'fill' calls closer to the operation that might change the
> attributes. This way they are avoided on some error paths.
>
> Having the locking explicit will simplify proposed future changes to
> 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.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs3proc.c | 6 ++++--
> fs/nfsd/nfs4proc.c | 6 ++++--
> fs/nfsd/nfsproc.c | 7 ++++---
> fs/nfsd/vfs.c | 30 +++++++++++++++++++-----------
> 4 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 3a67d0afb885..9629517344ff 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -254,7 +254,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)) {
> @@ -312,11 +312,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))
> @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
>
> 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 6ec22c69cbec..242f059e6788 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -306,7 +306,7 @@ nfsd4_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(open->op_fname, parent, open->op_fnamelen);
> if (IS_ERR(child)) {
> @@ -385,10 +385,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))
> @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
>
> out:
> - fh_unlock(fhp);
> + inode_unlock(inode);
> if (child && !IS_ERR(child))
> dput(child);
> fh_drop_write(fhp);
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index ed24fae09517..427c404bc52b 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -285,7 +285,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));
> @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> }
>
> resp->status = nfs_ok;
> + fh_fill_pre_attrs(dirfhp);
> if (!inode) {
> /* File doesn't exist. Create it and set attrs */
> resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name,
> @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> resp->status = nfsd_setattr(rqstp, newfhp, attr, 0,
> (time64_t)0);
> }
> + fh_fill_post_attrs(dirfhp);
>
> 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 8e050c6d112a..2ca748aa83bb 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1412,7 +1412,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)) {
> @@ -1427,12 +1427,14 @@ 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, iap, type,
> rdev, resfhp);
> if (!err && post_create)
> post_create(resfhp, data);
> + fh_fill_post_attrs(fhp);
> out_unlock:
> - fh_unlock(fhp);
> + inode_unlock(dentry->d_inode);
> return err;
> }
>
> @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (host_err)
> goto out_nfserr;
>
> - fh_lock(fhp);
> dentry = fhp->fh_dentry;
> + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> dnew = lookup_one_len(fname, dentry, flen);
> host_err = PTR_ERR(dnew);
> if (IS_ERR(dnew)) {
> - fh_unlock(fhp);
> + inode_unlock(dentry->d_inode);
> goto out_nfserr;
> }
> + fh_fill_pre_attrs(fhp);
> host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> err = nfserrno(host_err);
> if (!err)
> @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (err==0) err = cerr;
> if (!err && post_create)
> post_create(resfhp, data);
> - fh_unlock(fhp);
> + fh_fill_post_attrs(fhp);
> + inode_unlock(dentry->d_inode);
> out:
> return err;
>
> @@ -1569,9 +1573,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);
> host_err = PTR_ERR(dnew);
> @@ -1585,8 +1589,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)
> @@ -1606,7 +1612,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;
> }
>
> @@ -1781,9 +1787,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);
> @@ -1801,6 +1807,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);
> @@ -1808,8 +1815,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);
> @@ -1832,7 +1840,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;
> }
>
>
>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
next prev parent reply other threads:[~2022-07-06 14:05 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-06 4:18 [PATCH 0/8] NFSD: clean up locking NeilBrown
2022-07-06 4:18 ` [PATCH 6/8] NFSD: use explicit lock/unlock for directory ops NeilBrown
2022-07-06 14:05 ` Jeff Layton [this message]
2022-07-06 16:29 ` Chuck Lever III
2022-07-15 16:11 ` Jeff Layton
2022-07-15 18:22 ` Jeff Layton
2022-07-17 23:59 ` NeilBrown
2022-07-17 23:43 ` NeilBrown
2022-07-06 4:18 ` [PATCH 7/8] NFSD: use (un)lock_inode instead of fh_(un)lock for file operations NeilBrown
2022-07-06 14:10 ` Jeff Layton
2022-07-06 16:30 ` Chuck Lever III
2022-07-07 1:33 ` NeilBrown
2022-07-06 4:18 ` [PATCH 2/8] NFSD: change nfsd_create() to unlock directory before returning NeilBrown
2022-07-06 13:24 ` Jeff Layton
2022-07-06 16:29 ` Chuck Lever III
2022-07-06 4:18 ` [PATCH 8/8] NFSD: discard fh_locked flag and fh_lock/fh_unlock NeilBrown
2022-07-06 14:12 ` Jeff Layton
2022-07-06 4:18 ` [PATCH 4/8] NFSD: only call fh_unlock() once in nfsd_link() NeilBrown
2022-07-06 13:31 ` Jeff Layton
2022-07-06 16:29 ` Chuck Lever III
2022-07-06 4:18 ` [PATCH 1/8] NFSD: drop rqstp arg to do_set_nfs4_acl() NeilBrown
2022-07-06 13:17 ` Jeff Layton
2022-07-06 4:18 ` [PATCH 5/8] NFSD: reduce locking in nfsd_lookup() NeilBrown
2022-07-06 13:47 ` Jeff Layton
2022-07-07 1:26 ` NeilBrown
2022-07-06 16:29 ` Chuck Lever III
2022-07-07 1:29 ` NeilBrown
2022-07-06 4:18 ` [PATCH 3/8] NFSD: always drop directory lock in nfsd_unlink() NeilBrown
2022-07-06 13:30 ` Jeff Layton
2022-07-06 16:29 ` [PATCH 0/8] NFSD: clean up locking Chuck Lever III
2022-07-12 2:33 ` NeilBrown
2022-07-12 14:17 ` Chuck Lever III
2022-07-13 4:32 ` NeilBrown
2022-07-13 14:15 ` Chuck Lever III
2022-07-13 19:13 ` Jeff Layton
2022-07-14 14:32 ` Chuck Lever III
2022-07-15 2:36 ` NeilBrown
2022-07-15 13:01 ` Jeff Layton
2022-07-15 13:53 ` Jeff Layton
2022-07-15 14:06 ` Chuck Lever III
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=77cbc2a5fc499066a2a7ee42c2ded2bb5e761080.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox