Mounts not expiring in setups with nested submounts

Mounts not expiring in setups with nested submounts

[Leonardo Chiquitto]

Hi,

In some configurations that use nested submounts, a busy volume
can prevent AutoFS from expiring other mounts. This was reported
by a customer and I'm able to reproduce it with a "minimal" config:

In your NFS server, export the following structure of directories:
/nfs/vol/a/mount1
/nfs/vol/a/mount100
/nfs/vol/b/mount1
/nfs/vol/b/mount100
/nfs/vol/b/mount200

AutoFS configuration:
BROWSE_MODE="yes"
TIMEOUT=60

auto.master:
/vol /etc/auto.vol
--
auto.vol:
a -fstype=autofs file:/etc/auto.vol.a
b -fstype=autofs file:/etc/auto.vol.b
--
auto.vol.a:
disk1 -fstype=autofs file:/etc/auto.vol.a.disk1
mount1 server:/nfs/vol/a/mount1
--
auto.vol.b:
disk1 -fstype=autofs file:/etc/auto.vol.b.disk1
disk2 -fstype=autofs file:/etc/auto.vol.b.disk2
mount1 server:/nfs/vol/b/mount1
--
auto.vol.a.disk1:
mount100 server:/nfs/vol/a/mount100
--
auto.vol.b.disk1:
mount100 server:/nfs/vol/b/mount100
--
auto.vol.b.disk2:
mount200 server:/nfs/vol/b/mount200
--

Steps to reproduce the problem:

1. Trigger mount of /vol/b/disk2/mount200 first and keep it busy
2. Mount all the other exported volumes

mount(8) output must be something like this (I think the order matters):
server:/nfs/vol/b/mount200 on /vol/b/disk2/mount200 type nfs
(rw,addr=10.121.8.27)
server:/nfs/vol/b/mount1 on /vol/b/mount1 type nfs (rw,addr=10.121.8.27)
server:/nfs/vol/b/mount100 on /vol/b/disk1/mount100 type nfs
(rw,addr=10.121.8.27)
server:/nfs/vol/a/mount1 on /vol/a/mount1 type nfs (rw,addr=10.121.8.27)
server:/nfs/vol/a/mount100 on /vol/a/disk1/mount100 type nfs
(rw,addr=10.121.8.27)

After the timeout, /vol/b/disk1/mount100 will be correctly unmounted,
but the other mounts will never expire (not even with SIGUSR1).
In this example only 4 mounts are blocked, but in some occasions
we've seen dozens of non-expiring mounts because of this bug.

I debugged the problem and discovered that it happens because
the recursion implemented in master_notify_submount() stops
on purpose after the first level of nested submounts. This was
implemented in autofs-5.0.4-expire-specific-submount-only.patch.

The patch below fixes the problem, but re-introduces recursion to
deeper levels. Tests didn't reveal any regression so far.

Ian: do you think the "occasional deadlocks" mentioned in the
expire-specific-submount-only patch might have been addressed
by other changes (so we can use the patch below)? Or do you
see another way to fix the problem?

