autofs4: hang and proposed fix

Quoted 45 lines · Ram Pai

Quoted 35 lines · William H. Taber
Quoted 7 lines · Ian Kent
Quoted 3 lines · Ram Pai
The question is: Who is the culprit? stubfs? VFS? or
autofs4?
Quoted 7 lines
I'm happy to fix it in autofs unless you feel we need to address the wider issue.
I'll put together a patch which takes account of this and pushes the hold/release down into try_to_fill_dentry. But I would like a little time to think about whether there may be other implications.
Ian,
I don't think that you can fix this in the autofs by tinkering with holding and releasing the parent i_sem. The reason for this is that you
don't have any way of knowing if you hold that lock or not. The easy
case is that nobody holds the lock. But if the lock is held you have no way to know that you are the person holding the lock and you cannot unlock someone elses lock without serious consequences.
The only way to fix the lock handling is to fix the VFS. This means either changing all calls to the d_revalidate functions (or all calls to d_revalidate itself) so that the parent i_sem is obtained first, or to change lookup_one_len (or actually lookup_hash) to only get the lock around the filesystem lookup call, matching what is done in real_lookup.
I don't know which is better from a locking correctness perspective.
I would have to defer to the VFS experts on that one. I do know that lookup_one_len is called from about 40 places in kernel tree and probably from every filesystem outside the tree as well. Either way, it is a non-trivial piece of work.
Quoted 8 lines
If you take the inconsistant locking as a given, then the fix has to involve not doing the d_add on the new dentry until after the mount completes. This would eliminate the need for revalidate to wait. You would have to provide a mechanism for keeping track of the outstanding mount requests and looking for a a mount in progress before starting a new request. This would take the waiting out of revalidate and put it into the lookup request itself where you are guaranteed that the parent i_sem lock is held.

Quoted 4 lines

Even this has a issue I think. Because later when the automounter attempts to mount, VFS wont' find the corresponding dentry in the dcache and will allocate a new dentry. And this dentry is not the one which autofs4 is waiting to be mounted on. No?

Yes. The mount triggering depends on the dentry being present.

And there is the situation where the mount point directory pre-exists in the autofs (browseable automounts) so lookup is not called.

In the new version that I am working on now (when I eventually get it done) directories will pre-exist for all autofs mount points but simply not be displayed based on a mount option.

So this could be kinda difficult for me.

Ian

Ian Kent

maintainer

2005-11-18 01:44 UTC #11 lore raw

http://themaw.net/direct.txt

On Thu, 17 Nov 2005, William H. Taber wrote:

Hi Taber,

Quoted 16 lines

Quoted 7 lines · Ian Kent
Quoted 3 lines · Ram Pai
The question is: Who is the culprit? stubfs? VFS? or
autofs4?
Quoted 7 lines
I'm happy to fix it in autofs unless you feel we need to address the wider issue.
I'll put together a patch which takes account of this and pushes the hold/release down into try_to_fill_dentry. But I would like a little time to think about whether there may be other implications.

Quoted 7 lines

Ian,
I don't think that you can fix this in the autofs by tinkering with holding and releasing the parent i_sem. The reason for this is that you
don't have any way of knowing if you hold that lock or not. The easy
case is that nobody holds the lock. But if the lock is held you have no way to know that you are the person holding the lock and you cannot unlock someone elses lock without serious consequences.

Yes. I see.

But let me make sure I understand what you are saying.

The problem would be that if I release and then retake the lock for autofs to do it thing there is a risk of opening the caller to the potential races it is protecting itself from.

Correct?

Quoted 11 lines

The only way to fix the lock handling is to fix the VFS. This means either changing all calls to the d_revalidate functions (or all calls to d_revalidate itself) so that the parent i_sem is obtained first, or to change lookup_one_len (or actually lookup_hash) to only get the lock around the filesystem lookup call, matching what is done in real_lookup.
I don't know which is better from a locking correctness perspective.
I would have to defer to the VFS experts on that one. I do know that lookup_one_len is called from about 40 places in kernel tree and probably from every filesystem outside the tree as well. Either way, it is a non-trivial piece of work.

Sure is.

Given the description above my impulsive thought would be to move the synchronisation to backet the lookup call in lookup_hash as the low risk low impact approach.

As you say we need to get the attention of those that need to know before anything can be done.

Quoted 9 lines

If you take the inconsistant locking as a given, then the fix has to involve not doing the d_add on the new dentry until after the mount completes. This would eliminate the need for revalidate to wait. You would have to provide a mechanism for keeping track of the outstanding mount requests and looking for a a mount in progress before starting a new request. This would take the waiting out of revalidate and put it into the lookup request itself where you are guaranteed that the parent i_sem lock is held.

Sounds like this would be major change for autofs and would likely have an impact on the userspace daemon as well.

Quoted 2 lines

I hope this is helps.

Sure does. Thanks.

Ian

William H. Taber

2005-11-18 15:20 UTC #12 lore raw

Quoted 3 lines · Ian Kent

On Thu, 17 Nov 2005, William H. Taber wrote:
Hi Taber,

Hi,
You can call me Will. Most everyone else does. :^)

Quoted 8 lines

Quoted 6 lines
Ian,
I don't think that you can fix this in the autofs by tinkering with holding and releasing the parent i_sem. The reason for this is that you
don't have any way of knowing if you hold that lock or not. The easy
case is that nobody holds the lock. But if the lock is held you have no way to know that you are the person holding the lock and you cannot unlock someone elses lock without serious consequences.

Quoted 8 lines

Yes. I see.
But let me make sure I understand what you are saying.
The problem would be that if I release and then retake the lock for autofs to do it thing there is a risk of opening the caller to the potential races it is protecting itself from.
Correct?

No, it is actually a little more subtle than that. The problem is that since you can be called from two code paths, one of which get's the lock and one of them doesn't, you are stuck if you find that the lock is held because you don't know who holds it. The danger is that some innocent third party is holding the lock and counting on being protected by it. If you release the lock, then you can be creating the potential for a race in their code and there would be no way to detect it. Their code path would look correct because it is. Not only that but the lock itself could get confused because it would have more unlocks than locks, because presumably the process that thinks it has the lock would eventually unlock as well. I don't know how the semaphores are implemented on all architectures so I don't know if that would be an actual problem or not but it I would be surprised if they all handled that case gracefully.

Regards,

Will

Ian Kent

maintainer

2005-11-18 16:30 UTC #13 lore raw

Quoted 7 lines · William H. Taber

Quoted 3 lines · Ian Kent
On Thu, 17 Nov 2005, William H. Taber wrote:
Hi Taber,
Hi,
You can call me Will. Most everyone else does. :^)

Sorry.

Quoted 20 lines

Quoted 11 lines
Quoted 9 lines
Ian,
I don't think that you can fix this in the autofs by tinkering with holding and releasing the parent i_sem. The reason for this is that you don't have any way of knowing if you hold that lock or not. The easy case is that nobody holds the lock. But if the lock is held you have no way to know that you are the person holding the lock and you cannot unlock someone elses lock without serious consequences.
Quoted 9 lines
Yes. I see.
But let me make sure I understand what you are saying.
The problem would be that if I release and then retake the lock for autofs to do it thing there is a risk of opening the caller to the potential races it is protecting itself from.
Correct?

Quoted 13 lines

No, it is actually a little more subtle than that. The problem is that since you can be called from two code paths, one of which get's the lock and one of them doesn't, you are stuck if you find that the lock is held because you don't know who holds it. The danger is that some innocent third party is holding the lock and counting on being protected by it. If you release the lock, then you can be creating the potential for a race in their code and there would be no way to detect it. Their code path would look correct because it is. Not only that but the lock itself could get confused because it would have more unlocks than locks, because presumably the process that thinks it has the lock would eventually unlock as well. I don't know how the semaphores are implemented on all architectures so I don't know if that would be an actual problem or not but it I would be surprised if they all handled that case gracefully.

Yes. Thinking about it I hadn't considered the third process.

How can we get advice on this?

Ian

William H. Taber

2005-11-18 17:12 UTC #14 lore raw

Quoted 17 lines · Ian Kent

Quoted 14 lines · William H. Taber
No, it is actually a little more subtle than that. The problem is that since you can be called from two code paths, one of which get's the lock and one of them doesn't, you are stuck if you find that the lock is held because you don't know who holds it. The danger is that some innocent third party is holding the lock and counting on being protected by it. If you release the lock, then you can be creating the potential for a race in their code and there would be no way to detect it. Their code path would look correct because it is. Not only that but the lock itself could get confused because it would have more unlocks than locks, because presumably the process that thinks it has the lock would eventually unlock as well. I don't know how the semaphores are implemented on all architectures so I don't know if that would be an actual problem or not but it I would be surprised if they all handled that case gracefully.

Quoted 6 lines

Yes. Thinking about it I hadn't considered the third process.
How can we get advice on this?
Ian

Hey, I'm new here. I came here looking for advice. :^)
However, if there is no way to fix this in the autofs code then the only alternative is to fix the locking in the VFS. So VFS folks, which is the better solution. Should all callers to d_revalidate get the parent's i_sem lock or should d_revalidate never be called with the parent i_sem lock and so we need to change lookup_one_len so that it does not expect the parent i_sem lock to be held?

Will Taber

Quoted 5 lines

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

Ram Pai

author

2005-11-18 18:57 UTC #15 lore raw

Quoted 24 lines · William H. Taber

Quoted 17 lines · Ian Kent
Quoted 14 lines · William H. Taber
No, it is actually a little more subtle than that. The problem is that since you can be called from two code paths, one of which get's the lock and one of them doesn't, you are stuck if you find that the lock is held because you don't know who holds it. The danger is that some innocent third party is holding the lock and counting on being protected by it. If you release the lock, then you can be creating the potential for a race in their code and there would be no way to detect it. Their code path would look correct because it is. Not only that but the lock itself could get confused because it would have more unlocks than locks, because presumably the process that thinks it has the lock would eventually unlock as well. I don't know how the semaphores are implemented on all architectures so I don't know if that would be an actual problem or not but it I would be surprised if they all handled that case gracefully.
Quoted 6 lines
Yes. Thinking about it I hadn't considered the third process.
How can we get advice on this?
Ian

Quoted 7 lines

Hey, I'm new here. I came here looking for advice. :^) However, if there is no way to fix this in the autofs code then the only alternative is to fix the locking in the VFS. So VFS folks, which is the better solution. Should all callers to d_revalidate get the parent's i_sem lock or should d_revalidate never be called with the parent i_sem lock and so we need to change lookup_one_len so that it does not expect the parent i_sem lock to be held?

I think the problem is with cached_lookup(). It is the only place which calls ->revalidate() holding the parent's inode-semaphore AFAICT.

note: cached_lookup() is only called from __lookup_hash() and __lookup_hash() is always called holding the semaphore.

VFS experts agree?
RP

Quoted 9 lines

Will Taber
Quoted 5 lines
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

William H. Taber

2005-11-18 20:08 UTC #16 lore raw

Quoted 11 lines · Ram Pai

On Fri, 2005-11-18 at 09:12, William H. Taber wrote:
I think the problem is with cached_lookup(). It is the only place which calls ->revalidate() holding the parent's inode-semaphore AFAICT.
note: cached_lookup() is only called from __lookup_hash() and __lookup_hash() is always called holding the semaphore.
VFS experts agree?
RP

Ram,
Lookup_one_len calls lookup_hash and it is the callers of lookup_one_len that are problematical. Just as an example, lookup_one_len is called from nfs_sillyrename which is called, among other places in the nfs_rename code. In that path the parent i_sem is obtained in do_rename
in the vfs code (namei.c). I would think that it would be extremely
difficult to to change that usage. The alternative is to move the obtaining of the parent i_sem from real_lookup to do_lookup. We would also have to put the locking around the d_revalidate call at return_reval in __link_path_walk.

Again, what do the VFS experts think?

Will Taber

Ian Kent

maintainer

2005-11-19 02:52 UTC #17 lore raw

Quoted 13 lines · William H. Taber

Quoted 12 lines · Ram Pai
On Fri, 2005-11-18 at 09:12, William H. Taber wrote:
I think the problem is with cached_lookup(). It is the only place which calls ->revalidate() holding the parent's inode-semaphore AFAICT.
note: cached_lookup() is only called from __lookup_hash() and __lookup_hash() is always called holding the semaphore.
VFS experts agree?
RP

Quoted 10 lines

Ram,
Lookup_one_len calls lookup_hash and it is the callers of lookup_one_len that are problematical. Just as an example, lookup_one_len is called from nfs_sillyrename which is called, among other places in the nfs_rename code.
In that path the parent i_sem is obtained in do_rename in the vfs code
(namei.c). I would think that it would be extremely difficult to to change that usage. The alternative is to move the obtaining of the parent i_sem from real_lookup to do_lookup. We would also have to put the locking around the d_revalidate call at return_reval in __link_path_walk.

Perhaps we are making this altogether to complicated.

I'm sure that there are good reasons for the locking being the way it is and any attempt to change it is likely to be a disaster. So what about solving this by defining a usage policy based on the intent of the functions concerned.

For example.

The lookup_one_len a special use funtion to return the dentry corresponding to a path element and by definition it does not follow mounts or symlinks. To function correctly autofs needs to follow mounts and some time soon I will be posting patches that will use the the follow_link method as well.

So the policy could be that if autofs revalidate is called with the directory inode semaphore held it must validate the autofs dentry itself and not cause a mount request be triggered. The responsibility then moves to the filesystem to check if the dentry is an autofs dentry and to decide if it needs to then make an unlocked revalidate call. It is easy enough to check if the semaphore is held the autofs module. The filesystem check is easy enough to do once the filesystem magic number is moved to one of the common autofs header files.

Thoughts?

Ian

William H. Taber

2005-11-21 16:40 UTC #18 lore raw

Quoted 35 lines · Ian Kent

Quoted 14 lines · William H. Taber
Quoted 11 lines · Ram Pai
On Fri, 2005-11-18 at 09:12, William H. Taber wrote:
I think the problem is with cached_lookup(). It is the only place which calls ->revalidate() holding the parent's inode-semaphore AFAICT.
note: cached_lookup() is only called from __lookup_hash() and __lookup_hash() is always called holding the semaphore.
VFS experts agree?
RP
Quoted 10 lines
Ram,
Lookup_one_len calls lookup_hash and it is the callers of lookup_one_len that are problematical. Just as an example, lookup_one_len is called from nfs_sillyrename which is called, among other places in the nfs_rename code.
In that path the parent i_sem is obtained in do_rename in the vfs code
(namei.c). I would think that it would be extremely difficult to to change that usage. The alternative is to move the obtaining of the parent i_sem from real_lookup to do_lookup. We would also have to put the locking around the d_revalidate call at return_reval in __link_path_walk.
Perhaps we are making this altogether to complicated.
I'm sure that there are good reasons for the locking being the way it is and any attempt to change it is likely to be a disaster. So what about solving this by defining a usage policy based on the intent of the functions concerned.
For example.

Quoted 7 lines

The lookup_one_len a special use funtion to return the dentry corresponding to a path element and by definition it does not follow mounts or symlinks. To function correctly autofs needs to follow mounts and some time soon I will be posting patches that will use the the follow_link method as well.
So the policy could be that if autofs revalidate is called with the directory inode semaphore held it must validate the autofs dentry itself and not cause a mount request be triggered. The responsibility then moves to the filesystem to check if the dentry is an autofs dentry and to decide if it needs to then make an unlocked revalidate call. It is easy enough to check if the semaphore is held the autofs module. The filesystem check is easy enough to do once the filesystem magic number is moved to one of the common autofs header files.
Thoughts?
Ian

So you are asking that lookup_one_len be modified so that it knows about the internals of the autofs4 so that it can determine enough to know, before it makes the revalidate call that the the call is going to pend so that it can release the lock if it needs to? This does not seem like a good idea to me. The whole point of having the d_revalidate functions is so the VFS does not have to know the specifics of any individual filesystem.

Since there does not appear to be a clear locking policy on d_revalidate, then the autofs4 revalidate function cannot make assumptions about that locking state. This means that autofs4_revalidate cannot pend. I have looked some more at the real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it can find it again and wakes up the automount demon that it has work to do. It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue.
4)The automount demon will run and get the information that it needs to complete the mount request and then issue the mount. The lookup request from mount will call real_lookup. Since the demon is in OZ mode it does not pend, it fills in the dentry and when the dentry is fully ready for consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will discard the one that had been allocated.

This keeps all of the waiting inside the autofs4 lookup function where the lock state is defined. I realize that this may be a lot of work, but I haven't seen a possible solution that doesn't involve that.

Will

Ian Kent

maintainer

2005-11-22 13:13 UTC #19 lore raw

Quoted 56 lines · William H. Taber

Quoted 55 lines · Ian Kent
Quoted 14 lines · William H. Taber
Quoted 11 lines · Ram Pai
On Fri, 2005-11-18 at 09:12, William H. Taber wrote:
I think the problem is with cached_lookup(). It is the only place which calls ->revalidate() holding the parent's inode-semaphore AFAICT.
note: cached_lookup() is only called from __lookup_hash() and __lookup_hash() is always called holding the semaphore.
VFS experts agree?
RP
Quoted 10 lines
Ram,
Lookup_one_len calls lookup_hash and it is the callers of lookup_one_len that are problematical. Just as an example, lookup_one_len is called from nfs_sillyrename which is called, among other places in the nfs_rename code.
In that path the parent i_sem is obtained in do_rename in the vfs code
(namei.c). I would think that it would be extremely difficult to to change that usage. The alternative is to move the obtaining of the parent i_sem from real_lookup to do_lookup. We would also have to put the locking around the d_revalidate call at return_reval in __link_path_walk.
Perhaps we are making this altogether to complicated.
I'm sure that there are good reasons for the locking being the way it is and any attempt to change it is likely to be a disaster. So what about solving this by defining a usage policy based on the intent of the functions concerned.
For example.
The lookup_one_len a special use funtion to return the dentry corresponding to a path element and by definition it does not follow mounts or symlinks. To function correctly autofs needs to follow mounts and some time soon I will be posting patches that will use the the follow_link method as well.
So the policy could be that if autofs revalidate is called with the directory inode semaphore held it must validate the autofs dentry itself and not cause a mount request be triggered. The responsibility then moves to the filesystem to check if the dentry is an autofs dentry and to decide if it needs to then make an unlocked revalidate call. It is easy enough to check if the semaphore is held the autofs module. The filesystem check is easy enough to do once the filesystem magic number is moved to one of the common autofs header files.
Thoughts?

Quoted 8 lines

Quoted 1 line
Ian
So you are asking that lookup_one_len be modified so that it knows about the internals of the autofs4 so that it can determine enough to know, before it makes the revalidate call that the the call is going to pend so that it can release the lock if it needs to? This does not seem like a good idea to me. The whole point of having the d_revalidate functions is so the VFS does not have to know the specifics of any individual filesystem.

No not at all. Absolutely, that would be a bad idea.
I thought my description above was fairly clear but obviously not.

Quoted 4 lines

Since there does not appear to be a clear locking policy on d_revalidate, then the autofs4 revalidate function cannot make assumptions about that locking state. This means that

I'm not suggesting that either. However, it is relatively simple to check if a semaphore is held by someone outside of your code (my code in this case, see down_trylock()). I think that checking would be safe as if the semaphore is held by someone else trylock fails and autofs can assume an equivelent state to oz_mode, passing through not mounting anything. If the semaphore is not held trylock succeeds and autofs can immediately release the semaphore and continue. Can you think of any examples of this being unsafe?

Quoted 1 line

autofs4_revalidate cannot pend. I have looked some more at the

Exactly and that's what I'm suggesting. Take account of what the lookup_one_len is advertised to do. My point is that lookup_one_len is not supposed to follow mounts or soft links, by definition, so it shouldn't cause autofs to trigger any mounts. If a filesystem wants to use it then it then it needs to take account of its defined behaviour, warts and all.

Quoted 21 lines

real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it can find it again and wakes up the automount demon that it has work to do. It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue. 4)The automount demon will run and get the information that it needs to complete the mount request and then issue the mount. The lookup request from mount will call real_lookup. Since the demon is in OZ mode it does not pend, it fills in the dentry and when the dentry is fully ready for consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will discard the one that had been allocated.

Quoted 3 lines

This keeps all of the waiting inside the autofs4 lookup function where the lock state is defined. I realize that this may be a lot of work, but I haven't seen a possible solution that doesn't involve that.

Sounds like a lot of work and likely quite interesting but directories can and often do exist in the autofs filesystem that don't have active mounts.

For these directories only the revaidate method is called at auto mount time. It's worth remembering that, as autofs is a pseudo filesystem, it pins the dentry for each of its objects so they don't go away. Maybe you're suggesting I change this?

Sorry, I don't mean to be rude but I'm suggesting your using lookup_one_len incorectly. I'll need to look at other code to see if this actually holds true, but, as automounting is usually not the first thing that people think of when they are writting a filesystem I expect I won't get much support from there either.

What I'm proposing is:

1) lookup_one_len should never cause anything to be auto mounted because of its defined behaviour and autofs should behave in line with this definition.
2) The filesystem that calls lookup_one_len directly or indirectly is responsibe for checking if it has walked onto an autofs dentry and decide what action it should take.

I thought that this was quite sensible and a fairly simple resolution?

Ian

William H. Taber

2005-11-22 17:48 UTC #20 lore raw

Quoted 3 lines · Ian Kent

Quoted 3 lines
Quoted 3 lines
Perhaps we are making this altogether to complicated.
I'm sure that there are good reasons for the locking being the way it is and any attempt to change it is likely to be a disaster. So what about solving this by defining a usage policy based on the intent of the functions concerned.

While we have been discussing this I have been playing with adding locks around the d_revalidate calls and it is difficult (or I am obtuse). If I can get that to work it will be the simplest approach but so far I am getting worse deadlocks.

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
For example.
The lookup_one_len a special use funtion to return the dentry corresponding to a path element and by definition it does not follow mounts or symlinks. To function correctly autofs needs to follow mounts and some time soon I will be posting patches that will use the the follow_link method as well.
So the policy could be that if autofs revalidate is called with the directory inode semaphore held it must validate the autofs dentry itself and not cause a mount request be triggered. The responsibility then moves to the filesystem to check if the dentry is an autofs dentry and to decide if it needs to then make an unlocked revalidate call. It is easy enough to check if the semaphore is held the autofs module. The filesystem check is easy enough to do once the filesystem magic number is moved to one of the common autofs header files.
Thoughts?

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Ian
So you are asking that lookup_one_len be modified so that it knows about the internals of the autofs4 so that it can determine enough to know, before it makes the revalidate call that the the call is going to pend so that it can release the lock if it needs to? This does not seem like a good idea to me. The whole point of having the d_revalidate functions is so the VFS does not have to know the specifics of any individual filesystem.
No not at all. Absolutely, that would be a bad idea.
I thought my description above was fairly clear but obviously not.

