Trond Myklebust wrote:
On Wed, 2005-11-30 at 08:49 -0800, Badari Pulavarty wrote:
On Wed, 2005-11-30 at 09:02 -0500, Ian Kent wrote:
On Tue, 29 Nov 2005, William H. Taber wrote:
Ian Kent wrote:
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
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
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
_______________________________________________
autofs mailing list
[email protected]
http://linux.kernel.org/mailman/listinfo/autofs
