[PATCH] NFS: Revert default r/wsize behavior

[PATCH] NFS: Revert default r/wsize behavior

[Chuck Lever]

When the "rsize=" or "wsize=" mount options are not specified,
text-based mounts have slightly different behavior than legacy binary
mounts.  Text-based mounts use the smaller of the server's maximum
and the client's maximum, but binary mounts use the smaller of the
server's _preferred_ size and the client's maximum.

This difference is actually pretty subtle.  Most servers advertise
the same value as their largest and their preferred transfer size, so
the end result is the same in most cases.

The reason for this difference is that for text-based mounts, if
r/wsize are not specified, they are set to the largest value supported
by the client.  For binary mounts, the values are set to zero if these
options are not specified.

nfs_server_set_fsinfo() can negotiate the defaults correctly in any
case.  There's no need to specify any particular value as default in
the text-based option parsing logic.

Thanks to James Pearson <james-p-5Ol4pYTxKWu0ML75eksnrtBPR1lH4CV8@public.gmane.org> for reporting and
diagnosing the problem.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

James-

Is this what you want?

 fs/nfs/super.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index bde444b..cf0d754 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1635,8 +1635,6 @@ static int nfs_validate_mount_data(void *options,
 		goto out_no_data;
 
 	args->flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP);
-	args->rsize		= NFS_MAX_FILE_IO_SIZE;
-	args->wsize		= NFS_MAX_FILE_IO_SIZE;
 	args->acregmin		= NFS_DEF_ACREGMIN;
 	args->acregmax		= NFS_DEF_ACREGMAX;
 	args->acdirmin		= NFS_DEF_ACDIRMIN;
