Re: [PATCH 1/2] vfs: make real_lookup do dentry revalidation with i_mutex held

[Ian Kent <raven@themaw.net>]

Sage Weil wrote:
>>> 	http://marc.info/?l=linux-fsdevel&m=123831685111213&w=2
>>>
>>> Looking over it, the 
>>>
>>> +		unsigned int lock_held = mutex_is_locked(&dir->i_mutex);
>>> ...
>>> +		if (lock_held) {
>>> +			/* Already pending, send to ->lookup() */
>>> +			d_drop(dentry);
>>>
>>> bit looks highly suspect.  I'm guessing revalidate should never sleep, and 
>>> always kick things off to ->lookup() (to do any waiting on upcall 
>>> completion or whatever else) if the dentry isn't valid now...?
>> Yeah, I've heard that before, ;)
>>
>> And that maybe the case, but that was what I first had.
>>
>> Sending everything to ->lookup() might be possible but it certainly
>> isn't that simple.
>>
>> Waiting in ->d_revaidate() isn't that different to waiting in ->lookup()
>> anyway as that must always be done without the directory mutex held. If
>> the lock isn't held when in ->d_revalidate() I can't really see any
>> reason not to handle that right their, possibly preventing the need to
>> go to ->lookup().
> 
> IIRC, the fundamental problem was that if ->d_revalidate() can be called 
> either with or without the dir mutex, you won't know whether the mutex is 
> locked by the caller or by some other thread (mutex_is_locked() doesn't 
> tell you _who_ locked the mutex).  And if waiting in d_revalidate means 
> unlocking the mutex, that can't work, because you won't know if that's 
> safe.  Thus, no waiting allowed in revalidate?

Hehe, as I said, I've been through this before.

If the mutex is not held by anyone then it doesn't need to be dropped!

But now I'm thinking I'll get rid of that that check anyway, so it
doesn't really matter.

> 
>> There are several cases I need to deal with, apart from path walks
>> initiated by the daemon which don't cause any call backs, and so are
>> largely handled by trivially returning success. The cases are, an
>> expiring dentry that will go away which ->lookup() can't yet handle, an
>> expiring dentry that won't go away which ->lookup() should be able to
>> handle already, and a straight out mount request which ->lookup() should
>> also be able to handle. The tail end of the expire cases can progress
>> concurrently with a mount, which is further complicated by the two cases
>> of going away or not, so it's all a bit tricky.
> 
> It sounds to me like revalidate should only return success in the trivial, 
> non-blocking case.  And the ->lookup() should be able to handle all 
> (other) cases.  I.e., things should still work correctly (perhaps more 
> slowly) without any d_revalidate() at all.  (It still looks like the main 
> change needed is for lookup to use d_obtain_alias, instead of returning 
> NULL unconditionally...)

It doesn't return NULL unconditionally and hasn't for some time.

It cannot work at all without a d_revalidate().

Your assuming that an automount implies dentry creation at mount time
which isn't necessarily the case. The creation of a directory in the
autofs pseudo file system may be done (positive and hashed) without a
mount request, the mount request coming later. This is a case I missed
when I described them above.

> 
>> In any case I need to get this to work without the change you proposed,
>> except for cases that result from the locking change, and I'm using
>> printks to track incorrect returns to identify those cases. So what I
>> need right now is consistent behaviour and I'm not quite there. Once I
>> have that I'll work on any issues resulting from the locking change.
> 
> I suspect you want to work with the real_lookup patch applied... I don't 
> think you can get consistent behavior without it. The whole goal here is 
> to remove the wait-in-revalidate workaround that was compensating for that 
> bug; without either the fix or the workaround you'll get occasional 
> -ENOENT.

True, and I expect and watch for that.
But I'm tending to agree and am going to do that.

Ian