[PATCH] NFS: fix minor issues with automount expiry.

[PATCH] NFS: fix minor issues with automount expiry.

[NeilBrown]

1/ If automount expiry timeout is set to zero, new mounts are not added
   to the list.  If the timeout is then changed, those previously
   mounts will still not be timed out.  This is probably not what
   would be expected.  Simply refusing to start the timer
   is sufficient to prevent timeout.

2/ the MODULE_PARM_DESC for nfs_mountpoint_expiry_timeout is missing
   a space between to two sentences.  This is hidden by the fact that
   the string is broken onto to line - against current policy.
   So join onto a single (long) line, and add the space.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/namespace.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index bc0c698f3350..be5e77a80811 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -196,10 +196,10 @@ struct vfsmount *nfs_d_automount(struct path *path)
 		goto out_fc;
 
 	mntget(mnt); /* prevent immediate expiration */
+	mnt_set_expiry(mnt, &nfs_automount_list);
 	if (timeout <= 0)
 		goto out_fc;
 
-	mnt_set_expiry(mnt, &nfs_automount_list);
 	schedule_delayed_work(&nfs_automount_task, timeout);
 
 out_fc:
@@ -366,5 +366,4 @@ static const struct kernel_param_ops param_ops_nfs_timeout = {
 
 module_param(nfs_mountpoint_expiry_timeout, nfs_timeout, 0644);
 MODULE_PARM_DESC(nfs_mountpoint_expiry_timeout,
-		"Set the NFS automounted mountpoint timeout value (seconds)."
-		"Values <= 0 turn expiration off.");
+		"Set the NFS automounted mountpoint timeout value (seconds). Values <= 0 turn expiration off.");
-- 
2.32.0

Re: [PATCH] NFS: fix minor issues with automount expiry.

[Trond Myklebust]

On Wed, 2021-07-28 at 09:15 +1000, NeilBrown wrote:
> 
> 1/ If automount expiry timeout is set to zero, new mounts are not
> added
>    to the list.  If the timeout is then changed, those previously
>    mounts will still not be timed out.  This is probably not what
>    would be expected.  Simply refusing to start the timer
>    is sufficient to prevent timeout.
> 
> 2/ the MODULE_PARM_DESC for nfs_mountpoint_expiry_timeout is missing
>    a space between to two sentences.  This is hidden by the fact that
>    the string is broken onto to line - against current policy.
>    So join onto a single (long) line, and add the space.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfs/namespace.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> index bc0c698f3350..be5e77a80811 100644
> --- a/fs/nfs/namespace.c
> +++ b/fs/nfs/namespace.c
> @@ -196,10 +196,10 @@ struct vfsmount *nfs_d_automount(struct path
> *path)
>                 goto out_fc;
>  
>         mntget(mnt); /* prevent immediate expiration */
> +       mnt_set_expiry(mnt, &nfs_automount_list);
>         if (timeout <= 0)
>                 goto out_fc;
>  
> -       mnt_set_expiry(mnt, &nfs_automount_list);
>         schedule_delayed_work(&nfs_automount_task, timeout);

NACK.

The existing code is very deliberate about _not_ adding the mountpoint
to the expiration list if the timeout is negative or zero. That's the
whole point. Changing the timeout value after the fact is not supposed
to affect the state of those mountpoints.

>  
>  out_fc:
> @@ -366,5 +366,4 @@ static const struct kernel_param_ops
> param_ops_nfs_timeout = {
>  
>  module_param(nfs_mountpoint_expiry_timeout, nfs_timeout, 0644);
>  MODULE_PARM_DESC(nfs_mountpoint_expiry_timeout,
> -               "Set the NFS automounted mountpoint timeout value
> (seconds)."
> -               "Values <= 0 turn expiration off.");
> +               "Set the NFS automounted mountpoint timeout value
> (seconds). Values <= 0 turn expiration off.");

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] NFS: fix minor issues with automount expiry.

[NeilBrown]

On Mon, 04 Oct 2021, Trond Myklebust wrote:
> On Wed, 2021-07-28 at 09:15 +1000, NeilBrown wrote:
> > 
> > 1/ If automount expiry timeout is set to zero, new mounts are not
> > added
> >    to the list.  If the timeout is then changed, those previously
> >    mounts will still not be timed out.  This is probably not what
> >    would be expected.  Simply refusing to start the timer
> >    is sufficient to prevent timeout.
> > 
> > 2/ the MODULE_PARM_DESC for nfs_mountpoint_expiry_timeout is missing
> >    a space between to two sentences.  This is hidden by the fact that
> >    the string is broken onto to line - against current policy.
> >    So join onto a single (long) line, and add the space.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfs/namespace.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > index bc0c698f3350..be5e77a80811 100644
> > --- a/fs/nfs/namespace.c
> > +++ b/fs/nfs/namespace.c
> > @@ -196,10 +196,10 @@ struct vfsmount *nfs_d_automount(struct path
> > *path)
> >                 goto out_fc;
> >  
> >         mntget(mnt); /* prevent immediate expiration */
> > +       mnt_set_expiry(mnt, &nfs_automount_list);
> >         if (timeout <= 0)
> >                 goto out_fc;
> >  
> > -       mnt_set_expiry(mnt, &nfs_automount_list);
> >         schedule_delayed_work(&nfs_automount_task, timeout);
> 
> NACK.
> 
> The existing code is very deliberate about _not_ adding the mountpoint
> to the expiration list if the timeout is negative or zero. That's the
> whole point. Changing the timeout value after the fact is not supposed
> to affect the state of those mountpoints.
> 

Thanks for the reply.
It just seemed odd that setting <= 0 would turn expiry off for all
mounts, but setting in > 0 would only turn it back on for some mounts.
Those that were auto-mounted before it was set <= 0.
It's not really important though.

Thanks,
NeilBrown