@@ -2351,8 +2349,6 @@ static int nfs4_validate_mount_data(void *options,
 	if (data == NULL)
 		goto out_no_data;
 
-	args->rsize		= NFS_MAX_FILE_IO_SIZE;
-	args->wsize		= NFS_MAX_FILE_IO_SIZE;
 	args->acregmin		= NFS_DEF_ACREGMIN;
 	args->acregmax		= NFS_DEF_ACREGMAX;
 	args->acdirmin		= NFS_DEF_ACDIRMIN;

Re: [PATCH] NFS: Revert default r/wsize behavior

[Trond Myklebust]

On Thu, 2009-09-10 at 17:05 -0400, Chuck Lever wrote:
> When the "rsize=" or "wsize=" mount options are not specified,
> text-based mounts have slightly different behavior than legacy binary
> mounts.  Text-based mounts use the smaller of the server's maximum
> and the client's maximum, but binary mounts use the smaller of the
> server's _preferred_ size and the client's maximum.
> 
> This difference is actually pretty subtle.  Most servers advertise
> the same value as their largest and their preferred transfer size, so
> the end result is the same in most cases.
> 
> The reason for this difference is that for text-based mounts, if
> r/wsize are not specified, they are set to the largest value supported
> by the client.  For binary mounts, the values are set to zero if these
> options are not specified.
> 
> nfs_server_set_fsinfo() can negotiate the defaults correctly in any
> case.  There's no need to specify any particular value as default in
> the text-based option parsing logic.
> 
> Thanks to James Pearson <james-p-5Ol4pYTxKWu0ML75eksnrtBPR1lH4CV8@public.gmane.org> for reporting and
> diagnosing the problem.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> James-
> 
> Is this what you want?

Hmm.... Strictly speaking, I don't think we need to do this for the
NFSv4 case, since that has no equivalent to the NFSv3 rtpref/wtpref
attribute. It might help make the code look more consistent, though.

Note: at some point soon, I want to clean those functions up by
introducing an 'alloc_nfs_parsed_mount_data()' function that both
allocates the nfs_parsed_mount_data structure and initialises it with
the common defaults.


>  fs/nfs/super.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index bde444b..cf0d754 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1635,8 +1635,6 @@ static int nfs_validate_mount_data(void *options,
>  		goto out_no_data;
>  
>  	args->flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP);
> -	args->rsize		= NFS_MAX_FILE_IO_SIZE;
> -	args->wsize		= NFS_MAX_FILE_IO_SIZE;
>  	args->acregmin		= NFS_DEF_ACREGMIN;
>  	args->acregmax		= NFS_DEF_ACREGMAX;
>  	args->acdirmin		= NFS_DEF_ACDIRMIN;
> @@ -2351,8 +2349,6 @@ static int nfs4_validate_mount_data(void *options,
>  	if (data == NULL)
>  		goto out_no_data;
>  
> -	args->rsize		= NFS_MAX_FILE_IO_SIZE;
> -	args->wsize		= NFS_MAX_FILE_IO_SIZE;
>  	args->acregmin		= NFS_DEF_ACREGMIN;
>  	args->acregmax		= NFS_DEF_ACREGMAX;
>  	args->acdirmin		= NFS_DEF_ACDIRMIN;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NFS: Revert default r/wsize behavior

[Chuck Lever]

On Sep 10, 2009, at 5:17 PM, Trond Myklebust wrote:
> On Thu, 2009-09-10 at 17:05 -0400, Chuck Lever wrote:
>> When the "rsize=" or "wsize=" mount options are not specified,
>> text-based mounts have slightly different behavior than legacy binary
>> mounts.  Text-based mounts use the smaller of the server's maximum
>> and the client's maximum, but binary mounts use the smaller of the
>> server's _preferred_ size and the client's maximum.
>>
>> This difference is actually pretty subtle.  Most servers advertise
>> the same value as their largest and their preferred transfer size, so
>> the end result is the same in most cases.
>>
>> The reason for this difference is that for text-based mounts, if
>> r/wsize are not specified, they are set to the largest value  
>> supported
>> by the client.  For binary mounts, the values are set to zero if  
>> these
>> options are not specified.
>>
>> nfs_server_set_fsinfo() can negotiate the defaults correctly in any
>> case.  There's no need to specify any particular value as default in
>> the text-based option parsing logic.
>>
>> Thanks to James Pearson <james-p-5Ol4pYTxKWu0ML75eksnrtBPR1lH4CV8@public.gmane.org> for reporting  
>> and
>> diagnosing the problem.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> James-
>>
>> Is this what you want?
>
> Hmm.... Strictly speaking, I don't think we need to do this for the
> NFSv4 case, since that has no equivalent to the NFSv3 rtpref/wtpref
> attribute. It might help make the code look more consistent, though.

I was just about to check that against mount.nfs4.

> Note: at some point soon, I want to clean those functions up by
> introducing an 'alloc_nfs_parsed_mount_data()' function that both
> allocates the nfs_parsed_mount_data structure and initialises it with
> the common defaults.

Sounds good.

> fs/nfs/super.c |    4 ----
>> 1 files changed, 0 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index bde444b..cf0d754 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1635,8 +1635,6 @@ static int nfs_validate_mount_data(void  
>> *options,
>> 		goto out_no_data;
>>
>> 	args->flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP);
>> -	args->rsize		= NFS_MAX_FILE_IO_SIZE;
>> -	args->wsize		= NFS_MAX_FILE_IO_SIZE;
>> 	args->acregmin		= NFS_DEF_ACREGMIN;
>> 	args->acregmax		= NFS_DEF_ACREGMAX;
>> 	args->acdirmin		= NFS_DEF_ACDIRMIN;
>> @@ -2351,8 +2349,6 @@ static int nfs4_validate_mount_data(void  
>> *options,
>> 	if (data == NULL)
>> 		goto out_no_data;
>>
>> -	args->rsize		= NFS_MAX_FILE_IO_SIZE;
>> -	args->wsize		= NFS_MAX_FILE_IO_SIZE;
>> 	args->acregmin		= NFS_DEF_ACREGMIN;
>> 	args->acregmax		= NFS_DEF_ACREGMAX;
>> 	args->acdirmin		= NFS_DEF_ACDIRMIN;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

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

Re: [PATCH] NFS: Revert default r/wsize behavior

[James Pearson]

Trond Myklebust wrote:
> 
> Hmm.... Strictly speaking, I don't think we need to do this for the
> NFSv4 case, since that has no equivalent to the NFSv3 rtpref/wtpref
> attribute. It might help make the code look more consistent, though.
> 
> Note: at some point soon, I want to clean those functions up by
> introducing an 'alloc_nfs_parsed_mount_data()' function that both
> allocates the nfs_parsed_mount_data structure and initialises it with
> the common defaults.

I notice that 2.6.32-rc1 now has the 'alloc_nfs_parsed_mount_data()' 
function - but [rw]size still defaults to NFS_MAX_FILE_IO_SIZE - 
shouldn't these now be set to 0 by default?

Thanks

James Pearson

Re: [PATCH] NFS: Revert default r/wsize behavior

[James Pearson]

Chuck Lever wrote:
> When the "rsize=" or "wsize=" mount options are not specified,
> text-based mounts have slightly different behavior than legacy binary
> mounts.  Text-based mounts use the smaller of the server's maximum
> and the client's maximum, but binary mounts use the smaller of the
> server's _preferred_ size and the client's maximum.
> 
> This difference is actually pretty subtle.  Most servers advertise
> the same value as their largest and their preferred transfer size, so
> the end result is the same in most cases.
> 
> The reason for this difference is that for text-based mounts, if
> r/wsize are not specified, they are set to the largest value supported
> by the client.  For binary mounts, the values are set to zero if these
> options are not specified.
> 
> nfs_server_set_fsinfo() can negotiate the defaults correctly in any
> case.  There's no need to specify any particular value as default in
> the text-based option parsing logic.
> 
> Thanks to James Pearson <james-p-5Ol4pYTxKWu0ML75eksnrtBPR1lH4CV8@public.gmane.org> for reporting and
> diagnosing the problem.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> James-
> 
> Is this what you want?
> 
>  fs/nfs/super.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index bde444b..cf0d754 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1635,8 +1635,6 @@ static int nfs_validate_mount_data(void *options,
>  		goto out_no_data;
>  
>  	args->flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP);
> -	args->rsize		= NFS_MAX_FILE_IO_SIZE;
> -	args->wsize		= NFS_MAX_FILE_IO_SIZE;
>  	args->acregmin		= NFS_DEF_ACREGMIN;
>  	args->acregmax		= NFS_DEF_ACREGMAX;
>  	args->acdirmin		= NFS_DEF_ACDIRMIN;
> @@ -2351,8 +2349,6 @@ static int nfs4_validate_mount_data(void *options,
>  	if (data == NULL)
>  		goto out_no_data;
>  
> -	args->rsize		= NFS_MAX_FILE_IO_SIZE;
> -	args->wsize		= NFS_MAX_FILE_IO_SIZE;
>  	args->acregmin		= NFS_DEF_ACREGMIN;
>  	args->acregmax		= NFS_DEF_ACREGMAX;
>  	args->acdirmin		= NFS_DEF_ACDIRMIN;

That is what I've already done myself for the NFSv3 case - personally I 
would implicitly set args->[rw]size to zero (although args would have 
been initialled with kzalloc()), just to make it clear that the defaults 
are zero.

Thanks

James Pearson