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

[NeilBrown]

On Fri, 01 May 2026, Al Viro wrote:
> On Fri, May 01, 2026 at 09:51:28AM +1000, NeilBrown wrote:
> 
> > I saw this comment the first time I read this email, but I didn't
> > process it properly.  That code is wrong.
> 
> One in mainline isn't - d_wait comes from target, as it ought to.
> 
> > It only makes sense to 
> > __d_wake_in_lookup_waiters() a dentry that we know was in-lookup, and in
> > d_move, that is target.
> > This can only happen (I think) in nfs where nfs_lookup() skips the lookup
> > for LOOKUP_RENAME_TARGET and leaves the dentry in-lookup.  Other threads
> > looking up that name will then block.
> > After the rename completes that in-lookup dentry will now be unhashed
> > but we need to wake it up so other threads can continue (and repeat the
> > lookup). 
> > 
> > So we need
> > 
> > 		__d_wake_in_lookup_waiters(target);
> > 
> > in d_move.  target, not dentry.
> 
> Yep.
> 
> > Thanks for flagging this,
> > 
> > Also my testing has hit a problem with some sort of deadlock in the nfs
> > server (so accessing and XFS filesystem).  They are tring to unlink a
> > file and are waiting in d_alloc_parallel() under reconnect_path.
> > This is running generic/467.
> > 
> > So better hold off this patchset until I have that understood.
> 
> Let's deal with d_alloc_parallel() first; it doesn't have to be tied
> into the rest of patchset.  Does the variant I've posted + s/dentry/target/ in that
> call of __d_wake_in_lookup_waiters() trigger any problems in your testing?

I'll let you know.  Still building at present.

> 
> If it doesn't, let's get that part out of the way - it makes sense on its own
> and getting it into -next (I'm sitting on a bunch of fs/dcache.c patches, and
> it would fit there nicely) would be a good idea.

OK

> 
> FWIW, your "noblock" variant is a misnomer - it *is* a trylock analogue of
> d_alloc_parallel(), all right, but it might very well block; on allocations,
> if nothing else, and there's a chance of having that dput(dentry) in "wouldblock"
> case coming right after the sucker ceased to be in-lookup and dropping the sole
> remaining reference.  Which may block on real IO, final dput() being what it is...

I'm happy to rename it to d_alloc_trylock()

> 
> And I really dislike the "drop and regain a caller-held lock" games - we'd been
> there many times and it had ended up with race galore again and again; see
> https://lore.kernel.org/all/20250623213747.GJ1880847@ZenIV/ for one recent
> example...
> 

I dislike them too.  I doubt I can find solutions that either of us
like, but they should be relatively short-lived.  Once we push the
locking down in the the inode_operations the filesystem will be in a
position to hold the lock only when it actually needs it (if at all).

I'm confident that dropping the lock is safe.  If there was some way to
tell the VFS that the lock has already been dropped, then we wouldn't
need to reclaim it, but I cannot see a clean way to do that.

I'm certainly open to suggestions.

Thanks,
NeilBrown