[PATCH 0/3] mount.nfs: fix background mount issues with binary mount options

[PATCH 0/3] mount.nfs: fix background mount issues with binary mount options

[Jeff Layton]

While text-based mount options are the future for NFS, we are still
carrying the legacy binary mount option code in nfs-utils for now.
This patchset comprises some fixes for backgrounded mounts that I
cooked up while working on some older code.

Comments welcome...

Signed-off-by: Jeff Layton <jlayton@redhat.com>

[PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Jeff Layton]

Currently nfs4mount() sets the retry value to 10000 on both fg and bg
mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
properly, but there is a potential corner case. If someone explicitly
sets retry=10000 on a fg mount, then it will be reset to 2.

Fix this by having retry default to -1 for both flavors, and then reset if
needed after the mount options have been parsed.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/mount/nfs4mount.c |   10 +++++++++-
 utils/mount/nfsmount.c  |   12 ++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
index 311e5a0..af70551 100644
--- a/utils/mount/nfs4mount.c
+++ b/utils/mount/nfs4mount.c
@@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char *node, int flags,
 	nocto = 0;
 	noac = 0;
 	unshared = 0;
-	retry = 10000;		/* 10000 minutes ~ 1 week */
+	retry = -1;
 
 	/*
 	 * NFSv4 specifies that the default port should be 2049
@@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char *node, int flags,
 		}
 	}
 
+	/* if retry is still -1, then it wasn't set via an option */
+	if (retry == -1) {
+		if (bg)
+			retry = 10000;	/* 10000 mins == ~1 week */
+		else
+			retry = 2;	/* 2 min default on fg mounts */
+	}
+
 	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
 		| (intr ? NFS4_MOUNT_INTR : 0)
 		| (nocto ? NFS4_MOUNT_NOCTO : 0)
diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
index ff0ff93..27c46a7 100644
--- a/utils/mount/nfsmount.c
+++ b/utils/mount/nfsmount.c
@@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node, int flags,
 #endif
 
 	bg = 0;
-	retry = 10000;		/* 10000 minutes ~ 1 week */
+	retry = -1;
 
 	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
 	mnt_pmap->pm_prog = MOUNTPROG;
@@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node, int flags,
 		goto fail;
 	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
 		goto fail;
-	
-	if (retry == 10000 && !bg)
-		retry = 2; /* reset for fg mounts */
+
+	if (retry == -1) {
+		if (bg)
+			retry = 10000;	/* 10000 mins == ~1 week*/
+		else
+			retry = 2;	/* 2 min default on fg mounts */
+	}
 
 #ifdef NFS_MOUNT_DEBUG
 	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),
-- 
1.5.3.6

[PATCH 2/3] mount.nfs: allow backgrounded nfs4 mounts to work with binary mount opts

[Jeff Layton]

