autofs4 looks up wrong path element when ghosting is enabled

autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

Hi, Ian, list,

I have a bug filed against autofs when ghosting is enabled.  The best way
to describe the bug is to walk through the reproducer, I guess.

Take the following maps, for example:

auto.master
/sbox	auto.sbox

auto.sbox:
src	segfault:/sbox/src/

Let's say that there is a file, id3_0.12.orig.tar.gz, in segfault:/sbox/src/.

To reproduce the problem, stop the nfs service on the server.

On the client, do an 'ls /sbox/src/id3_012.orig.tar.gz'.  This will fail,
as well it should.  However, if we look in the logs, we find this:

automount[1182]: handle_packet_missing: token 1, name src 
automount[1182]: attempting to mount entry /sbox/src
...
automount[1481]: mount(nfs): calling mkdir_path /sbox/src
automount[1481]: mount(nfs): calling mount -t nfs -s-o tcp,intr,timeo=600,rsize=8192,wsize=8192,retrans=5 segfault:/sbox/src /sbox/src
automount[1481]: >> mount: RPC: Program not registered
automount[1481]: mount(nfs): add_bad_host: segfault:/sbox/src
automount[1481]: mount(nfs): nfs: mount failure segfault:/sbox/src on /sbox/src
automount[1481]: failed to mount /sbox/src
...
automount[1182]: send_fail: token=1 
automount[1182]: handle_packet: type = 0 
automount[1182]: handle_packet_missing: token 2, name src/id3_0.12.orig.tar.gz 
automount[1182]: attempting to mount entry /sbox/src/id3_0.12.orig.tar.gz

Noteworthy are these last two lines!  Even though the mount failed, we are
continuing the lookup.  The culprit is here, in cached_lookup:

    if (!dentry->d_op->d_revalidate(dentry, flags) && !d_invalidate(dentry)) { 
            dput(dentry); 
            dentry = NULL; 
    } 

d_revalidate points to autofs4_revalidate, which calls try_to_fill_dentry,
which will return a status of 0.  Since ghosting is enabled,
d_invalidate(dentry) will return -EBUSY, and so we return the dentry to the
caller, which then continues the lookup.

Ian, I'm not really sure how we can address this issue without VFS
changes.  Any ideas?

Oh, also note that, once the nfs service is started up again on the server,
the lookup of a specific file name will still fail!  In this case, the
daemon won't even be called.

-Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Tue, 20 Sep 2005, Jeff Moyer wrote:

> Hi, Ian, list,
> 
> I have a bug filed against autofs when ghosting is enabled.  The best way
> to describe the bug is to walk through the reproducer, I guess.
> 
> Take the following maps, for example:
> 
> auto.master
> /sbox	auto.sbox
> 
> auto.sbox:
> src	segfault:/sbox/src/
> 
> Let's say that there is a file, id3_0.12.orig.tar.gz, in segfault:/sbox/src/.
> 
> To reproduce the problem, stop the nfs service on the server.
> 
> On the client, do an 'ls /sbox/src/id3_012.orig.tar.gz'.  This will fail,
> as well it should.  However, if we look in the logs, we find this:
> 
> automount[1182]: handle_packet_missing: token 1, name src 
> automount[1182]: attempting to mount entry /sbox/src
> ...
> automount[1481]: mount(nfs): calling mkdir_path /sbox/src
> automount[1481]: mount(nfs): calling mount -t nfs -s-o tcp,intr,timeo=600,rsize=8192,wsize=8192,retrans=5 segfault:/sbox/src /sbox/src
> automount[1481]: >> mount: RPC: Program not registered
> automount[1481]: mount(nfs): add_bad_host: segfault:/sbox/src
> automount[1481]: mount(nfs): nfs: mount failure segfault:/sbox/src on /sbox/src
> automount[1481]: failed to mount /sbox/src
> ...
> automount[1182]: send_fail: token=1 
> automount[1182]: handle_packet: type = 0 
> automount[1182]: handle_packet_missing: token 2, name src/id3_0.12.orig.tar.gz 
> automount[1182]: attempting to mount entry /sbox/src/id3_0.12.orig.tar.gz
> 
> Noteworthy are these last two lines!  Even though the mount failed, we are
> continuing the lookup.  The culprit is here, in cached_lookup:
> 
>     if (!dentry->d_op->d_revalidate(dentry, flags) && !d_invalidate(dentry)) { 
>             dput(dentry); 
>             dentry = NULL; 
>     } 
> 
> d_revalidate points to autofs4_revalidate, which calls try_to_fill_dentry,
> which will return a status of 0.  Since ghosting is enabled,
> d_invalidate(dentry) will return -EBUSY, and so we return the dentry to 
the
> caller, which then continues the lookup.
> 
> Ian, I'm not really sure how we can address this issue without VFS
> changes.  Any ideas?
> 

I'm aware of this problem.
I'm not sure how to deal with it yet.
The case above is probably not that difficult to solve but if the last 
component is a directory it's hard to work out it's a problem.

There's more information here than I've gathhered so far.

> Oh, also note that, once the nfs service is started up again on the server,
> the lookup of a specific file name will still fail!  In this case, the
> daemon won't even be called.

I'll have to check this out.
It could be helpful.

> 
> -Jeff
> 

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Tue, 20 Sep 2005, Jeff Moyer wrote:
>> Hi, Ian, list,
>> 
>> I have a bug filed against autofs when ghosting is enabled.  The best way
>> to describe the bug is to walk through the reproducer, I guess.
>> 
>> Take the following maps, for example:
>> 
>> auto.master
>> /sbox	auto.sbox
>> 
>> auto.sbox:
>> src	segfault:/sbox/src/
>> 
>> Let's say that there is a file, id3_0.12.orig.tar.gz, in segfault:/sbox/src/.
>> 
>> To reproduce the problem, stop the nfs service on the server.
>> 
>> On the client, do an 'ls /sbox/src/id3_012.orig.tar.gz'.  This will fail,
>> as well it should.  However, if we look in the logs, we find this:
>> 
>> automount[1182]: handle_packet_missing: token 1, name src 
>> automount[1182]: attempting to mount entry /sbox/src
>> ...
>> automount[1481]: mount(nfs): calling mkdir_path /sbox/src
>> automount[1481]: mount(nfs): calling mount -t nfs -s-o tcp,intr,timeo=600,rsize=8192,wsize=8192,retrans=5 segfault:/sbox/src /sbox/src
>> automount[1481]: >> mount: RPC: Program not registered
>> automount[1481]: mount(nfs): add_bad_host: segfault:/sbox/src
>> automount[1481]: mount(nfs): nfs: mount failure segfault:/sbox/src on /sbox/src
>> automount[1481]: failed to mount /sbox/src
>> ...
>> automount[1182]: send_fail: token=1 
>> automount[1182]: handle_packet: type = 0 
>> automount[1182]: handle_packet_missing: token 2, name src/id3_0.12.orig.tar.gz 
>> automount[1182]: attempting to mount entry /sbox/src/id3_0.12.orig.tar.gz
>> 
>> Noteworthy are these last two lines!  Even though the mount failed, we are
>> continuing the lookup.  The culprit is here, in cached_lookup:
>> 
>> if (!dentry->d_op->d_revalidate(dentry, flags) && !d_invalidate(dentry)) { 
>> dput(dentry); 
>> dentry = NULL; 
>> } 
>> 
>> d_revalidate points to autofs4_revalidate, which calls try_to_fill_dentry,
>> which will return a status of 0.  Since ghosting is enabled,
>> d_invalidate(dentry) will return -EBUSY, and so we return the dentry to 
raven> the
>> caller, which then continues the lookup.
>> 
>> Ian, I'm not really sure how we can address this issue without VFS
>> changes.  Any ideas?
>> 

