Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.pingtimeout.net ([194.187.214.64]:52728 "EHLO mx1.pingtimeout.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751053Ab3LEIj0 (ORCPT ); Thu, 5 Dec 2013 03:39:26 -0500 Message-ID: <52A03BB5.3010803@pingtimeout.net> Date: Thu, 05 Dec 2013 10:39:17 +0200 From: =?UTF-8?B?QW50dGkgVMO2bmt5csOk?= MIME-Version: 1.0 To: Trond Myklebust , Dr Fields James Bruce CC: Linux NFS Mailing List Subject: Re: Patch for mapping EILSEQ into NFSERR_INVAL References: <529CEBC3.8060505@pingtimeout.net> <529CF322.4080701@pingtimeout.net> <20131203204806.GA2648@fieldses.org> <20131203212210.GC2648@fieldses.org> <529ED1C8.70208@pingtimeout.net> <20131204154104.GB14646@fieldses.org> <529F943D.30004@pingtimeout.net> <20131204210356.GA19452@fieldses.org> <22997028-AB15-4D61-A263-011867EE7512@primarydata.com> <20131204213804.GC19452@fieldses.org> <1386197350.31895.2.camel@leira.trondhjem.org> In-Reply-To: <1386197350.31895.2.camel@leira.trondhjem.org> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 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 > 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 > --- > 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