The bg option is essentially ignored with nfs4 currently. nfs4mount()
will never exit with EX_BG, so the mount will never be backgrounded.
Fix it so that when bg is specified that we error out with EX_BG as
soon as possible after the first failed mount attempt.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/mount/nfs4mount.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
index af70551..2b0fe2e 100644
--- a/utils/mount/nfs4mount.c
+++ b/utils/mount/nfs4mount.c
@@ -188,10 +188,9 @@ int nfs4mount(const char *spec, const char *node, int flags,
 	int bg, soft, intr;
 	int nocto, noac, unshared;
 	int retry;
-	int retval;
+	int retval = EX_FAIL;
 	time_t timeout, t;
 
-	retval = EX_FAIL;
 	if (strlen(spec) >= sizeof(hostdir)) {
 		nfs_error(_("%s: excessively long host:dir argument\n"),
 				progname);
@@ -443,6 +442,13 @@ int nfs4mount(const char *spec, const char *node, int flags,
 			rpc_mount_errors(hostname, 0, bg);
 			goto fail;
 		}
+
+		if (bg && !running_bg) {
+			if (retry > 0)
+				retval = EX_BG;
+			goto fail;
+		}
+
 		t = time(NULL);
 		if (t >= timeout) {
 			rpc_mount_errors(hostname, 0, bg);
-- 
1.5.3.6

[PATCH 3/3] mount.nfs: get rid of prev_bg_host cruft in nfsmount()

[Jeff Layton]

The prev_bg_host stuff made sense when NFS didn't have its own mount
handler. Now though, each mount.nfs invocation is really a one-shot
affair, and this check no longer works. It also leaked memory. Remove
it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/mount/nfsmount.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
index 27c46a7..3effcf3 100644
--- a/utils/mount/nfsmount.c
+++ b/utils/mount/nfsmount.c
@@ -494,7 +494,6 @@ int
 nfsmount(const char *spec, const char *node, int flags,
 	 char **extra_opts, int fake, int running_bg)
 {
-	static char *prev_bg_host;
 	char hostdir[1024];
 	char *hostname, *dirname, *old_opts, *mounthost = NULL;
 	char new_opts[1024], cbuf[1024];
@@ -628,18 +627,6 @@ nfsmount(const char *spec, const char *node, int flags,
 	if (flags & MS_REMOUNT)
 		goto out_ok;
 
-	/*
-	 * If the previous mount operation on the same host was
-	 * backgrounded, and the "bg" for this mount is also set,
-	 * give up immediately, to avoid the initial timeout.
-	 */
-	if (bg && !running_bg &&
-	    prev_bg_host && strcmp(hostname, prev_bg_host) == 0) {
-		if (retry > 0)
-			retval = EX_BG;
-		return retval;
-	}
-
 	/* create mount deamon client */
 
 	/*
@@ -708,7 +695,6 @@ nfsmount(const char *spec, const char *node, int flags,
 			continue;
 		}
 		if (!running_bg) {
-			prev_bg_host = xstrdup(hostname);
 			if (retry > 0)
 				retval = EX_BG;
 			goto fail;
-- 
1.5.3.6

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Steve Dickson]

Jeff Layton wrote:
> Currently nfs4mount() sets the retry value to 10000 on both fg and bg
> mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
> properly, but there is a potential corner case. If someone explicitly
> sets retry=10000 on a fg mount, then it will be reset to 2.
> 
> Fix this by having retry default to -1 for both flavors, and then reset if
> needed after the mount options have been parsed.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/mount/nfs4mount.c |   10 +++++++++-
>  utils/mount/nfsmount.c  |   12 ++++++++----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
> index 311e5a0..af70551 100644
> --- a/utils/mount/nfs4mount.c
> +++ b/utils/mount/nfs4mount.c
> @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char *node, int flags,
>  	nocto = 0;
>  	noac = 0;
>  	unshared = 0;
> -	retry = 10000;		/* 10000 minutes ~ 1 week */
> +	retry = -1;
>  
>  	/*
>  	 * NFSv4 specifies that the default port should be 2049
> @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char *node, int flags,
>  		}
>  	}
>  
> +	/* if retry is still -1, then it wasn't set via an option */
> +	if (retry == -1) {
> +		if (bg)
> +			retry = 10000;	/* 10000 mins == ~1 week */
> +		else
> +			retry = 2;	/* 2 min default on fg mounts */
> +	}
> +
>  	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
>  		| (intr ? NFS4_MOUNT_INTR : 0)
>  		| (nocto ? NFS4_MOUNT_NOCTO : 0)
> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> index ff0ff93..27c46a7 100644
> --- a/utils/mount/nfsmount.c
> +++ b/utils/mount/nfsmount.c
> @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node, int flags,
>  #endif
>  
>  	bg = 0;
> -	retry = 10000;		/* 10000 minutes ~ 1 week */
> +	retry = -1;
>  
>  	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
>  	mnt_pmap->pm_prog = MOUNTPROG;
> @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node, int flags,
>  		goto fail;
>  	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
>  		goto fail;
> -	
> -	if (retry == 10000 && !bg)
> -		retry = 2; /* reset for fg mounts */
> +
> +	if (retry == -1) {
> +		if (bg)
> +			retry = 10000;	/* 10000 mins == ~1 week*/
> +		else
> +			retry = 2;	/* 2 min default on fg mounts */
> +	}
>  
>  #ifdef NFS_MOUNT_DEBUG
>  	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),

Jeff, 

I believe all thats needed is to add the same code nfsmount() uses
reset retry into nfs4mount(). Something like:

--- a/utils/mount/nfs4mount.c
+++ b/utils/mount/nfs4mount.c
@@ -332,6 +332,9 @@ int nfs4mount(const char *spec, const char *node, int flags,
                }
        }
 
+       if (retry == 10000 && !bg)
+               retry = 2; /* reset for fg mounts */
+
        data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
                | (intr ? NFS4_MOUNT_INTR : 0)
                | (nocto ? NFS4_MOUNT_NOCTO : 0)

or missing something...


steved.

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Jeff Layton]

On Wed, 09 Apr 2008 14:00:39 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> Jeff Layton wrote:
> > Currently nfs4mount() sets the retry value to 10000 on both fg and bg
> > mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
> > properly, but there is a potential corner case. If someone explicitly
> > sets retry=10000 on a fg mount, then it will be reset to 2.
> > 
> > Fix this by having retry default to -1 for both flavors, and then reset if
> > needed after the mount options have been parsed.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  utils/mount/nfs4mount.c |   10 +++++++++-
> >  utils/mount/nfsmount.c  |   12 ++++++++----
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
> > index 311e5a0..af70551 100644
> > --- a/utils/mount/nfs4mount.c
> > +++ b/utils/mount/nfs4mount.c
> > @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char *node, int flags,
> >  	nocto = 0;
> >  	noac = 0;
> >  	unshared = 0;
> > -	retry = 10000;		/* 10000 minutes ~ 1 week */
> > +	retry = -1;
> >  
> >  	/*
> >  	 * NFSv4 specifies that the default port should be 2049
> > @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char *node, int flags,
> >  		}
> >  	}
> >  
> > +	/* if retry is still -1, then it wasn't set via an option */
> > +	if (retry == -1) {
> > +		if (bg)
> > +			retry = 10000;	/* 10000 mins == ~1 week */
> > +		else
> > +			retry = 2;	/* 2 min default on fg mounts */
> > +	}
> > +
> >  	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
> >  		| (intr ? NFS4_MOUNT_INTR : 0)
> >  		| (nocto ? NFS4_MOUNT_NOCTO : 0)
> > diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> > index ff0ff93..27c46a7 100644
> > --- a/utils/mount/nfsmount.c
> > +++ b/utils/mount/nfsmount.c
> > @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node, int flags,
> >  #endif
> >  
> >  	bg = 0;
> > -	retry = 10000;		/* 10000 minutes ~ 1 week */
> > +	retry = -1;
> >  
> >  	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
> >  	mnt_pmap->pm_prog = MOUNTPROG;
> > @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node, int flags,
> >  		goto fail;
> >  	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
> >  		goto fail;
> > -	
> > -	if (retry == 10000 && !bg)
> > -		retry = 2; /* reset for fg mounts */
> > +
> > +	if (retry == -1) {
> > +		if (bg)
> > +			retry = 10000;	/* 10000 mins == ~1 week*/
> > +		else
> > +			retry = 2;	/* 2 min default on fg mounts */
> > +	}
> >  
> >  #ifdef NFS_MOUNT_DEBUG
> >  	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),
> 
> Jeff, 
> 
> I believe all thats needed is to add the same code nfsmount() uses
> reset retry into nfs4mount(). Something like:
> 
> --- a/utils/mount/nfs4mount.c
> +++ b/utils/mount/nfs4mount.c
> @@ -332,6 +332,9 @@ int nfs4mount(const char *spec, const char *node, int flags,
>                 }
>         }
>  
> +       if (retry == 10000 && !bg)
> +               retry = 2; /* reset for fg mounts */
> +
>         data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
>                 | (intr ? NFS4_MOUNT_INTR : 0)
>                 | (nocto ? NFS4_MOUNT_NOCTO : 0)
> 
> or missing something...
> 
> 
> steved.