raven> I'm aware of this problem.
raven> I'm not sure how to deal with it yet.
raven> The case above is probably not that difficult to solve but if the last 
raven> component is a directory it's hard to work out it's a problem.

Ugh.  If you're thinking what I think you're thinking, that's an ugly hack.

raven> There's more information here than I've gathhered so far.

>> Oh, also note that, once the nfs service is started up again on the server,
>> the lookup of a specific file name will still fail!  In this case, the
>> daemon won't even be called.

raven> I'll have to check this out.
raven> It could be helpful.

Well, I've provided a reproducer.  If you'd like log output from the kernel
side, let me know.  I can certainly provide that.

-Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Thu, 22 Sep 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Tue, 20 Sep 2005, Jeff Moyer wrote:
> >> Hi, Ian, list,
> >> 
> >> I have a bug filed against autofs when ghosting is enabled.  The best way
> >> to describe the bug is to walk through the reproducer, I guess.
> >> 
> >> Take the following maps, for example:
> >> 
> >> auto.master
> >> /sbox	auto.sbox
> >> 
> >> auto.sbox:
> >> src	segfault:/sbox/src/
> >> 
> >> Let's say that there is a file, id3_0.12.orig.tar.gz, in segfault:/sbox/src/.
> >> 
> >> To reproduce the problem, stop the nfs service on the server.
> >> 
> >> On the client, do an 'ls /sbox/src/id3_012.orig.tar.gz'.  This will fail,
> >> as well it should.  However, if we look in the logs, we find this:
> >> 
> >> automount[1182]: handle_packet_missing: token 1, name src 
> >> automount[1182]: attempting to mount entry /sbox/src
> >> ...
> >> automount[1481]: mount(nfs): calling mkdir_path /sbox/src
> >> automount[1481]: mount(nfs): calling mount -t nfs -s-o tcp,intr,timeo=600,rsize=8192,wsize=8192,retrans=5 segfault:/sbox/src /sbox/src
> >> automount[1481]: >> mount: RPC: Program not registered
> >> automount[1481]: mount(nfs): add_bad_host: segfault:/sbox/src
> >> automount[1481]: mount(nfs): nfs: mount failure segfault:/sbox/src on /sbox/src
> >> automount[1481]: failed to mount /sbox/src
> >> ...
> >> automount[1182]: send_fail: token=1 
> >> automount[1182]: handle_packet: type = 0 
> >> automount[1182]: handle_packet_missing: token 2, name src/id3_0.12.orig.tar.gz 
> >> automount[1182]: attempting to mount entry /sbox/src/id3_0.12.orig.tar.gz
> >> 
> >> Noteworthy are these last two lines!  Even though the mount failed, we are
> >> continuing the lookup.  The culprit is here, in cached_lookup:
> >> 
> >> if (!dentry->d_op->d_revalidate(dentry, flags) && !d_invalidate(dentry)) { 
> >> dput(dentry); 
> >> dentry = NULL; 
> >> } 
> >> 
> >> d_revalidate points to autofs4_revalidate, which calls try_to_fill_dentry,
> >> which will return a status of 0.  Since ghosting is enabled,
> >> d_invalidate(dentry) will return -EBUSY, and so we return the dentry to 
> raven> the
> >> caller, which then continues the lookup.
> >> 
> >> Ian, I'm not really sure how we can address this issue without VFS
> >> changes.  Any ideas?
> >> 
> 
> raven> I'm aware of this problem.
> raven> I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the last 
> raven> component is a directory it's hard to work out it's a problem.
> 
> Ugh.  If you're thinking what I think you're thinking, that's an ugly hack.

Don't think so.

I've been seeing this for a while. I wasn't quite sure of the source but, 
for some reason your report has cleared that up.

The problem is not so much the success returned on the failed mount 
(revalidate). It's the return from the following lookup. This is a lookup 
in a non-root directory. I replaced the non-root lookup with the root 
lookup a while ago and I think this is an unexpected side affect of that. 
Becuase of other changes that lead to that decision I think that it should 
be now be OK to put back the null function (always return a negative 
dentry) that was there before I started working on the browable maps 
feature.

I'll change the module I use here and test it out for a while.
If you have time I could make a patch for the 2.4 code and send it over so 
that you could test it out a bit as well.

> 
> raven> There's more information here than I've gathhered so far.
> 
> >> Oh, also note that, once the nfs service is started up again on the server,
> >> the lookup of a specific file name will still fail!  In this case, the
> >> daemon won't even be called.
> 
> raven> I'll have to check this out.
> raven> It could be helpful.
> 
> Well, I've provided a reproducer.  If you'd like log output from the kernel
> side, let me know.  I can certainly provide that.

