Re: Patch for mapping EILSEQ into NFSERR_INVAL

["Antti Tönkyrä" <daedalus@pingtimeout.net>]

On 2013-12-05 00:49, Trond Myklebust wrote:
> 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@fieldses.org> 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@netapp.com>
> 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@primarydata.com>
> ---
>   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;
>   
I compiled 3.13-rc1 with that patch included.

Touching the file now returns I/O error just like before but the share 
doesn't die anymore so the patch works.

- Antti