That shouldn't be needed. With this patch, retry defaults to -1. After
we parse the options if it's still -1, then we reset it to whatever the
default should be.

Note the deltas to nfs4mount() above...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Steve Dickson]

Jeff Layton wrote:
> On Wed, 09 Apr 2008 14:00:39 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Jeff Layton wrote:
>>> Currently nfs4mount() sets the retry value to 10000 on both fg and bg
>>> mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
>>> properly, but there is a potential corner case. If someone explicitly
>>> sets retry=10000 on a fg mount, then it will be reset to 2.
>>>
>>> Fix this by having retry default to -1 for both flavors, and then reset if
>>> needed after the mount options have been parsed.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>>  utils/mount/nfs4mount.c |   10 +++++++++-
>>>  utils/mount/nfsmount.c  |   12 ++++++++----
>>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
>>> index 311e5a0..af70551 100644
>>> --- a/utils/mount/nfs4mount.c
>>> +++ b/utils/mount/nfs4mount.c
>>> @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char *node, int flags,
>>>  	nocto = 0;
>>>  	noac = 0;
>>>  	unshared = 0;
>>> -	retry = 10000;		/* 10000 minutes ~ 1 week */
>>> +	retry = -1;
>>>  
>>>  	/*
>>>  	 * NFSv4 specifies that the default port should be 2049
>>> @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char *node, int flags,
>>>  		}
>>>  	}
>>>  
>>> +	/* if retry is still -1, then it wasn't set via an option */
>>> +	if (retry == -1) {
>>> +		if (bg)
>>> +			retry = 10000;	/* 10000 mins == ~1 week */
>>> +		else
>>> +			retry = 2;	/* 2 min default on fg mounts */
>>> +	}
>>> +
>>>  	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
>>>  		| (intr ? NFS4_MOUNT_INTR : 0)
>>>  		| (nocto ? NFS4_MOUNT_NOCTO : 0)
>>> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
>>> index ff0ff93..27c46a7 100644
>>> --- a/utils/mount/nfsmount.c
>>> +++ b/utils/mount/nfsmount.c
>>> @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node, int flags,
>>>  #endif
>>>  
>>>  	bg = 0;
>>> -	retry = 10000;		/* 10000 minutes ~ 1 week */
>>> +	retry = -1;
>>>  
>>>  	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
>>>  	mnt_pmap->pm_prog = MOUNTPROG;
>>> @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node, int flags,
>>>  		goto fail;
>>>  	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
>>>  		goto fail;
>>> -	
>>> -	if (retry == 10000 && !bg)
>>> -		retry = 2; /* reset for fg mounts */
>>> +
>>> +	if (retry == -1) {
>>> +		if (bg)
>>> +			retry = 10000;	/* 10000 mins == ~1 week*/
>>> +		else
>>> +			retry = 2;	/* 2 min default on fg mounts */
>>> +	}
>>>  
>>>  #ifdef NFS_MOUNT_DEBUG
>>>  	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),
>> Jeff, 
>>
>> I believe all thats needed is to add the same code nfsmount() uses
>> reset retry into nfs4mount(). Something like:
>>
>> --- a/utils/mount/nfs4mount.c
>> +++ b/utils/mount/nfs4mount.c
>> @@ -332,6 +332,9 @@ int nfs4mount(const char *spec, const char *node, int flags,
>>                 }
>>         }
>>  
>> +       if (retry == 10000 && !bg)
>> +               retry = 2; /* reset for fg mounts */
>> +
>>         data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
>>                 | (intr ? NFS4_MOUNT_INTR : 0)
>>                 | (nocto ? NFS4_MOUNT_NOCTO : 0)
>>
>> or missing something...
>>
>>
>> steved.
> 
> That shouldn't be needed. 
You misunderstood or I was not clear...  only these three lines
are needed to do the same thing your entire patch does... 