Don't think I need it.
I'm fairly sure I understand what's happening here now. As I said above.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Thu, 22 Sep 2005, Jeff Moyer wrote:
>> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is
>> enabled; Ian Kent <raven@themaw.net> adds:
>> 
raven> On Tue, 20 Sep 2005, Jeff Moyer wrote:
>> >> Hi, Ian, list,
>> >> 
>> >> I have a bug filed against autofs when ghosting is enabled.  The best
>> way >> to describe the bug is to walk through the reproducer, I guess.
>> >> 
>> >> Take the following maps, for example:
>> >> 
>> >> auto.master >> /sbox auto.sbox
>> >> 
>> >> auto.sbox: >> src segfault:/sbox/src/
>> >> 
>> >> Let's say that there is a file, id3_0.12.orig.tar.gz, in
>> segfault:/sbox/src/.
>> >> 
>> >> To reproduce the problem, stop the nfs service on the server.
>> >> 
>> >> On the client, do an 'ls /sbox/src/id3_012.orig.tar.gz'.  This will
>> fail, >> as well it should.  However, if we look in the logs, we find
>> this:
>> >> 
>> >> automount[1182]: handle_packet_missing: token 1, name src >>
>> automount[1182]: attempting to mount entry /sbox/src >> ...  >>
>> automount[1481]: mount(nfs): calling mkdir_path /sbox/src >>
>> automount[1481]: mount(nfs): calling mount -t nfs -s-o
>> tcp,intr,timeo=600,rsize=8192,wsize=8192,retrans=5 segfault:/sbox/src
>> /sbox/src >> automount[1481]: >> mount: RPC: Program not registered >>
>> automount[1481]: mount(nfs): add_bad_host: segfault:/sbox/src >>
>> automount[1481]: mount(nfs): nfs: mount failure segfault:/sbox/src on
>> /sbox/src >> automount[1481]: failed to mount /sbox/src >> ...  >>
>> automount[1182]: send_fail: token=1 >> automount[1182]: handle_packet:
>> type = 0 >> automount[1182]: handle_packet_missing: token 2, name
>> src/id3_0.12.orig.tar.gz >> automount[1182]: attempting to mount entry
>> /sbox/src/id3_0.12.orig.tar.gz
>> >> 
>> >> Noteworthy are these last two lines!  Even though the mount failed,
>> we are >> continuing the lookup.  The culprit is here, in cached_lookup:
>> >> 
>> >> if (!dentry->d_op->d_revalidate(dentry, flags) &&
>> !d_invalidate(dentry)) { >> dput(dentry); >> dentry = NULL; >> }
>> >> 
>> >> d_revalidate points to autofs4_revalidate, which calls
>> try_to_fill_dentry, >> which will return a status of 0.  Since ghosting
>> is enabled, >> d_invalidate(dentry) will return -EBUSY, and so we return
>> the dentry to
raven> the
>> >> caller, which then continues the lookup.
>> >> 
>> >> Ian, I'm not really sure how we can address this issue without VFS >>
>> changes.  Any ideas?
>> >> 
>> 
raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
raven> The case above is probably not that difficult to solve but if the
raven> last component is a directory it's hard to work out it's a problem.
>> Ugh.  If you're thinking what I think you're thinking, that's an ugly
>> hack.

raven> Don't think so.

raven> I've been seeing this for a while. I wasn't quite sure of the source
raven> but, for some reason your report has cleared that up.

raven> The problem is not so much the success returned on the failed mount
raven> (revalidate). It's the return from the following lookup. This is a
raven> lookup in a non-root directory. I replaced the non-root lookup with
raven> the root lookup a while ago and I think this is an unexpected side
raven> affect of that. Becuase of other changes that lead to that decision
raven> I think that it should be now be OK to put back the null function
raven> (always return a negative dentry) that was there before I started
raven> working on the browable maps feature.

raven> I'll change the module I use here and test it out for a while.  If
raven> you have time I could make a patch for the 2.4 code and send it over
raven> so that you could test it out a bit as well.

Just send along the 2.6 patch, since I have to deal with that, too.  I'll
go through the trouble of backporting it.

>>
raven> There's more information here than I've gathhered so far.
>> >> Oh, also note that, once the nfs service is started up again on the
>> server, >> the lookup of a specific file name will still fail!  In this
>> case, the >> daemon won't even be called.
>> 
raven> I'll have to check this out.  It could be helpful.
>> Well, I've provided a reproducer.  If you'd like log output from the
>> kernel side, let me know.  I can certainly provide that.

raven> Don't think I need it.  I'm fairly sure I understand what's
raven> happening here now. As I said above.

Cool.  Thanks!

Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Sat, 24 Sep 2005, Jeff Moyer wrote:

> >> >> 
> >> >> Ian, I'm not really sure how we can address this issue without VFS >>
> >> changes.  Any ideas?
> >> >> 
> >> 
> raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the
> raven> last component is a directory it's hard to work out it's a problem.
> >> Ugh.  If you're thinking what I think you're thinking, that's an ugly
> >> hack.
> 
> raven> Don't think so.
> 
> raven> I've been seeing this for a while. I wasn't quite sure of the source
> raven> but, for some reason your report has cleared that up.
> 
> raven> The problem is not so much the success returned on the failed mount
> raven> (revalidate). It's the return from the following lookup. This is a
> raven> lookup in a non-root directory. I replaced the non-root lookup with
> raven> the root lookup a while ago and I think this is an unexpected side
> raven> affect of that. Becuase of other changes that lead to that decision
> raven> I think that it should be now be OK to put back the null function
> raven> (always return a negative dentry) that was there before I started
> raven> working on the browable maps feature.
> 
> raven> I'll change the module I use here and test it out for a while.  If
> raven> you have time I could make a patch for the 2.4 code and send it over
> raven> so that you could test it out a bit as well.
> 
> Just send along the 2.6 patch, since I have to deal with that, too.  I'll
> go through the trouble of backporting it.

I'm in the middle of working on lazy multi-mounts atm so I'm not in a good 
position to test. It's a little tricky so I don't want to forget where I'm 
at by getting side tracked.

But here's the patch that I will apply to my v5 tree for the initial 
testing. Hopefully you will be able to give it a run in a standard setup.

Ian

diff -Nurp linux-2.6.12.orig/fs/autofs4/root.c linux-2.6.12/fs/autofs4/root.c
--- linux-2.6.12.orig/fs/autofs4/root.c	2005-06-18 03:48:29.000000000 +0800
+++ linux-2.6.12/fs/autofs4/root.c	2005-09-25 09:15:11.000000000 +0800
@@ -28,7 +28,8 @@ static int autofs4_dir_open(struct inode
 static int autofs4_dir_close(struct inode *inode, struct file *file);
 static int autofs4_dir_readdir(struct file * filp, void * dirent, filldir_t filldir);
 static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t filldir);
-static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
+static struct dentry *autofs4_root_lookup(struct inode *,struct dentry *, struct nameidata *);
+static struct dentry *autofs4_dir_lookup(struct inode *,struct dentry *, struct nameidata *);
 static int autofs4_dcache_readdir(struct file *, void *, filldir_t);
 
 struct file_operations autofs4_root_operations = {
@@ -47,7 +48,7 @@ struct file_operations autofs4_dir_opera
 };
 
 struct inode_operations autofs4_root_inode_operations = {
-	.lookup		= autofs4_lookup,
+	.lookup		= autofs4_root_lookup,
 	.unlink		= autofs4_dir_unlink,
 	.symlink	= autofs4_dir_symlink,
 	.mkdir		= autofs4_dir_mkdir,
@@ -55,7 +56,7 @@ struct inode_operations autofs4_root_ino
 };
 
 struct inode_operations autofs4_dir_inode_operations = {
-	.lookup		= autofs4_lookup,
+	.lookup		= autofs4_dir_lookup,
 	.unlink		= autofs4_dir_unlink,
 	.symlink	= autofs4_dir_symlink,
 	.mkdir		= autofs4_dir_mkdir,
@@ -438,8 +439,19 @@ static struct dentry_operations autofs4_
 	.d_release	= autofs4_dentry_release,
 };
 
+static struct dentry *autofs4_dir_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
+{
+	DPRINTK(("ignoring lookup of %.*s/%.*s\n",
+		dentry->d_parent->d_name.len, dentry->d_parent->d_name.name,
+		dentry->d_name.len, dentry->d_name.name));
+
+	dentry->d_fsdata = NULL;
+	d_add(dentry, NULL);
+	return NULL;
+}
+
 /* Lookups in the root directory */
