Re: [pnfs] [PATCH 0/2] fix nfsd stateid encoding

Chuck Lever
2008-08-11T16:17:37Z

Hi Benny-

On Mon, Aug 11, 2008 at 12:11 PM, Benny Halevy <bhalevy@panasas.com> wrote:
> On Aug. 11, 2008, 18:58 +0300, "J. Bruce Fields" <bfields@fieldses.org> 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? Certainly there are already
missed opportunities for sharing XDR encoding and decoding between
callback and the forward channel.

> Hence, I think these patches are appropriate for 2.6.27 and
> even to older stable releases.
>
> Benny
>
>>
>> --b.
>>
>>> Patch #1 fixes that for cb_recall.
>>> Patch #2 fixes the deleg stateid returned by open.
>>>
>>> The patches should apply to linux-2.6/master
>>> commit 796aadeb1b2db9b5d463946766c5bbfd7717158c
>>>
>>> Benny
>> _______________________________________________
>> pNFS mailing list
>> pNFS@linux-nfs.org
>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
"Officer. Ma'am. Squeaker."
-- Mr. Incredible