Or maybe I was being dense. :^)

Quoted 7 lines

Quoted 2 lines
Since there does not appear to be a clear locking policy on d_revalidate, then the autofs4 revalidate function cannot make assumptions about that locking state. This means that
I'm not suggesting that either. However, it is relatively simple to check if a semaphore is held by someone outside of your code (my code in this case, see down_trylock()). I think that checking would be safe as if the semaphore is held by someone else trylock fails and autofs can assume an equivelent state to oz_mode, passing through not mounting anything. If the semaphore is not held trylock succeeds and autofs can immediately release the semaphore and continue. Can you think of any examples of this being unsafe?

I am not sure about safety. I haven't researched all of the callers to lookup_one_len. But what is the effect of this on the lookup itself? If your revalidate functions returns true, then the caller will expect to have a dentry that they can use. Most likely the next thing they will do is to try to cross the mountpoint. But the mountpoint might not be set up yet. Alternatively, you can return false but then the vfs will call d_invalidate on the dentry. Either d_invalidate succeeds and the dentry is unhashed and the autofs lookup function is called or it returns -EBUSY, at which point the lookup fails and returns the error. The first case is essentially what I was proposing, except that I said not to even bother putting the dentry into the hash chains at all. The EBUSY case is probably not what you want.

Quoted 5 lines

Quoted 1 line
autofs4_revalidate cannot pend. I have looked some more at the
Exactly and that's what I'm suggesting. Take account of what the lookup_one_len is advertised to do. My point is that lookup_one_len is not supposed to follow mounts or soft links, by definition, so it shouldn't cause autofs to trigger any mounts. If a filesystem wants to use it then it then it needs to take account of its defined behaviour, warts and all.

I am not expecting lookup_one_len to follow mount points. I expect to follow them myself. But I do expect that if this is a mountpoint, that autofs will set it up for me. If not, what is the point of an automounter?

Quoted 8 lines

Quoted 8 lines
real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it can find it again and wakes up the automount demon that it has work to do. It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue.
4)The automount demon will run and get the information that it needs to complete the mount request and then issue the mount. The lookup request from mount will call real_lookup. Since the demon is in OZ mode it does not pend, it fills in the dentry and when the dentry is fully ready for consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will discard the one that had been allocated.

Quoted 6 lines

Quoted 1 line
This keeps all of the waiting inside the autofs4 lookup function where the lock state is defined. I realize that this may be a lot of work, but I haven't seen a possible solution that doesn't involve that.
Sounds like a lot of work and likely quite interesting but directories can and often do exist in the autofs filesystem that don't have active mounts.
For these directories only the revaidate method is called at auto mount time. It's worth remembering that, as autofs is a pseudo filesystem, it pins the dentry for each of its objects so they don't go away. Maybe you're suggesting I change this?

Exactly. If the dentries are unhashed at umount time then the revalidate case is not an issue. I don't know the how much work is invovled in setting things up in the first place so you might want to cache your unused dentries yourself if that avoids having to reread the autofs configuration files. But that is an implementation detail for you to consider.

Quoted 2 lines

Sorry, I don't mean to be rude but I'm suggesting your using lookup_one_len incorectly. I'll need to look at other code to see if this actually holds true, but, as automounting is usually not the first thing that people think of when they are writting a filesystem I expect I won't get much support from there either.

I don't know what you mean by using it incorrectly. We have a shadow filesytem and our lookup function is being called and we are trying to find the corresponding file/directory in the root filesystem. We are calling lookup_one_len because we are trying to find the next name in the path. We are prepared to handle mountpoint crossings, but as I said above, the mountpoint needs to be setup so we can cross it. We cannot call path_walk or its variants because we do not have the entire path.

Quoted 6 lines

What I'm proposing is:
1) lookup_one_len should never cause anything to be auto mounted because of its defined behaviour and autofs should behave in line with this definition.

What defined behaviour? The purpose of autofs is to automount directories. I am not looking for you to cross a mountpoint for me, I just want you to setup the mount so I can cross it myself.

Quoted 4 lines

2) The filesystem that calls lookup_one_len directly or indirectly is responsibe for checking if it has walked onto an autofs dentry and decide what action it should take.

And what action would that be? Not enter into any autofs directory tree? Do the mount myself? Return ENOENT? Return inconsistent results based on whether someone else has triggered the automount for me?

And from an interface perspective, a caller of a function like lookup_one_len should never have to worry about the implementation of the underlying filesystem, or even have to know or care what the filesystem is.

Quoted 2 lines

I thought that this was quite sensible and a fairly simple resolution?

But it defeats the whole purpose of having an automounter.

Quoted 3 lines

Ian

Regards,
Will

Ian Kent

maintainer

2005-11-23 14:11 UTC #21 lore raw

Quoted 64 lines · William H. Taber

Quoted 6 lines · Ian Kent
Quoted 6 lines
Quoted 6 lines
Perhaps we are making this altogether to complicated.
I'm sure that there are good reasons for the locking being the way it is and any attempt to change it is likely to be a disaster. So what about solving this by defining a usage policy based on the intent of the functions concerned.
While we have been discussing this I have been playing with adding locks around the d_revalidate calls and it is difficult (or I am obtuse). If I can get that to work it will be the simplest approach but so far I am getting worse deadlocks.
Quoted 31 lines
Quoted 22 lines
Quoted 18 lines
For example.
The lookup_one_len a special use funtion to return the dentry corresponding to a path element and by definition it does not follow mounts or symlinks. To function correctly autofs needs to follow mounts and some time soon I will be posting patches that will use the the follow_link method as well.
So the policy could be that if autofs revalidate is called with the directory inode semaphore held it must validate the autofs dentry itself and not cause a mount request be triggered. The responsibility then moves to the filesystem to check if the dentry is an autofs dentry and to decide if it needs to then make an unlocked revalidate call. It is easy enough to check if the semaphore is held the autofs module. The filesystem check is easy enough to do once the filesystem magic number is moved to one of the common autofs header files.
Quoted 3 lines
Thoughts?
Ian
Quoted 7 lines
So you are asking that lookup_one_len be modified so that it knows about the internals of the autofs4 so that it can determine enough to know, before it makes the revalidate call that the the call is going to pend so that it can release the lock if it needs to? This does not seem like a good idea to me. The whole point of having the d_revalidate functions is so the VFS does not have to know the specifics of any individual filesystem.
Quoted 3 lines
No not at all. Absolutely, that would be a bad idea.
I thought my description above was fairly clear but obviously not.
Or maybe I was being dense. :^)
Quoted 6 lines
Quoted 4 lines
Since there does not appear to be a clear locking policy on d_revalidate, then the autofs4 revalidate function cannot make assumptions about that locking state. This means that
Quoted 9 lines
I'm not suggesting that either. However, it is relatively simple to check if a semaphore is held by someone outside of your code (my code in this case, see down_trylock()). I think that checking would be safe as if the semaphore is held by someone else trylock fails and autofs can assume an equivelent state to oz_mode, passing through not mounting anything. If the semaphore is not held trylock succeeds and autofs can immediately release the semaphore and continue. Can you think of any examples of this being unsafe?

Quoted 12 lines

I am not sure about safety. I haven't researched all of the callers to lookup_one_len. But what is the effect of this on the lookup itself? If your revalidate functions returns true, then the caller will expect to have a dentry that they can use. Most likely the next thing they will do is to try to cross the mountpoint. But the mountpoint might not be set up yet. Alternatively, you can return false but then the vfs will call d_invalidate on the dentry. Either d_invalidate succeeds and the dentry is unhashed and the autofs lookup function is called or it returns -EBUSY, at which point the lookup fails and returns the error. The first case is essentially what I was proposing, except that I said not to even bother putting the dentry into the hash chains at all. The
EBUSY case is probably not what you want.

OK I think I'm starting to get what your saying. I guess I didn't want to hear it because I've been moving toward pushing everything to revalidate.

For some reason I woke up early this morning. I read your reply and I've been thinking about it all day (hard to change gears when there's a challenge like this, bad for work, fun for me, I'm sure I'll get cained for being somewhere else).

To verify I've got it this time all you are saying is, avoid revalidate by keeping all unmounted dentrys unhashed? Nothing much more really?

I haven't really thought it through completely yet. You've noticed I'm not a quick study no doubt.

So far it seems doable though. One problem point could be if one of these lookups come in during user space daemon startup but that's detail atm.

Following startup I have two cases to deal with (i'm only listing them cause I think you missed case 2):

1) No directory exists - it's created by the daemon during the callback to the userspace before it perform the mount.

2) Directory already exists - created at startup before any mount activity.

Case 2 needs special attention. To achieve this I would need to have two types of unhashed dentry, valid and invalid (perhaps because of a ENOENT return from the daemon on mount).

Not really that hard to do I think.

I'd need to rework the readdir code to fill unhashed, valid dentrys and I can set status on the return codes from the daemon. There's likely a bunch of other detail as well.

Unfortuneatly for me this is only half of the solution ... see below.

Quoted 78 lines

Quoted 10 lines
Quoted 2 lines
autofs4_revalidate cannot pend. I have looked some more at the
Exactly and that's what I'm suggesting. Take account of what the lookup_one_len is advertised to do. My point is that lookup_one_len is not supposed to follow mounts or soft links, by definition, so it shouldn't cause autofs to trigger any mounts. If a filesystem wants to use it then it then it needs to take account of its defined behaviour, warts and all.
I am not expecting lookup_one_len to follow mount points. I expect to follow them myself. But I do expect that if this is a mountpoint, that autofs will set it up for me. If not, what is the point of an automounter?
Quoted 27 lines
Quoted 22 lines
real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it can find it again and wakes up the automount demon that it has work to do. It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue. 4)The automount demon will run and get the information that it needs to complete the mount request and then issue the mount. The lookup request from mount will call real_lookup. Since the demon is in OZ mode it does not pend, it fills in the dentry and when the dentry is fully ready for consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will discard the one that had been allocated.
Quoted 3 lines
This keeps all of the waiting inside the autofs4 lookup function where the lock state is defined. I realize that this may be a lot of work, but I haven't seen a possible solution that doesn't involve that.
Quoted 6 lines
Sounds like a lot of work and likely quite interesting but directories can and often do exist in the autofs filesystem that don't have active mounts. For these directories only the revaidate method is called at auto mount time. It's worth remembering that, as autofs is a pseudo filesystem, it pins the dentry for each of its objects so they don't go away. Maybe you're suggesting I change this?
Exactly. If the dentries are unhashed at umount time then the revalidate case is not an issue. I don't know the how much work is invovled in setting things up in the first place so you might want to cache your unused dentries yourself if that avoids having to reread the autofs configuration files. But that is an implementation detail for you to consider.
Quoted 6 lines
Sorry, I don't mean to be rude but I'm suggesting your using lookup_one_len incorectly. I'll need to look at other code to see if this actually holds true, but, as automounting is usually not the first thing that people think of when they are writting a filesystem I expect I won't get much support from there either.
I don't know what you mean by using it incorrectly. We have a shadow filesytem and our lookup function is being called and we are trying to find the corresponding file/directory in the root filesystem. We are calling lookup_one_len because we are trying to find the next name in the path. We are prepared to handle mountpoint crossings, but as I said above, the mountpoint needs to be setup so we can cross it. We cannot call path_walk or its variants because we do not have the entire path.
Quoted 6 lines
What I'm proposing is:
1) lookup_one_len should never cause anything to be auto mounted because of its defined behaviour and autofs should behave in line with this definition.
What defined behaviour? The purpose of autofs is to automount directories. I am not looking for you to cross a mountpoint for me, I just want you to setup the mount so I can cross it myself.

Quoted 7 lines

Quoted 4 lines
2) The filesystem that calls lookup_one_len directly or indirectly is responsibe for checking if it has walked onto an autofs dentry and decide what action it should take.
And what action would that be? Not enter into any autofs directory tree? Do the mount myself? Return ENOENT? Return inconsistent results based on whether someone else has triggered the automount for me?

I was thinking of something like EAGAIN to the calling fs - meaning, ok but I need to be revalidated (lockles) for you to be sure.

Quoted 7 lines

And from an interface perspective, a caller of a function like lookup_one_len should never have to worry about the implementation of the underlying filesystem, or even have to know or care what the filesystem is.
Quoted 2 lines
I thought that this was quite sensible and a fairly simple resolution?
But it defeats the whole purpose of having an automounter.

Yep. Sigh.

But there's more. The Linux autofs implementation lacks some crucial features which I really want to add.

I posted an RFC to LKML and had no interest but, since it relates to this issue, I'd really appreciate it if you could give it a quick read and perhaps help out a bit with your thoughts.

The issue that comes up with this is that, for this to work the fs would have to do

if (follow_link inode method is defined)
call follow_link

where as

if (S_ISLNK(...))
call follow_link

won't work.

Of course the other way to do it would be to add another method but that would complicate an already busy link_path_walk. I don't think people would agree with that.

The side affects of setting the link bit would have undesirable consequences.

Ideas?

Ian

William H. Taber

2005-11-23 16:42 UTC #22 lore raw

Quoted 8 lines · Ian Kent

OK I think I'm starting to get what your saying. I guess I didn't want to hear it because I've been moving toward pushing everything to revalidate.
For some reason I woke up early this morning. I read your reply and I've been thinking about it all day (hard to change gears when there's a challenge like this, bad for work, fun for me, I'm sure I'll get cained for being somewhere else).
To verify I've got it this time all you are saying is, avoid revalidate by keeping all unmounted dentrys unhashed? Nothing much more really?
I haven't really thought it through completely yet. You've noticed I'm not a quick study no doubt.

Who am I to complain about that? :^)

Quoted 8 lines

So far it seems doable though. One problem point could be if one of these lookups come in during user space daemon startup but that's detail atm.
Following startup I have two cases to deal with (i'm only listing them cause I think you missed case 2):
1) No directory exists - it's created by the daemon during the callback to the userspace before it perform the mount.
2) Directory already exists - created at startup before any mount activity.

I am a little unclear about this. Which directory are you talking about? The mount-over directory or the directory being mounted? My picture of things (and I haven't verified this by doing anything mundane like reading the code) is that There is a directory (such as /net ) which is of type autofs and as needed new directories of type autofs are created under it for the various hosts/filenames. Over these subdirectories (the mount-over directories) get mounted the remote filesystems, usually of type NFS. Is this a correct understanding?

Quoted 3 lines

Case 2 needs special attention. To achieve this I would need to have two types of unhashed dentry, valid and invalid (perhaps because of a ENOENT return from the daemon on mount).

If my understanding above is correct, I don't think you need to hide the dentry for the autofs mount-over directory. If there is an active mount, then the dentries d_mounted flag will be set and the normal mountpoint traversal will work. If nothing is mounted here then the autofs mount-over directory lookup functions will be called. This is where the actual mount request gets triggered. The dentry created here should not be added to the dentry cache until the dentry is actually ready to use. It has to be kept in a way that can be found by subsequent calls to lookup in case there are multiple requests for it. The trick is that the first lookup to succeed has to be the one for the mount request. But once it is on the dentry hash chain, revalidate has to be careful because if the revalidate fails then the dentry will be invalidated. And if revalidate succeeds then everything needs to be setup so that folow_down will work. Hmm. I will have to think about this some more.

Quoted 125 lines

Not really that hard to do I think.
I'd need to rework the readdir code to fill unhashed, valid dentrys and I can set status on the return codes from the daemon. There's likely a bunch of other detail as well.
Unfortuneatly for me this is only half of the solution ... see below.
Quoted 78 lines
Quoted 9 lines
Quoted 1 line
autofs4_revalidate cannot pend. I have looked some more at the
Exactly and that's what I'm suggesting. Take account of what the lookup_one_len is advertised to do. My point is that lookup_one_len is not supposed to follow mounts or soft links, by definition, so it shouldn't cause autofs to trigger any mounts. If a filesystem wants to use it then it then it needs to take account of its defined behaviour, warts and all.
I am not expecting lookup_one_len to follow mount points. I expect to follow them myself. But I do expect that if this is a mountpoint, that autofs will set it up for me. If not, what is the point of an automounter?
Quoted 26 lines
Quoted 21 lines
real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it can find it again and wakes up the automount demon that it has work to do. It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue. 4)The automount demon will run and get the information that it needs to complete the mount request and then issue the mount. The lookup request from mount will call real_lookup. Since the demon is in OZ mode it does not pend, it fills in the dentry and when the dentry is fully ready for consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will discard the one that had been allocated.
Quoted 3 lines
This keeps all of the waiting inside the autofs4 lookup function where the lock state is defined. I realize that this may be a lot of work, but I haven't seen a possible solution that doesn't involve that.
Quoted 5 lines
Sounds like a lot of work and likely quite interesting but directories can
and often do exist in the autofs filesystem that don't have active mounts. For these directories only the revaidate method is called at auto mount time. It's worth remembering that, as autofs is a pseudo filesystem, it pins
the dentry for each of its objects so they don't go away. Maybe you're
suggesting I change this?
Exactly. If the dentries are unhashed at umount time then the revalidate case is not an issue. I don't know the how much work is invovled in setting things up in the first place so you might want to cache your unused dentries yourself if that avoids having to reread the autofs configuration files. But that is an implementation detail for you to consider.
Quoted 5 lines
Sorry, I don't mean to be rude but I'm suggesting your using lookup_one_len incorectly. I'll need to look at other code to see if this actually holds true, but, as automounting is usually not the first thing that people think of when they are writting a filesystem I expect I won't get much support from there either.
I don't know what you mean by using it incorrectly. We have a shadow filesytem and our lookup function is being called and we are trying to find the corresponding file/directory in the root filesystem. We are calling lookup_one_len because we are trying to find the next name in the path. We are prepared to handle mountpoint crossings, but as I said above, the mountpoint needs to be setup so we can cross it. We cannot call path_walk or its variants because we do not have the entire path.
Quoted 5 lines
What I'm proposing is:
1) lookup_one_len should never cause anything to be auto mounted because of its defined behaviour and autofs should behave in line with this definition.
What defined behaviour? The purpose of autofs is to automount directories. I am not looking for you to cross a mountpoint for me, I just want you to setup the mount so I can cross it myself.
Quoted 8 lines
Quoted 4 lines
2) The filesystem that calls lookup_one_len directly or indirectly is responsibe for checking if it has walked onto an autofs dentry and decide what action it should take.
And what action would that be? Not enter into any autofs directory tree? Do the mount myself? Return ENOENT? Return inconsistent results based on whether someone else has triggered the automount for me?
I was thinking of something like EAGAIN to the calling fs - meaning, ok but I need to be revalidated (lockles) for you to be sure.
Quoted 7 lines
And from an interface perspective, a caller of a function like lookup_one_len should never have to worry about the implementation of the underlying filesystem, or even have to know or care what the filesystem is.
Quoted 1 line
I thought that this was quite sensible and a fairly simple resolution?
But it defeats the whole purpose of having an automounter.
Yep. Sigh.
But there's more. The Linux autofs implementation lacks some crucial features which I really want to add.
I posted an RFC to LKML and had no interest but, since it relates to this issue, I'd really appreciate it if you could give it a quick read and perhaps help out a bit with your thoughts.
http://themaw.net/direct.txt
The issue that comes up with this is that, for this to work the fs would have to do
if (follow_link inode method is defined)
call follow_link
where as
if (S_ISLNK(...))
call follow_link

Quoted 8 lines

won't work.
Of course the other way to do it would be to add another method but that would complicate an already busy link_path_walk. I don't think people would agree with that.
The side affects of setting the link bit would have undesirable consequences.
Ideas?

I have looked at your proposal quickly and it seems reasonable on its surface. I have implemented a stackable filesystem, you are wise to want to avoid doing so. I would need to give it some more thought. But
since I will be on holiday until Monday, it is not going to happen soon.

Regards,
Will

Ian Kent

maintainer

2005-11-23 17:52 UTC #23 lore raw

Quoted 27 lines · William H. Taber

Quoted 9 lines · Ian Kent
OK I think I'm starting to get what your saying. I guess I didn't want to hear it because I've been moving toward pushing everything to revalidate.
For some reason I woke up early this morning. I read your reply and I've been thinking about it all day (hard to change gears when there's a challenge like this, bad for work, fun for me, I'm sure I'll get cained for being somewhere else).
Quoted 5 lines
To verify I've got it this time all you are saying is, avoid revalidate by keeping all unmounted dentrys unhashed? Nothing much more really?
I haven't really thought it through completely yet. You've noticed I'm not a quick study no doubt.
Who am I to complain about that? :^)
Quoted 11 lines
So far it seems doable though. One problem point could be if one of these lookups come in during user space daemon startup but that's detail atm.
Following startup I have two cases to deal with (i'm only listing them cause I think you missed case 2):
1) No directory exists - it's created by the daemon during the callback to the userspace before it perform the mount.
2) Directory already exists - created at startup before any mount activity.

Quoted 8 lines

