On Mar 5, 2014, at 5:58, Andy Adamson <androsadamson@xxxxxxxxx> wrote: > On Tue, Mar 4, 2014 at 1:09 PM, Trond Myklebust > <trond.myklebust@xxxxxxxxxxxxxxx> wrote: >> >> On Mar 4, 2014, at 12:31, andros@xxxxxxxxxx wrote: >> >>> From: Andy Adamson <andros@xxxxxxxxxx> >>> >>> This is an expanded version of the "NFSv4 always compare stateids in nfs4_stateid_is_current" patch I sent on Feb 27. >>> >>> Found at Connectathon 2014 and NetApp internal testing. >>> >>> nfs4_stateid_is_current is used on the NFSv4 I/O path to determine if a >>> stateid has changed. The idea is that if there is a stateid expire error >>> such as NFS4ERR_BAD_STATEID and the stateid used that induced the error has >>> changed, then we can just resend the RPC from the call prepare state with >>> the changed stateid instead of kicking off recovery as the changed stateid >>> indicates it already had been recovered. >>> >>> This patch set fixes a false positive that resulted in an NFS4ERR_BAD_STATEID >>> infinite loop. Trond pointed out that the nfs4_select_rw_stateid -EIO error >>> is special in that it indicates a lost lock. >>> >>> As I examined the use of nfs4_select_rw_stateid, I addressed some other errors >>> in the use of nfs4_set_rw_stateid and friends in setattr and filelayout >>> prepare functions. >>> >>> Just tested with connectathon tests. Will test more once I'm back in town... >> >> One question: >> >> Do we need the EWOULDBLOCK case at all in nfs4_copy_lock_stateid/nfs4_copy_open_stateid? Looking at the code, it seems particularly redundant for the case of NFSv4.1, where we set the seqid to '0'. Given that we do the nfs4_stateid_is_current() test, it seems unnecessary in the case of NFSv4.0 too... > > Yes - Even though you mentioned it's importance at Connectathon, I > could not find a use of the EWOULDBLOCK return. > >> >> So, how about just throwing out the EWOULDBLOCK returns, then keeping nfs4_stateid_is_current() as it is, > > So, you mean keeping nfs4_stateid_is_current as a boolean, and then > assume that a false return == -EIO? > > I don't like that because that is how this bug was created - mixing > bool and int functions. Why is that such a problem? The rpc_call_prepare functions copy the stateid and should test the stateid validity flags again. >> fixing up _nfs4_do_setattr to only deal with the -EIO case, > > Yes. > > and then fixing up the file layout uses of nfs4_select_rw_stateid() > to check the return values? > > As I did in the last patch... Yes, but that patch failed to respect the locking requirements. _________________________________ Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@xxxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