> With this patch, retry defaults to -1. After
> we parse the options if it's still -1, then we reset it to whatever the
> default should be.
I guess I don't see much difference if retry is initialized to 10000
or -1... its still initialized... 

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Jeff Layton]

On Wed, 09 Apr 2008 15:16:11 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> Jeff Layton wrote:
> > On Wed, 09 Apr 2008 14:00:39 -0400
> > Steve Dickson <SteveD@redhat.com> wrote:
> > 
> >> Jeff Layton wrote:
> >>> Currently nfs4mount() sets the retry value to 10000 on both fg and bg
> >>> mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
> >>> properly, but there is a potential corner case. If someone explicitly
> >>> sets retry=10000 on a fg mount, then it will be reset to 2.
> >>>
> >>> Fix this by having retry default to -1 for both flavors, and then reset if
> >>> needed after the mount options have been parsed.
> >>>
> >>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>> ---
> >>>  utils/mount/nfs4mount.c |   10 +++++++++-
> >>>  utils/mount/nfsmount.c  |   12 ++++++++----
> >>>  2 files changed, 17 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
> >>> index 311e5a0..af70551 100644
> >>> --- a/utils/mount/nfs4mount.c
> >>> +++ b/utils/mount/nfs4mount.c
> >>> @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char *node, int flags,
> >>>  	nocto = 0;
> >>>  	noac = 0;
> >>>  	unshared = 0;
> >>> -	retry = 10000;		/* 10000 minutes ~ 1 week */
> >>> +	retry = -1;
> >>>  
> >>>  	/*
> >>>  	 * NFSv4 specifies that the default port should be 2049
> >>> @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char *node, int flags,
> >>>  		}
> >>>  	}
> >>>  
> >>> +	/* if retry is still -1, then it wasn't set via an option */
> >>> +	if (retry == -1) {
> >>> +		if (bg)
> >>> +			retry = 10000;	/* 10000 mins == ~1 week */
> >>> +		else
> >>> +			retry = 2;	/* 2 min default on fg mounts */
> >>> +	}
> >>> +
> >>>  	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
> >>>  		| (intr ? NFS4_MOUNT_INTR : 0)
> >>>  		| (nocto ? NFS4_MOUNT_NOCTO : 0)
> >>> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> >>> index ff0ff93..27c46a7 100644
> >>> --- a/utils/mount/nfsmount.c
> >>> +++ b/utils/mount/nfsmount.c
> >>> @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node, int flags,
> >>>  #endif
> >>>  
> >>>  	bg = 0;
> >>> -	retry = 10000;		/* 10000 minutes ~ 1 week */
> >>> +	retry = -1;
> >>>  
> >>>  	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
> >>>  	mnt_pmap->pm_prog = MOUNTPROG;
> >>> @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node, int flags,
> >>>  		goto fail;
> >>>  	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
> >>>  		goto fail;
> >>> -	
> >>> -	if (retry == 10000 && !bg)
> >>> -		retry = 2; /* reset for fg mounts */
> >>> +
> >>> +	if (retry == -1) {
> >>> +		if (bg)
> >>> +			retry = 10000;	/* 10000 mins == ~1 week*/
> >>> +		else
> >>> +			retry = 2;	/* 2 min default on fg mounts */
> >>> +	}
> >>>  
> >>>  #ifdef NFS_MOUNT_DEBUG
> >>>  	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),
> >> Jeff, 
> >>
> >> I believe all thats needed is to add the same code nfsmount() uses
> >> reset retry into nfs4mount(). Something like:
> >>
> >> --- a/utils/mount/nfs4mount.c
> >> +++ b/utils/mount/nfs4mount.c
> >> @@ -332,6 +332,9 @@ int nfs4mount(const char *spec, const char *node, int flags,
> >>                 }
> >>         }
> >>  
> >> +       if (retry == 10000 && !bg)
> >> +               retry = 2; /* reset for fg mounts */
> >> +
> >>         data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
> >>                 | (intr ? NFS4_MOUNT_INTR : 0)
> >>                 | (nocto ? NFS4_MOUNT_NOCTO : 0)
> >>
> >> or missing something...
> >>
> >>
> >> steved.
> > 
> > That shouldn't be needed. 
> You misunderstood or I was not clear...  only these three lines
> are needed to do the same thing your entire patch does... 
> 
> > With this patch, retry defaults to -1. After
> > we parse the options if it's still -1, then we reset it to whatever the
> > default should be.
> I guess I don't see much difference if retry is initialized to 10000
> or -1... its still initialized... 