I am a little unclear about this. Which directory are you talking about? The mount-over directory or the directory being mounted? My picture of things (and I haven't verified this by doing anything mundane like reading the code) is that There is a directory (such as /net ) which is of type autofs and as needed new directories of type autofs are created under it for the various hosts/filenames. Over these subdirectories (the mount-over directories) get mounted the remote filesystems, usually of type NFS. Is this a correct understanding?

Yep. That's an indirect mount.

The directories I'm refering to are the ones created inside the autofs mount point /net or other autofs mount point. Creating the directories makes them browsable without necessarily mounting them (as long as the module knows when to trigger a mount request). This lazyness is what causes all the fus.

Quoted 5 lines

Quoted 5 lines
Case 2 needs special attention. To achieve this I would need to have two types of unhashed dentry, valid and invalid (perhaps because of a ENOENT return from the daemon on mount).

Quoted 13 lines

If my understanding above is correct, I don't think you need to hide the dentry for the autofs mount-over directory. If there is an active mount, then the dentries d_mounted flag will be set and the normal mountpoint traversal will work. If nothing is mounted here then the autofs mount-over directory lookup functions will be called. This is where the actual mount request gets triggered. The dentry created here should not be added to the dentry cache until the dentry is actually ready to use. It has to be kept in a way that can be found by subsequent calls to lookup in case there are multiple requests for it. The trick is that the first lookup to succeed has to be the one for the mount request. But once it is on the dentry hash chain, revalidate has to be careful because if the revalidate fails then the dentry will be invalidated. And if revalidate succeeds then everything needs to be setup so that folow_down will work. Hmm. I will have to think about this some more.

That was how things used to work but late mounting is what's needed to provide the function expected of an automounter when people start using this in an enterprise environment. Basically autofs tells lies until it's forced to tell the truth.

Point is people expect to be able to see the mount points without causing them to mount until they actually try and access something inside the mount.

Hence "browseable".

Quoted 172 lines

Quoted 172 lines
Not really that hard to do I think.
I'd need to rework the readdir code to fill unhashed, valid dentrys and I can set status on the return codes from the daemon. There's likely a bunch of other detail as well.
Unfortuneatly for me this is only half of the solution ... see below.
Quoted 113 lines
Quoted 11 lines
Quoted 1 line
autofs4_revalidate cannot pend. I have looked some more at the
Exactly and that's what I'm suggesting. Take account of what the lookup_one_len is advertised to do. My point is that lookup_one_len is not
supposed to follow mounts or soft links, by definition, so it shouldn't cause autofs to trigger any mounts. If a filesystem wants to use it then it
then it needs to take account of its defined behaviour, warts and all.
I am not expecting lookup_one_len to follow mount points. I expect to follow
them myself. But I do expect that if this is a mountpoint, that autofs will
set it up for me. If not, what is the point of an automounter?
Quoted 36 lines
Quoted 29 lines
real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it
can find it again and wakes up the automount demon that it has work to do.
It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount
demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue. 4)The automount demon will run and get the information that it needs to
complete the mount request and then issue the mount. The lookup request
from mount will call real_lookup. Since the demon is in OZ mode it does
not pend, it fills in the dentry and when the dentry is fully ready for
consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will
discard the one that had been allocated.
Quoted 5 lines
This keeps all of the waiting inside the autofs4 lookup function where the
lock state is defined. I realize that this may be a lot of work, but I
haven't seen a possible solution that doesn't involve that.
Quoted 9 lines
Sounds like a lot of work and likely quite interesting but directories can
and often do exist in the autofs filesystem that don't have active mounts. For these directories only the revaidate method is called at auto mount
time. It's worth remembering that, as autofs is a pseudo filesystem, it pins
the dentry for each of its objects so they don't go away. Maybe you're suggesting I change this?
Exactly. If the dentries are unhashed at umount time then the revalidate case
is not an issue. I don't know the how much work is invovled in setting things
up in the first place so you might want to cache your unused dentries yourself
if that avoids having to reread the autofs configuration files. But that is
an implementation detail for you to consider.
Quoted 8 lines
Sorry, I don't mean to be rude but I'm suggesting your using lookup_one_len
incorectly. I'll need to look at other code to see if this actually holds
true, but, as automounting is usually not the first thing that people think
of when they are writting a filesystem I expect I won't get much support from there either.
I don't know what you mean by using it incorrectly. We have a shadow filesytem and our lookup function is being called and we are trying to find
the corresponding file/directory in the root filesystem. We are calling lookup_one_len because we are trying to find the next name in the path. We
are prepared to handle mountpoint crossings, but as I said above, the mountpoint needs to be setup so we can cross it. We cannot call path_walk or
its variants because we do not have the entire path.
Quoted 5 lines
What I'm proposing is:
1) lookup_one_len should never cause anything to be auto mounted because of its defined behaviour and autofs should behave in line with this definition.
What defined behaviour? The purpose of autofs is to automount directories. I
am not looking for you to cross a mountpoint for me, I just want you to setup
the mount so I can cross it myself.
Quoted 4 lines
2) The filesystem that calls lookup_one_len directly or indirectly is responsibe for checking if it has walked onto an autofs dentry and decide what action it should take.
Quoted 4 lines
And what action would that be? Not enter into any autofs directory tree? Do
the mount myself? Return ENOENT? Return inconsistent results based on whether someone else has triggered the automount for me?
I was thinking of something like EAGAIN to the calling fs - meaning, ok but I need to be revalidated (lockles) for you to be sure.
Quoted 8 lines
And from an interface perspective, a caller of a function like lookup_one_len
should never have to worry about the implementation of the underlying filesystem, or even have to know or care what the filesystem is.
Quoted 1 line
I thought that this was quite sensible and a fairly simple resolution?
But it defeats the whole purpose of having an automounter.
Yep. Sigh.
But there's more. The Linux autofs implementation lacks some crucial features which I really want to add.
I posted an RFC to LKML and had no interest but, since it relates to this issue, I'd really appreciate it if you could give it a quick read and perhaps help out a bit with your thoughts.
http://themaw.net/direct.txt
The issue that comes up with this is that, for this to work the fs would have to do
if (follow_link inode method is defined)
call follow_link
where as
if (S_ISLNK(...))
call follow_link
won't work.
Of course the other way to do it would be to add another method but that would complicate an already busy link_path_walk. I don't think people would agree with that.
The side affects of setting the link bit would have undesirable consequences.

Quoted 6 lines

Quoted 2 lines
Ideas?
I have looked at your proposal quickly and it seems reasonable on its surface. I have implemented a stackable filesystem, you are wise to want to avoid doing so. I would need to give it some more thought. But since I will be on holiday until Monday, it is not going to happen soon.

If you would like something to help you sleep at night then take a copy of this with you.

http://themaw.net/autofs_linux_kongress.pdf

Jeff and I wrote it and he presented a talk based on it.

Ian

Ian Kent

maintainer

2005-11-23 17:53 UTC #24 lore raw

http://themaw.net/autofs_linux_kongress.pdf

Quoted 27 lines · William H. Taber

Quoted 9 lines · Ian Kent
OK I think I'm starting to get what your saying. I guess I didn't want to hear it because I've been moving toward pushing everything to revalidate.
For some reason I woke up early this morning. I read your reply and I've been thinking about it all day (hard to change gears when there's a challenge like this, bad for work, fun for me, I'm sure I'll get cained for being somewhere else).
Quoted 5 lines
To verify I've got it this time all you are saying is, avoid revalidate by keeping all unmounted dentrys unhashed? Nothing much more really?
I haven't really thought it through completely yet. You've noticed I'm not a quick study no doubt.
Who am I to complain about that? :^)
Quoted 11 lines
So far it seems doable though. One problem point could be if one of these lookups come in during user space daemon startup but that's detail atm.
Following startup I have two cases to deal with (i'm only listing them cause I think you missed case 2):
1) No directory exists - it's created by the daemon during the callback to the userspace before it perform the mount.
2) Directory already exists - created at startup before any mount activity.

Quoted 8 lines

I am a little unclear about this. Which directory are you talking about? The mount-over directory or the directory being mounted? My picture of things (and I haven't verified this by doing anything mundane like reading the code) is that There is a directory (such as /net ) which is of type autofs and as needed new directories of type autofs are created under it for the various hosts/filenames. Over these subdirectories (the mount-over directories) get mounted the remote filesystems, usually of type NFS. Is this a correct understanding?

Yep. That's an indirect mount.

Quoted 5 lines

Quoted 5 lines
Case 2 needs special attention. To achieve this I would need to have two types of unhashed dentry, valid and invalid (perhaps because of a ENOENT return from the daemon on mount).

Quoted 13 lines

If my understanding above is correct, I don't think you need to hide the dentry for the autofs mount-over directory. If there is an active mount, then the dentries d_mounted flag will be set and the normal mountpoint traversal will work. If nothing is mounted here then the autofs mount-over directory lookup functions will be called. This is where the actual mount request gets triggered. The dentry created here should not be added to the dentry cache until the dentry is actually ready to use. It has to be kept in a way that can be found by subsequent calls to lookup in case there are multiple requests for it. The trick is that the first lookup to succeed has to be the one for the mount request. But once it is on the dentry hash chain, revalidate has to be careful because if the revalidate fails then the dentry will be invalidated. And if revalidate succeeds then everything needs to be setup so that folow_down will work. Hmm. I will have to think about this some more.

Point is people expect to be able to see the mount points without causing them to mount until they actually try and access something inside the mount.

Hence "browseable".

Quoted 172 lines

Quoted 172 lines
Not really that hard to do I think.
I'd need to rework the readdir code to fill unhashed, valid dentrys and I can set status on the return codes from the daemon. There's likely a bunch of other detail as well.
Unfortuneatly for me this is only half of the solution ... see below.
Quoted 113 lines
Quoted 11 lines
Quoted 1 line
autofs4_revalidate cannot pend. I have looked some more at the
Exactly and that's what I'm suggesting. Take account of what the lookup_one_len is advertised to do. My point is that lookup_one_len is not
supposed to follow mounts or soft links, by definition, so it shouldn't cause autofs to trigger any mounts. If a filesystem wants to use it then it
then it needs to take account of its defined behaviour, warts and all.
I am not expecting lookup_one_len to follow mount points. I expect to follow
them myself. But I do expect that if this is a mountpoint, that autofs will
set it up for me. If not, what is the point of an automounter?
Quoted 36 lines
Quoted 29 lines
real_lookup code, and it is prepared for the case in which the lookup function returns a dentry other than the one passed in. So here is a proposal that might work (but I haven't looked at the autofs4 code to verify this.)
1) A lookup request is made for a non-existant automounted file. Real_lookup calls autofs4_lookup.
2) Autofs4_lookup saves the information about this request somewhere it
can find it again and wakes up the automount demon that it has work to do.
It does not put a dentry in the dentry cache and it then releases the parent i_sem and waits for the mount to complete.
3) Any subsequent lookup for this directory that is not from the automount
demon will look for a mount request in progress, and if found, it will also release the parent lock and add itself to the wait queue. 4)The automount demon will run and get the information that it needs to
complete the mount request and then issue the mount. The lookup request
from mount will call real_lookup. Since the demon is in OZ mode it does
not pend, it fills in the dentry and when the dentry is fully ready for
consumption, it calls d_add and wakes up the waiters.
5) When the waiters wake up, they get the new dentry and real_lookup will
discard the one that had been allocated.
Quoted 5 lines
This keeps all of the waiting inside the autofs4 lookup function where the
lock state is defined. I realize that this may be a lot of work, but I
haven't seen a possible solution that doesn't involve that.
Quoted 9 lines
Sounds like a lot of work and likely quite interesting but directories can
and often do exist in the autofs filesystem that don't have active mounts. For these directories only the revaidate method is called at auto mount
time. It's worth remembering that, as autofs is a pseudo filesystem, it pins
the dentry for each of its objects so they don't go away. Maybe you're suggesting I change this?
Exactly. If the dentries are unhashed at umount time then the revalidate case
is not an issue. I don't know the how much work is invovled in setting things
up in the first place so you might want to cache your unused dentries yourself
if that avoids having to reread the autofs configuration files. But that is
an implementation detail for you to consider.
Quoted 8 lines
Sorry, I don't mean to be rude but I'm suggesting your using lookup_one_len
incorectly. I'll need to look at other code to see if this actually holds
true, but, as automounting is usually not the first thing that people think
of when they are writting a filesystem I expect I won't get much support from there either.
I don't know what you mean by using it incorrectly. We have a shadow filesytem and our lookup function is being called and we are trying to find
the corresponding file/directory in the root filesystem. We are calling lookup_one_len because we are trying to find the next name in the path. We
are prepared to handle mountpoint crossings, but as I said above, the mountpoint needs to be setup so we can cross it. We cannot call path_walk or
its variants because we do not have the entire path.
Quoted 5 lines
What I'm proposing is:
1) lookup_one_len should never cause anything to be auto mounted because of its defined behaviour and autofs should behave in line with this definition.
What defined behaviour? The purpose of autofs is to automount directories. I
am not looking for you to cross a mountpoint for me, I just want you to setup
the mount so I can cross it myself.
Quoted 4 lines
2) The filesystem that calls lookup_one_len directly or indirectly is responsibe for checking if it has walked onto an autofs dentry and decide what action it should take.
Quoted 4 lines
And what action would that be? Not enter into any autofs directory tree? Do
the mount myself? Return ENOENT? Return inconsistent results based on whether someone else has triggered the automount for me?
I was thinking of something like EAGAIN to the calling fs - meaning, ok but I need to be revalidated (lockles) for you to be sure.
Quoted 8 lines
And from an interface perspective, a caller of a function like lookup_one_len
should never have to worry about the implementation of the underlying filesystem, or even have to know or care what the filesystem is.
Quoted 1 line
I thought that this was quite sensible and a fairly simple resolution?
But it defeats the whole purpose of having an automounter.
Yep. Sigh.
But there's more. The Linux autofs implementation lacks some crucial features which I really want to add.
I posted an RFC to LKML and had no interest but, since it relates to this issue, I'd really appreciate it if you could give it a quick read and perhaps help out a bit with your thoughts.
http://themaw.net/direct.txt
The issue that comes up with this is that, for this to work the fs would have to do
if (follow_link inode method is defined)
call follow_link
where as
if (S_ISLNK(...))
call follow_link
won't work.
Of course the other way to do it would be to add another method but that would complicate an already busy link_path_walk. I don't think people would agree with that.
The side affects of setting the link bit would have undesirable consequences.

Quoted 6 lines

Quoted 2 lines
Ideas?
I have looked at your proposal quickly and it seems reasonable on its surface. I have implemented a stackable filesystem, you are wise to want to avoid doing so. I would need to give it some more thought. But since I will be on holiday until Monday, it is not going to happen soon.

If you would like something to help you sleep at night then take a copy of this with you.

Jeff and I wrote it and he presented a talk based on it.

Ian

William H. Taber

2005-11-23 18:47 UTC #25 lore raw

Quoted 11 lines · Ian Kent

Quoted 8 lines
I am a little unclear about this. Which directory are you talking about? The mount-over directory or the directory being mounted? My picture of things (and I haven't verified this by doing anything mundane like reading the code) is that There is a directory (such as /net ) which is of type autofs and as needed new directories of type autofs are created under it for the various hosts/filenames. Over these subdirectories (the mount-over directories) get mounted the remote filesystems, usually of type NFS. Is this a correct understanding?
Yep. That's an indirect mount.

OK. Got it.

Quoted 24 lines

The directories I'm refering to are the ones created inside the autofs mount point /net or other autofs mount point. Creating the directories makes them browsable without necessarily mounting them (as long as the module knows when to trigger a mount request). This lazyness is what causes all the fus.
Quoted 5 lines
Quoted 4 lines
Case 2 needs special attention. To achieve this I would need to have two types of unhashed dentry, valid and invalid (perhaps because of a ENOENT return from the daemon on mount).
Quoted 13 lines
If my understanding above is correct, I don't think you need to hide the dentry for the autofs mount-over directory. If there is an active mount, then the dentries d_mounted flag will be set and the normal mountpoint traversal will work. If nothing is mounted here then the autofs mount-over directory lookup functions will be called. This is where the actual mount request gets triggered. The dentry created here should not be added to the dentry cache until the dentry is actually ready to use. It has to be kept in a way that can be found by subsequent calls to lookup in case there are multiple requests for it. The trick is that the first lookup to succeed has to be the one for the mount request. But once it is on the dentry hash chain, revalidate has to be careful because if the revalidate fails then the dentry will be invalidated. And if revalidate succeeds then everything needs to be setup so that folow_down will work. Hmm. I will have to think about this some more.

Quoted 6 lines

That was how things used to work but late mounting is what's needed to provide the function expected of an automounter when people start using this in an enterprise environment. Basically autofs tells lies until it's forced to tell the truth.
Point is people expect to be able to see the mount points without causing them to mount until they actually try and access something inside the mount.
Hence "browseable".

OK. This is a whole other kettle of fish.
This means that readdir lies and reports about the existance of directories but doesn't mount them until someone is actually foolish enough to include them in a pathname. That's the problem. Too many fools. :^)

And thanks for the pointer to the paper.

Will

Ian Kent

maintainer

2005-11-19 01:40 UTC #26 lore raw

Quoted 24 lines · William H. Taber

Quoted 23 lines · Ian Kent
Quoted 20 lines · William H. Taber
No, it is actually a little more subtle than that. The problem is that since
you can be called from two code paths, one of which get's the lock and one of
them doesn't, you are stuck if you find that the lock is held because you don't know who holds it. The danger is that some innocent third party is holding the lock and counting on being protected by it. If you release the lock, then you can be creating the potential for a race in their code and there would be no way to detect it. Their code path would look correct because it is. Not only that but the lock itself could get confused because
it would have more unlocks than locks, because presumably the process that thinks it has the lock would eventually unlock as well. I don't know how the
semaphores are implemented on all architectures so I don't know if that would
be an actual problem or not but it I would be surprised if they all handled
that case gracefully.

Quoted 7 lines

Quoted 6 lines
Yes. Thinking about it I hadn't considered the third process.
How can we get advice on this?
Ian
Hey, I'm new here. I came here looking for advice. :^)

Ha. It's probably wearing a bit thin for me to say I'm new here but my patch is quite small and I've got a lot to learn and I'm lovin' it.

Quoted 6 lines

However, if there is no way to fix this in the autofs code then the only alternative is to fix the locking in the VFS. So VFS folks, which is the better solution. Should all callers to d_revalidate get the parent's i_sem lock or should d_revalidate never be called with the parent i_sem lock and so we need to change lookup_one_len so that it does not expect the parent i_sem lock to be held?

Maybe. See next post.

Ian

Jeff Moyer

2005-11-16 15:22 UTC #27 lore raw

http://www.sudhaa.com/~ram/readahead/stubfs.patch

==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:

ram> Autofs4 assumes that its ->revalidate() function gets called with the ram> parent_dentry's_inode_semaphore released. This is true mostly ram> but not in one particular case.

ram> Process P1 calls autofs4's ->lookup(). The lookup finds that the dentry ram> does not exist. It creates a dentry and adds to the cache. Releases ram> the parent's inode's semaphore and than calls ->revalidate().

ram> Process P2 meanwhile comes in and cached_lookup() gets called. It finds ram> the dentry in the cache and finds ->revalidate() function exists. So ram> it calls ->revalidate() holding the parent's inode's semaphore.

ram> Now the automounter daemon comes in and tries to hold the same semaphore ram> in order to mount. But since the semaphore is held by P2 it ram> goes to sleep.

ram> Process P1 and P2 continue waiting for the mount to complete and it never ram> happens. Deadlock.

ram> The stack of the deadlock is as follows:

ram> ls S 00000000 0 13049 11954 (NOTLB)
ram> f5221df0 00000000 00000000 00000000 00000000 00000000 00000000 00000000 ram> 00000000 f5d44a70 c721b520 00000000 d4f33800 003d0990 c721b9d8 f5d44030 ram> f5d44164 f5220000 f5221e3c f3dd6880 f5221e68 c0215207 f3b95580 80000000 ram> Call Trace:
ram> [<c0215207>] autofs4_wait+0x307/0x3d0
ram> [<c02141d3>] try_to_fill_dentry+0xf3/0x150
ram> [<c0214389>] autofs4_revalidate+0x159/0x170
ram> [<c02144e0>] autofs4_lookup+0x110/0x150
ram> [<c016f3f5>] __lookup_hash+0x85/0xb0
ram> [<c016f42a>] lookup_hash+0xa/0x10
ram> [<c016f483>] lookup_one_len+0x53/0x70
ram> [<f8851293>] stubfs_readdir+0x113/0x170 [stubfs]

What's stubfs?

-Jeff

Ram Pai

author

2005-11-16 17:00 UTC #28 lore raw

Quoted 37 lines · Jeff Moyer

==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:
ram> Autofs4 assumes that its ->revalidate() function gets called with the ram> parent_dentry's_inode_semaphore released. This is true mostly ram> but not in one particular case.
ram> Process P1 calls autofs4's ->lookup(). The lookup finds that the dentry ram> does not exist. It creates a dentry and adds to the cache. Releases ram> the parent's inode's semaphore and than calls ->revalidate().
ram> Process P2 meanwhile comes in and cached_lookup() gets called. It finds ram> the dentry in the cache and finds ->revalidate() function exists. So ram> it calls ->revalidate() holding the parent's inode's semaphore.
ram> Now the automounter daemon comes in and tries to hold the same semaphore ram> in order to mount. But since the semaphore is held by P2 it ram> goes to sleep.
ram> Process P1 and P2 continue waiting for the mount to complete and it never ram> happens. Deadlock.
ram> The stack of the deadlock is as follows:
ram> ls S 00000000 0 13049 11954 (NOTLB)
ram> f5221df0 00000000 00000000 00000000 00000000 00000000 00000000 00000000 ram> 00000000 f5d44a70 c721b520 00000000 d4f33800 003d0990 c721b9d8 f5d44030 ram> f5d44164 f5220000 f5221e3c f3dd6880 f5221e68 c0215207 f3b95580 80000000 ram> Call Trace:
ram> [<c0215207>] autofs4_wait+0x307/0x3d0
ram> [<c02141d3>] try_to_fill_dentry+0xf3/0x150
ram> [<c0214389>] autofs4_revalidate+0x159/0x170
ram> [<c02144e0>] autofs4_lookup+0x110/0x150
ram> [<c016f3f5>] __lookup_hash+0x85/0xb0
ram> [<c016f42a>] lookup_hash+0xa/0x10
ram> [<c016f483>] lookup_one_len+0x53/0x70
ram> [<f8851293>] stubfs_readdir+0x113/0x170 [stubfs]

Quoted 1 line

What's stubfs?

Its a small stub filesystem we wrote(thanks to Will Taber) to demonstrate the problem. All it does is holds the parent's inode-semaphore before calling lookup_one_len() on the dentry that needs a automount.

This problem demonstrates a very very small race window which cannot be triggered in normal operations. The stubfs kind of orchestrates the exact timing to demonstrate the problem.

note: the timing should be such that, process 1 should have added the newly created dentry in the dcache and jolted the automounter daemon. And then process P2 has to come in asking for the same dentry, and should go to sleep waiting on the automounter to mount at the dentry. And finally the automounter has to come in.

Quoted 2 lines

-Jeff

Jeff Moyer

2005-11-16 18:25 UTC #29 lore raw

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Ram Pai <[email protected]> adds:

Quoted 3 lines · Jeff Moyer

Quoted 3 lines
==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:

ram> Autofs4 assumes that its ->revalidate() function gets called with the ram> parent_dentry's_inode_semaphore released. This is true mostly but not ram> in one particular case.

