On Wed, 2013-12-04 at 16:38 -0500, Dr Fields James Bruce wrote:
On Wed, Dec 04, 2013 at 04:22:48PM -0500, Trond Myklebust wrote:
On Dec 4, 2013, at 16:03, Dr Fields James Bruce <bfields@xxxxxxxxxxxx> wrote:
On Wed, Dec 04, 2013 at 10:44:45PM +0200, Antti Tönkyrä wrote:
http://daedalus.pingtimeout.net/dbg/eilseq_ioerr.pcap
And I see something I'd overlooked before: the client is sending the
later opens with the same open owner and sequence id. But NFS4ERR_IO is
a seqid-mutating error. So now I think this probably *is* a client
bug....
Umm… Yes and no. The client should be able to recover when it discovers that the seqid is out of sync.
That said, I see that we do
status = decode_op_hdr(xdr, OP_OPEN);
if (status != -EIO)
nfs_increment_open_seqid(status, res->seqid);
and since NFS4ERR_IO == EIO, that means we skip the seqid update when you send us NFS4ERR_IO.
Oh, OK. Maybe decode_op_hdr could use -NFS4ERR_BADXDR for the two
decoding errors it catches and eliminate the need for this special -EIO
case?
That is sort of hackish. How about something like the following patch.
I think NFS4ERR_IO is a legal error for these operations. (Even if the
server should have returned something else in this case.)
It is legal for OPEN. It does not seem to be legal for OPEN_CONFIRM,
LOCK, LOCKU, CLOSE or OPEN_DOWNGRADE according to RFC3530bis.
Cheers
Trond
8<--------------------------------------------------------
From 968a89bfaf176d70352bb1c0003bf496fdc180aa Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
Date: Wed, 4 Dec 2013 17:39:23 -0500
Subject: [PATCH] NFSv4: OPEN must handle the NFS4ERR_IO return code correctly
decode_op_hdr() cannot distinguish between an XDR decoding error and
the perfectly valid errorcode NFS4ERR_IO. This is normally not a
problem, but for the particular case of OPEN, we need to be able
to increment the NFSv4 open sequence id when the server returns
a valid response.
Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
---
fs/nfs/nfs4xdr.c | 47 +++++++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5be2868c02f1..8c21d69a9dc1 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3097,7 +3097,8 @@ out_overflow:
return -EIO;
}
-static int decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected)
+static bool __decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected,
+ int *nfs_retval)
{
__be32 *p;
uint32_t opnum;
@@ -3107,19 +3108,32 @@ static int decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected)
if (unlikely(!p))
goto out_overflow;
opnum = be32_to_cpup(p++);
- if (opnum != expected) {
- dprintk("nfs: Server returned operation"
- " %d but we issued a request for %d\n",
- opnum, expected);
- return -EIO;
- }
+ if (unlikely(opnum != expected))
+ goto out_bad_operation;
nfserr = be32_to_cpup(p);
- if (nfserr != NFS_OK)
- return nfs4_stat_to_errno(nfserr);
- return 0;
+ if (nfserr == NFS_OK)
+ *nfs_retval = 0;
+ else
+ *nfs_retval = nfs4_stat_to_errno(nfserr);
+ return true;
+out_bad_operation:
+ dprintk("nfs: Server returned operation"
+ " %d but we issued a request for %d\n",
+ opnum, expected);
+ *nfs_retval = -EREMOTEIO;
+ return false;
out_overflow:
print_overflow_msg(__func__, xdr);
- return -EIO;
+ *nfs_retval = -EIO;
+ return false;
+}
+
+static int decode_op_hdr(struct xdr_stream *xdr, enum nfs_opnum4 expected)
+{
+ int retval;
+
+ __decode_op_hdr(xdr, expected, &retval);
+ return retval;
}
/* Dummy routine */
@@ -5001,11 +5015,12 @@ static int decode_open(struct xdr_stream *xdr, struct nfs_openres *res)
uint32_t savewords, bmlen, i;
int status;
- status = decode_op_hdr(xdr, OP_OPEN);
- if (status != -EIO)
- nfs_increment_open_seqid(status, res->seqid);
- if (!status)
- status = decode_stateid(xdr, &res->stateid);
+ if (!__decode_op_hdr(xdr, OP_OPEN, &status))
+ return status;
+ nfs_increment_open_seqid(status, res->seqid);
+ if (status)
+ return status;
+ status = decode_stateid(xdr, &res->stateid);
if (unlikely(status))
return status;
