On Sat, Feb 08, 2025 at 01:30:43AM +0000, Al Viro wrote: > On Thu, Feb 06, 2025 at 04:42:50PM +1100, NeilBrown wrote: > > When we call ->revalidate we want to be sure we are revalidating the > > expected name. As a shared lock on i_rwsem no longer prevents renames > > we need to lock the dentry and ensure it still has the expected name. > > *blink* > > We never had been guaranteed any lock on the parent - the most common > call chain doesn't (and didn't) have it taken. > > > So pass parent name to d_revalidate() and be prepared to retry the > > lookup if it returns -EAGAIN. > > I don't understand that one at all. What's the point of those retries > on -EAGAIN? Rename (or race with d_splice_alias(), for that matter) > can happen just as we return success from ->d_revalidate(), so we > don't get anything useful out of that check. > > What's more, why do we need that exclusion in the first place? > The instance *is* given a stable parent reference and stable name, > so there's no need for it to even look at ->d_parent or ->d_name. > > It looks like a bad rebase on top of ->d_revalidate() series that > had landed in -rc1, with the original variant trying to provide the > guarantees now offered by that series. > > Unless there's something subtle I'm missing here, I would suggest > dropping that one. Incidentally, d_update_trylock() would be > better off in fs/dcache.c - static and with just one argument. Sorry, lost a sentence here while editing: The only remaining caller of d_update_trylock() would be the one in __d_unalias(), just before the call of ->d_unalias_trylock() in there and it gets NULL/NULL in the last two arguments. > HOWEVER, if you do not bother with doing that before ->d_unalias_trylock() > (and there's no reason to do that), the whole thing becomes much simpler - > you can do the check inside __d_move(), after all locks had been taken. > > After > spin_lock_nested(&dentry->d_lock, 2); > spin_lock_nested(&target->d_lock, 3); > you have everything stable. Just make the sucker return bool instead > of void, check that crap and have it return false if there's a problem. > > Callers other than __d_unalias() would just do WARN_ON(!__d_move(...)) > instead of their __d_move() calls and __d_unalias() would have > if (__d_move(...)) > ret = 0; > and screw the d_update_trylock/d_update_unlock there. > > All there is to it...
