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

[Ian Kent <raven@themaw.net>]

Sage Weil wrote:
> On Thu, 25 Jun 2009, Ian Kent wrote:
>> Sage Weil wrote:
>>>> 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...)
>> How so?
>>
>> I don't understand how d_obtain_alias() can help in this situation.
>> Can you elaborate please?
> 
> Well, as you noticed I misread lookup (it doesn't always return NULL).  I 
> originally just meant that lookup needs to return an existing dentry if 
> there is one (found via autofs4_lookup_active?), since now it may not be 
> able to count on revalidate blocking on the hashed one if it's not usable 
> yet.  It looks like it's tracking mounts by dentry, not inode, so 
> d_obtain_alias isn't terribly helpful.

Yep, that's right.

It gets better too.

Previously, in lookup, the call back was unconditional, but to avoid
mount storms from colour ls, now it isn't. Except for this I could just
rely on ->lookup() using a new dentry and not add the revalidating
dentry to the active list.

> 
> As you can see I haven't spent much time looking at this code.  :)  I'm 
> happy to do so, though, or spend some time testing to get this resolved.. 
> just let me know how I can help.

You can't just yet as it's completely different from the original patch.
Let me get this to a sensible point then we can both go over it in detail.

Ian