Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix

[Will Taber <wtaber@us.ibm.com>]

Ian Kent wrote:
> On Fri, 2 Dec 2005, Will Taber wrote:
> 
> 
>>Ian Kent wrote:
>>
>>>On Thu, 1 Dec 2005, William H. Taber wrote:
>>>
>>>
>>>
>>>>Ian Kent wrote:
>>>>
>>>>
>>>>>On Wed, 30 Nov 2005, William H. Taber wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>Trond Myklebust wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>>On Wed, 2005-11-30 at 15:32 -0500, William H. Taber wrote:
>>>>>>>
>>>>>
>>>>>Lets see if I can keep this explaination simple.
>>>>>
>>>>>The user space process using the autofs filesystem (autodir or
>>>>>automount)
>>>>>needs to be able to call mkdir at mount time as a result of a callback
>>>>>from
>>>>>revalidate. Sometimes this comes indirectly from lookup (if the
>>>>>directory
>>>>>does not already exist).
>>>>>
>>>>>lookup_one_len requires the i_sem to be held so two instances of a
>>>>>filesystem calling it lead to a deadlock when mkdir is called from
>>>>>userspace
>>>>>(the third process). In the case we are discussing this happens because
>>>>>the
>>>>>first process calls lookup which releases the i_sem and calls revalidate
>>>>>itself. The second calls revalidate which doesn't release the i_sem and
>>>>>is
>>>>>places on a wait queue for mount completion. Consequently the mkdir
>>>>>blocks.
>>>>>
>>>>>So the requirement is that autofs release the i_sem during the callback,
>>>>>not
>>>>>obtain it.
>>>>>
>>>>>Will believes that it is not safe for autofs to release i_sem for the
>>>>>callback to user space because it is possible that path that aquired it
>>>>>may
>>>>>not be the path that has called revalidate and I can see his point.
>>>>>
>>>>>Never the less I'm still not convinced that this is possible given the
>>>>>restrictions of autofs.
>>>>>
>>>>>Let me try and describe this, hopefully more clearly than I've done so
>>>>>far.
>>>>>
>>>>>The only operations defined for autofs are:
>>>>>
>>>>>mkdir, rmdir, symlink and unlink and the only processes that can do
>>>>>these operations must be in the same
>>>>>process group that mounted the filesystem. EACCESS is returned for all
>>>>>other
>>>>>processes attempting these operations.
>>>>>
>>>>>The other functionality is read-only (and perhaps triggers a mount)
>>>>>being
>>>>>lookup, revalidate and readdir.
>>>>>
>>>>>So the question is, can anyone provide an example of a path that, upon
>>>>>calling autofs revalidate or lookup with the i_sem held, not be the path
>>>>>that aquired it?
>>>
>>>
>>>So still no counter example!
>>>
>>>
>>>
>>>>Any other process calling lookup_one_len on a file in /net.
>>>
>>>
>>>I'm afraid this is not an example it's an assertion.
>>>"Any other process" is a little broad I think.
>>>You'll need to be more specific.
>>>
>>>Consider the example reported by yourself and Ram.
>>>
>>>In that example we have processes P1, P2 and lets call the user space
>>>callback P1(mount). Also assume there is a mechamism to check the semaphore,
>>>release it if held and later re-take it if previously held, like the patch I
>>>offered before.
>>>
>>>Correct me if I'm wrong but, with the assumption above, you report goes
>>>like:
>>>
>>>P1 - calls lookup_one_len, takes i_sem and eventually calls autofs4_lookup
>>>and indirectly autofs4_revalidate.
>>>
>>>P2 - comes along and waits on i_sem.
>>
>>And what happens if P3 comes in with a normal lookup without i_sem held and
>>calls autofs4_revalidate from do_lookup and wakes up P2? Think both about what
>>will happen later in your code path and also what happens when P2 tries to
>>release the lock that was no longer held.
> 
> 
> P3 goes to the wait queue it can't wake up the waiters only mount 
> completion can do that.


But I understood your proposal to be that it d_revalidate would be 
unconditionally releasing the i_sem before went on the wait queue.  My 
point here was that P3 does not hold the i_sem lock so if it releases 
i_sem here, it will be waking up P2 before P1 has finished and released 
the lock.  Even if you don't end up in trouble from accessing something 
that hasn't been initialized yet, the counts on the semaphore are messed 
up because up has been called more often than down.

> 
> 
>>>P1 - autofs4_revalidate releases i_sem and posts a user space callback.
>>>
>>>P2 - aquires i_sem and eventually calls autofs4_revalidate, releases i_sem
>>>and is posted to the wait queue for mount completion.
>>>
>>>P1(mount) - calls mkdir, aquires i_sem, and calls autofs4_dir_mkdir,  i_sem
>>>is then released.
>>>
>>>Mount completion is signaled back to autofs4 and the waiters are released.
>>>
>>>P1, P2, in any order each (one after the other due to the semaphore) re-take
>>>i_sem and each complete their lookup_one_len calls.
>>>
>>>On both calls to autofs4_revalidate the calling process is itself the holder
>>>of i_sem.
>>>
>>>Further, any other process that does a path walk during this time has two
>>>possible paths.
>>>
>>>First case, the dentry exists, the process is placed on the wait queue along
>>>with P1 and P2 awaiting mount completion without taking i_sem.
>>>
>>>Second case, the dentry does not yet exist, this process either aquires the
>>>i_sem in do_lookup and follows a similar path to P1 and waits on the queue
>>>for mount completion or it waits on the i_sem while P1 does the lookup and
>>>triggers the mount request, it the aquires i_sem find the dentry exists,
>>>releases i_sem and calls autofs4_revalidate without i_sem held and is sent
>>>to the wait queue to wait for mount completion.
>>>
>>>Again in both these cases a process that enters autofs4_revalidate when the
>>>i_sem is held is the process that aquired it.
>>
>>But a regular lookup can enter autofs4_revalidate at anytime without holding
>>i_sem.
> 
> 
> And is a noop as far the semaphore is concerned. Neither taken or 
> released.

Only if you have the code in to check which code path you came from 
before you release the lock in d_revalidate.


> 
> 
>>The main lookup path does not hold i_sem and Trond was pretty clear about why
>>it cannot.  That is why devfs has the code which tries to guess whether it is
>>the person holding the lock before it releases it. If you put similar code
>>into autofs4_revalidate before you release i_sem it would probably work.  This
>>of course makes your code sensitive to changes in the lookup code because the
>>devfs code makes assumptions about what flags are set on different lookups.
>>The best fix would be to move all of the waiting into autofs4_lookup and not
>>hash the dentry until the mount was ready to run.  That is necessarily a large
>>piece of coding and would require a lot of testing.  That is why I am
>>suggesting for now a patch that determines if the lock was held by the caller
>>or not and releasing i_sem if it was, before waiting in autofs4_revalidate.
>>And of course remembering whether or not it needs to retake the lock after the
>>wait completes.
> 
> 
> It's sufficient to recognize the nameidata struct is NULL on a call 
> from lookup_hash nothing more that I'm aware of is needed. If that changes 
> then of course autofs will need to be changed. autofs also makes 
> assumptions about what flags are set for different reasons.
> 
> Your assuming that mount point directories don't exist before they are 
> mounted upon which is not the case.

OK.  I forgot that.  But I would still want you to think about the open 
case.  The only reason I say that is because, more times than I would 
like to admit, I am preparing to cd into a directory and vi a file to 
look at it, except that I get ahead of myself and I end up trying to vi 
the directory.  The automounter may never try to open the directory but 
you also have to consider fat fingered fools like myself.

Will

> 
> Ian
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>