[PATCH 0/2] Some fixes for NFS

[PATCH 0/2] Some fixes for NFS

[NeilBrown]

Hi Trond,
 I have mentioned both of these patches to you before, but each time
 with a certain amount of indecisiveness.

 I am now quite confident of them and would like to submit them to
 you, possibly even for the next merge window.

Thanks,
NeilBrown


---

NeilBrown (2):
      NFS: honour preferred rsize/wsize reported by server.
      NFS4ERR_FILE_OPEN handling in Linux/NFS


 fs/nfs/nfs4proc.c |    5 +++++
 fs/nfs/nfs4xdr.c  |    1 +
 fs/nfs/super.c    |    4 ++--
 3 files changed, 8 insertions(+), 2 deletions(-)

-- 
Signature

[PATCH 1/2] NFS4ERR_FILE_OPEN handling in Linux/NFS

[NeilBrown]

NFS4ERR_FILE_OPEN is returned by the server when an operation cannot be
performed because the file is currently open and local (to the server)
semantics prohibit the operation while the file is open.
A typical case is a RENAME operation on an MS-Windows platform, which
prevents rename while the file is open.

While it is possible that such a condition is transitory, it is also
very possible that the file will be held open for an extended period
of time thus preventing the operation.

The current behaviour of Linux/NFS is to retry the operation
indefinitely.  This is not appropriate - we do not expect a rename to
take an arbitrary amount of time to complete.

Rather, an error should be returned.  The most obvious error code
would be EBUSY, which is a legal at least for 'rename' and 'unlink',
and accurately captures the reason for the error.

