From: "John Stoffel" <john@stoffel.org>
To: NeilBrown <neilb@suse.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
Linus Torvalds <torvalds@linux-foundation.org>,
Daire Byrne <daire@dneg.com>,
Trond Myklebust <trond.myklebust@hammerspace.com>,
Chuck Lever <chuck.lever@oracle.com>,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
linux-fsdevel@vger.kernel.org,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 10/10] NFS: support parallel updates in the one directory.
Date: Fri, 26 Aug 2022 11:31:33 -0400 [thread overview]
Message-ID: <25352.59221.528023.534884@quad.stoffel.home> (raw)
In-Reply-To: <166147984378.25420.4023980607067991846.stgit@noble.brown>
>>>>> "NeilBrown" == NeilBrown <neilb@suse.de> writes:
NeilBrown> NFS can easily support parallel updates as the locking is done on the
NeilBrown> server, so this patch enables parallel updates for NFS.
Just curious, how is this handled if I have a server with an EXT#
filesystem which is exported via NFS to multiple clients. If those
NFS clients are doing lots of changes in a single directory, I can see
how the NFS server handles that, but what about if at the same time
someone does multiple changes on the server in that EXT# behind the
NFSd's back, how are the conflicts handled?
It would seem that this all really needs to purely happen at the VFS
layer, but I'm probably missing something.
I ask this because my home server exports /home to a couple of other
systems via NFS, but I still work in /home on the server.
NeilBrown> Handling of silly-rename - both for unlink and for the rename target -
NeilBrown> requires some care as we need to get an exclusive lock on the chosen
NeilBrown> silly name and for rename we need to keep the original target name
NeilBrown> locked after it has been renamed to the silly name.
NeilBrown> So nfs_sillyrename() now uses d_exchange() to swap the target and the
NeilBrown> silly name after the silly-rename has happened on the server, and the
NeilBrown> silly dentry - which now has the name of the target - is returned.
NeilBrown> For unlink(), this is immediately unlocked and discarded with a call to
NeilBrown> nfs_sillyrename_finish(). For rename it is kept locked until the
NeilBrown> originally requested rename completes.
NeilBrown> Signed-off-by: NeilBrown <neilb@suse.de>
NeilBrown> ---
NeilBrown> fs/dcache.c | 5 ++++-
NeilBrown> fs/nfs/dir.c | 28 ++++++++++++++++------------
NeilBrown> fs/nfs/fs_context.c | 6 ++++--
NeilBrown> fs/nfs/internal.h | 3 ++-
NeilBrown> fs/nfs/unlink.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
NeilBrown> 5 files changed, 64 insertions(+), 29 deletions(-)
NeilBrown> diff --git a/fs/dcache.c b/fs/dcache.c
NeilBrown> index 9bf346a9de52..a5eaab16d39f 100644
NeilBrown> --- a/fs/dcache.c
NeilBrown> +++ b/fs/dcache.c
NeilBrown> @@ -3056,7 +3056,9 @@ void d_exchange(struct dentry *dentry1, struct dentry *dentry2)
NeilBrown> write_seqlock(&rename_lock);
NeilBrown> WARN_ON(!dentry1->d_inode);
NeilBrown> - WARN_ON(!dentry2->d_inode);
NeilBrown> + /* Allow dentry2 to be negative, so we can do a rename
NeilBrown> + * but keep both names locked (DCACHE_PAR_UPDATE)
NeilBrown> + */
NeilBrown> WARN_ON(IS_ROOT(dentry1));
NeilBrown> WARN_ON(IS_ROOT(dentry2));
NeilBrown> @@ -3064,6 +3066,7 @@ void d_exchange(struct dentry *dentry1, struct dentry *dentry2)
NeilBrown> write_sequnlock(&rename_lock);
NeilBrown> }
NeilBrown> +EXPORT_SYMBOL(d_exchange);
NeilBrown> /**
NeilBrown> * d_ancestor - search for an ancestor
NeilBrown> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
NeilBrown> index 5d6c2ddc7ea6..fbb608fbe6bf 100644
NeilBrown> --- a/fs/nfs/dir.c
NeilBrown> +++ b/fs/nfs/dir.c
NeilBrown> @@ -1935,8 +1935,12 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
NeilBrown> /*
NeilBrown> * If we're doing an exclusive create, optimize away the lookup
NeilBrown> * but don't hash the dentry.
NeilBrown> + * A silly_rename is marked exclusive, but we need to do an
NeilBrown> + * explicit lookup.
NeilBrown> */
NeilBrown> - if (nfs_is_exclusive_create(dir, flags) || flags & LOOKUP_RENAME_TARGET)
NeilBrown> + if ((nfs_is_exclusive_create(dir, flags) ||
NeilBrown> + flags & LOOKUP_RENAME_TARGET) &&
NeilBrown> + !(flags & LOOKUP_SILLY_RENAME))
NeilBrown> return NULL;
NeilBrown> res = ERR_PTR(-ENOMEM);
NeilBrown> @@ -2472,10 +2476,14 @@ int nfs_unlink(struct inode *dir, struct dentry *dentry)
NeilBrown> spin_lock(&dentry->d_lock);
NeilBrown> if (d_count(dentry) > 1 && !test_bit(NFS_INO_PRESERVE_UNLINKED,
NeilBrown> &NFS_I(d_inode(dentry))->flags)) {
NeilBrown> + struct dentry *silly;
NeilBrown> +
NeilBrown> spin_unlock(&dentry->d_lock);
NeilBrown> /* Start asynchronous writeout of the inode */
NeilBrown> write_inode_now(d_inode(dentry), 0);
NeilBrown> - error = nfs_sillyrename(dir, dentry);
NeilBrown> + silly = nfs_sillyrename(dir, dentry);
NeilBrown> + error = PTR_ERR_OR_ZERO(silly);
NeilBrown> + nfs_sillyrename_finish(dir, silly);
NeilBrown> goto out;
NeilBrown> }
NeilBrown> /* We must prevent any concurrent open until the unlink
NeilBrown> @@ -2685,16 +2693,12 @@ int nfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
NeilBrown> spin_unlock(&new_dentry->d_lock);
NeilBrown> - /* copy the target dentry's name */
NeilBrown> - dentry = d_alloc(new_dentry->d_parent,
NeilBrown> - &new_dentry->d_name);
NeilBrown> - if (!dentry)
NeilBrown> - goto out;
NeilBrown> -
NeilBrown> /* silly-rename the existing target ... */
NeilBrown> - err = nfs_sillyrename(new_dir, new_dentry);
NeilBrown> - if (err)
NeilBrown> + dentry = nfs_sillyrename(new_dir, new_dentry);
NeilBrown> + if (IS_ERR(dentry)) {
NeilBrown> + err = PTR_ERR(dentry);
NeilBrown> goto out;
NeilBrown> + }
NeilBrown> new_dentry = dentry;
NeilBrown> new_inode = NULL;
NeilBrown> @@ -2750,9 +2754,9 @@ int nfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
NeilBrown> } else if (error == -ENOENT)
NeilBrown> nfs_dentry_handle_enoent(old_dentry);
NeilBrown> - /* new dentry created? */
NeilBrown> if (dentry)
NeilBrown> - dput(dentry);
NeilBrown> + nfs_sillyrename_finish(new_dir, dentry);
NeilBrown> +
NeilBrown> return error;
NeilBrown> }
NeilBrown> EXPORT_SYMBOL_GPL(nfs_rename);
NeilBrown> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
NeilBrown> index 4da701fd1424..7133ca9433d2 100644
NeilBrown> --- a/fs/nfs/fs_context.c
NeilBrown> +++ b/fs/nfs/fs_context.c
NeilBrown> @@ -1577,7 +1577,8 @@ struct file_system_type nfs_fs_type = {
NeilBrown> .init_fs_context = nfs_init_fs_context,
NeilBrown> .parameters = nfs_fs_parameters,
NeilBrown> .kill_sb = nfs_kill_super,
NeilBrown> - .fs_flags = FS_RENAME_DOES_D_MOVE|FS_BINARY_MOUNTDATA,
NeilBrown> + .fs_flags = FS_RENAME_DOES_D_MOVE|FS_BINARY_MOUNTDATA|
NeilBrown> + FS_PAR_DIR_UPDATE,
NeilBrown> };
NeilBrown> MODULE_ALIAS_FS("nfs");
NeilBrown> EXPORT_SYMBOL_GPL(nfs_fs_type);
NeilBrown> @@ -1589,7 +1590,8 @@ struct file_system_type nfs4_fs_type = {
NeilBrown> .init_fs_context = nfs_init_fs_context,
NeilBrown> .parameters = nfs_fs_parameters,
NeilBrown> .kill_sb = nfs_kill_super,
NeilBrown> - .fs_flags = FS_RENAME_DOES_D_MOVE|FS_BINARY_MOUNTDATA,
NeilBrown> + .fs_flags = FS_RENAME_DOES_D_MOVE|FS_BINARY_MOUNTDATA|
NeilBrown> + FS_PAR_DIR_UPDATE,
NeilBrown> };
NeilBrown> MODULE_ALIAS_FS("nfs4");
NeilBrown> MODULE_ALIAS("nfs4");
NeilBrown> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
NeilBrown> index 27c720d71b4e..3a7fd30a8e29 100644
NeilBrown> --- a/fs/nfs/internal.h
NeilBrown> +++ b/fs/nfs/internal.h
NeilBrown> @@ -611,7 +611,8 @@ extern struct rpc_task *
NeilBrown> nfs_async_rename(struct inode *old_dir, struct inode *new_dir,
NeilBrown> struct dentry *old_dentry, struct dentry *new_dentry,
NeilBrown> void (*complete)(struct rpc_task *, struct nfs_renamedata *));
NeilBrown> -extern int nfs_sillyrename(struct inode *dir, struct dentry *dentry);
NeilBrown> +extern struct dentry *nfs_sillyrename(struct inode *dir, struct dentry *dentry);
NeilBrown> +extern void nfs_sillyrename_finish(struct inode *dir, struct dentry *dentry);
NeilBrown> /* direct.c */
NeilBrown> void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
NeilBrown> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
NeilBrown> index 9697cd5d2561..c8a718f09fe6 100644
NeilBrown> --- a/fs/nfs/unlink.c
NeilBrown> +++ b/fs/nfs/unlink.c
NeilBrown> @@ -428,6 +428,10 @@ nfs_complete_sillyrename(struct rpc_task *task, struct nfs_renamedata *data)
NeilBrown> *
NeilBrown> * The final cleanup is done during dentry_iput.
NeilBrown> *
NeilBrown> + * We exchange the original with the new (silly) dentries, and return
NeilBrown> + * the new dentry which will have the original name. This ensures that
NeilBrown> + * the target name remains locked until the rename completes.
NeilBrown> + *
NeilBrown> * (Note: NFSv4 is stateful, and has opens, so in theory an NFSv4 server
NeilBrown> * could take responsibility for keeping open files referenced. The server
NeilBrown> * would also need to ensure that opened-but-deleted files were kept over
NeilBrown> @@ -436,7 +440,7 @@ nfs_complete_sillyrename(struct rpc_task *task, struct nfs_renamedata *data)
NeilBrown> * use to advertise that it does this; some day we may take advantage of
NeilBrown> * it.))
NeilBrown> */
NeilBrown> -int
NeilBrown> +struct dentry *
NeilBrown> nfs_sillyrename(struct inode *dir, struct dentry *dentry)
NeilBrown> {
NeilBrown> static unsigned int sillycounter;
NeilBrown> @@ -445,6 +449,8 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
NeilBrown> struct dentry *sdentry;
NeilBrown> struct inode *inode = d_inode(dentry);
NeilBrown> struct rpc_task *task;
NeilBrown> + DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
NeilBrown> + struct path path = {};
NeilBrown> int error = -EBUSY;
NeilBrown> dfprintk(VFS, "NFS: silly-rename(%pd2, ct=%d)\n",
NeilBrown> @@ -459,10 +465,11 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
NeilBrown> fileid = NFS_FILEID(d_inode(dentry));
NeilBrown> + path.dentry = d_find_alias(dir);
NeilBrown> sdentry = NULL;
NeilBrown> do {
NeilBrown> int slen;
NeilBrown> - dput(sdentry);
NeilBrown> +
NeilBrown> sillycounter++;
NeilBrown> slen = scnprintf(silly, sizeof(silly),
NeilBrown> SILLYNAME_PREFIX "%0*llx%0*x",
NeilBrown> @@ -472,14 +479,19 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
NeilBrown> dfprintk(VFS, "NFS: trying to rename %pd to %s\n",
NeilBrown> dentry, silly);
NeilBrown> - sdentry = lookup_one_len(silly, dentry->d_parent, slen);
NeilBrown> - /*
NeilBrown> - * N.B. Better to return EBUSY here ... it could be
NeilBrown> - * dangerous to delete the file while it's in use.
NeilBrown> - */
NeilBrown> - if (IS_ERR(sdentry))
NeilBrown> - goto out;
NeilBrown> - } while (d_inode(sdentry) != NULL); /* need negative lookup */
NeilBrown> + sdentry = filename_create_one_len(silly, slen,
NeilBrown> + &path,
NeilBrown> + LOOKUP_CREATE | LOOKUP_EXCL |
NeilBrown> + LOOKUP_SILLY_RENAME,
NeilBrown> + &wq);
NeilBrown> + } while (PTR_ERR_OR_ZERO(sdentry) == -EEXIST);
NeilBrown> + dput(path.dentry);
NeilBrown> + /*
NeilBrown> + * N.B. Better to return EBUSY here ... it could be
NeilBrown> + * dangerous to delete the file while it's in use.
NeilBrown> + */
NeilBrown> + if (IS_ERR(sdentry))
NeilBrown> + goto out;
NeilBrown> ihold(inode);
NeilBrown> @@ -513,7 +525,7 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
NeilBrown> NFS_INO_INVALID_CTIME |
NeilBrown> NFS_INO_REVAL_FORCED);
NeilBrown> spin_unlock(&inode->i_lock);
NeilBrown> - d_move(dentry, sdentry);
NeilBrown> + d_exchange(dentry, sdentry);
NeilBrown> break;
NeilBrown> case -ERESTARTSYS:
NeilBrown> /* The result of the rename is unknown. Play it safe by
NeilBrown> @@ -524,7 +536,20 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
NeilBrown> rpc_put_task(task);
NeilBrown> out_dput:
NeilBrown> iput(inode);
NeilBrown> - dput(sdentry);
NeilBrown> + if (!error)
NeilBrown> + return sdentry;
NeilBrown> +
NeilBrown> + d_lookup_done(sdentry);
NeilBrown> + __done_path_update(&path, sdentry, true, LOOKUP_SILLY_RENAME);
NeilBrown> out:
NeilBrown> - return error;
NeilBrown> + return ERR_PTR(error);
NeilBrown> +}
NeilBrown> +
NeilBrown> +void nfs_sillyrename_finish(struct inode *dir, struct dentry *dentry)
NeilBrown> +{
NeilBrown> + struct path path = { .dentry = d_find_alias(dir) };
NeilBrown> +
NeilBrown> + if (!IS_ERR(dentry))
NeilBrown> + __done_path_update(&path, dentry, true, LOOKUP_SILLY_RENAME);
NeilBrown> + dput(path.dentry);
NeilBrown> }
next prev parent reply other threads:[~2022-08-26 15:31 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-26 2:10 [PATCH/RFC 00/10 v5] Improve scalability of directory operations NeilBrown
2022-08-26 2:10 ` [PATCH 09/10] VFS: add LOOKUP_SILLY_RENAME NeilBrown
2022-08-27 1:21 ` Al Viro
2022-08-29 3:15 ` NeilBrown
2022-08-26 2:10 ` [PATCH 01/10] VFS: support parallel updates in the one directory NeilBrown
2022-08-26 19:06 ` Linus Torvalds
2022-08-26 23:06 ` NeilBrown
2022-08-27 0:13 ` Linus Torvalds
2022-08-27 0:23 ` Al Viro
2022-08-27 21:14 ` Al Viro
2022-08-27 0:17 ` Al Viro
2022-09-01 0:31 ` NeilBrown
2022-09-01 3:44 ` Al Viro
2022-08-27 3:43 ` Al Viro
2022-08-29 1:59 ` NeilBrown
2022-09-03 0:06 ` Al Viro
2022-09-03 1:40 ` NeilBrown
2022-09-03 2:12 ` Al Viro
2022-09-03 17:52 ` Al Viro
2022-09-04 23:33 ` NeilBrown
2022-08-26 2:10 ` [PATCH 08/10] NFSD: allow parallel creates from nfsd NeilBrown
2022-08-27 4:37 ` Al Viro
2022-08-29 3:12 ` NeilBrown
2022-08-26 2:10 ` [PATCH 05/10] VFS: export done_path_update() NeilBrown
2022-08-26 2:10 ` [PATCH 02/10] VFS: move EEXIST and ENOENT tests into lookup_hash_update() NeilBrown
2022-08-26 2:10 ` [PATCH 06/10] VFS: support concurrent renames NeilBrown
2022-08-27 4:12 ` Al Viro
2022-08-29 3:08 ` NeilBrown
2022-08-26 2:10 ` [PATCH 10/10] NFS: support parallel updates in the one directory NeilBrown
2022-08-26 15:31 ` John Stoffel [this message]
2022-08-26 23:13 ` NeilBrown
2022-08-26 2:10 ` [PATCH 03/10] VFS: move want_write checks into lookup_hash_update() NeilBrown
2022-08-27 3:48 ` Al Viro
2022-08-26 2:10 ` [PATCH 04/10] VFS: move dput() and mnt_drop_write() into done_path_update() NeilBrown
2022-08-26 2:10 ` [PATCH 07/10] VFS: hold DCACHE_PAR_UPDATE lock across d_revalidate() NeilBrown
2022-08-26 14:42 ` [PATCH/RFC 00/10 v5] Improve scalability of directory operations John Stoffel
2022-08-26 23:30 ` NeilBrown
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=25352.59221.528023.534884@quad.stoffel.home \
--to=john@stoffel.org \
--cc=chuck.lever@oracle.com \
--cc=daire@dneg.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
--cc=torvalds@linux-foundation.org \
--cc=trond.myklebust@hammerspace.com \
--cc=viro@zeniv.linux.org.uk \
/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;
as well as URLs for NNTP newsgroup(s).