On Aug. 11, 2008, 20:50 +0300, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: > On Mon, Aug 11, 2008 at 08:39:59PM +0300, Benny Halevy wrote: >> On Aug. 11, 2008, 19:28 +0300, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: >>> On Mon, Aug 11, 2008 at 12:17:35PM -0400, Chuck Lever wrote: >>>> Hi Benny- >>>> >>>> On Mon, Aug 11, 2008 at 12:11 PM, Benny Halevy <bhalevy@xxxxxxxxxxx> wrote: >>>>> On Aug. 11, 2008, 18:58 +0300, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: >>>>>> On Mon, Aug 11, 2008 at 05:09:36PM +0300, Benny Halevy wrote: >>>>>>> Bruce, in a couple locations the nfsd needs to encode the stateid.seqid >>>>>>> as a uint32_t rather than as opaque. >>>>>> Agreed, thanks. >>>>>> >>>>>> Though I have a hard time figuring out whether this has any impact in >>>>>> practice. Presumably the only change on the wire is that we'll get the >>>>>> endianness of the stateid4.seqid right? But that field is mostly opaque >>>>>> to the client anyway; 3530 says >>>>>> >>>>>> The server is required to increment the seqid field >>>>>> monotonically at each transition of the stateid. This is >>>>>> important since the client will inspect the seqid in OPEN >>>>>> stateids to determine the order of OPEN processing done by the >>>>>> server. >>>>>> >>>>>> but doesn't say why this is important. I'm sure this has been brought >>>>>> up on the ietf list before, but can't recall whether someone came up >>>>>> with a justification for the importance of this. >>>>>> >>>>>> Anyway, so I figure these should be queued up for the next (2.6.28) >>>>>> merge window. Thanks! >>>>> Actually, I think this breaks delegreturn. >>>>> Since we decode the stateid.si_generation correctly, it will get swabbed >>>>> in delegreturn on little-endian servers. This will cause >>>>> nfs4_preprocess_stateid_op/check_stateid_generation as called by >>>>> nfsd4_delegreturn to fail. And eventually, unhash_delegation >>>>> wouldn't be called. >>>> Sounds plausible, good catch. Yet another reason we should have an >>>> easy-to-access delegation counter metric on both the client and >>>> server. >>>> >>>> I wonder, since you found three separate places where this is needed: >>>> should you construct a helper function? >>> A stateid encoder/decoder? Sure, that could be a good idea. >> Cool. Would you like me to rework the patches I've already sent >> or send a patch adding the helpers on top of them? > > I'm fine with whichever you think makes the most sense. > > --b. OK. It's better to separate the fixes from the refactoring of code. The 7-patch patchset in reply to this message does the following: [PATCH 1/7] nfsd: properly xdr-encode stateid4.seqid as uint32_t for cb_recall encoding fix for cb_recall. There's no helper defined since there's only one use. I've defined a helper and used in the nfs41 series since there, there are 2 (cb_recall and cb_layoutrecall). [PATCH 2/7] nfsd: properly xdr-encode deleg stateid returned from open [PATCH 3/7] nfsd: fix nfsd4_encode_open buffer space reservation xdr encoding fixes [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function [PATCH 5/7] nfsd: don't declare p in ENCODE_SEQID_OP_HEAD xdr encode_stateid refactoring [PATCH 6/7] nfsd: properly xdr-decode NFS4_OPEN_CLAIM_DELEGATE_CUR stateid xdr decoding fix [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function xdr decode_stateid refactoring Benny -- 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