Quoted 1 line

Quoted 1 line

ram> Process P1 calls autofs4's ->lookup(). The lookup finds that the ram> dentry does not exist. It creates a dentry and adds to the ram> cache. Releases the parent's inode's semaphore and than calls ram> ->revalidate().

Quoted 1 line

Quoted 1 line

ram> Process P2 meanwhile comes in and cached_lookup() gets called. It ram> finds the dentry in the cache and finds ->revalidate() function ram> exists. So it calls ->revalidate() holding the parent's inode's ram> semaphore.

Quoted 1 line

Quoted 1 line

ram> Now the automounter daemon comes in and tries to hold the same ram> semaphore in order to mount. But since the semaphore is held by P2 it ram> goes to sleep.

Quoted 1 line

Quoted 1 line

ram> Process P1 and P2 continue waiting for the mount to complete and it ram> never happens. Deadlock.

Quoted 1 line

Quoted 1 line

ram> The stack of the deadlock is as follows:

Quoted 1 line

Quoted 1 line

ram> ls S 00000000 0 13049 11954 (NOTLB) f5221df0 00000000 00000000 ram> 00000000 00000000 00000000 00000000 00000000 00000000 f5d44a70 ram> c721b520 00000000 d4f33800 003d0990 c721b9d8 f5d44030 f5d44164 ram> f5220000 f5221e3c f3dd6880 f5221e68 c0215207 f3b95580 80000000 Call ram> Trace: [<c0215207>] autofs4_wait+0x307/0x3d0 [<c02141d3>] ram> try_to_fill_dentry+0xf3/0x150 [<c0214389>]
ram> autofs4_revalidate+0x159/0x170 [<c02144e0>] autofs4_lookup+0x110/0x150 ram> [<c016f3f5>] __lookup_hash+0x85/0xb0 [<c016f42a>] lookup_hash+0xa/0x10 ram> [<c016f483>] lookup_one_len+0x53/0x70 [<f8851293>]
ram> stubfs_readdir+0x113/0x170 [stubfs]

Quoted 1 line

Quoted 1 line
What's stubfs?

linuxram> Its a small stub filesystem we wrote(thanks to Will Taber) to linuxram> demonstrate the problem. All it does is holds the parent's linuxram> inode-semaphore before calling lookup_one_len() on the dentry linuxram> that needs a automount.

linuxram> This problem demonstrates a very very small race window which linuxram> cannot be triggered in normal operations. The stubfs kind of linuxram> orchestrates the exact timing to demonstrate the problem.

linuxram> note: the timing should be such that, process 1 should have added linuxram> the newly created dentry in the dcache and jolted the automounter linuxram> daemon. And then process P2 has to come in asking for the same linuxram> dentry, and should go to sleep waiting on the automounter to linuxram> mount at the dentry. And finally the automounter has to come in.

I've been trying to reproduce this using sleeps in the user space daemon, and I can't. Can you post your test code so that I'm not guessing at what's going on? For example, one thing that's unclear is how you are stuffing stubfs in between the vfs and autofs.

-Jeff

William H. Taber

2005-11-16 19:24 UTC #30 lore raw

Review

Quoted 10 lines · Jeff Moyer

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Ram Pai <[email protected]> adds:
I've been trying to reproduce this using sleeps in the user space daemon, and I can't. Can you post your test code so that I'm not guessing at what's going on? For example, one thing that's unclear is how you are stuffing stubfs in between the vfs and autofs.
(quoted diff) +0 -2
-Jeff
-

The stubfs is just a test filesystem I wrote to reproduce this problem.
It doesn't sit between the vfs and autofs. What it does is to do a
lookup on /net and save the inode for it. Then it gets the i_sem on /net and calls lookup_one_len on a given hostname. The second time in it omits the lookup (it already has the inode for /net) and down on the i_sem lock and then calls lookup_one_len. It has some of it's own locking to get properly synchronized to force the race condition. What happens is that both processes are waiting on i_sem for /net. The first one gets it, and calls into the autofs which creates the new dentry, starts the automount deamon, and waits for the mount to complete. Since the second lookup is already queued on the i_sem, it get's in second, finds the dentry, calls revalidate which waits for the mount to complete without releasing i_sem. This of course prevents the automounter from completing the mount.

Does this clarify?

Will Taber

Ram Pai

author

2005-11-16 19:51 UTC #31 lore raw

Quoted 26 lines · William H. Taber

Review
Quoted 10 lines · Jeff Moyer
==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Ram Pai <[email protected]> adds:
I've been trying to reproduce this using sleeps in the user space daemon, and I can't. Can you post your test code so that I'm not guessing at what's going on? For example, one thing that's unclear is how you are stuffing stubfs in between the vfs and autofs.
(quoted diff) +0 -2
-Jeff
-
The stubfs is just a test filesystem I wrote to reproduce this problem.
It doesn't sit between the vfs and autofs. What it does is to do a
lookup on /net and save the inode for it. Then it gets the i_sem on /net and calls lookup_one_len on a given hostname. The second time in it omits the lookup (it already has the inode for /net) and down on the i_sem lock and then calls lookup_one_len. It has some of it's own locking to get properly synchronized to force the race condition. What happens is that both processes are waiting on i_sem for /net. The first one gets it, and calls into the autofs which creates the new dentry, starts the automount deamon, and waits for the mount to complete. Since the second lookup is already queued on the i_sem, it get's in second, finds the dentry, calls revalidate which waits for the mount to complete without releasing i_sem. This of course prevents the automounter from completing the mount.

Quoted 1 line

Does this clarify?

here is the pointer to the patch that applies and compiles on 2.6.15-rc1.

you will have to set up /net/ram as the automounter location. That string is hardcoded. Maybe you can change that string to your environment. And you can modify to it make it a module.

the instructions on how to reproduce is there in fs/stubfs/stub.c line number 129 roughly.

This GPL code is for testing purpose only,
RP

Quoted 1 line

Will Taber

Ian Kent

maintainer

2005-11-27 10:47 UTC #32 lore raw

Quoted 59 lines · Ram Pai

Autofs4 assumes that its ->revalidate() function gets called with the parent_dentry's_inode_semaphore released. This is true mostly but not in one particular case.
Process P1 calls autofs4's ->lookup(). The lookup finds that the dentry does not exist. It creates a dentry and adds to the cache. Releases the parent's inode's semaphore and than calls ->revalidate().
Process P2 meanwhile comes in and cached_lookup() gets called. It finds the dentry in the cache and finds ->revalidate() function exists. So it calls ->revalidate() holding the parent's inode's semaphore.
Now the automounter daemon comes in and tries to hold the same semaphore in order to mount. But since the semaphore is held by P2 it goes to sleep.
Process P1 and P2 continue waiting for the mount to complete and it never happens. Deadlock.
The stack of the deadlock is as follows:
ls S 00000000 0 13049 11954 (NOTLB)
f5221df0 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 f5d44a70 c721b520 00000000 d4f33800 003d0990 c721b9d8 f5d44030 f5d44164 f5220000 f5221e3c f3dd6880 f5221e68 c0215207 f3b95580 80000000
Call Trace:
[<c0215207>] autofs4_wait+0x307/0x3d0
[<c02141d3>] try_to_fill_dentry+0xf3/0x150
[<c0214389>] autofs4_revalidate+0x159/0x170
[<c02144e0>] autofs4_lookup+0x110/0x150
[<c016f3f5>] __lookup_hash+0x85/0xb0
[<c016f42a>] lookup_hash+0xa/0x10
[<c016f483>] lookup_one_len+0x53/0x70
[<f8851293>] stubfs_readdir+0x113/0x170 [stubfs]
[<c0172fcb>] vfs_readdir+0x8b/0xa0
[<c01733b3>] sys_getdents64+0x63/0xb5
[<c010464d>] syscall_call+0x7/0xb
ls S C011B1AF 0 13050 11898 (NOTLB)
f1337df0 00000082 f1337e04 c011b1af 06ce3f60 00000027 00000027 00000080 06d03f60 00000000 c721b520 00000000 d4f33800 003d0990 f1337df0 f5d44a70 f5d44ba4 f1336000 f1337e3c f3dd6880 f1337e68 c0215207 f3b95580 80000000
Call Trace:
[<c0215207>] autofs4_wait+0x307/0x3d0
[<c02141d3>] try_to_fill_dentry+0xf3/0x150
[<c0214389>] autofs4_revalidate+0x159/0x170
[<c016dc77>] cached_lookup+0x47/0x80
[<c016f3ca>] __lookup_hash+0x5a/0xb0
[<c016f42a>] lookup_hash+0xa/0x10
[<c016f483>] lookup_one_len+0x53/0x70  
[<f88512e3>] stubfs_readdir+0x163/0x170 [stubfs]
[<c0172fcb>] vfs_readdir+0x8b/0xa0  
[<c01733b3>] sys_getdents64+0x63/0xb5
[<c010464d>] syscall_call+0x7/0xb
automount D 00000010 0 13052 13016 (NOTLB)
f3321f00 fff80000 00000007 00000010 f3321f68 c7b1cd20 00000000 f3321f34 f3321ee8 f5e92a70 c7233520 00000000 d5304100 003d0990 c7233560 f1e31a70 f1e31ba4 f5f59914 f5f5991c 00000296 f3321f38 c03b4cd3 f1e31a70 00000001

Quoted 8 lines

Call Trace:
[<c03b4cd3>] __down+0x83/0xe0
[<c03b3632>] __down_failed+0xa/0x10
[<c0171e6d>] .text.lock.namei+0xeb/0x1de
[<c0170482>] sys_mkdir+0x52/0xd0
[<c010464d>] syscall_call+0x7/0xb
BUG: soft lockup detected on CPU#0!

Hi guys,

I've been thinking about this one for a while now and have a suggestion about how it may be fixed.

To re-state the problem:

The autofs4 revalidate callback needs to function properly when called with the inode semaphore either held or not.

Summary:

Ram Pai provided the excelent problem profile above and offered a patch for comment which droped the inode semaphore. Will pointed out that droping the semaphore was not a good thing to do because of possible side affects.

A fair bit of interesting discussion followed.

My thoughts:

The cause of this issue is user space programs using autofs4 need to call services that must be able to take the inode semaphore. Notably sys_mkdir and sys_symlink in order to complete their task.

I believe that, in this case, releasing the semaphore is ok since the entry is part of the autofs filesystem and so autofs is responsible for taking care of it, provided that it is done carefully. The semaphore is meant to serialize changes being to the directory and these changes are done in autofs by asking the user space process to do it. Which are themselves serialized by the same semaphore.

The only tricky thing I can think of here is that care must be taken to ensure that the semaphore is not released before the DCACHE_AUTOFS_PENDING flag is set to make sure that other incoming requests are sent to the wait queue.

The attached patch does this and opts for a conservative approach by broadening the critical region instead of narrowing it.

It may also be necessary to review the return codes from revaliate but I'm only part way through that.

Please review and test this patch and offer further comment. Sorry guys but I haven't been able to test this at all save verifying that it compiles.

Hopefully I haven't missed anything completely obvious ... DOH!

Ian

fs/autofs4/root.c renamed from fs/autofs4/root.c.lookup-deadlock +1 -4

