NFS4ERR_FILE_OPEN handling in Linux

NFS4ERR_FILE_OPEN handling in Linux

[Neil Brown]

Hi Trond,

 Following up for a customer who had problem with NFSv4 when talking
 to a Solaris server with "nbman" enabled, I have questions about the
 handling of NFS4ERR_FILE_OPEN.
 In particular:
   1/ should commit c514983d8d2260020543a81589a2b8c7d4bdab4e be
      reverted, and
   2/ should nfs_errtbl map NFS4ERR_FILE_OPEN to -EBUSY rather than
      defaulting to -EIO

 That commit, included below for reference, causes the NFS client to
 retry indefinitely if NFS4ERR_FILE_OPEN is returned.  This
 contradicts the comment which suggests it will only "retry a few
 times" and cannot be correct as a file could be held open (thus
 causing the error) indefinitely.

 The problem that the patch claims to address (relating to ordering of
 async calls) is, I believe, addressed properly by the subsequent
 patch

commit a49c3c7736a2e77931dabc5bc4a83fb4b2da013e
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Thu Oct 18 18:03:27 2007 -0400

    NFSv4: Ensure that we wait for the CLOSE request to complete
    

 So I believe that first patch is both unnecessary and incorrect.


 When the server refuses to perform an action because the file is
 currently open, I think 'EIO' does not do justice to the situation at
 all.  EBUSY seems a lot more appropriate and does seem to be a
 permitted error code for e.g. rename(2) and unlink(2).  EACCES might
 be a possible alternate.  It doesn't really mean the same thing, but
 it is likely to be handled in an appropriate way.  Apparently the
 Solaris client returns EACCES.

 Thoughts?

Thanks,
NeilBrown



>From c514983d8d2260020543a81589a2b8c7d4bdab4e Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 15 Sep 2006 08:25:04 -0400
Subject: [PATCH] NFSv4: Handle the condition NFS4ERR_FILE_OPEN

Retry a few times before we give up: the error is usually due to ordering
issues with asynchronous RPC calls.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4proc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c218cc4..c49ac3e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2891,6 +2891,7 @@ int nfs4_handle_exception(const struct nfs_server *server, int errorcode, struct
 			if (ret == 0)
 				exception->retry = 1;
 			break;
+		case -NFS4ERR_FILE_OPEN:
 		case -NFS4ERR_GRACE:
 		case -NFS4ERR_DELAY:
 			ret = nfs4_delay(server->client, &exception->timeout);
-- 
1.6.4.3

Re: NFS4ERR_FILE_OPEN handling in Linux

[Trond Myklebust]

On Thu, 2009-10-15 at 11:35 +1100, Neil Brown wrote:
> Hi Trond,
> 
>  Following up for a customer who had problem with NFSv4 when talking
>  to a Solaris server with "nbman" enabled, I have questions about the
>  handling of NFS4ERR_FILE_OPEN.
>  In particular:
>    1/ should commit c514983d8d2260020543a81589a2b8c7d4bdab4e be
>       reverted, and
>    2/ should nfs_errtbl map NFS4ERR_FILE_OPEN to -EBUSY rather than
>       defaulting to -EIO
> 
>  That commit, included below for reference, causes the NFS client to
>  retry indefinitely if NFS4ERR_FILE_OPEN is returned.  This
>  contradicts the comment which suggests it will only "retry a few
>  times" and cannot be correct as a file could be held open (thus
>  causing the error) indefinitely.

I'm unfortunately unfamiliar with 'nbman' on Solaris. Could you please
explain what it is, and why it should cause the nfs server to start
returning NFS4ERR_FILE_OPEN?

As far as I know, the purpose of the NFS4ERR_FILE_OPEN error was to
address the issue of MS Windows semantics, which do not allow certain
operations (mainly unlink() and rename()) on an open file. Since the
error is supposed to be transient (i.e. is caused by a lock) the current
behaviour was chosen in order to try to provide posix-like semantics in
these situations.
While we could change the behaviour to return -EBUSY, that might
conceivably break applications that expect to be able to create and
destroy temporary files. I therefore think that some attempt should be
made to wait and retry before we start returning application errors.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: NFS4ERR_FILE_OPEN handling in Linux

[NeilBrown]

On Thu, October 15, 2009 11:53 pm, Trond Myklebust wrote:
> On Thu, 2009-10-15 at 11:35 +1100, Neil Brown wrote:
>> Hi Trond,
>>
>>  Following up for a customer who had problem with NFSv4 when talking
>>  to a Solaris server with "nbman" enabled, I have questions about the
>>  handling of NFS4ERR_FILE_OPEN.
>>  In particular:
>>    1/ should commit c514983d8d2260020543a81589a2b8c7d4bdab4e be
>>       reverted, and
>>    2/ should nfs_errtbl map NFS4ERR_FILE_OPEN to -EBUSY rather than
>>       defaulting to -EIO
>>
>>  That commit, included below for reference, causes the NFS client to
>>  retry indefinitely if NFS4ERR_FILE_OPEN is returned.  This
>>  contradicts the comment which suggests it will only "retry a few
>>  times" and cannot be correct as a file could be held open (thus
>>  causing the error) indefinitely.
>
> I'm unfortunately unfamiliar with 'nbman' on Solaris. Could you please
> explain what it is, and why it should cause the nfs server to start
> returning NFS4ERR_FILE_OPEN?

I'm sorry, that should have been "nbmand" - my mistake.
'nbmand' is an abbreviation of "non-blocking mandatory locks".
It seems to be a ZFS option rather than an NFSv4 option and seems to
relate to providing better CIFS semantics on ZFS.  So it is
unsurprising that it causes and error message that, as you say, we
would normally expect only from an MS-windows server.

>
> As far as I know, the purpose of the NFS4ERR_FILE_OPEN error was to
> address the issue of MS Windows semantics, which do not allow certain
> operations (mainly unlink() and rename()) on an open file. Since the
> error is supposed to be transient (i.e. is caused by a lock) the current
> behaviour was chosen in order to try to provide posix-like semantics in
> these situations.
> While we could change the behaviour to return -EBUSY, that might
> conceivably break applications that expect to be able to create and
> destroy temporary files. I therefore think that some attempt should be
> made to wait and retry before we start returning application errors.

Given that this is a question of what a POSIX application can expect from
a non-POSIX filesystem, there can be no 'definitely right' answers.

A few retries could be justified I believe.  I don't think indefinite
retries is really justifiable.  So how many retries?
If we retries until exception->timeout exceeds HZ, that would be a max
delay of about 2 seconds, and about 5 retries.  Does that seem a
fair balance to you?

Maybe the below (untested and probably space-damaged, given for
illustration purposes only).

Thanks,
NeilBrown




diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ed7c269..1cb6b0f 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -270,6 +270,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 83ad47c..dcf5e03 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5685,6 +5685,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.