This patch allows a few retries until about 2 seconds have elapsed,
then returns EBUSY.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 fs/nfs/nfs4proc.c |    5 +++++
 fs/nfs/nfs4xdr.c  |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ff37454..be26c0c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -275,6 +275,11 @@ static int nfs4_handle_exception(const struct nfs_server *server, int errorcode,
 			/* FALLTHROUGH */
 #endif /* !defined(CONFIG_NFS_V4_1) */
 		case -NFS4ERR_FILE_OPEN:
+			if (exception->timeout > HZ)
+				/* We have retried a decent amount, time to
+				 * fail
+				 */
+				break;
 		case -NFS4ERR_GRACE:
 		case -NFS4ERR_DELAY:
 			ret = nfs4_delay(server->client, &exception->timeout);
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 20b4e30..e50246d 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5684,6 +5684,7 @@ static struct {
 	{ NFS4ERR_SYMLINK,	-ELOOP		},
 	{ NFS4ERR_OP_ILLEGAL,	-EOPNOTSUPP	},
 	{ NFS4ERR_DEADLOCK,	-EDEADLK	},
+	{ NFS4ERR_FILE_OPEN,	-EBUSY		},
 	{ NFS4ERR_WRONGSEC,	-EPERM		}, /* FIXME: this needs
 						    * to be handled by a
 						    * middle-layer.

[PATCH 2/2] NFS: honour preferred rsize/wsize reported by server.

[NeilBrown]

When a filesystem is mounted with ASCII mount options, and no rsize
or wsize is given, the client should default to using the preferred
sizes reported in the FSINFO request.
However it does not.  These values are initialised to
NFS_MAX_FILE_IO_SIZE and as they are now non-zero, the default does
not get applied.

So initialise to 0 thus allowing appropriate defaults to apply.

Signed-off-by: NeilBrown <neilb@suse.de>
---

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

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 90be551..18c5f85 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -734,8 +734,8 @@ static struct nfs_parsed_mount_data *nfs_alloc_parsed_mount_data(unsigned int ve
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (data) {
-		data->rsize		= NFS_MAX_FILE_IO_SIZE;
-		data->wsize		= NFS_MAX_FILE_IO_SIZE;
+		data->rsize		= 0;
+		data->wsize		= 0;
 		data->acregmin		= NFS_DEF_ACREGMIN;
 		data->acregmax		= NFS_DEF_ACREGMAX;
 		data->acdirmin		= NFS_DEF_ACDIRMIN;

Re: [PATCH 1/2] NFS4ERR_FILE_OPEN handling in Linux/NFS

[Trond Myklebust]

On Tue, 2009-11-17 at 15:21 +1100, NeilBrown wrote: 
> NFS4ERR_FILE_OPEN is returned by the server when an operation cannot be
> performed because the file is currently open and local (to the server)
> semantics prohibit the operation while the file is open.
> A typical case is a RENAME operation on an MS-Windows platform, which
> prevents rename while the file is open.
> 
> While it is possible that such a condition is transitory, it is also
> very possible that the file will be held open for an extended period
> of time thus preventing the operation.
> 
> The current behaviour of Linux/NFS is to retry the operation
> indefinitely.  This is not appropriate - we do not expect a rename to
> take an arbitrary amount of time to complete.
> 
> Rather, an error should be returned.  The most obvious error code
> would be EBUSY, which is a legal at least for 'rename' and 'unlink',
> and accurately captures the reason for the error.
> 
> This patch allows a few retries until about 2 seconds have elapsed,
> then returns EBUSY.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
>  fs/nfs/nfs4proc.c |    5 +++++
>  fs/nfs/nfs4xdr.c  |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ff37454..be26c0c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -275,6 +275,11 @@ static int nfs4_handle_exception(const struct nfs_server *server, int errorcode,
>  			/* FALLTHROUGH */
>  #endif /* !defined(CONFIG_NFS_V4_1) */
>  		case -NFS4ERR_FILE_OPEN:
> +			if (exception->timeout > HZ)
> +				/* We have retried a decent amount, time to
> +				 * fail
> +				 */
> +				break;
>  		case -NFS4ERR_GRACE:
>  		case -NFS4ERR_DELAY:
>  			ret = nfs4_delay(server->client, &exception->timeout);
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 20b4e30..e50246d 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -5684,6 +5684,7 @@ static struct {
>  	{ NFS4ERR_SYMLINK,	-ELOOP		},
>  	{ NFS4ERR_OP_ILLEGAL,	-EOPNOTSUPP	},
>  	{ NFS4ERR_DEADLOCK,	-EDEADLK	},
> +	{ NFS4ERR_FILE_OPEN,	-EBUSY		},
>  	{ NFS4ERR_WRONGSEC,	-EPERM		}, /* FIXME: this needs
>  						    * to be handled by a
>  						    * middle-layer.
> 
> 

If you add an entry for NFS4ERR_FILE_OPEN into nfs4_stat_to_errno(),
then the result of the RPC call will be -EBUSY rather than
NFS4ERR_FILE_OPEN.
In that case, I can't see how the NFS4ERR_FILE_OPEN is being propagated
to nfs4_handle_exception().

See
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git&a=commitdiff&h=52567b03ca38b6e556ced450d64dba8d66e23b0e for how we fixed up a similar problem with NFS4ERR_RESOURCE...

Cheers
  Trond

Re: [PATCH 1/2] NFS4ERR_FILE_OPEN handling in Linux/NFS

[Neil Brown]

On Fri, 20 Nov 2009 14:58:33 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> If you add an entry for NFS4ERR_FILE_OPEN into nfs4_stat_to_errno(),
> then the result of the RPC call will be -EBUSY rather than
> NFS4ERR_FILE_OPEN.
> In that case, I can't see how the NFS4ERR_FILE_OPEN is being
> propagated to nfs4_handle_exception().
> 
> See
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git&a=commitdiff&h=52567b03ca38b6e556ced450d64dba8d66e23b0e
> for how we fixed up a similar problem with NFS4ERR_RESOURCE...

Thanks.  That all makes lots of sense.
I don't think I need nfs4_map_errors to understand NFS4ERR_FILE_OPEN
in this case as it can be completely handled in nfs4_handle_exception.

So how about this:

Thanks,
NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Tue, 24 Nov 2009 13:50:52 +1100
Subject: [PATCH] NFS4ERR_FILE_OPEN handling in Linux/NFS

NFS4ERR_FILE_OPEN is return by the server when an operation cannot be
performed because the file is currently open and local (to the server)
semantics prohibit the operation while the file is open.
A typical case is a RENAME operation on an MS-Windows platform, which
prevents rename while the file is open.

While it is possible that such a condition is transitory, it is also
very possible that the file will be held open for an extended period
of time thus preventing the operation.

The current behaviour of Linux/NFS is to retry the operation
indefinitely.  This is not appropriate - we do not expect a rename to
take an arbitrary amount of time to complete.

Rather, and error should be returned.  The most obvious error code
would be EBUSY, which is a legal at least for 'rename' and 'unlink',
and accurately captures the reason for the error.

This patch allows a few retries until about 2 seconds have elapsed,
then returns EBUSY.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/nfs4proc.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ff37454..f37f466 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -275,6 +275,13 @@ static int nfs4_handle_exception(const struct nfs_server *server, int errorcode,
 			/* FALLTHROUGH */
 #endif /* !defined(CONFIG_NFS_V4_1) */
 		case -NFS4ERR_FILE_OPEN:
+			if (exception->timeout > HZ) {
+				/* We have retried a decent amount, time to
+				 * fail
+				 */
+				err = -EBUSY;
+				break;
+			}
 		case -NFS4ERR_GRACE:
 		case -NFS4ERR_DELAY:
 			ret = nfs4_delay(server->client, &exception->timeout);
-- 
1.6.4.3