@@ -487,11 +487,8 @@ static struct dentry *autofs4_lookup(str
 	dentry->d_fsdata = NULL;
 	d_add(dentry, NULL);
 
-	if (dentry->d_op && dentry->d_op->d_revalidate) {
-		up(&dir->i_sem);
+	if (dentry->d_op && dentry->d_op->d_revalidate)
 		(dentry->d_op->d_revalidate)(dentry, nd);
-		down(&dir->i_sem);
-	}
 
 	/*
 	 * If we are still pending, check if we had to handle

fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +14 -0

@@ -161,6 +161,8 @@ int autofs4_wait(struct autofs_sb_info *
 		enum autofs_notify notify)
 {
 	struct autofs_wait_queue *wq;
+	struct inode *dir = dentry->d_parent->d_inode;
+	int i_sem_held;
 	char *name;
 	int len, status;

@@ -227,6 +229,14 @@ int autofs4_wait(struct autofs_sb_info *
 			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
 	}
 
+	/*
+	 * If we are called from lookup or lookup_hash the
+	 * the inode semaphore needs to be released for
+	 * userspace to do its thing.
+	 */
+	i_sem_held = down_trylock(&dir->i_sem);
+	up(&dir->i_sem);
+
 	if (notify != NFY_NONE && atomic_dec_and_test(&wq->notified)) {
 		int type = (notify == NFY_MOUNT ?
 			autofs_ptype_missing : autofs_ptype_expire_multi);

@@ -268,6 +278,10 @@ int autofs4_wait(struct autofs_sb_info *
 		DPRINTK("skipped sleeping");
 	}
 
+	/* Re-take the inode semaphore if it was held */
+	if (i_sem_held)
+		down(&dir->i_sem);
+
 	status = wq->status;
 
 	/* Are we the last process to need status? */

William H. Taber

2005-11-28 17:19 UTC #33 lore raw

Quoted 62 lines · Ian Kent

My thoughts:
The cause of this issue is user space programs using autofs4 need to call services that must be able to take the inode semaphore. Notably sys_mkdir and sys_symlink in order to complete their task.
I believe that, in this case, releasing the semaphore is ok since the entry is part of the autofs filesystem and so autofs is responsible for taking care of it, provided that it is done carefully. The semaphore is meant to serialize changes being to the directory and these changes are done in autofs by asking the user space process to do it. Which are themselves serialized by the same semaphore.
The only tricky thing I can think of here is that care must be taken to ensure that the semaphore is not released before the DCACHE_AUTOFS_PENDING flag is set to make sure that other incoming requests are sent to the wait queue.
The attached patch does this and opts for a conservative approach by broadening the critical region instead of narrowing it.
It may also be necessary to review the return codes from revaliate but I'm only part way through that.
Please review and test this patch and offer further comment.
Sorry guys but I haven't been able to test this at all save verifying that it compiles.
Hopefully I haven't missed anything completely obvious ... DOH!
Ian
fs/autofs4/root.c renamed from fs/autofs4/root.c.lookup-deadlock +1 -4
@@ -487,11 +487,8 @@ static struct dentry *autofs4_lookup(str
 	dentry->d_fsdata = NULL;
 	d_add(dentry, NULL);
 
-	if (dentry->d_op && dentry->d_op->d_revalidate) {
-		up(&dir->i_sem);
+	if (dentry->d_op && dentry->d_op->d_revalidate)
 		(dentry->d_op->d_revalidate)(dentry, nd);
-		down(&dir->i_sem);
-	}
 
 	/*
 	 * If we are still pending, check if we had to handle
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +10 -0
@@ -161,6 +161,8 @@ int autofs4_wait(struct autofs_sb_info *
 		enum autofs_notify notify)
 {
 	struct autofs_wait_queue *wq;
+	struct inode *dir = dentry->d_parent->d_inode;
+	int i_sem_held;
 	char *name;
 	int len, status;
 
@@ -227,6 +229,14 @@ int autofs4_wait(struct autofs_sb_info *
 			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
 	}
 
+	/*
+	 * If we are called from lookup or lookup_hash the
+	 * the inode semaphore needs to be released for
+	 * userspace to do its thing.
+	 */
+	i_sem_held = down_trylock(&dir->i_sem);
+	up(&dir->i_sem);
+
 	if (notify != NFY_NONE && atomic_dec_and_test(&wq->notified)) {
 		int type = (notify == NFY_MOUNT ?
 			autofs_ptype_missing : autofs_ptype_expire_multi);
@@ -268,6 +278,10 @@ int autofs4_wait(struct autofs_sb_info *
 		DPRINTK("skipped sleeping");

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *

Quoted 10 lines

fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +4 -1

 	}
 
+	/* Re-take the inode semaphore if it was held */
+	if (i_sem_held)
+		down(&dir->i_sem);
+
 	status = wq->status;
 
 	/* Are we the last process to need status? */
-

Ian,
I have not tested this patch but it seems to have a serious flaw. Given that do_lookup does not get the parent i_sem lock before calling revalidate, you have the possibility that you are being called without having gotten the lock but the lock may be held by another process. In that case you do not want to be releasing their lock while they are relying on it.

Will

Badari Pulavarty

2005-11-28 23:12 UTC #34 lore raw

Quoted 85 lines · William H. Taber

Quoted 83 lines · Ian Kent
My thoughts:
The cause of this issue is user space programs using autofs4 need to call services that must be able to take the inode semaphore. Notably sys_mkdir and sys_symlink in order to complete their task.
I believe that, in this case, releasing the semaphore is ok since the entry is part of the autofs filesystem and so autofs is responsible for taking care of it, provided that it is done carefully. The semaphore is meant to serialize changes being to the directory and these changes are done in autofs by asking the user space process to do it. Which are themselves serialized by the same semaphore.
The only tricky thing I can think of here is that care must be taken to ensure that the semaphore is not released before the DCACHE_AUTOFS_PENDING flag is set to make sure that other incoming requests are sent to the wait queue.
The attached patch does this and opts for a conservative approach by broadening the critical region instead of narrowing it.
It may also be necessary to review the return codes from revaliate but I'm only part way through that.
Please review and test this patch and offer further comment. Sorry guys but I haven't been able to test this at all save verifying that it compiles.
Hopefully I haven't missed anything completely obvious ... DOH!
Ian
fs/autofs4/root.c renamed from fs/autofs4/root.c.lookup-deadlock +1 -4
@@ -487,11 +487,8 @@ static struct dentry *autofs4_lookup(str
 	dentry->d_fsdata = NULL;
 	d_add(dentry, NULL);
 
-	if (dentry->d_op && dentry->d_op->d_revalidate) {
-		up(&dir->i_sem);
+	if (dentry->d_op && dentry->d_op->d_revalidate)
 		(dentry->d_op->d_revalidate)(dentry, nd);
-		down(&dir->i_sem);
-	}
 
 	/*
 	 * If we are still pending, check if we had to handle
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +14 -0
@@ -161,6 +161,8 @@ int autofs4_wait(struct autofs_sb_info *
 		enum autofs_notify notify)
 {
 	struct autofs_wait_queue *wq;
+	struct inode *dir = dentry->d_parent->d_inode;
+	int i_sem_held;
 	char *name;
 	int len, status;
 
@@ -227,6 +229,14 @@ int autofs4_wait(struct autofs_sb_info *
 			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
 	}
 
+	/*
+	 * If we are called from lookup or lookup_hash the
+	 * the inode semaphore needs to be released for
+	 * userspace to do its thing.
+	 */
+	i_sem_held = down_trylock(&dir->i_sem);
+	up(&dir->i_sem);
+
 	if (notify != NFY_NONE && atomic_dec_and_test(&wq->notified)) {
 		int type = (notify == NFY_MOUNT ?
 			autofs_ptype_missing : autofs_ptype_expire_multi);
@@ -268,6 +278,10 @@ int autofs4_wait(struct autofs_sb_info *
 		DPRINTK("skipped sleeping");
 	}
 
+	/* Re-take the inode semaphore if it was held */
+	if (i_sem_held)
+		down(&dir->i_sem);
+
 	status = wq->status;
 

Quoted 10 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *
Quoted 2 lines
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +0 -1
 	/* Are we the last process to need status? */
-
Ian,
I have not tested this patch but it seems to have a serious flaw. Given that do_lookup does not get the parent i_sem lock before calling revalidate, you have the possibility that you are being called without having gotten the lock but the lock may be held by another process. In that case you do not want to be releasing their lock while they are relying on it.

Here is the patch Will Taber proposed and I am posting on his behalf.

Thanks,
Badari

This patch changes the semantics of d_revalidate so that it is always called with the parent i_sem lock held. This allows the autofs4 code to release the lock if it needs to pend. Without this patch the autofs has a race condition in which it pends in the revalidate code while holding the parent i_sem lock which prevents the mount from ever completing. There have been other patches proposed for this problem which check to see if the parent i_sem lock is held before releasing it but those solutions ignore the possibility that the lock may be held by another process.

fs/autofs4/root.c +6 -0

diff -ur linux-2.6.13.3/fs/autofs4/root.c linux-2.6.13.3-autofspatch/fs/autofs4/root.c
@@ -302,7 +302,9 @@
 		DPRINTK("waiting for expire %p name=%.*s",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 
+		up(&dentry->d_parent->d_inode->i_sem);
 		status = autofs4_wait(sbi, dentry, NFY_NONE);
+		down(&dentry->d_parent->d_inode->i_sem);
 		
 		DPRINTK("expire done status=%d", status);

@@ -324,7 +326,9 @@
 		DPRINTK("waiting for mount name=%.*s",
 			 dentry->d_name.len, dentry->d_name.name);
 
+		up(&dentry->d_parent->d_inode->i_sem);
 		status = autofs4_wait(sbi, dentry, NFY_MOUNT);
+		down(&dentry->d_parent->d_inode->i_sem);
 		 
 		DPRINTK("mount done status=%d", status);

@@ -351,7 +355,9 @@
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_AUTOFS_PENDING;
 		spin_unlock(&dentry->d_lock);
+		up(&dentry->d_parent->d_inode->i_sem);
 		status = autofs4_wait(sbi, dentry, NFY_MOUNT);
+		down(&dentry->d_parent->d_inode->i_sem);
 
 		DPRINTK("mount done status=%d", status);

fs/namei.c +14 -5

diff -ur linux-2.6.13.3/fs/namei.c linux-2.6.13.3-autofspatch/fs/namei.c
@@ -393,7 +393,6 @@
 	struct dentry * result;
 	struct inode *dir = parent->d_inode;
 
-	down(&dir->i_sem);
 	/*
 	 * First re-do the cached lookup just in case it was created
 	 * while we waited for the directory semaphore..

@@ -419,7 +418,6 @@
 			else
 				result = dentry;
 		}
-		up(&dir->i_sem);
 		return result;
 	}

@@ -427,7 +425,6 @@
 	 * Uhhuh! Nasty case: the cache was re-populated while
 	 * we waited on the semaphore. Need to revalidate.
 	 */
-	up(&dir->i_sem);
 	if (result->d_op && result->d_op->d_revalidate) {
 		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
 			dput(result);

@@ -676,13 +673,16 @@
 		     struct path *path)
 {
 	struct vfsmount *mnt = nd->mnt;
+	struct inode *parent = nd->dentry->d_inode;
 	struct dentry *dentry = __d_lookup(nd->dentry, name);
 
+	down(&parent->i_sem);
 	if (!dentry)
 		goto need_lookup;
 	if (dentry->d_op && dentry->d_op->d_revalidate)
 		goto need_revalidate;
 done:
+	up(&parent->i_sem);
 	path->mnt = mnt;
 	path->dentry = dentry;
 	__follow_mount(path);

@@ -703,6 +703,7 @@
 	goto need_lookup;
 
 fail:
+	up(&parent->i_sem);
 	return PTR_ERR(dentry);
 }

@@ -718,7 +719,7 @@
 {
 	struct path next;
 	struct inode *inode;
-	int err;
+	int err, reval;
 	unsigned int lookup_flags = nd->flags;
 	
 	while (*name=='/')

@@ -893,9 +894,17 @@
 		 */
 		if (nd->dentry && nd->dentry->d_sb &&
 		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+			struct dentry *nparent;
+
 			err = -ESTALE;
 			/* Note: we do not d_invalidate() */
-			if (!nd->dentry->d_op->d_revalidate(nd->dentry, nd))
+			/* Revalidate requires us to lock the parent.
+			 */
+			nparent = nd->dentry->d_parent;
+			down(&nparent->d_inode->i_sem);
+			reval = nd->dentry->d_op->d_revalidate(nd->dentry, nd);
+			up(&nparent->d_inode->i_sem);
+			if (!reval)
 				break;
 		}
 return_base:

Ian Kent

maintainer

2005-11-29 01:19 UTC #35 lore raw

We'll need to do an analysis of all callers of the revalidate method.

Quoted 105 lines · Badari Pulavarty

Quoted 85 lines · William H. Taber
Quoted 83 lines · Ian Kent
My thoughts:
The cause of this issue is user space programs using autofs4 need to call services that must be able to take the inode semaphore. Notably sys_mkdir and sys_symlink in order to complete their task.
I believe that, in this case, releasing the semaphore is ok since the entry is part of the autofs filesystem and so autofs is responsible for taking care of it, provided that it is done carefully. The semaphore is meant to serialize changes being to the directory and these changes are done in autofs by asking the user space process to do it. Which are themselves serialized by the same semaphore.
The only tricky thing I can think of here is that care must be taken to ensure that the semaphore is not released before the DCACHE_AUTOFS_PENDING flag is set to make sure that other incoming requests are sent to the wait queue.
The attached patch does this and opts for a conservative approach by broadening the critical region instead of narrowing it.
It may also be necessary to review the return codes from revaliate but I'm only part way through that.
Please review and test this patch and offer further comment. Sorry guys but I haven't been able to test this at all save verifying that it compiles.
Hopefully I haven't missed anything completely obvious ... DOH!
Ian
fs/autofs4/root.c renamed from fs/autofs4/root.c.lookup-deadlock +1 -4
@@ -487,11 +487,8 @@ static struct dentry *autofs4_lookup(str
 	dentry->d_fsdata = NULL;
 	d_add(dentry, NULL);
 
-	if (dentry->d_op && dentry->d_op->d_revalidate) {
-		up(&dir->i_sem);
+	if (dentry->d_op && dentry->d_op->d_revalidate)
 		(dentry->d_op->d_revalidate)(dentry, nd);
-		down(&dir->i_sem);
-	}
 
 	/*
 	 * If we are still pending, check if we had to handle
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +14 -0
@@ -161,6 +161,8 @@ int autofs4_wait(struct autofs_sb_info *
 		enum autofs_notify notify)
 {
 	struct autofs_wait_queue *wq;
+	struct inode *dir = dentry->d_parent->d_inode;
+	int i_sem_held;
 	char *name;
 	int len, status;
 
@@ -227,6 +229,14 @@ int autofs4_wait(struct autofs_sb_info *
 			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
 	}
 
+	/*
+	 * If we are called from lookup or lookup_hash the
+	 * the inode semaphore needs to be released for
+	 * userspace to do its thing.
+	 */
+	i_sem_held = down_trylock(&dir->i_sem);
+	up(&dir->i_sem);
+
 	if (notify != NFY_NONE && atomic_dec_and_test(&wq->notified)) {
 		int type = (notify == NFY_MOUNT ?
 			autofs_ptype_missing : autofs_ptype_expire_multi);
@@ -268,6 +278,10 @@ int autofs4_wait(struct autofs_sb_info *
 		DPRINTK("skipped sleeping");
 	}
 
+	/* Re-take the inode semaphore if it was held */
+	if (i_sem_held)
+		down(&dir->i_sem);
+
 	status = wq->status;
 
Quoted 10 lines
fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *
Quoted 2 lines
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +0 -1
 	/* Are we the last process to need status? */
-
Ian,
I have not tested this patch but it seems to have a serious flaw. Given that do_lookup does not get the parent i_sem lock before calling revalidate, you have the possibility that you are being called without having gotten the lock but the lock may be held by another process. In that case you do not want to be releasing their lock while they are relying on it.
Here is the patch Will Taber proposed and I am posting on his behalf.
Thanks,
Badari

William H. Taber

2005-11-29 16:34 UTC #36 lore raw

Quoted 1 line · Ian Kent

We'll need to do an analysis of all callers of the revalidate method.

You are right. Searching through the sources, it would appear that I missed fixing autofs and devfs. Everyone else just defines a revalidate routine but doesn't call one. You may find devfs to be interesting because they have code to determine whether they need to release the i_sem lock or not. I am working on an updated patch to include the changes needed for these two modules.

Will

Ian Kent

maintainer

2005-11-30 01:02 UTC #37 lore raw

Quoted 8 lines · William H. Taber

Quoted 1 line · Ian Kent
We'll need to do an analysis of all callers of the revalidate method.
You are right. Searching through the sources, it would appear that I missed fixing autofs and devfs. Everyone else just defines a revalidate routine but doesn't call one. You may find devfs to be interesting because they have code to determine whether they need to release the i_sem lock or not. I am working on an updated patch to include the changes needed for these two modules.

I've looked at devfs before but that bit of code sounds interesting to me.

The other thing that concerns me is that we may be increasing the latency of some code paths that need to be really fast. I was thinking that perhaps it might be good to try a change more in line with the locking used in link_patch_walk (ie. i_sem free revalidate) rather than that used in lookup_one_len. My only justification being that lookup is called to create stuff where revalidate is called to check stuff. I've been poking around and this change looks fairly difficult as well (I seem to remember you also looked at this).

Anyway, I'm keen to have a look at your patch.
Thanks much for your interest and help.

Ian

Badari Pulavarty

2005-11-30 16:49 UTC #38 lore raw

Quoted 22 lines · Ian Kent

Quoted 8 lines · William H. Taber
Quoted 1 line · Ian Kent
We'll need to do an analysis of all callers of the revalidate method.
You are right. Searching through the sources, it would appear that I missed fixing autofs and devfs. Everyone else just defines a revalidate routine but doesn't call one. You may find devfs to be interesting because they have code to determine whether they need to release the i_sem lock or not. I am working on an updated patch to include the changes needed for these two modules.
I've looked at devfs before but that bit of code sounds interesting to me.
The other thing that concerns me is that we may be increasing the latency of some code paths that need to be really fast. I was thinking that perhaps it might be good to try a change more in line with the locking used in link_patch_walk (ie. i_sem free revalidate) rather than that used in lookup_one_len. My only justification being that lookup is called to create stuff where revalidate is called to check stuff. I've been poking around and this change looks fairly difficult as well (I seem to remember you also looked at this).

Quoted 5 lines

Anyway, I'm keen to have a look at your patch.
Thanks much for your interest and help.
Ian

Again, I am posting Will's latest patch on his behalf.

Any thoughts on how acceptable are the VFS changes ?

Thanks,
Badari

This patch has been expanded to include changes to autofs and devfs. The autofs changes mimic the changes to autofs4 in the original patch. The devfs changes remove the checking it did to try to determine where it was called from so that it could get its locking right.

fs/autofs/root.c +6 -3

diff -ur -x '*.orig' -x '*.new' linux-2.6.13.3/fs/autofs/root.c linux-2.6.13.3-autofspatch/fs/autofs/root.c
@@ -104,7 +104,9 @@
 				/* Return a negative dentry, but leave it "pending" */
 				return 1;
 			}
+			up(&dentry->d_parent->d_inode->i_sem);
 			status = autofs_wait(sbi, &dentry->d_name);
+			down(&dentry->d_parent->d_inode->i_sem);
 		} while (!(ent = autofs_hash_lookup(&sbi->dirhash, &dentry->d_name)) );
 	}

@@ -124,7 +126,10 @@
 	/* If this is a directory that isn't a mount point, bitch at the
 	   daemon and fix it in user space */
 	if ( S_ISDIR(dentry->d_inode->i_mode) && !d_mountpoint(dentry) ) {
-		return !autofs_wait(sbi, &dentry->d_name);
+		up(&dentry->d_parent->d_inode->i_sem);
+		status = !autofs_wait(sbi, &dentry->d_name);
+		down(&dentry->d_parent->d_inode->i_sem);
+		return (status);
 	}
 
 	/* We don't update the usages for the autofs daemon itself, this

@@ -229,9 +234,7 @@
 	dentry->d_flags |= DCACHE_AUTOFS_PENDING;
 	d_add(dentry, NULL);
 
-	up(&dir->i_sem);
 	autofs_revalidate(dentry, nd);
-	down(&dir->i_sem);
 
 	/*
 	 * If we are still pending, check if we had to handle

fs/autofs4/root.c +6 -0

diff -ur -x '*.orig' -x '*.new' linux-2.6.13.3/fs/autofs4/root.c linux-2.6.13.3-autofspatch/fs/autofs4/root.c
@@ -302,7 +302,9 @@
 		DPRINTK("waiting for expire %p name=%.*s",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 
+		up(&dentry->d_parent->d_inode->i_sem);
 		status = autofs4_wait(sbi, dentry, NFY_NONE);
+		down(&dentry->d_parent->d_inode->i_sem);
 		
 		DPRINTK("expire done status=%d", status);

@@ -324,7 +326,9 @@
 		DPRINTK("waiting for mount name=%.*s",
 			 dentry->d_name.len, dentry->d_name.name);
 
+		up(&dentry->d_parent->d_inode->i_sem);
 		status = autofs4_wait(sbi, dentry, NFY_MOUNT);
+		down(&dentry->d_parent->d_inode->i_sem);
 		 
 		DPRINTK("mount done status=%d", status);

@@ -351,7 +355,9 @@
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_AUTOFS_PENDING;
 		spin_unlock(&dentry->d_lock);
+		up(&dentry->d_parent->d_inode->i_sem);
 		status = autofs4_wait(sbi, dentry, NFY_MOUNT);
+		down(&dentry->d_parent->d_inode->i_sem);
 
 		DPRINTK("mount done status=%d", status);

fs/devfs/base.c +0 ~~-24~~

diff -ur -x '*.orig' -x '*.new' linux-2.6.13.3/fs/devfs/base.c linux-2.6.13.3-autofspatch/fs/devfs/base.c
@@ -2155,34 +2155,12 @@
 	devfs_handle_t parent = get_devfs_entry_from_vfs_inode(dir);
 	struct devfs_lookup_struct *lookup_info = dentry->d_fsdata;
 	DECLARE_WAITQUEUE(wait, current);
-	int need_lock;
 
 	/*
-	 * FIXME HACK
-	 *
 	 * make sure that
 	 *   d_instantiate always runs under lock
 	 *   we release i_sem lock before going to sleep
-	 *
-	 * unfortunately sometimes d_revalidate is called with
-	 * and sometimes without i_sem lock held. The following checks
-	 * attempt to deduce when we need to add (and drop resp.) lock
-	 * here. This relies on current (2.6.2) calling coventions:
-	 *
-	 *   lookup_hash is always run under i_sem and is passing NULL
-	 *   as nd
-	 *
-	 *   open(...,O_CREATE,...) calls _lookup_hash under i_sem
-	 *   and sets flags to LOOKUP_OPEN|LOOKUP_CREATE
-	 *
-	 *   all other invocations of ->d_revalidate seem to happen
-	 *   outside of i_sem
 	 */
-	need_lock = nd &&
-	    (!(nd->flags & LOOKUP_CREATE) || (nd->flags & LOOKUP_PARENT));
-
-	if (need_lock)
-		down(&dir->i_sem);
 
 	if (is_devfsd_or_child(fs_info)) {
 		devfs_handle_t de = lookup_info->de;

@@ -2237,8 +2215,6 @@
 		read_unlock(&parent->u.dir.lock);
 
       out:
-	if (need_lock)
-		up(&dir->i_sem);
 	return 1;
 }				/*  End Function devfs_d_revalidate_wait  */

fs/namei.c +14 -5

diff -ur -x '*.orig' -x '*.new' linux-2.6.13.3/fs/namei.c linux-2.6.13.3-autofspatch/fs/namei.c
@@ -393,7 +393,6 @@
 	struct dentry * result;
 	struct inode *dir = parent->d_inode;
 
-	down(&dir->i_sem);
 	/*
 	 * First re-do the cached lookup just in case it was created
 	 * while we waited for the directory semaphore..

@@ -419,7 +418,6 @@
 			else
 				result = dentry;
 		}
-		up(&dir->i_sem);
 		return result;
 	}

@@ -427,7 +425,6 @@
 	 * Uhhuh! Nasty case: the cache was re-populated while
 	 * we waited on the semaphore. Need to revalidate.
 	 */
-	up(&dir->i_sem);
 	if (result->d_op && result->d_op->d_revalidate) {
 		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
 			dput(result);

@@ -676,13 +673,16 @@
 		     struct path *path)
 {
 	struct vfsmount *mnt = nd->mnt;
+	struct inode *parent = nd->dentry->d_inode;
 	struct dentry *dentry = __d_lookup(nd->dentry, name);
 
+	down(&parent->i_sem);
 	if (!dentry)
 		goto need_lookup;
 	if (dentry->d_op && dentry->d_op->d_revalidate)
 		goto need_revalidate;
 done:
+	up(&parent->i_sem);
 	path->mnt = mnt;
 	path->dentry = dentry;
 	__follow_mount(path);

@@ -703,6 +703,7 @@
 	goto need_lookup;
 
 fail:
+	up(&parent->i_sem);
 	return PTR_ERR(dentry);
 }

@@ -718,7 +719,7 @@
 {
 	struct path next;
 	struct inode *inode;
-	int err;
+	int err, reval;
 	unsigned int lookup_flags = nd->flags;
 	
 	while (*name=='/')

@@ -893,9 +894,17 @@
 		 */
 		if (nd->dentry && nd->dentry->d_sb &&
 		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+			struct dentry *nparent;
+
 			err = -ESTALE;
 			/* Note: we do not d_invalidate() */
-			if (!nd->dentry->d_op->d_revalidate(nd->dentry, nd))
+			/* Revalidate requires us to lock the parent.
+			 */
+			nparent = nd->dentry->d_parent;
+			down(&nparent->d_inode->i_sem);
+			reval = nd->dentry->d_op->d_revalidate(nd->dentry, nd);
+			up(&nparent->d_inode->i_sem);
+			if (!reval)
 				break;
 		}
 return_base:

Trond Myklebust

2005-11-30 17:04 UTC #39 lore raw

Quoted 26 lines · Badari Pulavarty

Quoted 25 lines · Ian Kent
Quoted 8 lines · William H. Taber
Quoted 1 line · Ian Kent
We'll need to do an analysis of all callers of the revalidate method.
You are right. Searching through the sources, it would appear that I missed fixing autofs and devfs. Everyone else just defines a revalidate routine but doesn't call one. You may find devfs to be interesting because they have code to determine whether they need to release the i_sem lock or not. I am working on an updated patch to include the changes needed for these two modules.
I've looked at devfs before but that bit of code sounds interesting to me.
The other thing that concerns me is that we may be increasing the latency of some code paths that need to be really fast. I was thinking that perhaps it might be good to try a change more in line with the locking used in link_patch_walk (ie. i_sem free revalidate) rather than that used in lookup_one_len. My only justification being that lookup is called to create stuff where revalidate is called to check stuff. I've been poking around and this change looks fairly difficult as well (I seem to remember you also looked at this).
Anyway, I'm keen to have a look at your patch.
Thanks much for your interest and help.

Quoted 6 lines

Quoted 2 lines
Ian
Again, I am posting Will's latest patch on his behalf.
Any thoughts on how acceptable are the VFS changes ?

That will slow link_path_walk() for commonly accessed shared directories (/lib, /usr/share,...) down to a crawl.

Instead of having lock-free lookups of cached dentries, you are suddenly serialising everybody in the parent directory.

Cheers,
Trond

William H. Taber

2005-11-30 21:10 UTC #40 lore raw

Quoted 28 lines · Trond Myklebust

Quoted 18 lines · Badari Pulavarty
Quoted 16 lines · Ian Kent
Quoted 5 lines · William H. Taber
Quoted 1 line · Ian Kent
We'll need to do an analysis of all callers of the revalidate method.
You are right. Searching through the sources, it would appear that I missed fixing autofs and devfs. Everyone else just defines a revalidate routine but doesn't call one. You may find devfs to be interesting because they have code to determine whether they need to release the i_sem lock or not. I am working on an updated patch to include the changes needed for these two modules.
I've looked at devfs before but that bit of code sounds interesting to me.
The other thing that concerns me is that we may be increasing the latency of some code paths that need to be really fast. I was thinking that perhaps it might be good to try a change more in line with the locking used in link_patch_walk (ie. i_sem free revalidate) rather than that used in lookup_one_len. My only justification being that lookup is called to create stuff where revalidate is called to check stuff. I've been poking around and this change looks fairly difficult as well (I seem to remember you also looked at this).
Anyway, I'm keen to have a look at your patch.
Thanks much for your interest and help.
Quoted 6 lines
Quoted 2 lines
Ian
Again, I am posting Will's latest patch on his behalf.
Any thoughts on how acceptable are the VFS changes ?

Quoted 9 lines

That will slow link_path_walk() for commonly accessed shared directories (/lib, /usr/share,...) down to a crawl.
Instead of having lock-free lookups of cached dentries, you are suddenly serialising everybody in the parent directory.
Cheers,
Trond

Fair enough. But what do we do? The original problem was that we were seeing deadlocks on /net because the autofs was not releasing the parent i_sem when it pended in its revalidate routine. There was a race in which a process could be waiting on the revalidate before the automount demon ran but the automount demon needed the parent i_sem to be able to do the mount.

I was proposing this patch to make it easy for the automounter (and devfs) to be able to release the parent i_sem if they were going to pend. But as I described in a previous post, I am not sure that it is safe in the case of a rename, to allow a filesystem to release the parent i_sem in any event. Oops. I missed the s_vfs_rename_sem in the superblock which serializes renames on a given filesystem. And since renames across filesystems are not allowed, I guess it shouldn't be a problem after all.

So I guess that a sufficient fix is for the autofs to add code similar to that in devfs so that the revalidate code can decide whether or not it needs to release the parent i_sem. The best fix would be to take the code out of devfs and put it into fs/namei.c as an exported function (or into fs.h as an inline function) so that it only has to be changed in one place the next time the lookup code changes. It may be an ugly fix but the alternative is to be consistent in our locking when we call d_revalidate and I don't see an easy solution to that problem.

Will

Ian Kent

maintainer

2005-11-29 01:20 UTC #41 lore raw

Quoted 85 lines · William H. Taber

Quoted 83 lines · Ian Kent
My thoughts:
The cause of this issue is user space programs using autofs4 need to call services that must be able to take the inode semaphore. Notably sys_mkdir and sys_symlink in order to complete their task.
I believe that, in this case, releasing the semaphore is ok since the entry is part of the autofs filesystem and so autofs is responsible for taking care of it, provided that it is done carefully. The semaphore is meant to serialize changes being to the directory and these changes are done in autofs by asking the user space process to do it. Which are themselves serialized by the same semaphore.
The only tricky thing I can think of here is that care must be taken to ensure that the semaphore is not released before the DCACHE_AUTOFS_PENDING flag is set to make sure that other incoming requests are sent to the wait queue.
The attached patch does this and opts for a conservative approach by broadening the critical region instead of narrowing it.
It may also be necessary to review the return codes from revaliate but I'm only part way through that.
Please review and test this patch and offer further comment. Sorry guys but I haven't been able to test this at all save verifying that it compiles.
Hopefully I haven't missed anything completely obvious ... DOH!
Ian
fs/autofs4/root.c renamed from fs/autofs4/root.c.lookup-deadlock +1 -4
@@ -487,11 +487,8 @@ static struct dentry *autofs4_lookup(str
 	dentry->d_fsdata = NULL;
 	d_add(dentry, NULL);
 
-	if (dentry->d_op && dentry->d_op->d_revalidate) {
-		up(&dir->i_sem);
+	if (dentry->d_op && dentry->d_op->d_revalidate)
 		(dentry->d_op->d_revalidate)(dentry, nd);
-		down(&dir->i_sem);
-	}
 
 	/*
 	 * If we are still pending, check if we had to handle
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +14 -0
@@ -161,6 +161,8 @@ int autofs4_wait(struct autofs_sb_info *
 		enum autofs_notify notify)
 {
 	struct autofs_wait_queue *wq;
+	struct inode *dir = dentry->d_parent->d_inode;
+	int i_sem_held;
 	char *name;
 	int len, status;
 
@@ -227,6 +229,14 @@ int autofs4_wait(struct autofs_sb_info *
 			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
 	}
 
+	/*
+	 * If we are called from lookup or lookup_hash the
+	 * the inode semaphore needs to be released for
+	 * userspace to do its thing.
+	 */
+	i_sem_held = down_trylock(&dir->i_sem);
+	up(&dir->i_sem);
+
 	if (notify != NFY_NONE && atomic_dec_and_test(&wq->notified)) {
 		int type = (notify == NFY_MOUNT ?
 			autofs_ptype_missing : autofs_ptype_expire_multi);
@@ -268,6 +278,10 @@ int autofs4_wait(struct autofs_sb_info *
 		DPRINTK("skipped sleeping");
 	}
 
+	/* Re-take the inode semaphore if it was held */
+	if (i_sem_held)
+		down(&dir->i_sem);
+
 	status = wq->status;
 

Quoted 9 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *
Quoted 2 lines
fs/autofs4/waitq.c renamed from fs/autofs4/waitq.c.lookup-deadlock +0 -1
 	/* Are we the last process to need status? */
-
Ian,
I have not tested this patch but it seems to have a serious flaw. Given that do_lookup does not get the parent i_sem lock before calling revalidate, you have the possibility that you are being called without having gotten the lock but the lock may be held by another process. In that case you do not want to be releasing their lock while they are relying on it.

Oops.

Jeff Moyer

2005-11-30 01:16 UTC #42 lore raw

==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:

linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case.

linuxram> Process P1 calls autofs4's ->lookup(). The lookup finds that the linuxram> dentry does not exist. It creates a dentry and adds to the linuxram> cache. Releases the parent's inode's semaphore and than calls linuxram> ->revalidate().

linuxram> Process P2 meanwhile comes in and cached_lookup() gets called. It linuxram> finds the dentry in the cache and finds ->revalidate() function linuxram> exists. So it calls ->revalidate() holding the parent's inode's linuxram> semaphore.

Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held:

static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) {
...
/*
* Note: we're not holding inode->i_sem and so may be racing with
* operations that change the directory. We therefore save the
* change attribute *before* we do the RPC call.
*/

Can you try out a patch which does this?

-Jeff

fs/namei.c renamed from fs/namei.c.orig +2 -0

@@ -332,10 +332,12 @@ static struct dentry * cached_lookup(str
 		dentry = d_lookup(parent, name);
 
 	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+		up(&parent->d_inode->i_sem);
 		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
 			dput(dentry);
 			dentry = NULL;
 		}
+		down(&parent->d_inode->i_sem);
 	}
 	return dentry;
 }

Ian Kent

maintainer

2005-11-30 01:48 UTC #43 lore raw

Quoted 30 lines · Jeff Moyer

==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:
linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case.
linuxram> Process P1 calls autofs4's ->lookup(). The lookup finds that the linuxram> dentry does not exist. It creates a dentry and adds to the linuxram> cache. Releases the parent's inode's semaphore and than calls linuxram> ->revalidate().
linuxram> Process P2 meanwhile comes in and cached_lookup() gets called. It linuxram> finds the dentry in the cache and finds ->revalidate() function linuxram> exists. So it calls ->revalidate() holding the parent's inode's linuxram> semaphore.
Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held:
static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) {
...
/*
* Note: we're not holding inode->i_sem and so may be racing with
* operations that change the directory. We therefore save the
* change attribute *before* we do the RPC call.
*/

Quoted 1 line

Can you try out a patch which does this?

Could it be as simple as that?
Food for more thought.

Ian

Trond Myklebust

2005-11-30 01:56 UTC #44 lore raw

In reply to Jeff Moyer

Quoted 39 lines · Jeff Moyer

==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:
linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case.
linuxram> Process P1 calls autofs4's ->lookup(). The lookup finds that the linuxram> dentry does not exist. It creates a dentry and adds to the linuxram> cache. Releases the parent's inode's semaphore and than calls linuxram> ->revalidate().
linuxram> Process P2 meanwhile comes in and cached_lookup() gets called. It linuxram> finds the dentry in the cache and finds ->revalidate() function linuxram> exists. So it calls ->revalidate() holding the parent's inode's linuxram> semaphore.
Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held:
static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) {
...
/*
* Note: we're not holding inode->i_sem and so may be racing with
* operations that change the directory. We therefore save the
* change attribute *before* we do the RPC call.
*/
Can you try out a patch which does this?
(quoted diff) +0 -1
-Jeff
fs/namei.c renamed from fs/namei.c.orig +0 -0
@@ -332,10 +332,12 @@ static struct dentry * cached_lookup(str
 		dentry = d_lookup(parent, name);
 

fs/namei.c static struct dentry * cached_lookup(str · line 343

Quoted 10 lines

fs/namei.c renamed from fs/namei.c.orig +2 -0

 	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+		up(&parent->d_inode->i_sem);
 		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
 			dput(dentry);
 			dentry = NULL;
 		}
+		down(&parent->d_inode->i_sem);
 	}
 	return dentry;
 }

Woah! Definitely not safe. NFS might not care, but the VFS will certainly barf over that!

By dropping the dir->i_sem in cached_lookup() you are allowing 2 processes to allocate and lookup multiple dentries for the same file inside __lookup_hash().

Cheers,
Trond

Jeff Moyer

2005-11-30 04:15 UTC #45 lore raw

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Trond Myklebust <[email protected]> adds:

Quoted 2 lines · Jeff Moyer

Quoted 2 lines
==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:

linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 24 lines

Quoted 24 lines
Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held:
static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) {
...
/*
* Note: we're not holding inode->i_sem and so may be racing with
* operations that change the directory. We therefore save the
* change attribute *before* we do the RPC call.
*/
Can you try out a patch which does this?
(quoted diff) +0 -1
-Jeff
fs/namei.c renamed from fs/namei.c.orig +0 -0
@@ -332,10 +332,12 @@ static struct dentry * cached_lookup(str
 	dentry = d_lookup(parent, name);
 

Quoted 10 lines

fs/namei.c static struct dentry * cached_lookup(str · line 343

Quoted 10 lines

fs/namei.c renamed from fs/namei.c.orig +2 -0

 	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+		up(&parent->d_inode->i_sem);
 		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
 			dput(dentry);
 			dentry = NULL;
 		}
+		down(&parent->d_inode->i_sem);
 	}
 	return dentry;
 }

trond> Woah! Definitely not safe. NFS might not care, but the VFS will trond> certainly barf over that!

trond> By dropping the dir->i_sem in cached_lookup() you are allowing 2 trond> processes to allocate and lookup multiple dentries for the same file trond> inside __lookup_hash().

The patch only drops the semaphore if d_lookup finds the dentry and the dentry has a revalidate routine. I don't follow how you can end up with multiple dentries for the same file in this case.

Sorry if I'm missing something obvious.

-Jeff

Trond Myklebust

2005-11-30 06:14 UTC #46 lore raw

Quoted 5 lines · Jeff Moyer

The patch only drops the semaphore if d_lookup finds the dentry and the dentry has a revalidate routine. I don't follow how you can end up with multiple dentries for the same file in this case.
Sorry if I'm missing something obvious.

The inode->i_sem is what ensures that nobody can insert a new dentry between the lookup of the cached dentry by d_lookup() and the call to ->lookup() (which instantiates a new dentry).

Imagine you have two separate processes that are doing a __lookup_hash() of the same cached dentry. Now imagine that d_revalidate() of the dentry fails.

Process 1 Process 2

__lookup_hash("/foo", "bar") __lookup_hash("/foo", "bar")
... ....
down(&inode->i_sem)
....
<enter cached_lookup("/foo","bar")>
....
dentry = d_lookup("/foo", "bar")
up(&inode->i_sem)

					down(&inode->i_sem)
					....
					<enters cached_lookup("/foo","bar")>
d_revalidate(dentry) (fails)		dentry = d_lookup("/foo","bar")
					up(&inode->i_sem)

down(&inode->i_sem) d_revalidate(dentry); (fails)
...
<returns NULL to __lookup_hash>
....
dentry = d_alloc(parent,name)
->lookup(dentry) (instantiates "dentry")
up(&inode->i_sem)

					down(&inode->i_sem)
					....
					<returns NULL to __lookup_hash>
					....
					dentry = d_alloc(parent,name)
					->lookup(dentry) (instantiates "dentry")

....

Whoops. Suddenly you have called ->lookup() for 2 dentries that represent the same filename in the same directory.

Cheers,
Trond

Ian Kent

maintainer

2005-11-30 15:44 UTC #47 lore raw

Quoted 8 lines · Trond Myklebust

Quoted 5 lines · Jeff Moyer
The patch only drops the semaphore if d_lookup finds the dentry and the dentry has a revalidate routine. I don't follow how you can end up with multiple dentries for the same file in this case.
Sorry if I'm missing something obvious.

Quoted 7 lines

The inode->i_sem is what ensures that nobody can insert a new dentry between the lookup of the cached dentry by d_lookup() and the call to ->lookup() (which instantiates a new dentry).
Imagine you have two separate processes that are doing a __lookup_hash() of the same cached dentry. Now imagine that d_revalidate() of the dentry fails.

And that would be why lookup is only called if d_lookup fails in do_lookup when called from the link_path_walk routine. Do you think it is possible to do this safely in __lookup_hash with a similar re-ordering?

Ian

Trond Myklebust

2005-11-30 15:53 UTC #48 lore raw

Quoted 14 lines · Ian Kent

Quoted 12 lines · Trond Myklebust
Quoted 5 lines · Jeff Moyer
The patch only drops the semaphore if d_lookup finds the dentry and the dentry has a revalidate routine. I don't follow how you can end up with multiple dentries for the same file in this case.
Sorry if I'm missing something obvious.
The inode->i_sem is what ensures that nobody can insert a new dentry between the lookup of the cached dentry by d_lookup() and the call to ->lookup() (which instantiates a new dentry).

Quoted 7 lines

Quoted 3 lines
Imagine you have two separate processes that are doing a __lookup_hash() of the same cached dentry. Now imagine that d_revalidate() of the dentry fails.
And that would be why lookup is only called if d_lookup fails in do_lookup when called from the link_path_walk routine. Do you think it is possible to do this safely in __lookup_hash with a similar re-ordering?

2.4 kernels did a second d_lookup() after retaking the i_sem. The problem is what do you do in the case of a race: you either have to loop back (may lead to infinite loops of d_lookup()+d_invalidate()) or you do something like retry once, then return an error (which is what 2.4 kernels did).

Either choice is unsatisfying which is (I assume) why the current behaviour was chosen for 2.6 kernels.

Cheers,
Trond

Ian Kent

maintainer

2005-11-30 16:12 UTC #49 lore raw

Quoted 23 lines · Trond Myklebust

Quoted 14 lines · Ian Kent
Quoted 12 lines · Trond Myklebust
Quoted 5 lines · Jeff Moyer
The patch only drops the semaphore if d_lookup finds the dentry and the dentry has a revalidate routine. I don't follow how you can end up with multiple dentries for the same file in this case.
Sorry if I'm missing something obvious.
The inode->i_sem is what ensures that nobody can insert a new dentry between the lookup of the cached dentry by d_lookup() and the call to ->lookup() (which instantiates a new dentry).
Quoted 7 lines
Quoted 3 lines
Imagine you have two separate processes that are doing a __lookup_hash() of the same cached dentry. Now imagine that d_revalidate() of the dentry fails.
And that would be why lookup is only called if d_lookup fails in do_lookup when called from the link_path_walk routine. Do you think it is possible to do this safely in __lookup_hash with a similar re-ordering?

Quoted 5 lines

2.4 kernels did a second d_lookup() after retaking the i_sem. The problem is what do you do in the case of a race: you either have to loop back (may lead to infinite loops of d_lookup()+d_invalidate()) or you do something like retry once, then return an error (which is what 2.4 kernels did).

But in this case the semaphore is already held up until the revalidate so I guess the concern is that the dentry goes away after releasing the semaphore?

btw, a little aside.
I'm having trouble understanding where EEXIST is returned in a call such as sys_mkdir. Can you help?

If a call such as sys_mkdir (perhaps in lookup_create) was able to determine the directory exists before taking the semaphore the problem we have here goes away.

Quoted 12 lines

Either choice is unsatisfying which is (I assume) why the current behaviour was chosen for 2.6 kernels.
Cheers,
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ian Kent

maintainer

2005-11-30 16:27 UTC #50 lore raw

Quoted 4 lines · Ian Kent

If a call such as sys_mkdir (perhaps in lookup_create) was able to determine the directory exists before taking the semaphore the problem we have here goes away.

Sorry, that's rubbish, ignore it.

Ian

Trond Myklebust

2005-11-30 16:45 UTC #51 lore raw

In reply to Trond Myklebust

Quoted 9 lines · Ian Kent

Quoted 5 lines
2.4 kernels did a second d_lookup() after retaking the i_sem. The problem is what do you do in the case of a race: you either have to loop back (may lead to infinite loops of d_lookup()+d_invalidate()) or you do something like retry once, then return an error (which is what 2.4 kernels did).
But in this case the semaphore is already held up until the revalidate so I guess the concern is that the dentry goes away after releasing the semaphore?

The problem is the same: dealing with the case of someone else populating the d_cache while you have temporarily dropped the dir->i_sem.

Quoted 3 lines

btw, a little aside.
I'm having trouble understanding where EEXIST is returned in a call such as sys_mkdir. Can you help?

The call to may_create() (at the top of vfs_mkdir()) will return EEXIST if the dentry is already positive i.e. dentry->d_inode is non-null.

Cheers,
Trond

William H. Taber

2005-11-30 20:32 UTC #52 lore raw

Quoted 56 lines · Trond Myklebust

Quoted 39 lines · Jeff Moyer
==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; [email protected] (Ram Pai) adds:
linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case.
linuxram> Process P1 calls autofs4's ->lookup(). The lookup finds that the linuxram> dentry does not exist. It creates a dentry and adds to the linuxram> cache. Releases the parent's inode's semaphore and than calls linuxram> ->revalidate().
linuxram> Process P2 meanwhile comes in and cached_lookup() gets called. It linuxram> finds the dentry in the cache and finds ->revalidate() function linuxram> exists. So it calls ->revalidate() holding the parent's inode's linuxram> semaphore.
Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held:
static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) {
...
/*
* Note: we're not holding inode->i_sem and so may be racing with
* operations that change the directory. We therefore save the
* change attribute *before* we do the RPC call.
*/
Can you try out a patch which does this?
(quoted diff) +0 -1
-Jeff
fs/namei.c renamed from fs/namei.c.orig +0 -0
@@ -332,10 +332,12 @@ static struct dentry * cached_lookup(str
 		dentry = d_lookup(parent, name);
 
fs/namei.c static struct dentry * cached_lookup(str · line 343
Quoted 10 lines
fs/namei.c renamed from fs/namei.c.orig +2 -0
 	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+		up(&parent->d_inode->i_sem);
 		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
 			dput(dentry);
 			dentry = NULL;
 		}
+		down(&parent->d_inode->i_sem);
 	}
 	return dentry;
 }
Woah! Definitely not safe. NFS might not care, but the VFS will certainly barf over that!

Quoted 6 lines

By dropping the dir->i_sem in cached_lookup() you are allowing 2 processes to allocate and lookup multiple dentries for the same file inside __lookup_hash().
Cheers,
Trond

Not only is there this case, but the original premise is wrong as well.
There is a second case in which a d_revalidate function is called with
the parent i_sem and that is when it is called from inside of lookup_one_len. What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we risk breaking the lock ordering and creating a deadly embrace.

When I started writing this I thought that it would be safe for the autofs revalidate code to release the parent semaphore because they do not have a rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the parents so the potential for a deadly embrace still exists unless there is some other assurance that these final lookups will never pend waiting on the automounter in either their revalidate or lookup routines. (Actually the requirement is that they never give up the parent i_sem lock, but the lookup code has to give up the lock so that the autofs demon can run and perform the mount so it amounts to the same thing.)

The same issue exists for devfs which also releases the parent i_sem lock so that it can wait inside its revalidation routine.

Will

Trond Myklebust

2005-11-30 20:53 UTC #53 lore raw

Quoted 25 lines · William H. Taber

Not only is there this case, but the original premise is wrong as well.
There is a second case in which a d_revalidate function is called with
the parent i_sem and that is when it is called from inside of lookup_one_len. What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we risk breaking the lock ordering and creating a deadly embrace.
When I started writing this I thought that it would be safe for the autofs revalidate code to release the parent semaphore because they do not have a rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the parents so the potential for a deadly embrace still exists unless there is some other assurance that these final lookups will never pend waiting on the automounter in either their revalidate or lookup routines. (Actually the requirement is that they never give up the parent i_sem lock, but the lookup code has to give up the lock so that the autofs demon can run and perform the mount so it amounts to the same thing.)

Quoted 2 lines

The same issue exists for devfs which also releases the parent i_sem lock so that it can wait inside its revalidation routine.

So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate in the first place? Can't we move any code that requires dir->i_sem to be held into a ->lookup() method?

Trivially, if you have a d_revalidate that does something like

int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}

then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.

Cheers,
Trond

William H. Taber

2005-11-30 21:30 UTC #54 lore raw

Quoted 7 lines · Trond Myklebust

Quoted 4 lines · William H. Taber
Not only is there this case, but the original premise is wrong as well.
There is a second case in which a d_revalidate function is called with
the parent i_sem and that is when it is called from inside of lookup_one_len. What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we risk breaking the lock ordering and creating a deadly embrace.

Quoted 8 lines

Quoted 3 lines
When I started writing this I thought that it would be safe for the autofs revalidate code to release the parent semaphore because they do not have a rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the parents so the potential for a deadly embrace still exists unless there is some other assurance that these final lookups will never pend waiting on the automounter in either their revalidate or lookup routines. (Actually the requirement is that they never give up the parent i_sem lock, but the lookup code has to give up the lock so that the autofs demon can run and perform the mount so it amounts to the same thing.)
The same issue exists for devfs which also releases the parent i_sem lock so that it can wait inside its revalidation routine.
So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate in the first place? Can't we move any code that requires dir->i_sem to be held into a ->lookup() method?

It's not that d_revalidate wants or doesn't want to hold the lock. The caller of lookup_one_len is required to get the lock and this function calls lookup_hash which calls cached_lookup which calls d_revalidate.

Quoted 9 lines

Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}

Quoted 5 lines

then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.

Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and marks it as waiting for mounting and wakes up the automount demon. The demon completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is what I understand from Ian's answer when I proposed the same sort of thing to him. Even if they end up doing something like that in a future version of the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.

Will

Trond Myklebust

2005-11-30 22:32 UTC #55 lore raw

Quoted 15 lines · William H. Taber

Quoted 9 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
Quoted 5 lines
then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.

Quoted 7 lines

Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and marks it as waiting for mounting and wakes up the automount demon. The demon completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is what I understand from Ian's answer when I proposed the same sort of thing to him.

What do you mean by "removing the dentry on us"? It is perfectly possible to have lookup() return the original dentry every time, which is precisely what I suggested above.

Quoted 4 lines

Even if they end up
doing something like that in a future version of the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.

"Interim" fixes to the entire VFS API such as the ones that have proposed here tend to be a poor idea...

Cheers,
Trond

William H. Taber

2005-12-01 16:27 UTC #56 lore raw

Quoted 17 lines · Trond Myklebust

Quoted 14 lines · William H. Taber
Quoted 13 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.

Quoted 6 lines

Quoted 1 line
Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and marks it as waiting for mounting and wakes up the automount demon. The demon completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is what I understand from Ian's answer when I proposed the same sort of thing to him.
What do you mean by "removing the dentry on us"? It is perfectly possible to have lookup() return the original dentry every time, which is precisely what I suggested above.

What I meant was that the autofs code created this dentry and then called d_add to put it in the hash chain, woke up the autofs demon to perform the mount then waited for the mount to complete. The autofs code is, I think, intending for the demon to find this entry in a revalidate and complete the mount. But Ian knows better than I. Anyway I did not think that it would be good for a racing call to do_lookup to unhash a dentry that the automounter was expecting to find.

I was probably unclear when I referred to lookup. I meant it in the generic sense of do_lookup or lookup_one_len and not the i_op->lookup function. I had already suggested to Ian that they not d_add the dentry
in autofs4_lookup until the mount demon came in to complete the mount and have autofs4_lookup be responsible for queing up subsequent lookups until the mount completed and moving the code for that out of autofs4_revalidate. He allowed how it would be possible but a lot of work.

Quoted 9 lines

Quoted 4 lines
Even if they end up
doing something like that in a future version of the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.
"Interim" fixes to the entire VFS API such as the ones that have proposed here tend to be a poor idea...

Which is why I was discussing the ideas here. From the start I have been asking for input from people with more understanding than I have of the subleties of VFS locking. I have been trying to find a solution short of waiting for autofs5 since we are seeing the problem now. But obviously we don't want a fix that causes more problems. My original thought was that the solution to the problem was to make the locking requirements for d_revalidate consistent. I now have a greater understanding of why things are as they are. In another post I have outlined what I think a workable solution is that is confined to the autofs code. Your input it has been quite helpful in helping me understand all of this. Thanks.

Will

Ian Kent

maintainer

2005-12-01 12:09 UTC #57 lore raw

Quoted 54 lines · William H. Taber

Quoted 26 lines · Trond Myklebust
Quoted 23 lines · William H. Taber
Not only is there this case, but the original premise is wrong as well. There is a second case in which a d_revalidate function is called with the parent i_sem and that is when it is called from inside of lookup_one_len. What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we risk breaking the lock ordering and creating a deadly embrace.
When I started writing this I thought that it would be safe for the autofs revalidate code to release the parent semaphore because they do not have a rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the parents so the potential for a deadly embrace still exists unless there is some other assurance that these final lookups will never pend waiting on the automounter in either their revalidate or lookup routines. (Actually the requirement is that they never give up the parent i_sem lock, but the lookup code has to give up the lock so that the autofs demon can run and perform the mount so it amounts to the same thing.)
Quoted 7 lines
Quoted 2 lines
The same issue exists for devfs which also releases the parent i_sem lock so that it can wait inside its revalidation routine.
So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate in the first place? Can't we move any code that requires dir->i_sem to be held into a ->lookup() method?
It's not that d_revalidate wants or doesn't want to hold the lock. The caller of lookup_one_len is required to get the lock and this function calls lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 9 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
Quoted 5 lines
then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.

Quoted 9 lines

Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and marks it as waiting for mounting and wakes up the automount demon. The demon completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is what I understand from Ian's answer when I proposed the same sort of thing to
him. Even if they end up doing something like that in a future version of
the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.

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?

Ian

William H. Taber

2005-12-01 16:30 UTC #58 lore raw

Quoted 86 lines · Ian Kent

Quoted 56 lines · William H. Taber
Quoted 27 lines · Trond Myklebust
Quoted 23 lines · William H. Taber
Not only is there this case, but the original premise is wrong as well. There is a second case in which a d_revalidate function is called with the parent i_sem and that is when it is called from inside of lookup_one_len. What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we risk breaking the lock ordering and creating a deadly embrace.
When I started writing this I thought that it would be safe for the autofs revalidate code to release the parent semaphore because they do not have a rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the parents so the potential for a deadly embrace still exists unless there is some other assurance that these final lookups will never pend waiting on the automounter in either their revalidate or lookup routines. (Actually the requirement is that they never give up the parent i_sem lock, but the lookup code has to give up the lock so that the autofs demon can run and perform the mount so it amounts to the same thing.)
Quoted 7 lines
Quoted 2 lines
The same issue exists for devfs which also releases the parent i_sem lock so that it can wait inside its revalidation routine.
So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate in the first place? Can't we move any code that requires dir->i_sem to be held into a ->lookup() method?
It's not that d_revalidate wants or doesn't want to hold the lock. The caller of lookup_one_len is required to get the lock and this function calls lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 8 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
Quoted 5 lines
then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.
Quoted 9 lines
Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and marks it as waiting for mounting and wakes up the automount demon. The demon completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is what I understand from Ian's answer when I proposed the same sort of thing to
him. Even if they end up doing something like that in a future version of
the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.
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:

Quoted 7 lines

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?

Any other process calling lookup_one_len on a file in /net.

Will

Ian Kent

maintainer

2005-12-02 13:49 UTC #59 lore raw

Quoted 125 lines · William H. Taber

Quoted 124 lines · Ian Kent
Quoted 72 lines · William H. Taber
Quoted 41 lines · Trond Myklebust
Quoted 37 lines · William H. Taber
Not only is there this case, but the original premise is wrong as well.
There is a second case in which a d_revalidate function is called with the
parent i_sem and that is when it is called from inside of lookup_one_len.
What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in
which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not
get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we
risk breaking the lock ordering and creating a deadly embrace.
When I started writing this I thought that it would be safe for the autofs
revalidate code to release the parent semaphore because they do not have a
rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the
parents so the potential for a deadly embrace still exists unless there is
some other assurance that these final lookups will never pend waiting on
the automounter in either their revalidate or lookup routines. (Actually
the requirement is that they never give up the parent i_sem lock, but the
lookup code has to give up the lock so that the autofs demon can run and
perform the mount so it amounts to the same thing.)
Quoted 8 lines
Quoted 3 lines
The same issue exists for devfs which also releases the parent i_sem lock
so that it can wait inside its revalidation routine.
So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate in the first place? Can't we move any code that requires dir->i_sem to be held into a ->lookup() method?
It's not that d_revalidate wants or doesn't want to hold the lock. The caller
of lookup_one_len is required to get the lock and this function calls lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 8 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
Quoted 5 lines
then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.
Quoted 14 lines
Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and
marks it as waiting for mounting and wakes up the automount demon. The demon
completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is
what I understand from Ian's answer when I proposed the same sort of thing to
him. Even if they end up doing something like that in a future version
of
the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.
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.

Quoted 6 lines

Quoted 6 lines
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!

Quoted 2 lines

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.

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.

Jeff Moyer

2005-12-02 14:07 UTC #60 lore raw

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Ian Kent <[email protected]> adds:

Quoted 112 lines · William H. Taber

Quoted 112 lines
Quoted 9 lines · William H. Taber
Quoted 9 lines
Quoted 5 lines · Trond Myklebust
Quoted 1 line · William H. Taber
Not only is there this case, but the original premise is wrong
as > > > > well. > > > > There is a second case in which a d_revalidate function is called with > > > > the > > > > parent i_sem and that is when it is called from inside of > > > > lookup_one_len. > > > > What makes this tricky is that lookup_one_len is called from > > > > nfs_sillyrename from inside of nfs_rename which is called, naturally > >
Quoted 1 line
Quoted 1 line
enough by sys_rename. The rename code is very careful about the
order > > > > in > > > > which it obtains the parent semaphores because it needs to get two of > > > > them. It must always obtain the locks in the same order so that does > > > > not > > > > get into a deadly embrace. If we start arbitrarily releasing a parent > > > > semaphore in cached_lookup and taking it again after the revalidate, > > > > we >
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
risk breaking the lock ordering and creating a deadly embrace.
Quoted 2 lines
When I started writing this I thought that it would be safe for
the > > > > autofs > > > > revalidate code to release the parent semaphore because they do not > > > > have a > > > > rename callback. But I looked again at the rename code and it calls > > > > lookup_hash on the final source and destination files after locking > > > > the > >
Quoted 2 lines
Quoted 2 lines
parents so the potential for a deadly embrace still exists unless >
Quoted 1 line
there is > > > > some other assurance that these final lookups
will never pend waiting > > > > on > > > > the automounter in either their revalidate or lookup routines. > > > > (Actually > > > > the requirement is that they never give up the parent i_sem lock, but > > >
Quoted 1 line
the > > > > lookup code has to give up the lock so that the autofs
demon can run > > > > and > > > > perform the mount so it amounts to the same thing.)
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
The same issue exists for devfs which also releases the parent
i_sem > > > > lock > > > > so that it can wait inside its revalidation routine.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So exactly why does autofs4 want to hold the dir->i_sem in
d_revalidate > > > in the first place? Can't we move any code that requires dir->i_sem to > > > be held into a ->lookup() method?
Quoted 2 lines
Quoted 2 lines
It's not that d_revalidate wants or doesn't want to hold the lock.
The > > caller > > of lookup_one_len is required to get the lock and this function calls > > lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) { > > > d_drop(dentry); > > > return 0; > > > }
then the VFS will currently allocate a new dentry with the same
name, > > > and call ->lookup() on it without dropping dir->i_sem. If you still need > > > to reference the old dentry, then put it on a private list somewhere. > > > That would also allow you to return the old dentry as the result of the > > > ->lookup() operation if that is desirable.
Quoted 2 lines
Quoted 2 lines
Problem with that, as I understand it and Ian Kent knows better than
I, is > > that the autofs lookup code creates the dentry and fills it in partially > > and > > marks it as waiting for mounting and wakes up the automount demon. The > > demon > > completes the mount and finishes filling in the dentry. So we cannot have > > some other lookup coming in and removing the dentry on us. At least that > > is > > what I understand from Ian's answer when I proposed the same sort of thing > > to > > him. Even if they end up doing something like that in a future version > > of > > the automounter, I would still like a simple patch that can be applied to > > existing systems as an interim fix.
Quoted 5 lines
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).
Quoted 2 lines
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.
Quoted 2 lines
So the requirement is that autofs release the i_sem during the
callback, not > obtain it.
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 1 line

Quoted 6 lines

Quoted 6 lines
Quoted 1 line
The other functionality is read-only (and perhaps triggers a mount)
being > lookup, revalidate and readdir.
Quoted 3 lines
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?

raven> So still no counter example!

Quoted 1 line

Quoted 1 line
Any other process calling lookup_one_len on a file in /net.

raven> I'm afraid this is not an example it's an assertion. "Any other raven> process" is a little broad I think. You'll need to be more raven> specific.

raven> Consider the example reported by yourself and Ram.

raven> In that example we have processes P1, P2 and lets call the user raven> space callback P1(mount). Also assume there is a mechamism to check raven> the semaphore, release it if held and later re-take it if previously raven> held, like the patch I offered before.

raven> Correct me if I'm wrong but, with the assumption above, you report raven> goes like:

raven> P1 - calls lookup_one_len, takes i_sem and eventually calls raven> autofs4_lookup and indirectly autofs4_revalidate.

raven> P2 - comes along and waits on i_sem.

raven> P1 - autofs4_revalidate releases i_sem and posts a user space raven> callback.

raven> P2 - aquires i_sem and eventually calls autofs4_revalidate, releases raven> i_sem and is posted to the wait queue for mount completion.

raven> P1(mount) - calls mkdir, aquires i_sem, and calls autofs4_dir_mkdir, raven> i_sem is then released.

raven> Mount completion is signaled back to autofs4 and the waiters are raven> released.

raven> P1, P2, in any order each (one after the other due to the semaphore) raven> re-take i_sem and each complete their lookup_one_len calls.

raven> On both calls to autofs4_revalidate the calling process is itself raven> the holder of i_sem.

raven> Further, any other process that does a path walk during this time raven> has two possible paths.

raven> First case, the dentry exists, the process is placed on the wait raven> queue along with P1 and P2 awaiting mount completion without taking raven> i_sem.

raven> Second case, the dentry does not yet exist, this process either raven> aquires the i_sem in do_lookup and follows a similar path to P1 and raven> waits on the queue for mount completion or it waits on the i_sem raven> while P1 does the lookup and triggers the mount request, it the raven> aquires i_sem find the dentry exists, releases i_sem and calls raven> autofs4_revalidate without i_sem held and is sent to the wait queue raven> to wait for mount completion.

raven> Again in both these cases a process that enters autofs4_revalidate raven> when the i_sem is held is the process that aquired it.

Now consider that revalidate is called without semaphore held. Also consider that another process holds this semaphore for any valid reason (could be mkdir or whatever[1]). Now, you're code says, hey, the semaphore is held, let's drop it! Bad juju follows.

You can't do this.

-Jeff

1 - Yes, mkdir shouldn't be called by anything but the automount daemon. However, that doesn't prevent someone from calling it, and having it fail. That code path will still acquire the semaphore. I'm sure there are other code paths that will get the semaphore, and not end up in revalidate, too.

Ian Kent

maintainer

2005-12-02 15:21 UTC #61 lore raw

Quoted 181 lines · Jeff Moyer

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Ian Kent <[email protected]> adds:
Quoted 112 lines · William H. Taber
Quoted 112 lines
Quoted 9 lines · William H. Taber
Quoted 9 lines
Quoted 5 lines · Trond Myklebust
Quoted 1 line · William H. Taber
Not only is there this case, but the original premise is wrong
as > > > > well. > > > > There is a second case in which a d_revalidate function is called with > > > > the > > > > parent i_sem and that is when it is called from inside of > > > > lookup_one_len. > > > > What makes this tricky is that lookup_one_len is called from > > > > nfs_sillyrename from inside of nfs_rename which is called, naturally > >
Quoted 1 line
Quoted 1 line
enough by sys_rename. The rename code is very careful about the
order > > > > in > > > > which it obtains the parent semaphores because it needs to get two of > > > > them. It must always obtain the locks in the same order so that does > > > > not > > > > get into a deadly embrace. If we start arbitrarily releasing a parent > > > > semaphore in cached_lookup and taking it again after the revalidate, > > > > we >
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
risk breaking the lock ordering and creating a deadly embrace.
Quoted 2 lines
When I started writing this I thought that it would be safe for
the > > > > autofs > > > > revalidate code to release the parent semaphore because they do not > > > > have a > > > > rename callback. But I looked again at the rename code and it calls > > > > lookup_hash on the final source and destination files after locking > > > > the > >
Quoted 2 lines
Quoted 2 lines
parents so the potential for a deadly embrace still exists unless >
Quoted 1 line
there is > > > > some other assurance that these final lookups
will never pend waiting > > > > on > > > > the automounter in either their revalidate or lookup routines. > > > > (Actually > > > > the requirement is that they never give up the parent i_sem lock, but > > >
Quoted 1 line
the > > > > lookup code has to give up the lock so that the autofs
demon can run > > > > and > > > > perform the mount so it amounts to the same thing.)
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
The same issue exists for devfs which also releases the parent
i_sem > > > > lock > > > > so that it can wait inside its revalidation routine.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So exactly why does autofs4 want to hold the dir->i_sem in
d_revalidate > > > in the first place? Can't we move any code that requires dir->i_sem to > > > be held into a ->lookup() method?
Quoted 2 lines
Quoted 2 lines
It's not that d_revalidate wants or doesn't want to hold the lock.
The > > caller > > of lookup_one_len is required to get the lock and this function calls > > lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) { > > > d_drop(dentry); > > > return 0; > > > }
then the VFS will currently allocate a new dentry with the same
name, > > > and call ->lookup() on it without dropping dir->i_sem. If you still need > > > to reference the old dentry, then put it on a private list somewhere. > > > That would also allow you to return the old dentry as the result of the > > > ->lookup() operation if that is desirable.
Quoted 2 lines
Quoted 2 lines
Problem with that, as I understand it and Ian Kent knows better than
I, is > > that the autofs lookup code creates the dentry and fills it in partially > > and > > marks it as waiting for mounting and wakes up the automount demon. The > > demon > > completes the mount and finishes filling in the dentry. So we cannot have > > some other lookup coming in and removing the dentry on us. At least that > > is > > what I understand from Ian's answer when I proposed the same sort of thing > > to > > him. Even if they end up doing something like that in a future version > > of > > the automounter, I would still like a simple patch that can be applied to > > existing systems as an interim fix.
Quoted 5 lines
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).
Quoted 2 lines
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.
Quoted 2 lines
So the requirement is that autofs release the i_sem during the
callback, not > obtain it.
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
The other functionality is read-only (and perhaps triggers a mount)
being > lookup, revalidate and readdir.
Quoted 3 lines
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?
raven> So still no counter example!
Quoted 1 line
Quoted 1 line
Any other process calling lookup_one_len on a file in /net.
raven> I'm afraid this is not an example it's an assertion. "Any other raven> process" is a little broad I think. You'll need to be more raven> specific.
raven> Consider the example reported by yourself and Ram.
raven> In that example we have processes P1, P2 and lets call the user raven> space callback P1(mount). Also assume there is a mechamism to check raven> the semaphore, release it if held and later re-take it if previously raven> held, like the patch I offered before.
raven> Correct me if I'm wrong but, with the assumption above, you report raven> goes like:
raven> P1 - calls lookup_one_len, takes i_sem and eventually calls raven> autofs4_lookup and indirectly autofs4_revalidate.
raven> P2 - comes along and waits on i_sem.
raven> P1 - autofs4_revalidate releases i_sem and posts a user space raven> callback.
raven> P2 - aquires i_sem and eventually calls autofs4_revalidate, releases raven> i_sem and is posted to the wait queue for mount completion.
raven> P1(mount) - calls mkdir, aquires i_sem, and calls autofs4_dir_mkdir, raven> i_sem is then released.
raven> Mount completion is signaled back to autofs4 and the waiters are raven> released.
raven> P1, P2, in any order each (one after the other due to the semaphore) raven> re-take i_sem and each complete their lookup_one_len calls.
raven> On both calls to autofs4_revalidate the calling process is itself raven> the holder of i_sem.
raven> Further, any other process that does a path walk during this time raven> has two possible paths.
raven> First case, the dentry exists, the process is placed on the wait raven> queue along with P1 and P2 awaiting mount completion without taking raven> i_sem.
raven> Second case, the dentry does not yet exist, this process either raven> aquires the i_sem in do_lookup and follows a similar path to P1 and raven> waits on the queue for mount completion or it waits on the i_sem raven> while P1 does the lookup and triggers the mount request, it the raven> aquires i_sem find the dentry exists, releases i_sem and calls raven> autofs4_revalidate without i_sem held and is sent to the wait queue raven> to wait for mount completion.
raven> Again in both these cases a process that enters autofs4_revalidate raven> when the i_sem is held is the process that aquired it.

