Re: [PATCH 09/11] VFS: use global wait-queue table for d_alloc_parallel()

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Aug 12, 2025 at 12:25:12PM +1000, NeilBrown wrote:

> +** mandatory**
> +
> +d_alloc_parallel() no longer requires a waitqueue_head.  It uses one
> +from an internal table when needed.

Misleading, IMO - that sounds like "giving it a wq is optional, it will
pick one if needed" when reality is "calling conventions have changed,
no more passing it a waitqueue at all".

> +#define	PAR_LOOKUP_WQ_BITS	8
> +#define PAR_LOOKUP_WQS (1 << PAR_LOOKUP_WQ_BITS)
> +static wait_queue_head_t par_wait_table[PAR_LOOKUP_WQS] __cacheline_aligned;

I wonder how hot these cachelines will be...

> +static int __init par_wait_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < PAR_LOOKUP_WQS; i++)
> +		init_waitqueue_head(&par_wait_table[i]);
> +	return 0;
> +}
> +fs_initcall(par_wait_init);

Let's not open _that_ can of worms; just call it from dcache_init().

> +static inline void d_wake_waiters(struct wait_queue_head *d_wait,
> +				  struct dentry *dentry)
> +{
> +	/* ->d_wait is only set if some thread is actually waiting.
> +	 * If we find it is NULL - the common case - then there was no
> +	 * contention and there are no waiters to be woken.
> +	 */
> +	if (d_wait)
> +		__wake_up(d_wait, TASK_NORMAL, 0, dentry);

Might be worth a note re "this is wake_up_all(), except that key is dentry
rather than NULL" - or a helper in wait.h to that effect, for that matter.
I see several other places where we have the same thing (do_notify_pidfd(),
nfs4_callback_notify_lock(), etc.), so...


> +		struct wait_queue_head *wq;
> +		if (!dentry->d_wait)
> +			dentry->d_wait = &par_wait_table[hash_ptr(dentry,
> +								  PAR_LOOKUP_WQ_BITS)];
> +		wq = dentry->d_wait;

Yecchhh...  Cosmetic change: take
	&par_wait_table[hash_ptr(dentry, PAR_LOOKUP_WQ_BITS)];
into an inlined helper, please.

BTW, while we are at it - one change I have for that function is
(in the current form)
static bool d_wait_lookup(struct dentry *dentry,
			  struct dentry *parent,
			  const struct qstr *name)
{
	bool valid = true;
	spin_lock(&dentry->d_lock);
        if (d_in_lookup(dentry)) {
		DECLARE_WAITQUEUE(wait, current);
		add_wait_queue(dentry->d_wait, &wait);
		do {   
			set_current_state(TASK_UNINTERRUPTIBLE);
			spin_unlock(&dentry->d_lock);
			schedule();
			spin_lock(&dentry->d_lock);
		} while (d_in_lookup(dentry));
	}
	/*
	 * it's not in-lookup anymore; in principle the caller should repeat
	 * everything from dcache lookup, but it's likely to be what
	 * d_lookup() would've found anyway.  If so, they can use it as-is.
	 */
	if (unlikely(dentry->d_name.hash != name->hash ||
		     dentry->d_parent != parent ||
		     d_unhashed(dentry) ||
		     !d_same_name(dentry, parent, name)))
		valid = false;
	spin_unlock(&dentry->d_lock);
	return valid;
}

with
	if (unlikely(d_wait_lookup(dentry, parent, name))) {
                dput(dentry);
		goto retry;
	}
	dput(new);
	return dentry;
in the caller (d_alloc_parallel()).  Caller easier to follow and fewer functions
that are not neutral wrt ->d_lock...  I'm not suggesting to fold that with
yours - just a heads-up on needing to coordinate.

Anyway, modulo fs_initcall() thing it's all cosmetical; I certainly like
the simplified callers, if nothing else.

That's another patch I'd like to see pulled in front of the queue.