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

On Wed, Apr 29, 2026 at 09:26:54AM +1000, NeilBrown wrote:

> > More to the point, why not set DCACHE_LOCK_WAITER as soon as we grab ->d_lock
> > there?  Then waiting becomes simply "wait until !d_in_lookup()".  Sure, we
> > might end up setting DCACHE_LOCK_WAITER on a dentry that has just dropped
> > DCACHE_PAR_LOOKUP - who cares?
> > 
> > While we are at it, do we need to drop it when we clear PAR_LOOKUP?  Because
> > if we do not, the whole "what do we return from __d_lookup_unhash()" thing
> > disappears - we simply pass the dentry to end_dir_add() and have it check
> > ->d_flags & DCACHE_LOCK_WAITER to decide whether to bother with wakeup.
> > 
> 
> Yes, your are right.
> 
> I've been thinking of this mostly in the context of locking the dentry for
> directory ops, for which lookup is just one special case.
> In that context the dentry can be locked and unlocked multiple time so
> we really want to clear DCACHE_LOCK_WAITERS on each unlock.
> 
> However in the current code it is only used for lookup which only
> happens once on a given dentry so we never need to clear
> DCACHE_LOCK_WAITERS.
> 
> On the basis that we shouldn't add complexity until we actually need it,
> I'll rename DCACHE_LOCK_WAITERS to DCACHE_LOOKUP_WAITERS and never clear
> it, as you suggest.

Alternative variant (and I'm pretty sure that it will generate good code)
would be this:

static inline void d_wait_lookup(struct dentry *dentry)
{
        if (likely(d_in_lookup(dentry)) {
		dentry->d_flags |= DCACHE_LOOKUP_WAITERS;
		wait_var_event_spinlock(&dentry->d_flags,
					!d_in_lookup(dentry),
					&dentry->d_lock);
	}
}

In __d_lookup_unhash(): just don't return anything and lose the parts
related to ->d_wait (including ->d_lru initialization - same as in your
patch, for the same reason).  Similar to your variant, except that
DCACHE_LOOKUP_WAITERS is *not* cleared.  Or checked, for that matter -
you only do that to find the return value.

In d_alloc_parallel(): lose the 'wq' argument, along with the store
to ->d_wait.

Add
// must be in the same ->d_lock scope as __d_lookup_unhash()
static inline void __d_wake_in_lookup_waiters(struct dentry *dentry)
{
	if (dentry->d_flags & DCACHE_LOOKUP_WAITERS) {
		wake_up_var_locked(&dentry->d_flags, &dentry->d_lock);
		dentry->d_flags &= ~DCACHE_LOOKUP_WAITERS;
	}
}

and have
void __d_lookup_unhash_wake(struct dentry *dentry)
{
        spin_lock(&dentry->d_lock);
	__d_lookup_unhash(dentry);
	__d_wake_in_lookup_waiters(dentry);
	spin_unlock(&dentry->d_lock);
}

static inline void end_dir_add(struct inode *dir, unsigned int n,
                               struct dentry *dentry)
{
	smp_store_release(&dir->i_dir_seq, n + 2);
	preempt_enable_nested();
	__d_wake_in_lookup_waiters(dentry);
}

with obvious adjustments in end_dir_add().  That's it.  Outside of fs/dcache.c,
same as in the patch you've posted, modulo renaming you've suggested for new flag.

That yields the same semantics for flags as your variant does (DCACHE_LOOKUP_WAITERS
may be present only along with DCACHE_PAR_LOOKUP), and fs/dcache.c part is more
straightforward that way, IMO.