Quoted 6 lines

Now consider that revalidate is called without semaphore held. Also consider that another process holds this semaphore for any valid reason (could be mkdir or whatever[1]). Now, you're code says, hey, the semaphore is held, let's drop it! Bad juju follows.
You can't do this.

Good call. A single example is sufficient to refute my assertion.

But to focus the discussion, the goal is to identify that the calling process is the one that holds the semaphore to justify releasing it. For lookup it's easy as it's always called with it held. lookup_hash will always call revalidate or lookup with the nameidata struct NULL and so can be used to identify the semaphore ownership similar to the devfs code Will mentioned. I don't think the other tests there are relevant to autofs. The patch will require some rework but still the same general idea.

Can you think of an example code path where __lookup_hash is called with a non null nameidata struct which leads to autofs4 revalidate or lookup.

Ian

Will Taber

2005-12-02 16:35 UTC #62 lore raw

Quoted 7 lines · Ian Kent

Good call. A single example is sufficient to refute my assertion.
But to focus the discussion, the goal is to identify that the calling process is the one that holds the semaphore to justify releasing it. For lookup it's easy as it's always called with it held. lookup_hash will always call revalidate or lookup with the nameidata struct NULL and so can be used to identify the semaphore ownership similar to the devfs code Will mentioned. I don't think the other tests there are relevant to autofs. The patch will require some rework but still the same general idea.
Can you think of an example code path where __lookup_hash is called with a non null nameidata struct which leads to autofs4 revalidate or lookup.