Yes. But like I mentioned in the description, there is a potential
corner case here. If someone does a foreground mount and
explicitly sets retry=10000 then it will be reset to 2. It's not very
likely, but is simple enough to prevent...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Steve Dickson]

Jeff Layton wrote:
> 
> Yes. But like I mentioned in the description, there is a potential
> corner case here. If someone does a foreground mount and
> explicitly sets retry=10000 then it will be reset to 2. It's not very
> likely, but is simple enough to prevent...
hmm.. I guess... but programing for every single brain dead
corner case can really make the code more complicated than
it need to be, which I think is the case here... I think I would
rather just added the three lines and live that corner case...

steved.

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Jeff Layton]

On Wed, 09 Apr 2008 16:11:29 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> Jeff Layton wrote:
> > 
> > Yes. But like I mentioned in the description, there is a potential
> > corner case here. If someone does a foreground mount and
> > explicitly sets retry=10000 then it will be reset to 2. It's not very
> > likely, but is simple enough to prevent...
> hmm.. I guess... but programing for every single brain dead
> corner case can really make the code more complicated than
> it need to be, which I think is the case here... I think I would
> rather just added the three lines and live that corner case...
> 
> steved.
> 

I don't feel strongly about it either way. If you think the original
method is better then I'm OK with it.