-static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
+static struct dentry *autofs4_root_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
 {
 	struct autofs_sb_info *sbi;
 	int oz_mode;

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
>> >> >> >> >> Ian, I'm not really sure how we can address this issue
>> without VFS >> >> changes.  Any ideas?
>> >> >> 
>> >> 
raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
raven> The case above is probably not that difficult to solve but if the
raven> last component is a directory it's hard to work out it's a problem.
>> >> Ugh.  If you're thinking what I think you're thinking, that's an ugly
>> >> hack.
>> 
raven> Don't think so.
>>
raven> I've been seeing this for a while. I wasn't quite sure of the source
raven> but, for some reason your report has cleared that up.
>>
raven> The problem is not so much the success returned on the failed mount
raven> (revalidate). It's the return from the following lookup. This is a
raven> lookup in a non-root directory. I replaced the non-root lookup with
raven> the root lookup a while ago and I think this is an unexpected side
raven> affect of that. Becuase of other changes that lead to that decision
raven> I think that it should be now be OK to put back the null function
raven> (always return a negative dentry) that was there before I started
raven> working on the browable maps feature.
>>
raven> I'll change the module I use here and test it out for a while.  If
raven> you have time I could make a patch for the 2.4 code and send it over
raven> so that you could test it out a bit as well.
>> Just send along the 2.6 patch, since I have to deal with that, too.
>> I'll go through the trouble of backporting it.

raven> I'm in the middle of working on lazy multi-mounts atm so I'm not in
raven> a good position to test. It's a little tricky so I don't want to
raven> forget where I'm at by getting side tracked.

raven> But here's the patch that I will apply to my v5 tree for the initial
raven> testing. Hopefully you will be able to give it a run in a standard
raven> setup.

Ian, this will introduce a regression.  That code was changed to fix a
different bug with ghosted direct maps.  So, at least for autofs4, this
isn't a good fix.  To be more specific, the problem comes when you update a
ghosted direct map.  So, if you had a direct map that looks like:

/opt/local/bin server:/export/local/bin

An ls of /opt/local would show the 'bin' directory.  If we update the map,
adding this entry:

/opt/local/share	server:/export/local/share

An ls of '/opt/local/share' will return -ENOENT.  The current behaviour is
to list the contents of /opt/local/share, which is correct.

-Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
>> >> >> >> >> Ian, I'm not really sure how we can address this issue
>> without VFS >> >> changes.  Any ideas?
>> >> >> 
>> >> 
raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
raven> The case above is probably not that difficult to solve but if the
raven> last component is a directory it's hard to work out it's a problem.
>> >> Ugh.  If you're thinking what I think you're thinking, that's an ugly
>> >> hack.
>> 
raven> Don't think so.
>>
raven> I've been seeing this for a while. I wasn't quite sure of the source
raven> but, for some reason your report has cleared that up.
>>
raven> The problem is not so much the success returned on the failed mount
raven> (revalidate). It's the return from the following lookup. This is a
raven> lookup in a non-root directory. I replaced the non-root lookup with
raven> the root lookup a while ago and I think this is an unexpected side
raven> affect of that. Becuase of other changes that lead to that decision
raven> I think that it should be now be OK to put back the null function
raven> (always return a negative dentry) that was there before I started
raven> working on the browable maps feature.
>>
raven> I'll change the module I use here and test it out for a while.  If
raven> you have time I could make a patch for the 2.4 code and send it over
raven> so that you could test it out a bit as well.
>> Just send along the 2.6 patch, since I have to deal with that, too.
>> I'll go through the trouble of backporting it.

raven> I'm in the middle of working on lazy multi-mounts atm so I'm not in
raven> a good position to test. It's a little tricky so I don't want to
raven> forget where I'm at by getting side tracked.

raven> But here's the patch that I will apply to my v5 tree for the initial
raven> testing. Hopefully you will be able to give it a run in a standard
raven> setup.

I put together a different patch, but I want to get your input on the
approach before I post it.  It requires both user-space and kernel-space
changes.

Basically, you identify that a given automount tree is a direct map tree.
This information is passed along to the kernel (I did this via a mount
option), and recorded (in the super block info).  Then, when a directory
lookup occurs, if we are in a direct map tree, and ghosting is enabled,
then we pass the lookup on to the real lookup code.

I'm not sold on the approach, as I haven't thought through all of the
implications.  Care to comment?

-Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Mon, 26 Sep 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
> >> >> >> >> >> Ian, I'm not really sure how we can address this issue
> >> without VFS >> >> changes.  Any ideas?
> >> >> >> 
> >> >> 
> raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the
> raven> last component is a directory it's hard to work out it's a problem.
> >> >> Ugh.  If you're thinking what I think you're thinking, that's an ugly
> >> >> hack.
> >> 
> raven> Don't think so.
> >>
> raven> I've been seeing this for a while. I wasn't quite sure of the source
> raven> but, for some reason your report has cleared that up.
> >>
> raven> The problem is not so much the success returned on the failed mount
> raven> (revalidate). It's the return from the following lookup. This is a
> raven> lookup in a non-root directory. I replaced the non-root lookup with
> raven> the root lookup a while ago and I think this is an unexpected side
> raven> affect of that. Becuase of other changes that lead to that decision
> raven> I think that it should be now be OK to put back the null function
> raven> (always return a negative dentry) that was there before I started
> raven> working on the browable maps feature.
> >>
> raven> I'll change the module I use here and test it out for a while.  If
> raven> you have time I could make a patch for the 2.4 code and send it over
> raven> so that you could test it out a bit as well.
> >> Just send along the 2.6 patch, since I have to deal with that, too.
> >> I'll go through the trouble of backporting it.
> 
> raven> I'm in the middle of working on lazy multi-mounts atm so I'm not in
> raven> a good position to test. It's a little tricky so I don't want to
> raven> forget where I'm at by getting side tracked.
> 
> raven> But here's the patch that I will apply to my v5 tree for the initial
> raven> testing. Hopefully you will be able to give it a run in a standard
> raven> setup.
> 
> Ian, this will introduce a regression.  That code was changed to fix a
> different bug with ghosted direct maps.  So, at least for autofs4, this
> isn't a good fix.  To be more specific, the problem comes when you update a
> ghosted direct map.  So, if you had a direct map that looks like:

Yep. I see that too. Missed that.

I'll have to look more closely at the returns from lookup.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Mon, 26 Sep 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
> >> >> >> >> >> Ian, I'm not really sure how we can address this issue
> >> without VFS >> >> changes.  Any ideas?
> >> >> >> 
> >> >> 
> raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the
> raven> last component is a directory it's hard to work out it's a problem.
> >> >> Ugh.  If you're thinking what I think you're thinking, that's an ugly
> >> >> hack.
> >> 
> raven> Don't think so.
> >>
> raven> I've been seeing this for a while. I wasn't quite sure of the source
> raven> but, for some reason your report has cleared that up.
> >>
> raven> The problem is not so much the success returned on the failed mount
> raven> (revalidate). It's the return from the following lookup. This is a
> raven> lookup in a non-root directory. I replaced the non-root lookup with
> raven> the root lookup a while ago and I think this is an unexpected side
> raven> affect of that. Becuase of other changes that lead to that decision
> raven> I think that it should be now be OK to put back the null function
> raven> (always return a negative dentry) that was there before I started
> raven> working on the browable maps feature.
> >>
> raven> I'll change the module I use here and test it out for a while.  If
> raven> you have time I could make a patch for the 2.4 code and send it over
> raven> so that you could test it out a bit as well.
> >> Just send along the 2.6 patch, since I have to deal with that, too.
> >> I'll go through the trouble of backporting it.
> 
> raven> I'm in the middle of working on lazy multi-mounts atm so I'm not in
> raven> a good position to test. It's a little tricky so I don't want to
> raven> forget where I'm at by getting side tracked.
> 
> raven> But here's the patch that I will apply to my v5 tree for the initial
> raven> testing. Hopefully you will be able to give it a run in a standard
> raven> setup.
> 
> I put together a different patch, but I want to get your input on the
> approach before I post it.  It requires both user-space and kernel-space
> changes.
> 
> Basically, you identify that a given automount tree is a direct map tree.
> This information is passed along to the kernel (I did this via a mount
> option), and recorded (in the super block info).  Then, when a directory
> lookup occurs, if we are in a direct map tree, and ghosting is enabled,
> then we pass the lookup on to the real lookup code.
> 
> I'm not sold on the approach, as I haven't thought through all of the
> implications.  Care to comment?
> 

Based on your description I'm not sure that this is the simplest approach.

The fundamental problem here is, I think, an incorrect return code from 
lookup. revalidate is called from lookup and it's return code is not 
checked. Even if it was the return is often success even if there is a 
mount failure.

revalidate calls try_to_fill_dentry (I think that's the function name) 
which is responsible for the false return code. Some thought about why it 
returns these would be good to do first. This may well be historic and 
overdue to be updated. If so correcting the return codes and checking 
them in lookup my be a better solution.

I'll have a closer look at the patch and the return code issue tonight.

Thanks for your continued help.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Mon, 26 Sep 2005, Jeff Moyer wrote:

> 
> I put together a different patch, but I want to get your input on the
> approach before I post it.  It requires both user-space and kernel-space
> changes.
> 
> Basically, you identify that a given automount tree is a direct map tree.
> This information is passed along to the kernel (I did this via a mount
> option), and recorded (in the super block info).  Then, when a directory
> lookup occurs, if we are in a direct map tree, and ghosting is enabled,
> then we pass the lookup on to the real lookup code.
> 
> I'm not sold on the approach, as I haven't thought through all of the
> implications.  Care to comment?

Can you post your patch please Jeff.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Mon, 26 Sep 2005, Jeff Moyer wrote:
>> I put together a different patch, but I want to get your input on the
>> approach before I post it.  It requires both user-space and kernel-space
>> changes.
>> 
>> Basically, you identify that a given automount tree is a direct map
>> tree.  This information is passed along to the kernel (I did this via a
>> mount option), and recorded (in the super block info).  Then, when a
>> directory lookup occurs, if we are in a direct map tree, and ghosting is
>> enabled, then we pass the lookup on to the real lookup code.
>> 
>> I'm not sold on the approach, as I haven't thought through all of the
>> implications.  Care to comment?

raven> Can you post your patch please Jeff.

I would, if it worked!  I'm away until the 17th.  I'll see what I can
come up with when I get back.  Sorry!

-Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Sat, 8 Oct 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Mon, 26 Sep 2005, Jeff Moyer wrote:
> >> I put together a different patch, but I want to get your input on the
> >> approach before I post it.  It requires both user-space and kernel-space
> >> changes.
> >> 
> >> Basically, you identify that a given automount tree is a direct map
> >> tree.  This information is passed along to the kernel (I did this via a
> >> mount option), and recorded (in the super block info).  Then, when a
> >> directory lookup occurs, if we are in a direct map tree, and ghosting is
> >> enabled, then we pass the lookup on to the real lookup code.
> >> 
> >> I'm not sold on the approach, as I haven't thought through all of the
> >> implications.  Care to comment?
> 
> raven> Can you post your patch please Jeff.
> 
> I would, if it worked!  I'm away until the 17th.  I'll see what I can
> come up with when I get back.  Sorry!

No problem.

I'm investigating as well.
It's a bit tricky this case.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Mon, 26 Sep 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
> >> >> >> >> >> Ian, I'm not really sure how we can address this issue
> >> without VFS >> >> changes.  Any ideas?
> >> >> >> 
> >> >> 
> raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the
> raven> last component is a directory it's hard to work out it's a problem.
> >> >> Ugh.  If you're thinking what I think you're thinking, that's an ugly
> >> >> hack.
> >> 
> raven> Don't think so.
> >>
> raven> I've been seeing this for a while. I wasn't quite sure of the source
> raven> but, for some reason your report has cleared that up.
> >>
> raven> The problem is not so much the success returned on the failed mount
> raven> (revalidate). It's the return from the following lookup. This is a
> raven> lookup in a non-root directory. I replaced the non-root lookup with
> raven> the root lookup a while ago and I think this is an unexpected side
> raven> affect of that. Becuase of other changes that lead to that decision
> raven> I think that it should be now be OK to put back the null function
> raven> (always return a negative dentry) that was there before I started
> raven> working on the browable maps feature.
> >>

I've had a look at this a bit more deeply.

As we know we can't make the path walk lookup fail by autofs4_revalidate 
simply returning 0 and to change that in the kernel would be far to 
dangerous. So we need to deal with this during the following lookup. This 
just means we get an unwanted callback to the daemon which will fail 
and should not cause a problem.

I'm still not fully clear on the reasoning behind the logic in 
try_to_fill_dentry when called with a negative dentry. One of the things 
it attempts to do is cache a lookup failure (ENOENT return from the wait). 
Unfortuneatly the subsequent test in autofs4_revalidate is a tautology, 
always returning true. So d_invalidate is never called to cleanup what 
might be a stale dentry. While this is not causing the problem stale 
dentrys are the problem.

I still haven't decided whether it would be a good idea to return 0 
instead of 1 from try_to_fill_dentry for these failed mount attempts. All 
this would do is give the kernel more chances to clean up the stale 
dentries. The dentry in question won't be released at this point as it has 
a non zero reference count (I believe). But sooner or later they will go 
anyway when d_invalidate is called.

So to resolve this we need to ignore negative and unhashed dentries when 
checking if directory dentry is empty.

Please test this patch and let me know how you go.

diff -Nurp linux-2.6.12.orig/fs/autofs4/expire.c linux-2.6.12/fs/autofs4/expire.c
--- linux-2.6.12.orig/fs/autofs4/expire.c	2005-06-18 03:48:29.000000000 +0800
+++ linux-2.6.12/fs/autofs4/expire.c	2005-10-09 15:11:37.000000000 +0800
@@ -177,7 +177,7 @@ resume:
 		DPRINTK("dentry %p %.*s",
 			dentry, (int)dentry->d_name.len, dentry->d_name.name);
 
-		if (!list_empty(&dentry->d_subdirs)) {
+		if (!simple_empty_nolock(dentry)) {
 			this_parent = dentry;
 			goto repeat;
 		}
@@ -269,7 +269,7 @@ static struct dentry *autofs4_expire(str
 			goto next;
 		}
 
-		if ( simple_empty(dentry) )
+		if (simple_empty(dentry))
 			goto next;
 
 		/* Case 2: tree mount, expire iff entire tree is not busy */
diff -Nurp linux-2.6.12.orig/fs/autofs4/root.c linux-2.6.12/fs/autofs4/root.c
--- linux-2.6.12.orig/fs/autofs4/root.c	2005-06-18 03:48:29.000000000 +0800
+++ linux-2.6.12/fs/autofs4/root.c	2005-10-09 15:52:04.000000000 +0800
@@ -386,13 +386,13 @@ static int autofs4_revalidate(struct den
 
 	/* Negative dentry.. invalidate if "old" */
 	if (dentry->d_inode == NULL)
-		return (dentry->d_time - jiffies <= AUTOFS_NEGATIVE_TIMEOUT);
+		return (dentry->d_time - jiffies <= 0);
 
 	/* Check for a non-mountpoint directory with no contents */
 	spin_lock(&dcache_lock);
 	if (S_ISDIR(dentry->d_inode->i_mode) &&
 	    !d_mountpoint(dentry) && 
-	    list_empty(&dentry->d_subdirs)) {
+	    simple_empty_nolock(dentry)) {
 		DPRINTK("dentry=%p %.*s, emptydir",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 		spin_unlock(&dcache_lock);

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Mon, 26 Sep 2005, Jeff Moyer wrote:
>> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is
>> enabled; Ian Kent <raven@themaw.net> adds:
>> 
raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
>> >> >> >> >> >> Ian, I'm not really sure how we can address this issue >>
>> without VFS >> >> changes.  Any ideas?
>> >> >> >> 
>> >> >> 
raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
raven> The case above is probably not that difficult to solve but if the
raven> last component is a directory it's hard to work out it's a problem.
>> >> >> Ugh.  If you're thinking what I think you're thinking, that's an
>> ugly >> >> hack.
>> >> 
raven> Don't think so.
>> >>
raven> I've been seeing this for a while. I wasn't quite sure of the source
raven> but, for some reason your report has cleared that up.
>> >>
raven> The problem is not so much the success returned on the failed mount
raven> (revalidate). It's the return from the following lookup. This is a
raven> lookup in a non-root directory. I replaced the non-root lookup with
raven> the root lookup a while ago and I think this is an unexpected side
raven> affect of that. Becuase of other changes that lead to that decision
raven> I think that it should be now be OK to put back the null function
raven> (always return a negative dentry) that was there before I started
raven> working on the browable maps feature.
>> >>

raven> I've had a look at this a bit more deeply.

raven> As we know we can't make the path walk lookup fail by
raven> autofs4_revalidate simply returning 0 and to change that in the
raven> kernel would be far to dangerous. So we need to deal with this
raven> during the following lookup. This just means we get an unwanted
raven> callback to the daemon which will fail and should not cause a
raven> problem.

raven> I'm still not fully clear on the reasoning behind the logic in
raven> try_to_fill_dentry when called with a negative dentry. One of the
raven> things it attempts to do is cache a lookup failure (ENOENT return
raven> from the wait). Unfortuneatly the subsequent test in
raven> autofs4_revalidate is a tautology, always returning true. So
raven> d_invalidate is never called to cleanup what might be a stale
raven> dentry. While this is not causing the problem stale dentrys are the
raven> problem.

raven> I still haven't decided whether it would be a good idea to return 0
raven> instead of 1 from try_to_fill_dentry for these failed mount
raven> attempts. All this would do is give the kernel more chances to clean
raven> up the stale dentries. The dentry in question won't be released at
raven> this point as it has a non zero reference count (I believe). But
raven> sooner or later they will go anyway when d_invalidate is called.

raven> So to resolve this we need to ignore negative and unhashed dentries
raven> when checking if directory dentry is empty.

raven> Please test this patch and let me know how you go.

OK, I've finally got 'round to testing your patch.  It does fix the test
case I was using.  My only concern is the potential for regressions.  I'll
try making sure all of my various maps still work as advertised.

Thanks!

Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Wed, 19 Oct 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Mon, 26 Sep 2005, Jeff Moyer wrote:
> >> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is
> >> enabled; Ian Kent <raven@themaw.net> adds:
> >> 
> raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
> >> >> >> >> >> >> Ian, I'm not really sure how we can address this issue >>
> >> without VFS >> >> changes.  Any ideas?
> >> >> >> >> 
> >> >> >> 
> raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the
> raven> last component is a directory it's hard to work out it's a problem.
> >> >> >> Ugh.  If you're thinking what I think you're thinking, that's an
> >> ugly >> >> hack.
> >> >> 
> raven> Don't think so.
> >> >>
> raven> I've been seeing this for a while. I wasn't quite sure of the source
> raven> but, for some reason your report has cleared that up.
> >> >>
> raven> The problem is not so much the success returned on the failed mount
> raven> (revalidate). It's the return from the following lookup. This is a
> raven> lookup in a non-root directory. I replaced the non-root lookup with
> raven> the root lookup a while ago and I think this is an unexpected side
> raven> affect of that. Becuase of other changes that lead to that decision
> raven> I think that it should be now be OK to put back the null function
> raven> (always return a negative dentry) that was there before I started
> raven> working on the browable maps feature.
> >> >>
> 
> raven> I've had a look at this a bit more deeply.
> 
> raven> As we know we can't make the path walk lookup fail by
> raven> autofs4_revalidate simply returning 0 and to change that in the
> raven> kernel would be far to dangerous. So we need to deal with this
> raven> during the following lookup. This just means we get an unwanted
> raven> callback to the daemon which will fail and should not cause a
> raven> problem.
> 
> raven> I'm still not fully clear on the reasoning behind the logic in
> raven> try_to_fill_dentry when called with a negative dentry. One of the
> raven> things it attempts to do is cache a lookup failure (ENOENT return
> raven> from the wait). Unfortuneatly the subsequent test in
> raven> autofs4_revalidate is a tautology, always returning true. So
> raven> d_invalidate is never called to cleanup what might be a stale
> raven> dentry. While this is not causing the problem stale dentrys are the
> raven> problem.
> 
> raven> I still haven't decided whether it would be a good idea to return 0
> raven> instead of 1 from try_to_fill_dentry for these failed mount
> raven> attempts. All this would do is give the kernel more chances to clean
> raven> up the stale dentries. The dentry in question won't be released at
> raven> this point as it has a non zero reference count (I believe). But
> raven> sooner or later they will go anyway when d_invalidate is called.
> 
> raven> So to resolve this we need to ignore negative and unhashed dentries
> raven> when checking if directory dentry is empty.
> 
> raven> Please test this patch and let me know how you go.
> 
> OK, I've finally got 'round to testing your patch.  It does fix the test
> case I was using.  My only concern is the potential for regressions.  I'll
> try making sure all of my various maps still work as advertised.

Yep. That's always a danger.

Indeed I had a similar regression in the past. It was quickly obvious there 
was something wrong. oth this change is looking OK so far but more 
testing is definitely needed.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Wed, 19 Oct 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Mon, 26 Sep 2005, Jeff Moyer wrote:
> >> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is
> >> enabled; Ian Kent <raven@themaw.net> adds:
> >> 
> raven> On Sat, 24 Sep 2005, Jeff Moyer wrote:
> >> >> >> >> >> >> Ian, I'm not really sure how we can address this issue >>
> >> without VFS >> >> changes.  Any ideas?
> >> >> >> >> 
> >> >> >> 
> raven> I'm aware of this problem.  I'm not sure how to deal with it yet.
> raven> The case above is probably not that difficult to solve but if the
> raven> last component is a directory it's hard to work out it's a problem.
> >> >> >> Ugh.  If you're thinking what I think you're thinking, that's an
> >> ugly >> >> hack.
> >> >> 
> raven> Don't think so.
> >> >>
> raven> I've been seeing this for a while. I wasn't quite sure of the source
> raven> but, for some reason your report has cleared that up.
> >> >>
> raven> The problem is not so much the success returned on the failed mount
> raven> (revalidate). It's the return from the following lookup. This is a
> raven> lookup in a non-root directory. I replaced the non-root lookup with
> raven> the root lookup a while ago and I think this is an unexpected side
> raven> affect of that. Becuase of other changes that lead to that decision
> raven> I think that it should be now be OK to put back the null function
> raven> (always return a negative dentry) that was there before I started
> raven> working on the browable maps feature.
> >> >>
> 
> raven> I've had a look at this a bit more deeply.
> 
> raven> As we know we can't make the path walk lookup fail by
> raven> autofs4_revalidate simply returning 0 and to change that in the
> raven> kernel would be far to dangerous. So we need to deal with this
> raven> during the following lookup. This just means we get an unwanted
> raven> callback to the daemon which will fail and should not cause a
> raven> problem.
> 
> raven> I'm still not fully clear on the reasoning behind the logic in
> raven> try_to_fill_dentry when called with a negative dentry. One of the
> raven> things it attempts to do is cache a lookup failure (ENOENT return
> raven> from the wait). Unfortuneatly the subsequent test in
> raven> autofs4_revalidate is a tautology, always returning true. So
> raven> d_invalidate is never called to cleanup what might be a stale
> raven> dentry. While this is not causing the problem stale dentrys are the
> raven> problem.
> 
> raven> I still haven't decided whether it would be a good idea to return 0
> raven> instead of 1 from try_to_fill_dentry for these failed mount
> raven> attempts. All this would do is give the kernel more chances to clean
> raven> up the stale dentries. The dentry in question won't be released at
> raven> this point as it has a non zero reference count (I believe). But
> raven> sooner or later they will go anyway when d_invalidate is called.
> 
> raven> So to resolve this we need to ignore negative and unhashed dentries
> raven> when checking if directory dentry is empty.
> 
> raven> Please test this patch and let me know how you go.
> 
> OK, I've finally got 'round to testing your patch.  It does fix the test
> case I was using.  My only concern is the potential for regressions.  I'll
> try making sure all of my various maps still work as advertised.

I've spotted a regression with this patch.
It doesn't stop autofs from appearing to function correctly. It causes 
mount callbacks when they shouldn't made. So it seems that there is a 
case when an autofs directory is, as yet unhashed, but should be used.

Grumble.

Ian

Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> So to resolve this we need to ignore negative and unhashed dentries
raven> when checking if directory dentry is empty.
>>
raven> Please test this patch and let me know how you go.
>> OK, I've finally got 'round to testing your patch.  It does fix the test
>> case I was using.  My only concern is the potential for regressions.
>> I'll try making sure all of my various maps still work as advertised.

raven> I've spotted a regression with this patch.  It doesn't stop autofs
raven> from appearing to function correctly. It causes mount callbacks when
raven> they shouldn't made. So it seems that there is a case when an autofs
raven> directory is, as yet unhashed, but should be used.

I'm not sure I follow.  What do you mean it doesn't stop autofs from
*appearing* to function correctly?  Do you have a reproducer that fails?

-Jeff

Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Mon, 31 Oct 2005, Jeff Moyer wrote:

> ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> So to resolve this we need to ignore negative and unhashed dentries
> raven> when checking if directory dentry is empty.
> >>
> raven> Please test this patch and let me know how you go.
> >> OK, I've finally got 'round to testing your patch.  It does fix the test
> >> case I was using.  My only concern is the potential for regressions.
> >> I'll try making sure all of my various maps still work as advertised.
> 
> raven> I've spotted a regression with this patch.  It doesn't stop autofs
> raven> from appearing to function correctly. It causes mount callbacks when
> raven> they shouldn't made. So it seems that there is a case when an autofs
> raven> directory is, as yet unhashed, but should be used.
> 
> I'm not sure I follow.  What do you mean it doesn't stop autofs from
> *appearing* to function correctly?  Do you have a reproducer that fails?

Any pseudo direct map will produce the behaviour.

It behaves as if the ghost option was not specified even when it has. 
This is because the altered test for an empty directory is always 
returning true even though the directory isn't empty.

I'm still trying to understand why this happens. In theory it's just not 
the expected behaviour. I must be missing something in the directory 
creation. I've been here before and looked several times and I just can't 
see why it happens this way.

Ian

Re: [autofs] Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Mon, 31 Oct 2005, Ian Kent wrote:

> On Mon, 31 Oct 2005, Jeff Moyer wrote:
> 
> > ==> Regarding Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> > 
> > raven> So to resolve this we need to ignore negative and unhashed dentries
> > raven> when checking if directory dentry is empty.
> > >>
> > raven> Please test this patch and let me know how you go.
> > >> OK, I've finally got 'round to testing your patch.  It does fix the test
> > >> case I was using.  My only concern is the potential for regressions.
> > >> I'll try making sure all of my various maps still work as advertised.
> > 
> > raven> I've spotted a regression with this patch.  It doesn't stop autofs
> > raven> from appearing to function correctly. It causes mount callbacks when
> > raven> they shouldn't made. So it seems that there is a case when an autofs
> > raven> directory is, as yet unhashed, but should be used.
> > 
> > I'm not sure I follow.  What do you mean it doesn't stop autofs from
> > *appearing* to function correctly?  Do you have a reproducer that fails?
> 
> Any pseudo direct map will produce the behaviour.
> 
> It behaves as if the ghost option was not specified even when it has. 
> This is because the altered test for an empty directory is always 
> returning true even though the directory isn't empty.
> 
> I'm still trying to understand why this happens. In theory it's just not 
> the expected behaviour. I must be missing something in the directory 
> creation. I've been here before and looked several times and I just can't 
> see why it happens this way.

I couldn't work out why this patch shouldn't work so I tried to duplicate 
the problem I saw before and I can't.

I've tested the patch against autofs-4.1.3-123 (latest source rpm I could 
find) and autofs-4.1.4-8 with a couple of my patches (only on the 4.1.4 
version) that shouldn't affect it and I can't seem to break it.

I must have got my kernel modules mixed up. It probably means there's some 
backward compatibilty work to be done on the version 5 module.

Anyway, some more testing, as you suggested, would be great.

The only other question is what to do about the cacheing of mount failures 
which has never worked by the look of it. We can remove it or fix it.

Ian

Re: [autofs] Re: autofs4 looks up wrong path element when ghosting is enabled

[Jeff Moyer]

==> Regarding Re: [autofs] Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:

raven> On Mon, 31 Oct 2005, Ian Kent wrote:
>> On Mon, 31 Oct 2005, Jeff Moyer wrote:
>> 
>> > ==> Regarding Re: autofs4 looks up wrong path element when ghosting is
>> enabled; Ian Kent <raven@themaw.net> adds:
>> > 
>> > raven> So to resolve this we need to ignore negative and unhashed
>> dentries > raven> when checking if directory dentry is empty.
>> > >>
>> > raven> Please test this patch and let me know how you go.  > >> OK,
>> I've finally got 'round to testing your patch.  It does fix the test >
>> >> case I was using.  My only concern is the potential for regressions.
>> > >> I'll try making sure all of my various maps still work as
>> advertised.
>> > 
>> > raven> I've spotted a regression with this patch.  It doesn't stop
>> autofs > raven> from appearing to function correctly. It causes mount
>> callbacks when > raven> they shouldn't made. So it seems that there is a
>> case when an autofs > raven> directory is, as yet unhashed, but should
>> be used.
>> > 
>> > I'm not sure I follow.  What do you mean it doesn't stop autofs from >
>> *appearing* to function correctly?  Do you have a reproducer that fails?
>> 
>> Any pseudo direct map will produce the behaviour.
>> 
>> It behaves as if the ghost option was not specified even when it
>> has. This is because the altered test for an empty directory is always
>> returning true even though the directory isn't empty.
>> 
>> I'm still trying to understand why this happens. In theory it's just not
>> the expected behaviour. I must be missing something in the directory
>> creation. I've been here before and looked several times and I just
>> can't see why it happens this way.

raven> I couldn't work out why this patch shouldn't work so I tried to
raven> duplicate the problem I saw before and I can't.

raven> I've tested the patch against autofs-4.1.3-123 (latest source rpm I
raven> could find) and autofs-4.1.4-8 with a couple of my patches (only on
raven> the 4.1.4 version) that shouldn't affect it and I can't seem to
raven> break it.

raven> I must have got my kernel modules mixed up. It probably means
raven> there's some backward compatibilty work to be done on the version 5
raven> module.

raven> Anyway, some more testing, as you suggested, would be great.

raven> The only other question is what to do about the cacheing of mount
raven> failures which has never worked by the look of it. We can remove it
raven> or fix it.

OK, good to know that it works.  I'm really not sure why we're caching
mount failures, either.  Perhaps it was thought to be a performance
optimization?  I see no reason to keep it around, to be honest.

Note that the patch you posted has been in Fedora Core 5's devel
repository since October 20th, and no bugs have been filed.

-Jeff

Re: [autofs] Re: autofs4 looks up wrong path element when ghosting is enabled

[Ian Kent]

On Thu, 10 Nov 2005, Jeff Moyer wrote:

> ==> Regarding Re: [autofs] Re: autofs4 looks up wrong path element when ghosting is enabled; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Mon, 31 Oct 2005, Ian Kent wrote:
> >> On Mon, 31 Oct 2005, Jeff Moyer wrote:
> >> 
> >> > ==> Regarding Re: autofs4 looks up wrong path element when ghosting is
> >> enabled; Ian Kent <raven@themaw.net> adds:
> >> > 
> >> > raven> So to resolve this we need to ignore negative and unhashed
> >> dentries > raven> when checking if directory dentry is empty.
> >> > >>
> >> > raven> Please test this patch and let me know how you go.  > >> OK,
> >> I've finally got 'round to testing your patch.  It does fix the test >
> >> >> case I was using.  My only concern is the potential for regressions.
> >> > >> I'll try making sure all of my various maps still work as
> >> advertised.
> >> > 
> >> > raven> I've spotted a regression with this patch.  It doesn't stop
> >> autofs > raven> from appearing to function correctly. It causes mount
> >> callbacks when > raven> they shouldn't made. So it seems that there is a
> >> case when an autofs > raven> directory is, as yet unhashed, but should
> >> be used.
> >> > 
> >> > I'm not sure I follow.  What do you mean it doesn't stop autofs from >
> >> *appearing* to function correctly?  Do you have a reproducer that fails?
> >> 
> >> Any pseudo direct map will produce the behaviour.
> >> 
> >> It behaves as if the ghost option was not specified even when it
> >> has. This is because the altered test for an empty directory is always
> >> returning true even though the directory isn't empty.
> >> 
> >> I'm still trying to understand why this happens. In theory it's just not
> >> the expected behaviour. I must be missing something in the directory
> >> creation. I've been here before and looked several times and I just
> >> can't see why it happens this way.
> 
> raven> I couldn't work out why this patch shouldn't work so I tried to
> raven> duplicate the problem I saw before and I can't.
> 
> raven> I've tested the patch against autofs-4.1.3-123 (latest source rpm I
> raven> could find) and autofs-4.1.4-8 with a couple of my patches (only on
> raven> the 4.1.4 version) that shouldn't affect it and I can't seem to
> raven> break it.
> 
> raven> I must have got my kernel modules mixed up. It probably means
> raven> there's some backward compatibilty work to be done on the version 5
> raven> module.
> 
> raven> Anyway, some more testing, as you suggested, would be great.
> 
> raven> The only other question is what to do about the cacheing of mount
> raven> failures which has never worked by the look of it. We can remove it
> raven> or fix it.
> 
> OK, good to know that it works.  I'm really not sure why we're caching
> mount failures, either.  Perhaps it was thought to be a performance
> optimization?  I see no reason to keep it around, to be honest.

It may have been used to prevent mount storms but I'm still haveing 
trouble justifying keeping it. I'll get rid of it.

> 
> Note that the patch you posted has been in Fedora Core 5's devel
> repository since October 20th, and no bugs have been filed.

Testing has always been a very difficult and time consuming task for me.
I'm looking forward to completeing the core infrastructure for the direct 
mounts and lazy multi-mounts (ready for a hosts map) so that we can get on 
to the initiator utility. Using an unmodified conecathon suite for 
validation will be a big win.

Ian