It looks as though __lookup_hash is called from open_namei with the lock held and a nameidata structure. It also looks as though LOOKUP_OPEN is set in the nameidata->flags so you should be able to identify this case.
No, that's not because lookup_open does a path_walk to get the parent
without the lock. So it looks as though if you have a nameidata structure and the flags have LOOKUP_OPEN and LOOKUP_CREATE set but not LOOKUP_PARENT, then you are holding the i_sem lock. The devfs code ignores LOOKUP_OPEN. I have to go right now and I am not sure if the checks are equivalent so I will leave that as an exercise for the reader.

Regards,
Will

Will

Ian Kent

maintainer

2005-12-02 17:11 UTC #63 lore raw

Quoted 17 lines · Will Taber

Quoted 11 lines · Ian Kent
Good call. A single example is sufficient to refute my assertion.
But to focus the discussion, the goal is to identify that the calling process is the one that holds the semaphore to justify releasing it. For lookup it's easy as it's always called with it held. lookup_hash will always call revalidate or lookup with the nameidata struct NULL and so can be used to identify the semaphore ownership similar to the devfs code Will mentioned. I don't think the other tests there are relevant to autofs. The patch will require some rework but still the same general idea.
Quoted 3 lines
Can you think of an example code path where __lookup_hash is called with a non null nameidata struct which leads to autofs4 revalidate or lookup.

Quoted 9 lines

It looks as though __lookup_hash is called from open_namei with the lock held and a nameidata structure. It also looks as though LOOKUP_OPEN is set in the nameidata->flags so you should be able to identify this case. No, that's not because lookup_open does a path_walk to get the parent without the lock. So it looks as though if you have a nameidata structure and the flags have LOOKUP_OPEN and LOOKUP_CREATE set but not LOOKUP_PARENT, then you are holding the i_sem lock. The devfs code ignores LOOKUP_OPEN. I have to go right now and I am not sure if the checks are equivalent so I will leave that as an exercise for the reader.

The create case is not used by autofs. Shouldn't need to be considered.

Ian

Will Taber

2005-12-02 15:34 UTC #64 lore raw

Quoted 152 lines · Ian Kent

Quoted 129 lines · William H. Taber
Quoted 127 lines · Ian Kent
Quoted 75 lines · William H. Taber
Quoted 42 lines · Trond Myklebust
Quoted 37 lines · William H. Taber
Not only is there this case, but the original premise is wrong as well.
There is a second case in which a d_revalidate function is called with the
parent i_sem and that is when it is called from inside of lookup_one_len.
What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally enough by sys_rename. The rename code is very careful about the order in
which it obtains the parent semaphores because it needs to get two of them. It must always obtain the locks in the same order so that does not
get into a deadly embrace. If we start arbitrarily releasing a parent semaphore in cached_lookup and taking it again after the revalidate, we
risk breaking the lock ordering and creating a deadly embrace.
When I started writing this I thought that it would be safe for the autofs
revalidate code to release the parent semaphore because they do not have a
rename callback. But I looked again at the rename code and it calls lookup_hash on the final source and destination files after locking the
parents so the potential for a deadly embrace still exists unless there is
some other assurance that these final lookups will never pend waiting on
the automounter in either their revalidate or lookup routines. (Actually
the requirement is that they never give up the parent i_sem lock, but the
lookup code has to give up the lock so that the autofs demon can run and
perform the mount so it amounts to the same thing.)
Quoted 8 lines
Quoted 3 lines
The same issue exists for devfs which also releases the parent i_sem lock
so that it can wait inside its revalidation routine.
So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate in the first place? Can't we move any code that requires dir->i_sem to be held into a ->lookup() method?
It's not that d_revalidate wants or doesn't want to hold the lock. The caller
of lookup_one_len is required to get the lock and this function calls lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 8 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
Quoted 5 lines
then the VFS will currently allocate a new dentry with the same name, and call ->lookup() on it without dropping dir->i_sem. If you still need to reference the old dentry, then put it on a private list somewhere. That would also allow you to return the old dentry as the result of the ->lookup() operation if that is desirable.
Quoted 14 lines
Problem with that, as I understand it and Ian Kent knows better than I, is that the autofs lookup code creates the dentry and fills it in partially and
marks it as waiting for mounting and wakes up the automount demon. The demon
completes the mount and finishes filling in the dentry. So we cannot have some other lookup coming in and removing the dentry on us. At least that is
what I understand from Ian's answer when I proposed the same sort of thing to
him. Even if they end up doing something like that in a future version
of
the automounter, I would still like a simple patch that can be applied to existing systems as an interim fix.
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.
Quoted 6 lines
Quoted 6 lines
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!
Quoted 1 line
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.

Quoted 7 lines

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.

Quoted 13 lines

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.

Quoted 7 lines

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.

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.

Will

Ian Kent

maintainer

2005-12-02 17:29 UTC #65 lore raw

Quoted 204 lines · Will Taber

Quoted 203 lines · Ian Kent
Quoted 171 lines · William H. Taber
Quoted 169 lines · Ian Kent
Quoted 111 lines · William H. Taber
Quoted 62 lines · Trond Myklebust
Quoted 51 lines · William H. Taber
Not only is there this case, but the original premise is wrong as well.
There is a second case in which a d_revalidate function is called with
the
parent i_sem and that is when it is called from inside of lookup_one_len.
What makes this tricky is that lookup_one_len is called from nfs_sillyrename from inside of nfs_rename which is called, naturally
enough by sys_rename. The rename code is very careful about the order
in
which it obtains the parent semaphores because it needs to get two of
them. It must always obtain the locks in the same order so that does
not
get into a deadly embrace. If we start arbitrarily releasing a parent
semaphore in cached_lookup and taking it again after the revalidate,
we
risk breaking the lock ordering and creating a deadly embrace.
When I started writing this I thought that it would be safe for the
autofs
revalidate code to release the parent semaphore because they do not
have a
rename callback. But I looked again at the rename code and it calls
lookup_hash on the final source and destination files after locking
the
parents so the potential for a deadly embrace still exists unless there is
some other assurance that these final lookups will never pend waiting
on
the automounter in either their revalidate or lookup routines. (Actually
the requirement is that they never give up the parent i_sem lock, but
the
lookup code has to give up the lock so that the autofs demon can run
and
perform the mount so it amounts to the same thing.)
Quoted 4 lines
The same issue exists for devfs which also releases the parent i_sem
lock
so that it can wait inside its revalidation routine.
Quoted 5 lines
So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate
in the first place? Can't we move any code that requires dir->i_sem to
be held into a ->lookup() method?
It's not that d_revalidate wants or doesn't want to hold the lock. The
caller
of lookup_one_len is required to get the lock and this function calls lookup_hash which calls cached_lookup which calls d_revalidate.
Quoted 8 lines
Trivially, if you have a d_revalidate that does something like
int autofs_revalidate(struct dentry *dentry, struct nameidata *nd) {
d_drop(dentry);
return 0;
}
Quoted 9 lines
then the VFS will currently allocate a new dentry with the same name,
and call ->lookup() on it without dropping dir->i_sem. If you still need
to reference the old dentry, then put it on a private list somewhere.
That would also allow you to return the old dentry as the result of the
->lookup() operation if that is desirable.
Problem with that, as I understand it and Ian Kent knows better than I, is
that the autofs lookup code creates the dentry and fills it in partially
and
marks it as waiting for mounting and wakes up the automount demon. The
demon
completes the mount and finishes filling in the dentry. So we cannot have
some other lookup coming in and removing the dentry on us. At least that
is
what I understand from Ian's answer when I proposed the same sort of
Quoted 8 lines
thing
to
him. Even if they end up doing something like that in a future
version
of
the automounter, I would still like a simple patch that can be applied to
existing systems as an interim fix.
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.
Quoted 7 lines
Quoted 7 lines
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!
Quoted 1 line
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:

Quoted 8 lines

Quoted 4 lines
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.

Quoted 31 lines

Quoted 31 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 14 lines

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.

Ian

Trond Myklebust

2005-12-02 18:12 UTC #66 lore raw

Quoted 4 lines · Ian Kent

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.

Consider those cases where the VFS calls ->lookup()/->d_revalidate() with the nameidata argument set to NULL to be bugs. They are pretty much all slated to be fixed soon in order to enable features like read-only bind mounts etc.

Cheers,
Trond

Christoph Hellwig

2005-12-04 12:56 UTC #67 lore raw

Quoted 11 lines · Trond Myklebust

Quoted 4 lines · Ian Kent
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.
Consider those cases where the VFS calls ->lookup()/->d_revalidate() with the nameidata argument set to NULL to be bugs. They are pretty much all slated to be fixed soon in order to enable features like read-only bind mounts etc.

They are still NULL in exactly one case: lookup_one_len. Given the design of lookup_one_len we can't get at a nameidata there at all.

Christoph Hellwig

2005-12-04 12:57 UTC #68 lore raw

Quoted 2 lines · Christoph Hellwig

They are still NULL in exactly one case: lookup_one_len. Given the design of lookup_one_len we can't get at a nameidata there at all.

Oh, forgot the most important bit here :) lookup_one_len is a library helper never called by the VFS. autofs (v4 at least) doesn't use it so now always get a nameidata. In fact if you look in -mm there's a patch from me that makes use of that fact.

Ian Kent

maintainer

2005-12-04 14:58 UTC #69 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 2 lines · Christoph Hellwig
They are still NULL in exactly one case: lookup_one_len. Given the design of lookup_one_len we can't get at a nameidata there at all.
Oh, forgot the most important bit here :) lookup_one_len is a library helper never called by the VFS. autofs (v4 at least) doesn't use it so now always get a nameidata. In fact if you look in -mm there's a patch from me that makes use of that fact.

But Will is calling it in a something like a stacking context and autofs fails to handle it. Hence this discussion.

Ian

Christoph Hellwig

2005-12-04 17:17 UTC #70 lore raw

Quoted 7 lines · Ian Kent

Quoted 4 lines
never called by the VFS. autofs (v4 at least) doesn't use it so now always get a nameidata. In fact if you look in -mm there's a patch from me that makes use of that fact.
But Will is calling it in a something like a stacking context and autofs fails to handle it. Hence this discussion.

No, for current TOT that can't happen. It could happen for older kernels but nothing is doing it in the tree anymore and if anything outside is doing it it's fundamentally broken.

Ian Kent

maintainer

2005-12-05 01:03 UTC #71 lore raw

Quoted 6 lines · Christoph Hellwig

Quoted 5 lines · Ian Kent
Quoted 4 lines
never called by the VFS. autofs (v4 at least) doesn't use it so now always get a nameidata. In fact if you look in -mm there's a patch from me that makes use of that fact.

Quoted 7 lines

Quoted 2 lines
But Will is calling it in a something like a stacking context and autofs fails to handle it. Hence this discussion.
No, for current TOT that can't happen. It could happen for older kernels but nothing is doing it in the tree anymore and if anything outside is doing it it's fundamentally broken.

Yes. So I should continue with my focus on handling link_path_walk cases as all paths should pass through their?

Ian

Jeff Moyer

2005-12-06 21:20 UTC #72 lore raw

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Christoph Hellwig <[email protected]> adds:

Quoted 7 lines · Ian Kent

Quoted 7 lines
Quoted 1 line
never called by the VFS. autofs (v4 at least) doesn't use it so now
always > get a nameidata. In fact if you look in -mm there's a patch from me that > makes use of that fact.
Quoted 1 line
But Will is calling it in a something like a stacking context and autofs fails to handle it. Hence this discussion.

hch> No, for current TOT that can't happen. It could happen for older hch> kernels but nothing is doing it in the tree anymore and if anything hch> outside is doing it it's fundamentally broken.

This is a bit unclear to me. What do you mean when you refer to "it" and "that" above? Oh, and TOT is a TLA I haven't run across before.

We know that there is at least one out of tree module that calls lookup_one_len, and ends up in the autofs4 revalidate code without the valid nameidata structure. In this case, with your patch, wouldn't we blindly dereference the structure and cause an oops? If so, who is at fault?

Thanks,

Jeff

Christoph Hellwig

2005-12-06 21:40 UTC #73 lore raw

Quoted 6 lines · Jeff Moyer

hch> No, for current TOT that can't happen. It could happen for older hch> kernels but nothing is doing it in the tree anymore and if anything hch> outside is doing it it's fundamentally broken.
This is a bit unclear to me. What do you mean when you refer to "it" and "that" above? Oh, and TOT is a TLA I haven't run across before.

TOT = top of tree.

To rephrease the above: With current mainline the nameidata argument is always valid when ->lookup or ->d_revalidate are called except when the filesystem uses lookup_one_len. lookup_one_len is a helper for fileystem usage that is only valid to be used on the filesystems own trees.

Quoted 5 lines

We know that there is at least one out of tree module that calls lookup_one_len, and ends up in the autofs4 revalidate code without the valid nameidata structure. In this case, with your patch, wouldn't we blindly dereference the structure and cause an oops? If so, who is at fault?

This out of tree module is wrong and always has been wrong. Any actual breakage of such a module is expected.

Do you happen to know what module that is?

Jeff Moyer

2005-12-06 22:37 UTC #74 lore raw

==> Regarding Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix; Christoph Hellwig <[email protected]> adds:

hch> On Tue, Dec 06, 2005 at 04:20:29PM -0500, Jeff Moyer wrote: No, for hch> current TOT that can't happen. It could happen for older kernels but hch> nothing is doing it in the tree anymore and if anything outside is hch> doing it it's fundamentally broken.

Quoted 2 lines

Quoted 2 lines
This is a bit unclear to me. What do you mean when you refer to "it" and "that" above? Oh, and TOT is a TLA I haven't run across before.

hch> TOT = top of tree.

hch> To rephrease the above: With current mainline the nameidata argument hch> is always valid when ->lookup or ->d_revalidate are called except when hch> the filesystem uses lookup_one_len. lookup_one_len is a helper for hch> fileystem usage that is only valid to be used on the filesystems own hch> trees.

Quoted 5 lines

Quoted 5 lines
We know that there is at least one out of tree module that calls lookup_one_len, and ends up in the autofs4 revalidate code without the valid nameidata structure. In this case, with your patch, wouldn't we blindly dereference the structure and cause an oops? If so, who is at fault?

hch> This out of tree module is wrong and always has been wrong. Any hch> actual breakage of such a module is expected.

Thanks for the clarification. This was my interpretation, but I wanted to be sure.

hch> Do you happen to know what module that is?

Well, the example originally posted was stubfs, which was purported to be a sample fs used to show this problem. Perhaps the original reporter can tell us what other code does this.

-Jeff

Will Taber

2005-12-07 14:52 UTC #75 lore raw

Quoted 5 lines · Christoph Hellwig

To rephrease the above: With current mainline the nameidata argument is always valid when ->lookup or ->d_revalidate are called except when the filesystem uses lookup_one_len. lookup_one_len is a helper for fileystem usage that is only valid to be used on the filesystems own trees.

Is this documented anywhere? How is one to know about this restriction since it isn't obvious from the code? And if this function is only to be used to lookup in ones own filesystem how is a filesystem supposed to lookup a file in another filesystem if they already have a directory dentry in hand? Walking up the dentry tree to recreate a path name so you can call path_walk seems a bit much. Without this capablility how does one write a stackable file system?

Will

Christoph Hellwig

2005-12-07 15:18 UTC #76 lore raw

Quoted 8 lines · Will Taber

Quoted 6 lines · Christoph Hellwig
To rephrease the above: With current mainline the nameidata argument is always valid when ->lookup or ->d_revalidate are called except when the filesystem uses lookup_one_len. lookup_one_len is a helper for fileystem
usage that is only valid to be used on the filesystems own trees.
Is this documented anywhere?

It's documented in the mail on lkml where it was introduce long time ago. I have a patch in my queue to add a comment about this and various other bits. It'll go into 2.6.26 I hope.

Quoted 2 lines

How is one to know about this restriction
since it isn't obvious from the code?

Actually it is obvious for the code :) Because it doesn't get an nfsmount it's inherently dangerous to use to look up random files.

Quoted 4 lines

And if this function is only to
be used to lookup in ones own filesystem how is a filesystem supposed to lookup a file in another filesystem if they already have a directory dentry in hand?

It's not supposed to do that. All these lookups must be done by VFS code. The lowest level entry points to do something on the namespace of a filesystem are the vfs_* routines or thing like dentry_open().

Brian Long

2005-12-07 15:22 UTC #77 lore raw

Quoted 21 lines · Christoph Hellwig

Quoted 6 lines · Jeff Moyer
hch> No, for current TOT that can't happen. It could happen for older hch> kernels but nothing is doing it in the tree anymore and if anything hch> outside is doing it it's fundamentally broken.
This is a bit unclear to me. What do you mean when you refer to "it" and "that" above? Oh, and TOT is a TLA I haven't run across before.
TOT = top of tree.
To rephrease the above: With current mainline the nameidata argument is always valid when ->lookup or ->d_revalidate are called except when the filesystem uses lookup_one_len. lookup_one_len is a helper for fileystem usage that is only valid to be used on the filesystems own trees.
Quoted 5 lines
We know that there is at least one out of tree module that calls lookup_one_len, and ends up in the autofs4 revalidate code without the valid nameidata structure. In this case, with your patch, wouldn't we blindly dereference the structure and cause an oops? If so, who is at fault?

Quoted 4 lines

This out of tree module is wrong and always has been wrong. Any actual breakage of such a module is expected.
Do you happen to know what module that is?

I believe the filesystem is IBM Rational's mvfs (multi-version filesystem) used in ClearCase. My team is the internal support organization at Cisco for Red Hat Enterprise Linux issues and we opened the support case with Red Hat about this issue. Our internal ClearCase support folks also have a case opened with IBM Rational Tech Support. Thank you for clarifying that mvfs is doing the "wrong thing" by calling lookup_one_len.

/Brian/

Signature · 4 lines

       Brian Long                      |         |           |
       IT Data Center Systems          |       .|||.       .|||.
       Cisco Linux Developer           |   ..:|||||||:...:|||||||:..
       Phone: (919) 392-7363           |   C i s c o   S y s t e m s

Christoph Hellwig

2005-12-07 15:25 UTC #78 lore raw

Quoted 7 lines · Brian Long

I believe the filesystem is IBM Rational's mvfs (multi-version filesystem) used in ClearCase. My team is the internal support organization at Cisco for Red Hat Enterprise Linux issues and we opened the support case with Red Hat about this issue. Our internal ClearCase support folks also have a case opened with IBM Rational Tech Support. Thank you for clarifying that mvfs is doing the "wrong thing" by calling lookup_one_len.

Well, mvfs is the worst filesystem ever. I wish could beat up the people at rational that wrote it personally. Everytime some implementation detail in the VFS changes it turns out mvfs uses it in the most braindead way.

I also really doubt they could claim it's not derived work of the linux kernel given all this poking it does.

Maybe I should sue IBM about so they take it off the market ;-)

*evil grin*

Will Taber

2005-12-07 17:46 UTC #79 lore raw

In reply to Brian Long

Quoted 39 lines · Brian Long

Quoted 23 lines · Christoph Hellwig
Quoted 6 lines · Jeff Moyer
hch> No, for current TOT that can't happen. It could happen for older hch> kernels but nothing is doing it in the tree anymore and if anything hch> outside is doing it it's fundamentally broken.
This is a bit unclear to me. What do you mean when you refer to "it" and "that" above? Oh, and TOT is a TLA I haven't run across before.
TOT = top of tree.
To rephrease the above: With current mainline the nameidata argument is always valid when ->lookup or ->d_revalidate are called except when the filesystem uses lookup_one_len. lookup_one_len is a helper for fileystem usage that is only valid to be used on the filesystems own trees.
Quoted 5 lines
We know that there is at least one out of tree module that calls lookup_one_len, and ends up in the autofs4 revalidate code without the valid nameidata structure. In this case, with your patch, wouldn't we blindly dereference the structure and cause an oops? If so, who is at fault?
Quoted 4 lines
This out of tree module is wrong and always has been wrong. Any actual breakage of such a module is expected.
Do you happen to know what module that is?
I believe the filesystem is IBM Rational's mvfs (multi-version filesystem) used in ClearCase. My team is the internal support organization at Cisco for Red Hat Enterprise Linux issues and we opened the support case with Red Hat about this issue. Our internal ClearCase support folks also have a case opened with IBM Rational Tech Support. Thank you for clarifying that mvfs is doing the "wrong thing" by calling lookup_one_len.

Quoted 1 line

/Brian/

Maybe we are doing "the wrong thing" by calling lookup_one_len on the autofs but that begs the question of what the right thing would be. There is no vfs_lookup function that will look up a single component in a pathname. There may be things we can do in the future so that we don't have to make this call at all, but that doesn't resolve the problems you have today.

Likewise one could argue that the vfs layer is broken because it is inconsistent in its handling of the parent i_sem lock on d_revalidate calls. While this may be true in some abstract software engineering sense I have learned enough from this thread already to realize that there is no easy solution there.

On the assumption that our use of lookup_one_len was appropriate, Ian has been working on a fix to autofs. I am not particularly interested in where the change is made. What I have been looking for was a solution that could then be backported to earlier releases to fix the problem at hand. Anyway, with this information, is it appropriate for us to replace our calls to lookup_one_len with calls to path_walk or is that also forbidden?

Will

Ian Kent

maintainer

2005-12-08 01:17 UTC #80 lore raw

Quoted 55 lines · Will Taber

Quoted 39 lines · Brian Long
Quoted 23 lines · Christoph Hellwig
Quoted 6 lines · Jeff Moyer
hch> No, for current TOT that can't happen. It could happen for older hch> kernels but nothing is doing it in the tree anymore and if anything hch> outside is doing it it's fundamentally broken.
This is a bit unclear to me. What do you mean when you refer to "it" and "that" above? Oh, and TOT is a TLA I haven't run across before.
TOT = top of tree.
To rephrease the above: With current mainline the nameidata argument is always valid when ->lookup or ->d_revalidate are called except when the filesystem uses lookup_one_len. lookup_one_len is a helper for fileystem usage that is only valid to be used on the filesystems own trees.
Quoted 5 lines
We know that there is at least one out of tree module that calls lookup_one_len, and ends up in the autofs4 revalidate code without the valid nameidata structure. In this case, with your patch, wouldn't we blindly dereference the structure and cause an oops? If so, who is at fault?
Quoted 4 lines
This out of tree module is wrong and always has been wrong. Any actual breakage of such a module is expected.
Do you happen to know what module that is?
I believe the filesystem is IBM Rational's mvfs (multi-version filesystem) used in ClearCase. My team is the internal support organization at Cisco for Red Hat Enterprise Linux issues and we opened the support case with Red Hat about this issue. Our internal ClearCase support folks also have a case opened with IBM Rational Tech Support. Thank you for clarifying that mvfs is doing the "wrong thing" by calling lookup_one_len.
Quoted 1 line
/Brian/
Maybe we are doing "the wrong thing" by calling lookup_one_len on the autofs but that begs the question of what the right thing would be. There is no vfs_lookup function that will look up a single component in a pathname. There may be things we can do in the future so that we don't have to make this call at all, but that doesn't resolve the problems you have today.
Likewise one could argue that the vfs layer is broken because it is inconsistent in its handling of the parent i_sem lock on d_revalidate calls. While this may be true in some abstract software engineering sense I have learned enough from this thread already to realize that there is no easy solution there.

Quoted 7 lines

On the assumption that our use of lookup_one_len was appropriate, Ian has been working on a fix to autofs. I am not particularly interested in where the change is made. What I have been looking for was a solution that could then be backported to earlier releases to fix the problem at hand. Anyway, with this information, is it appropriate for us to replace our calls to lookup_one_len with calls to path_walk or is that also forbidden?

One thing that I can't allow for is the need for a non NULL nameidata struct following Christophs touch_atime patch. Requiring a valid nameidata struct is clearly a good thing.

Perhaps you could use lookup_hash. There is a depricated_for_modules patch for it in the -mm series. At least it's almost forbidden (:.

Ian

Christoph Hellwig

2005-12-09 12:12 UTC #81 lore raw

In reply to Will Taber

Quoted 3 lines · Will Taber

problem at hand. Anyway, with this information, is it appropriate for us to replace our calls to lookup_one_len with calls to path_walk or is that also forbidden?

path_walk and firends resolve a full path. As such they are definitly not suitable for stackable filesystem, they are intendeded only for implementating syscalls (and syscall-like interfaces, e.g. some ioctls)

John T. Kohl

2005-12-09 13:33 UTC #82 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Christoph" == Christoph Hellwig <[email protected]> writes:

Quoted 3 lines · Will Taber

Quoted 3 lines
problem at hand. Anyway, with this information, is it appropriate for us to replace our calls to lookup_one_len with calls to path_walk or is that also forbidden?

Christoph> path_walk and firends resolve a full path. As such they are Christoph> definitly not suitable for stackable filesystem, they are Christoph> intendeded only for implementating syscalls (and syscall-like Christoph> interfaces, e.g. some ioctls)

path_lookup() takes a pathname and interprets it relative to cwd or root, so it's definitely not useful for stacking. But path_walk() takes a pathname fragment and an initialized nameidata (start point), so it *could* be used to resolve a single component, or multiple components.

If lookup_one_len() is for use within the caller's file system only, and path_walk() is not suitable for stacking, then what calls *are* suitable for stacking file systems to use? We want to start with an existing (dentry,vfsmnt) and a pathname component, converting it to the (dentry,vfsmnt) of the result.

Signature · 5 lines

John Kohl
Senior Software Engineer - Rational Software - IBM Software Group
Lexington, Massachusetts, USA
[email protected]
<http://www.ibm.com/software/rational/>

Christoph Hellwig

2005-12-13 18:39 UTC #83 lore raw

Quoted 10 lines · John T. Kohl

Christoph> path_walk and firends resolve a full path. As such they are Christoph> definitly not suitable for stackable filesystem, they are Christoph> intendeded only for implementating syscalls (and syscall-like Christoph> interfaces, e.g. some ioctls)
path_lookup() takes a pathname and interprets it relative to cwd or root, so it's definitely not useful for stacking. But path_walk() takes a pathname fragment and an initialized nameidata (start point), so it *could* be used to resolve a single component, or multiple components.

Quoted 5 lines

If lookup_one_len() is for use within the caller's file system only, and path_walk() is not suitable for stacking, then what calls *are* suitable for stacking file systems to use? We want to start with an existing (dentry,vfsmnt) and a pathname component, converting it to the (dentry,vfsmnt) of the result.

As there are no stackable filesystems in the tree there's currently no interface designed for them, and once we'll get interfaces for stackable filesystems they'll surely be _GPL as they're clearly internal and any stackable filesystem needs to know a lot about the VFS and will have to change far more frequently than leaf filesystems.

path_walk is an internal implementation detail that will hopefull go away as an export.

Ian Kent

maintainer

2005-12-04 14:56 UTC #84 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 7 lines · Trond Myklebust
Quoted 4 lines · Ian Kent
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.

Quoted 8 lines

Quoted 4 lines
Consider those cases where the VFS calls ->lookup()/->d_revalidate() with the nameidata argument set to NULL to be bugs. They are pretty much all slated to be fixed soon in order to enable features like read-only bind mounts etc.
They are still NULL in exactly one case: lookup_one_len. Given the design of lookup_one_len we can't get at a nameidata there at all.

and I'll use that to detect the lookup_one_len call.

Ian

Will Taber

2005-12-02 19:04 UTC #85 lore raw

Quoted 103 lines · Ian Kent

Quoted 100 lines · Will Taber
Quoted 97 lines · Ian Kent
Quoted 59 lines · William H. Taber
Quoted 56 lines · Ian Kent
Quoted 6 lines · William H. Taber
Quoted 2 lines · Trond Myklebust
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.
Quoted 7 lines
Quoted 7 lines
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!
Quoted 1 line
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 31 lines

Quoted 31 lines
Quoted 31 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Quoted 17 lines

Quoted 15 lines
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.

Quoted 3 lines

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

Quoted 8 lines

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

Ian Kent

maintainer

2005-12-04 09:39 UTC #86 lore raw

Quoted 131 lines · Will Taber

Quoted 126 lines · Ian Kent
Quoted 113 lines · Will Taber
Quoted 111 lines · Ian Kent
Quoted 73 lines · William H. Taber
Quoted 70 lines · Ian Kent
Quoted 6 lines · William H. Taber
Quoted 2 lines · Trond Myklebust
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.
Quoted 5 lines
Quoted 5 lines
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!
Quoted 1 line
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.
Quoted 8 lines
Quoted 1 line
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.
Quoted 2 lines
P3 goes to the wait queue it can't wake up the waiters only mount completion can do that.

Quoted 7 lines

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.

I wasn't saying unconditionally but never the less Jeffs argument points out the error of my ways quite well.

Quoted 93 lines

Quoted 47 lines
Quoted 47 lines
Quoted 46 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 27 lines
Quoted 17 lines
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
Quoted 8 lines
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.
Quoted 7 lines
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.

Quoted 7 lines

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.

Of course it doesn't yet exist.

Yes.

Ian

Jeff Moyer

2005-12-02 16:04 UTC #87 lore raw