Cheers,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Peter Staubach]

Steve Dickson wrote:
> Jeff Layton wrote:
>   
>> Yes. But like I mentioned in the description, there is a potential
>> corner case here. If someone does a foreground mount and
>> explicitly sets retry=10000 then it will be reset to 2. It's not very
>> likely, but is simple enough to prevent...
>>     
> hmm.. I guess... but programing for every single brain dead
> corner case can really make the code more complicated than
> it need to be, which I think is the case here... I think I would
> rather just added the three lines and live that corner case...

Customers tend to be remarkably good at finding corner cases and
then wondering why they don't work as advertised...

       ps

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[J. Bruce Fields]

On Wed, Apr 09, 2008 at 05:29:47PM -0400, Peter Staubach wrote:
> Steve Dickson wrote:
>> Jeff Layton wrote:
>>   
>>> Yes. But like I mentioned in the description, there is a potential
>>> corner case here. If someone does a foreground mount and
>>> explicitly sets retry=10000 then it will be reset to 2. It's not very
>>> likely, but is simple enough to prevent...
>>>     
>> hmm.. I guess... but programing for every single brain dead
>> corner case can really make the code more complicated than
>> it need to be, which I think is the case here... I think I would
>> rather just added the three lines and live that corner case...
>
> Customers tend to be remarkably good at finding corner cases and
> then wondering why they don't work as advertised...