diff --git a/lib/master.c b/lib/master.c
index a0e62f2..99b1092 100644
--- a/lib/master.c
+++ b/lib/master.c
@@ -906,8 +906,10 @@ int master_notify_submount(struct autofs_point
*ap, const char *path, enum state
 		p = p->prev;

 		if (!master_submount_list_empty(this)) {
-			mounts_mutex_unlock(ap);
-			return master_notify_submount(this, path, state);
+			if (!master_notify_submount(this, path, state)) {
+				ret = 0;
+				break;
+			}
 		}

 		/* path not the same */

Thanks,
Leonardo

Re: Mounts not expiring in setups with nested submounts

[Ian Kent]

On Fri, 2013-03-29 at 17:33 -0300, Leonardo Chiquitto wrote:
> Hi,
> 
> In some configurations that use nested submounts, a busy volume
> can prevent AutoFS from expiring other mounts. This was reported
> by a customer and I'm able to reproduce it with a "minimal" config:
> 
> In your NFS server, export the following structure of directories:
> /nfs/vol/a/mount1
> /nfs/vol/a/mount100
> /nfs/vol/b/mount1
> /nfs/vol/b/mount100
> /nfs/vol/b/mount200
> 
> AutoFS configuration:
> BROWSE_MODE="yes"
> TIMEOUT=60
> 
> auto.master:
> /vol /etc/auto.vol
> --
> auto.vol:
> a -fstype=autofs file:/etc/auto.vol.a
> b -fstype=autofs file:/etc/auto.vol.b
> --
> auto.vol.a:
> disk1 -fstype=autofs file:/etc/auto.vol.a.disk1
> mount1 server:/nfs/vol/a/mount1
> --
> auto.vol.b:
> disk1 -fstype=autofs file:/etc/auto.vol.b.disk1
> disk2 -fstype=autofs file:/etc/auto.vol.b.disk2
> mount1 server:/nfs/vol/b/mount1
> --
> auto.vol.a.disk1:
> mount100 server:/nfs/vol/a/mount100
> --
> auto.vol.b.disk1:
> mount100 server:/nfs/vol/b/mount100
> --
> auto.vol.b.disk2:
> mount200 server:/nfs/vol/b/mount200
> --
> 
> Steps to reproduce the problem:
> 
> 1. Trigger mount of /vol/b/disk2/mount200 first and keep it busy
> 2. Mount all the other exported volumes
> 
> mount(8) output must be something like this (I think the order matters):
> server:/nfs/vol/b/mount200 on /vol/b/disk2/mount200 type nfs
> (rw,addr=10.121.8.27)
> server:/nfs/vol/b/mount1 on /vol/b/mount1 type nfs (rw,addr=10.121.8.27)
> server:/nfs/vol/b/mount100 on /vol/b/disk1/mount100 type nfs
> (rw,addr=10.121.8.27)
> server:/nfs/vol/a/mount1 on /vol/a/mount1 type nfs (rw,addr=10.121.8.27)
> server:/nfs/vol/a/mount100 on /vol/a/disk1/mount100 type nfs
> (rw,addr=10.121.8.27)
> 
> After the timeout, /vol/b/disk1/mount100 will be correctly unmounted,
> but the other mounts will never expire (not even with SIGUSR1).
> In this example only 4 mounts are blocked, but in some occasions
> we've seen dozens of non-expiring mounts because of this bug.
> 
> I debugged the problem and discovered that it happens because
> the recursion implemented in master_notify_submount() stops
> on purpose after the first level of nested submounts. This was
> implemented in autofs-5.0.4-expire-specific-submount-only.patch.
> 
> The patch below fixes the problem, but re-introduces recursion to
> deeper levels. Tests didn't reveal any regression so far.
> 
> Ian: do you think the "occasional deadlocks" mentioned in the
> expire-specific-submount-only patch might have been addressed
> by other changes (so we can use the patch below)? Or do you
> see another way to fix the problem?

Not sure, probably not, but master_notify_submount() should be called
for each submount from expire_proc_(indirect|direct)(). The recursive
step shouldn't actually be done in master_notify_submounts().

I don't see why that isn't being done.
I'll need to spend some time on it when I get a chance.

Re: Mounts not expiring in setups with nested submounts

[Ian Kent]

On Fri, 2013-03-29 at 17:33 -0300, Leonardo Chiquitto wrote:
> 
> Ian: do you think the "occasional deadlocks" mentioned in the
> expire-specific-submount-only patch might have been addressed
> by other changes (so we can use the patch below)? Or do you
> see another way to fix the problem?

Think so.

> 
> diff --git a/lib/master.c b/lib/master.c
> index a0e62f2..99b1092 100644
> --- a/lib/master.c
> +++ b/lib/master.c
> @@ -906,8 +906,10 @@ int master_notify_submount(struct autofs_point
> *ap, const char *path, enum state
>  		p = p->prev;
> 
>  		if (!master_submount_list_empty(this)) {
> -			mounts_mutex_unlock(ap);
> -			return master_notify_submount(this, path, state);
> +			if (!master_notify_submount(this, path, state)) {
> +				ret = 0;
> +				break;
> +			}
>  		}
> 
>  		/* path not the same */

I think the mounts mutex needs to be re-taken and then check if the
submount has gone away.

The list curor for the next one to check (set to p->prev) should always
be ok because there's only one expire process for a given mount but the
one we are checking (this) could go away, in which case we're done.

This patch seems to work.

autofs-5.0.7 - fix submount tree not all expiring

From: Ian Kent <ikent@redhat.com>

Due to the change in the expire-specific-submount-only patch, sub-mounts
within an indirect mount that follow a submount (in the check order) won't
be expired if that submount is busy.
---
 lib/master.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/master.c b/lib/master.c
index a0e62f2..64dbcb1 100644
--- a/lib/master.c
+++ b/lib/master.c
@@ -905,15 +905,24 @@ int master_notify_submount(struct autofs_point *ap, const char *path, enum state
 		this = list_entry(p, struct autofs_point, mounts);
 		p = p->prev;
 
-		if (!master_submount_list_empty(this)) {
-			mounts_mutex_unlock(ap);
-			return master_notify_submount(this, path, state);
-		}
-
 		/* path not the same */
 		if (strcmp(this->path, path))
 			continue;
 
+		if (!master_submount_list_empty(this)) {
+			char *this_path = strdup(this->path);
+			if (this_path) {
+				mounts_mutex_unlock(ap);
+				master_notify_submount(this, path, state);
+				mounts_mutex_lock(ap);
+				if (!__master_find_submount(ap, this_path)) {
+					free(this_path);
+					continue;
+				}
+				free(this_path);
+			}
+		}
+
 		/* Now we have found the submount we want to expire */
 
 		st_mutex_lock();
@@ -959,10 +968,7 @@ int master_notify_submount(struct autofs_point *ap, const char *path, enum state
 			st_mutex_lock();
 		}
 		st_mutex_unlock();
-		mounts_mutex_unlock(ap);
-
-		return ret;
-
+		break;
 	}
 
 	mounts_mutex_unlock(ap);

Re: Mounts not expiring in setups with nested submounts

[Leonardo Chiquitto]

On Mon, Apr 8, 2013 at 4:48 AM, Ian Kent <raven@themaw.net> wrote:
> On Fri, 2013-03-29 at 17:33 -0300, Leonardo Chiquitto wrote:
>>
>> Ian: do you think the "occasional deadlocks" mentioned in the
>> expire-specific-submount-only patch might have been addressed
>> by other changes (so we can use the patch below)? Or do you
>> see another way to fix the problem?
>
> Think so.
>
>>
>> diff --git a/lib/master.c b/lib/master.c
>> index a0e62f2..99b1092 100644
>> --- a/lib/master.c
>> +++ b/lib/master.c
>> @@ -906,8 +906,10 @@ int master_notify_submount(struct autofs_point
>> *ap, const char *path, enum state
>>               p = p->prev;
>>
>>               if (!master_submount_list_empty(this)) {
>> -                     mounts_mutex_unlock(ap);
>> -                     return master_notify_submount(this, path, state);
>> +                     if (!master_notify_submount(this, path, state)) {
>> +                             ret = 0;
>> +                             break;
>> +                     }
>>               }
>>
>>               /* path not the same */
>
> I think the mounts mutex needs to be re-taken and then check if the
> submount has gone away.
>
> The list curor for the next one to check (set to p->prev) should always
> be ok because there's only one expire process for a given mount but the
> one we are checking (this) could go away, in which case we're done.
>
> This patch seems to work.
>
> autofs-5.0.7 - fix submount tree not all expiring
>
> From: Ian Kent <ikent@redhat.com>
>
> Due to the change in the expire-specific-submount-only patch, sub-mounts
> within an indirect mount that follow a submount (in the check order) won't
> be expired if that submount is busy.
> ---
>  lib/master.c |   24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/lib/master.c b/lib/master.c
> index a0e62f2..64dbcb1 100644
> --- a/lib/master.c
> +++ b/lib/master.c
> @@ -905,15 +905,24 @@ int master_notify_submount(struct autofs_point *ap, const char *path, enum state
>                 this = list_entry(p, struct autofs_point, mounts);
>                 p = p->prev;
>
> -               if (!master_submount_list_empty(this)) {
> -                       mounts_mutex_unlock(ap);
> -                       return master_notify_submount(this, path, state);
> -               }
> -
>                 /* path not the same */
>                 if (strcmp(this->path, path))
>                         continue;
>
> +               if (!master_submount_list_empty(this)) {
> +                       char *this_path = strdup(this->path);
> +                       if (this_path) {
> +                               mounts_mutex_unlock(ap);
> +                               master_notify_submount(this, path, state);
> +                               mounts_mutex_lock(ap);
> +                               if (!__master_find_submount(ap, this_path)) {
> +                                       free(this_path);
> +                                       continue;
> +                               }
> +                               free(this_path);
> +                       }
> +               }
> +
>                 /* Now we have found the submount we want to expire */
>
>                 st_mutex_lock();
> @@ -959,10 +968,7 @@ int master_notify_submount(struct autofs_point *ap, const char *path, enum state
>                         st_mutex_lock();
>                 }
>                 st_mutex_unlock();
> -               mounts_mutex_unlock(ap);
> -
> -               return ret;
> -
> +               break;
>         }
>
>         mounts_mutex_unlock(ap);

Hi Ian,

I tested the patch and verified that it resolves the problem.

Thanks,
Leonardo

Re: Mounts not expiring in setups with nested submounts

[Ian Kent]

On Mon, 2013-04-08 at 22:24 -0300, Leonardo Chiquitto wrote:
> On Mon, Apr 8, 2013 at 4:48 AM, Ian Kent <raven@themaw.net> wrote:
> > On Fri, 2013-03-29 at 17:33 -0300, Leonardo Chiquitto wrote:
> >>
> >> Ian: do you think the "occasional deadlocks" mentioned in the
> >> expire-specific-submount-only patch might have been addressed
> >> by other changes (so we can use the patch below)? Or do you
> >> see another way to fix the problem?
> >
> > Think so.
> >
> >>
> >> diff --git a/lib/master.c b/lib/master.c
> >> index a0e62f2..99b1092 100644
> >> --- a/lib/master.c
> >> +++ b/lib/master.c
> >> @@ -906,8 +906,10 @@ int master_notify_submount(struct autofs_point
> >> *ap, const char *path, enum state
> >>               p = p->prev;
> >>
> >>               if (!master_submount_list_empty(this)) {
> >> -                     mounts_mutex_unlock(ap);
> >> -                     return master_notify_submount(this, path, state);
> >> +                     if (!master_notify_submount(this, path, state)) {
> >> +                             ret = 0;
> >> +                             break;
> >> +                     }
> >>               }
> >>
> >>               /* path not the same */
> >
> > I think the mounts mutex needs to be re-taken and then check if the
> > submount has gone away.
> >
> > The list curor for the next one to check (set to p->prev) should always
> > be ok because there's only one expire process for a given mount but the
> > one we are checking (this) could go away, in which case we're done.
> >
> > This patch seems to work.
> >
> > autofs-5.0.7 - fix submount tree not all expiring
> >
> > From: Ian Kent <ikent@redhat.com>
> >
> > Due to the change in the expire-specific-submount-only patch, sub-mounts
> > within an indirect mount that follow a submount (in the check order) won't
> > be expired if that submount is busy.
> > ---
> >  lib/master.c |   24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/master.c b/lib/master.c
> > index a0e62f2..64dbcb1 100644
> > --- a/lib/master.c
> > +++ b/lib/master.c
> > @@ -905,15 +905,24 @@ int master_notify_submount(struct autofs_point *ap, const char *path, enum state
> >                 this = list_entry(p, struct autofs_point, mounts);
> >                 p = p->prev;
> >
> > -               if (!master_submount_list_empty(this)) {
> > -                       mounts_mutex_unlock(ap);
> > -                       return master_notify_submount(this, path, state);
> > -               }
> > -
> >                 /* path not the same */
> >                 if (strcmp(this->path, path))
> >                         continue;
> >
> > +               if (!master_submount_list_empty(this)) {
> > +                       char *this_path = strdup(this->path);
> > +                       if (this_path) {
> > +                               mounts_mutex_unlock(ap);
> > +                               master_notify_submount(this, path, state);
> > +                               mounts_mutex_lock(ap);
> > +                               if (!__master_find_submount(ap, this_path)) {
> > +                                       free(this_path);
> > +                                       continue;
> > +                               }
> > +                               free(this_path);
> > +                       }
> > +               }
> > +
> >                 /* Now we have found the submount we want to expire */
> >
> >                 st_mutex_lock();
> > @@ -959,10 +968,7 @@ int master_notify_submount(struct autofs_point *ap, const char *path, enum state
> >                         st_mutex_lock();
> >                 }
> >                 st_mutex_unlock();
> > -               mounts_mutex_unlock(ap);
> > -
> > -               return ret;
> > -
> > +               break;
> >         }
> >
> >         mounts_mutex_unlock(ap);
> 
> Hi Ian,
> 
> I tested the patch and verified that it resolves the problem.

Thanks, I'll commit it next time I push an update to the repo.

> 
> Thanks,
> Leonardo