From: Steve Dickson <SteveD@redhat.com>
Subject: Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary
	mount options
Date: Wed, 09 Apr 2008 15:16:11 -0400
Message-ID: <47FD15FB.6090208@RedHat.com>
References: <1206822608-20154-1-git-send-email-jlayton@redhat.com>	<1206822608-20154-2-git-send-email-jlayton@redhat.com>	<47FD0447.9030704@RedHat.com>
	<20080409141323.0c594185@barsoom.rdu.redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Cc: linux-nfs@vger.kernel.org, nfsv4@linux-nfs.org
To: Jeff Layton <jlayton@redhat.com>
In-Reply-To: <20080409141323.0c594185@barsoom.rdu.redhat.com>
Sender: nfsv4-bounces@linux-nfs.org
Errors-To: nfsv4-bounces@linux-nfs.org


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...