And 10000 is a nice round number.  Given more than a few users,
*somebody* is going to try it eventually, and get extremely confused.

--b.

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Steve Dickson]

Peter Staubach wrote:
> Steve Dickson wrote:
>> Jeff Layton wrote:
>>  
>>> Yes. But like I mentioned in the description, there is a potential
>>> corner case here. If someone does a foreground mount and
>>> explicitly sets retry=10000 then it will be reset to 2. It's not very
>>> likely, but is simple enough to prevent...
>>>     
>> hmm.. I guess... but programing for every single brain dead
>> corner case can really make the code more complicated than
>> it need to be, which I think is the case here... I think I would
>> rather just added the three lines and live that corner case...
> 
> Customers tend to be remarkably good at finding corner cases and
> then wondering why they don't work as advertised...
True... but... this feature has been busted for a while now
and nobody even noticed... plus why would someone want to
wait a week for a mount to finish... it seems a bit unreasonable

My point was programing for every possible corner case will
make the code unnecessarily complicated in a very quick way.
Something I'm just trying avoid.. 

steved.

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Steve Dickson]

J. Bruce Fields wrote:
> On Wed, Apr 09, 2008 at 05:29:47PM -0400, Peter Staubach wrote:
>> Steve Dickson wrote:
>>> Jeff Layton wrote:
>>>   
>>>> Yes. But like I mentioned in the description, there is a potential
>>>> corner case here. If someone does a foreground mount and
>>>> explicitly sets retry=10000 then it will be reset to 2. It's not very
>>>> likely, but is simple enough to prevent...
>>>>     
>>> hmm.. I guess... but programing for every single brain dead
>>> corner case can really make the code more complicated than
>>> it need to be, which I think is the case here... I think I would
>>> rather just added the three lines and live that corner case...
>> Customers tend to be remarkably good at finding corner cases and
>> then wondering why they don't work as advertised...
> 
> And 10000 is a nice round number.  Given more than a few users,
> *somebody* is going to try it eventually, and get extremely confused.
Ok... I'll leave the patch as is...

steved.

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Chuck Lever]

Hi Jeff-

Is this also an issue for text-based mounts?

On Mar 29, 2008, at 4:30 PM, Jeff Layton wrote:
> Currently nfs4mount() sets the retry value to 10000 on both fg and bg
> mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
> properly, but there is a potential corner case. If someone explicitly
> sets retry=10000 on a fg mount, then it will be reset to 2.
>
> Fix this by having retry default to -1 for both flavors, and then  
> reset if
> needed after the mount options have been parsed.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/mount/nfs4mount.c |   10 +++++++++-
>  utils/mount/nfsmount.c  |   12 ++++++++----
>  2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
> index 311e5a0..af70551 100644
> --- a/utils/mount/nfs4mount.c
> +++ b/utils/mount/nfs4mount.c
> @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char  
> *node, int flags,
>  	nocto = 0;
>  	noac = 0;
>  	unshared = 0;
> -	retry = 10000;		/* 10000 minutes ~ 1 week */
> +	retry = -1;
>
>  	/*
>  	 * NFSv4 specifies that the default port should be 2049
> @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char  
> *node, int flags,
>  		}
>  	}
>
> +	/* if retry is still -1, then it wasn't set via an option */
> +	if (retry == -1) {
> +		if (bg)
> +			retry = 10000;	/* 10000 mins == ~1 week */
> +		else
> +			retry = 2;	/* 2 min default on fg mounts */
> +	}
> +
>  	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
>  		| (intr ? NFS4_MOUNT_INTR : 0)
>  		| (nocto ? NFS4_MOUNT_NOCTO : 0)
> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> index ff0ff93..27c46a7 100644
> --- a/utils/mount/nfsmount.c
> +++ b/utils/mount/nfsmount.c
> @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node,  
> int flags,
>  #endif
>
>  	bg = 0;
> -	retry = 10000;		/* 10000 minutes ~ 1 week */
> +	retry = -1;
>
>  	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
>  	mnt_pmap->pm_prog = MOUNTPROG;
> @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node,  
> int flags,
>  		goto fail;
>  	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
>  		goto fail;
> -	
> -	if (retry == 10000 && !bg)
> -		retry = 2; /* reset for fg mounts */
> +
> +	if (retry == -1) {
> +		if (bg)
> +			retry = 10000;	/* 10000 mins == ~1 week*/
> +		else
> +			retry = 2;	/* 2 min default on fg mounts */
> +	}
>
>  #ifdef NFS_MOUNT_DEBUG
>  	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),
> -- 
> 1.5.3.6
>
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options

