From: Amir Goldstein <amir73il@gmail.com>
To: NeilBrown <neil@brown.name>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Alexander Viro <viro@zeniv.linux.org.uk>,
Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
Jeff Layton <jlayton@kernel.org>,
Trond Myklebust <trondmy@kernel.org>,
Anna Schumaker <anna@kernel.org>,
Miklos Szeredi <miklos@szeredi.hu>, Jeremy Kerr <jk@ozlabs.org>,
Ard Biesheuvel <ardb@kernel.org>,
linux-efi@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-nfs@vger.kernel.org, linux-unionfs@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 08/19] VFS/xfs/ntfs: drop parent lock across d_alloc_parallel() in d_add_ci()
Date: Mon, 27 Apr 2026 09:49:36 +0200 [thread overview]
Message-ID: <CAOQ4uxjj6iJNnwUqV+cRtqo4qRD4v-f-ct92=ofCZWPDATXM7g@mail.gmail.com> (raw)
In-Reply-To: <20260427040517.828226-9-neilb@ownmail.net>
On Mon, Apr 27, 2026 at 6:07 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> A proposed change will invert the lock ordering between
> d_alloc_parallel() and inode_lock() on the parent.
> When that happens it will not be safe to call d_alloc_parallel() while
> holding the parent lock - even shared.
>
> We don't need to keep the parent lock held when d_add_ci() is run - the
> VFS doesn't need it as dentry is exclusively held due to
> DCACHE_PAR_LOOKUP and the filesystem has finished its work.
>
> So drop and reclaim the lock (shared or exclusive as determined by
> LOOKUP_SHARED) to avoid future deadlock.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/dcache.c | 18 +++++++++++++++++-
> fs/ntfs/namei.c | 4 +++-
> fs/xfs/xfs_iops.c | 3 ++-
> include/linux/dcache.h | 3 ++-
> 4 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 569a8ddf4c0d..a2ddfe811df3 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2294,6 +2294,7 @@ EXPORT_SYMBOL(d_obtain_root);
> * @dentry: the negative dentry that was passed to the parent's lookup func
> * @inode: the inode case-insensitive lookup has found
> * @name: the case-exact name to be associated with the returned dentry
> + * @bool: %true if lookup was performed with LOOKUP_SHARED
I do not understand the choice of API.
Seems better to pass the lookup flags and avoid exposing this very internal
logic in the call sites...
> *
> * This is to avoid filling the dcache with case-insensitive names to the
> * same inode, only the actual correct case is stored in the dcache for
> @@ -2306,7 +2307,7 @@ EXPORT_SYMBOL(d_obtain_root);
> * the exact case, and return the spliced entry.
> */
> struct dentry *d_add_ci(struct dentry *dentry, struct inode *inode,
> - struct qstr *name)
> + struct qstr *name, bool shared)
> {
> struct dentry *found, *res;
>
> @@ -2319,6 +2320,17 @@ struct dentry *d_add_ci(struct dentry *dentry, struct inode *inode,
> iput(inode);
> return found;
> }
> + /*
> + * We are holding parent lock and so don't want to wait for a
> + * d_in_lookup() dentry. We can safely drop the parent lock and
> + * reclaim it as we have exclusive access to dentry as it is
> + * d_in_lookup() (so ->d_parent is stable) and we are near the
> + * end ->lookup() and will shortly drop the lock anyway.
> + */
> + if (shared)
> + inode_unlock_shared(d_inode(dentry->d_parent));
> + else
> + inode_unlock(d_inode(dentry->d_parent));
> if (d_in_lookup(dentry)) {
> found = d_alloc_parallel(dentry->d_parent, name);
> if (IS_ERR(found) || !d_in_lookup(found)) {
> @@ -2332,6 +2344,10 @@ struct dentry *d_add_ci(struct dentry *dentry, struct inode *inode,
> return ERR_PTR(-ENOMEM);
> }
> }
> + if (shared)
> + inode_lock_shared(d_inode(dentry->d_parent));
> + else
> + inode_lock_nested(d_inode(dentry->d_parent), I_MUTEX_PARENT);
> res = d_splice_alias(inode, found);
> if (res) {
> d_lookup_done(found);
> diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
> index 10894de519c3..30dddef43300 100644
> --- a/fs/ntfs/namei.c
> +++ b/fs/ntfs/namei.c
> @@ -8,6 +8,7 @@
>
> #include <linux/exportfs.h>
> #include <linux/iversion.h>
> +#include <linux/namei.h> // for LOOKUP_SHARED
... this won't be needed
>
> #include "ntfs.h"
> #include "time.h"
> @@ -310,7 +311,8 @@ static struct dentry *ntfs_lookup(struct inode *dir_ino, struct dentry *dent,
> }
> nls_name.hash = full_name_hash(dent, nls_name.name, nls_name.len);
>
> - dent = d_add_ci(dent, dent_inode, &nls_name);
> + dent = d_add_ci(dent, dent_inode, &nls_name,
> + !!(flags & LOOKUP_SHARED));
> kfree(nls_name.name);
> return dent;
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 325c2200c501..f03d832f1468 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -35,6 +35,7 @@
> #include <linux/security.h>
> #include <linux/iversion.h>
> #include <linux/fiemap.h>
> +#include <linux/namei.h> // for LOOKUP_SHARED
>
> /*
> * Directories have different lock order w.r.t. mmap_lock compared to regular
> @@ -369,7 +370,7 @@ xfs_vn_ci_lookup(
> /* else case-insensitive match... */
> dname.name = ci_name.name;
> dname.len = ci_name.len;
> - dentry = d_add_ci(dentry, VFS_I(ip), &dname);
> + dentry = d_add_ci(dentry, VFS_I(ip), &dname, !!(flags & LOOKUP_SHARED));
... and this ugliness could be avoided.
Thanks,
Amir.
next prev parent reply other threads:[~2026-04-27 7:49 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-27 4:01 [PATCH v3 00/19] Prepare to lift lookup out of exclusive lock for directory ops NeilBrown
2026-04-27 4:01 ` [PATCH v3 01/19] VFS: fix various typos in documentation for start_creating start_removing etc NeilBrown
2026-04-27 4:01 ` [PATCH v3 02/19] VFS: enhance d_splice_alias() to handle in-lookup dentries NeilBrown
2026-04-27 4:01 ` [PATCH v3 03/19] VFS: allow d_alloc_name() to be used with ->d_hash NeilBrown
2026-04-28 2:10 ` Al Viro
2026-04-29 2:44 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 04/19] VFS: use wait_var_event for waiting in d_alloc_parallel() NeilBrown
2026-04-28 3:37 ` Al Viro
2026-04-28 11:18 ` NeilBrown
2026-04-28 14:22 ` Al Viro
2026-04-28 23:26 ` NeilBrown
2026-04-29 5:26 ` Al Viro
2026-04-29 17:07 ` Al Viro
2026-04-29 21:03 ` Linus Torvalds
2026-04-30 23:51 ` NeilBrown
2026-05-01 1:11 ` Al Viro
2026-05-01 1:39 ` NeilBrown
2026-05-01 1:45 ` NeilBrown
2026-05-01 3:37 ` Al Viro
2026-05-01 10:46 ` NeilBrown
2026-05-01 1:20 ` NeilBrown
2026-04-28 16:32 ` Linus Torvalds
2026-04-27 4:01 ` [PATCH v3 05/19] VFS: introduce d_alloc_noblock() NeilBrown
2026-04-28 2:22 ` Al Viro
2026-04-28 11:24 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 06/19] VFS: add d_duplicate() NeilBrown
2026-04-27 4:01 ` [PATCH v3 07/19] VFS: Add LOOKUP_SHARED flag NeilBrown
2026-04-27 7:43 ` Amir Goldstein
2026-04-27 8:47 ` NeilBrown
2026-04-27 9:05 ` Amir Goldstein
2026-04-27 23:51 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 08/19] VFS/xfs/ntfs: drop parent lock across d_alloc_parallel() in d_add_ci() NeilBrown
2026-04-27 7:49 ` Amir Goldstein [this message]
2026-04-27 8:48 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 09/19] ovl: stop using lookup_one() in iterate_shared() handling NeilBrown
2026-04-27 10:10 ` Amir Goldstein
2026-04-28 0:24 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 10/19] VFS/ovl: add d_alloc_noblock_return() NeilBrown
2026-04-27 9:40 ` Amir Goldstein
2026-04-28 0:34 ` NeilBrown
2026-04-28 4:35 ` Al Viro
2026-04-28 11:44 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 11/19] efivarfs: use d_alloc_name() NeilBrown
2026-04-27 4:01 ` [PATCH v3 12/19] shmem: use d_duplicate() NeilBrown
2026-04-27 4:01 ` [PATCH v3 13/19] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open() NeilBrown
2026-04-27 4:01 ` [PATCH v3 14/19] nfs: use d_splice_alias() in nfs_link() NeilBrown
2026-04-27 4:01 ` [PATCH v3 15/19] nfs: don't d_drop() before d_splice_alias() NeilBrown
2026-04-27 4:01 ` [PATCH v3 16/19] nfs: don't d_drop() before d_splice_alias() in atomic_create NeilBrown
2026-04-27 4:01 ` [PATCH v3 17/19] nfs: Use d_alloc_noblock() in nfs_prime_dcache() NeilBrown
2026-04-27 4:01 ` [PATCH v3 18/19] nfs: use d_alloc_noblock() in silly-rename NeilBrown
2026-04-27 4:01 ` [PATCH v3 19/19] nfs: use d_duplicate() NeilBrown
2026-04-27 8:42 ` [syzbot ci] Re: Prepare to lift lookup out of exclusive lock for directory ops syzbot ci
2026-04-28 23:16 ` 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='CAOQ4uxjj6iJNnwUqV+cRtqo4qRD4v-f-ct92=ofCZWPDATXM7g@mail.gmail.com' \
--to=amir73il@gmail.com \
--cc=anna@kernel.org \
--cc=ardb@kernel.org \
--cc=brauner@kernel.org \
--cc=jack@suse.cz \
--cc=jk@ozlabs.org \
--cc=jlayton@kernel.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=neil@brown.name \
--cc=torvalds@linux-foundation.org \
--cc=trondmy@kernel.org \
--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 read-only IMAP folder(s) and NNTP newsgroup(s).