Re: [PATCH v3 04/19] VFS: use wait_var_event for waiting in d_alloc_parallel()

Al Viro
2026-04-28T03:37:35Z

On Mon, Apr 27, 2026 at 02:01:22PM +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> d_alloc_parallel() currently requires a wait_queue_head to be passed in.
> This must have a life time which extends until the lookup is completed.
>
> This makes it awkward to use and particularly make it hard to use in
> lookup_one_qstr_excl() which I hope to do in the future.
>
> This patch changes d_alloc_parallel() to use wake_up_var_locked() to
> wake up waiters, and wait_var_event_spinlock() to wait. dentry->d_lock
> is used for synchronisation as it is already held and the relevant
> times.
>
> In most cases there will be no waiters so the wake_up_var_locked()
> call is a complete waste. To optimise this a new ->d_flags flag is
> added: DCACHE_LOCK_WAITERS. This is set whenever any thread prepares to
> wait for the dentry, and if it isn't set when DCACHE_PAR_LOOKUP is
> cleared, no wakeup is sent.
> (The name is deliberately generic as I plan to replace DCACHE_PAR_LOOKUP
> with more generic per-dentry locking in the future).
>
> __d_lookup_unhash() now returns a bool rather than a wq. This is true
> if DCACHE_LOCK_WAITERS was sent and is used to decide to send the wake
> up. It would be easier to send the wakeup immediately when clearing
> DCACHE_LOCK_WAITERS, but then the waiter could wake a bit earlier and
> then spend time spinning on ->d_lock. I don't know if that cost is
> interesting.

I definitely like the calling conventions change, so much that I'd be glad
to pick that one Right Fucking Now. I'd probably make the store in
d_must_wait() conditional, though - ->d_flags and ->d_lock are in different
cachelines and there's no need to dirty both every time we are called.
IOW, have d_must_wait() do this:
if (!d_in_lookup(dentry))
return false;
if (!(dentry->d_flags & DCACHE_LOCK_WAITER))
dentry->d_flags |= DCACHE_LOCK_WAITER;
return true;
Not sure if it would flow better with return values inverted (and
negation removed from wait_var_event_spinlock(), that is)...

> static inline void end_dir_add(struct inode *dir, unsigned int n,
> - wait_queue_head_t *d_wait)
> + bool do_wake, struct dentry *de)
> {
> smp_store_release(&dir->i_dir_seq, n + 2);
> preempt_enable_nested();
> - if (wq_has_sleeper(d_wait))
> - wake_up_all(d_wait);
> + if (do_wake)
> + wake_up_var_locked(&de->d_flags, &de->d_lock);
> }

This calling conventions change, OTOH, I don't like at all. I mean,
(dir, n, false, unused) vs. (dir, n, true, never_NULL) is seriously
asking to be reduced to (dir, n, NULL) vs. (dir, n, never_NULL).

> @@ -2800,29 +2793,29 @@ EXPORT_SYMBOL(d_alloc_parallel);
> * - Retrieve and clear the waitqueue head in dentry
> * - Return the waitqueue head

... not anymore. The entire comment needs replacement, TBH - 2 lines of
3 are stale with that patch and remaining one is... ambiguous, since
there are *two* hashes around and "Unhash the dentry" usually refers to
the other one.

Something like

/*
* Move dentry from in-lookup state to busy-negative one.
*
* From now on d_in_lookup(dentry) will return false and dentry is gone from
* in-lookup hash.
*
* Anyone who had been waiting on it in d_alloc_parallel() is free to
* proceed after that. Note that waking such waiters up is left to
* the callers; we might be called in write-side critical area for ->i_dir_seq,
* and PREEMPT_RT kernels can't have that wakeup done in those.
*
* Returns whether there are waiters to be woken up.
*/

perhaps?