[Jeff Layton]

On Thu, 10 Apr 2008 10:43:46 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Hi Jeff-
> 
> Is this also an issue for text-based mounts?
> 

It looks like text-based mounts handle this correctly. They initialize
a "timeout" variable to the default and only clobber it if a retry
option is specified.


> On Mar 29, 2008, at 4:30 PM, Jeff Layton wrote:
> > Currently nfs4mount() sets the retry value to 10000 on both fg and bg
> > mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it
> > properly, but there is a potential corner case. If someone explicitly
> > sets retry=10000 on a fg mount, then it will be reset to 2.
> >
> > Fix this by having retry default to -1 for both flavors, and then  
> > reset if
> > needed after the mount options have been parsed.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  utils/mount/nfs4mount.c |   10 +++++++++-
> >  utils/mount/nfsmount.c  |   12 ++++++++----
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c
> > index 311e5a0..af70551 100644
> > --- a/utils/mount/nfs4mount.c
> > +++ b/utils/mount/nfs4mount.c
> > @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char  
> > *node, int flags,
> >  	nocto = 0;
> >  	noac = 0;
> >  	unshared = 0;
> > -	retry = 10000;		/* 10000 minutes ~ 1 week */
> > +	retry = -1;
> >
> >  	/*
> >  	 * NFSv4 specifies that the default port should be 2049
> > @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char  
> > *node, int flags,
> >  		}
> >  	}
> >
> > +	/* if retry is still -1, then it wasn't set via an option */
> > +	if (retry == -1) {
> > +		if (bg)
> > +			retry = 10000;	/* 10000 mins == ~1 week */
> > +		else
> > +			retry = 2;	/* 2 min default on fg mounts */
> > +	}
> > +
> >  	data.flags = (soft ? NFS4_MOUNT_SOFT : 0)
> >  		| (intr ? NFS4_MOUNT_INTR : 0)
> >  		| (nocto ? NFS4_MOUNT_NOCTO : 0)
> > diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> > index ff0ff93..27c46a7 100644
> > --- a/utils/mount/nfsmount.c
> > +++ b/utils/mount/nfsmount.c
> > @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node,  
> > int flags,
> >  #endif
> >
> >  	bg = 0;
> > -	retry = 10000;		/* 10000 minutes ~ 1 week */
> > +	retry = -1;
> >
> >  	memset(mnt_pmap, 0, sizeof(*mnt_pmap));
> >  	mnt_pmap->pm_prog = MOUNTPROG;
> > @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node,  
> > int flags,
> >  		goto fail;
> >  	if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap))
> >  		goto fail;
> > -	
> > -	if (retry == 10000 && !bg)
> > -		retry = 2; /* reset for fg mounts */
> > +
> > +	if (retry == -1) {
> > +		if (bg)
> > +			retry = 10000;	/* 10000 mins == ~1 week*/
> > +		else
> > +			retry = 2;	/* 2 min default on fg mounts */
> > +	}
> >
> >  #ifdef NFS_MOUNT_DEBUG
> >  	printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"),
> > -- 
> > 1.5.3.6
> >
> > _______________________________________________
> > NFSv4 mailing list
> > NFSv4@linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>