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.
