[PATCH 0/2] fix nfsd stateid encoding

[PATCH 0/2] fix nfsd stateid encoding

[Benny Halevy]

Bruce, in a couple locations the nfsd needs to encode the stateid.seqid
as a uint32_t rather than as opaque.

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

[PATCH 1/2] nfsd: properly xdr-encode stateid4.seqid as uint32_t for cb_recall

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4callback.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index ecae593..30d3130 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -222,7 +222,8 @@ encode_cb_recall(struct xdr_stream *xdr, struct nfs4_cb_recall *cb_rec)
 
 	RESERVE_SPACE(12+sizeof(cb_rec->cbr_stateid) + len);
 	WRITE32(OP_CB_RECALL);
-	WRITEMEM(&cb_rec->cbr_stateid, sizeof(stateid_t));
+	WRITE32(cb_rec->cbr_stateid.si_generation);
+	WRITEMEM(&cb_rec->cbr_stateid.si_opaque, sizeof(stateid_opaque_t));
 	WRITE32(cb_rec->cbr_trunc);
 	WRITE32(len);
 	WRITEMEM(cb_rec->cbr_fhval, len);
-- 
1.5.6.5

[PATCH 2/2] nfsd: properly xdr-encode deleg stateid returned from open

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e1a1a5e..cfe9d5c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2158,7 +2158,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 		break;
 	case NFS4_OPEN_DELEGATE_READ:
 		RESERVE_SPACE(20 + sizeof(stateid_t));
-		WRITEMEM(&open->op_delegate_stateid, sizeof(stateid_t));
+		WRITE32(open->op_delegate_stateid.si_generation);
+		WRITEMEM(&open->op_delegate_stateid.si_opaque,
+			 sizeof(stateid_opaque_t));
 		WRITE32(open->op_recall);
 
 		/*
@@ -2172,7 +2174,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 		break;
 	case NFS4_OPEN_DELEGATE_WRITE:
 		RESERVE_SPACE(32 + sizeof(stateid_t));
-		WRITEMEM(&open->op_delegate_stateid, sizeof(stateid_t));
+		WRITE32(open->op_delegate_stateid.si_generation);
+		WRITEMEM(&open->op_delegate_stateid.si_opaque,
+			 sizeof(stateid_opaque_t));
 		WRITE32(0);
 
 		/*
-- 
1.5.6.5

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

[J. Bruce Fields]

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!

--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

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

[Benny Halevy]

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.

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

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

[Chuck Lever]

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

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

[Benny Halevy]

On Aug. 11, 2008, 19:17 +0300, "Chuck Lever" <chucklever@gmail.com> wrote:
> 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:

Note, I looked at all other places we encode or decode stateids and
they seem ok at first glance.

> should you construct a helper function?  Certainly there are already
> missed opportunities for sharing XDR encoding and decoding between
> callback and the forward channel.

That's fine with me.  In fact, my first implementation did exactly
that but when I saw that everywhere else the encoding of si_generation
followed by 12 opaque bytes is done inline I reverted to keeping
the current convention.  If everybody is ok with having such helper
functions I'll be happy to send patches implementing them.

The scope of _this_, however, is less urgent and can easily be
done for 2.6.28.

Benny

> 
>> 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
>>
> 
> 
> 

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

[J. Bruce Fields]

On Mon, Aug 11, 2008 at 07:11:40PM +0300, Benny Halevy 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.

Yipes.  If delegations have always been broken and we haven't noticed,
then there's a more serious problem here--we should at least look into
why pynfs isn't catching that.

Oh, I see: si_generation is always zero for delegation stateid's!

> Hence, I think these patches are appropriate for 2.6.27 and
> even to older stable releases.

So unless I've missed something, I think this still looks more like a
style/consistency question?

--b.

> 
> 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
> 

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

[J. Bruce Fields]

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@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?

A stateid encoder/decoder?  Sure, that could be a good idea.

--b.

> Certainly there are already
> missed opportunities for sharing XDR encoding and decoding between
> callback and the forward channel.

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

[Benny Halevy]

On Aug. 11, 2008, 19:27 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Mon, Aug 11, 2008 at 07:11:40PM +0300, Benny Halevy 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.
> 
> Yipes.  If delegations have always been broken and we haven't noticed,
> then there's a more serious problem here--we should at least look into
> why pynfs isn't catching that.
> 
> Oh, I see: si_generation is always zero for delegation stateid's!

Phew :-)

> 
>> Hence, I think these patches are appropriate for 2.6.27 and
>> even to older stable releases.
> 
> So unless I've missed something, I think this still looks more like a
> style/consistency question?

Yup.

Benny

> 
> --b.
> 
>> 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

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

[Benny Halevy]

On Aug. 11, 2008, 19:28 +0300, "J. Bruce Fields" <bfields@fieldses.org> 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@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?
> 
> 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?

Benny

> 
> --b.
> 
>> Certainly there are already
>> missed opportunities for sharing XDR encoding and decoding between
>> callback and the forward channel.

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

[J. Bruce Fields]

On Mon, Aug 11, 2008 at 08:39:59PM +0300, Benny Halevy wrote:
> On Aug. 11, 2008, 19:28 +0300, "J. Bruce Fields" <bfields@fieldses.org> 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@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?
> > 
> > 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.

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

[Benny Halevy]

On Aug. 11, 2008, 20:50 +0300, "J. Bruce Fields" <bfields@fieldses.org> 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@fieldses.org> 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@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?
>>> 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

[PATCH 1/7] nfsd: properly xdr-encode stateid4.seqid as uint32_t for cb_recall

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4callback.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index ecae593..30d3130 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -222,7 +222,8 @@ encode_cb_recall(struct xdr_stream *xdr, struct nfs4_cb_recall *cb_rec)
 
 	RESERVE_SPACE(12+sizeof(cb_rec->cbr_stateid) + len);
 	WRITE32(OP_CB_RECALL);
-	WRITEMEM(&cb_rec->cbr_stateid, sizeof(stateid_t));
+	WRITE32(cb_rec->cbr_stateid.si_generation);
+	WRITEMEM(&cb_rec->cbr_stateid.si_opaque, sizeof(stateid_opaque_t));
 	WRITE32(cb_rec->cbr_trunc);
 	WRITE32(len);
 	WRITEMEM(cb_rec->cbr_fhval, len);
-- 
1.5.6.5

[PATCH 2/7] nfsd: properly xdr-encode deleg stateid returned from open

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e1a1a5e..cfe9d5c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2158,7 +2158,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 		break;
 	case NFS4_OPEN_DELEGATE_READ:
 		RESERVE_SPACE(20 + sizeof(stateid_t));
-		WRITEMEM(&open->op_delegate_stateid, sizeof(stateid_t));
+		WRITE32(open->op_delegate_stateid.si_generation);
+		WRITEMEM(&open->op_delegate_stateid.si_opaque,
+			 sizeof(stateid_opaque_t));
 		WRITE32(open->op_recall);
 
 		/*
@@ -2172,7 +2174,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 		break;
 	case NFS4_OPEN_DELEGATE_WRITE:
 		RESERVE_SPACE(32 + sizeof(stateid_t));
-		WRITEMEM(&open->op_delegate_stateid, sizeof(stateid_t));
+		WRITE32(open->op_delegate_stateid.si_generation);
+		WRITEMEM(&open->op_delegate_stateid.si_opaque,
+			 sizeof(stateid_opaque_t));
 		WRITE32(0);
 
 		/*
-- 
1.5.6.5

[PATCH 3/7] nfsd: fix nfsd4_encode_open buffer space reservation

[Benny Halevy]

nfsd4_encode_open first reservation is currently for 36 + sizeof(stateid_t)
while it writes after the stateid a cinfo (20 bytes) and 5 more 4-bytes
words, for a total of 40 + sizeof(stateid_t).

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index cfe9d5c..4ec52f9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2142,7 +2142,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 	if (nfserr)
 		goto out;
 
-	RESERVE_SPACE(36 + sizeof(stateid_t));
+	RESERVE_SPACE(40 + sizeof(stateid_t));
 	WRITE32(open->op_stateid.si_generation);
 	WRITEMEM(&open->op_stateid.si_opaque, sizeof(stateid_opaque_t));
 	WRITECINFO(open->op_cinfo);
-- 
1.5.6.5

[PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |   76 ++++++++++++++++++++++------------------------------
 1 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 4ec52f9..96da1aa 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1959,6 +1959,17 @@ fail:
 	return -EINVAL;
 }
 
+static void
+nfsd4_encode_stateid(struct nfsd4_compoundres *resp, stateid_t *sid)
+{
+	ENCODE_HEAD;
+
+	RESERVE_SPACE(sizeof(stateid_t));
+	WRITE32(sid->si_generation);
+	WRITEMEM(&sid->si_opaque, sizeof(stateid_opaque_t));
+	ADJUST_ARGS();
+}
+
 static __be32
 nfsd4_encode_access(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_access *access)
 {
@@ -1978,12 +1989,9 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
 {
 	ENCODE_SEQID_OP_HEAD;
 
-	if (!nfserr) {
-		RESERVE_SPACE(sizeof(stateid_t));
-		WRITE32(close->cl_stateid.si_generation);
-		WRITEMEM(&close->cl_stateid.si_opaque, sizeof(stateid_opaque_t));
-		ADJUST_ARGS();
-	}
+	if (!nfserr)
+		nfsd4_encode_stateid(resp, &close->cl_stateid);
+
 	ENCODE_SEQID_OP_TAIL(close->cl_stateowner);
 	return nfserr;
 }
@@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
 {
 	ENCODE_SEQID_OP_HEAD;
 
-	if (!nfserr) {
-		RESERVE_SPACE(4 + sizeof(stateid_t));
-		WRITE32(lock->lk_resp_stateid.si_generation);
-		WRITEMEM(&lock->lk_resp_stateid.si_opaque, sizeof(stateid_opaque_t));
-		ADJUST_ARGS();
-	} else if (nfserr == nfserr_denied)
+	if (!nfserr)
+		nfsd4_encode_stateid(resp, &lock->lk_resp_stateid);
+	else if (nfserr == nfserr_denied)
 		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
 
 	ENCODE_SEQID_OP_TAIL(lock->lk_replay_owner);
@@ -2108,13 +2113,9 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
 {
 	ENCODE_SEQID_OP_HEAD;
 
-	if (!nfserr) {
-		RESERVE_SPACE(sizeof(stateid_t));
-		WRITE32(locku->lu_stateid.si_generation);
-		WRITEMEM(&locku->lu_stateid.si_opaque, sizeof(stateid_opaque_t));
-		ADJUST_ARGS();
-	}
-				        
+	if (!nfserr)
+		nfsd4_encode_stateid(resp, &locku->lu_stateid);
+
 	ENCODE_SEQID_OP_TAIL(locku->lu_stateowner);
 	return nfserr;
 }
@@ -2142,9 +2143,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 	if (nfserr)
 		goto out;
 
-	RESERVE_SPACE(40 + sizeof(stateid_t));
-	WRITE32(open->op_stateid.si_generation);
-	WRITEMEM(&open->op_stateid.si_opaque, sizeof(stateid_opaque_t));
+	nfsd4_encode_stateid(resp, &open->op_stateid);
+	RESERVE_SPACE(40);
 	WRITECINFO(open->op_cinfo);
 	WRITE32(open->op_rflags);
 	WRITE32(2);
@@ -2157,10 +2157,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 	case NFS4_OPEN_DELEGATE_NONE:
 		break;
 	case NFS4_OPEN_DELEGATE_READ:
-		RESERVE_SPACE(20 + sizeof(stateid_t));
-		WRITE32(open->op_delegate_stateid.si_generation);
-		WRITEMEM(&open->op_delegate_stateid.si_opaque,
-			 sizeof(stateid_opaque_t));
+		nfsd4_encode_stateid(resp, &open->op_delegate_stateid);
+		RESERVE_SPACE(20);
 		WRITE32(open->op_recall);
 
 		/*
@@ -2173,10 +2171,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 		ADJUST_ARGS();
 		break;
 	case NFS4_OPEN_DELEGATE_WRITE:
-		RESERVE_SPACE(32 + sizeof(stateid_t));
-		WRITE32(open->op_delegate_stateid.si_generation);
-		WRITEMEM(&open->op_delegate_stateid.si_opaque,
-			 sizeof(stateid_opaque_t));
+		nfsd4_encode_stateid(resp, &open->op_delegate_stateid);
+		RESERVE_SPACE(32);
 		WRITE32(0);
 
 		/*
@@ -2208,13 +2204,9 @@ static __be32
 nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open_confirm *oc)
 {
 	ENCODE_SEQID_OP_HEAD;
-				        
-	if (!nfserr) {
-		RESERVE_SPACE(sizeof(stateid_t));
-		WRITE32(oc->oc_resp_stateid.si_generation);
-		WRITEMEM(&oc->oc_resp_stateid.si_opaque, sizeof(stateid_opaque_t));
-		ADJUST_ARGS();
-	}
+
+	if (!nfserr)
+		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
 
 	ENCODE_SEQID_OP_TAIL(oc->oc_stateowner);
 	return nfserr;
@@ -2224,13 +2216,9 @@ static __be32
 nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open_downgrade *od)
 {
 	ENCODE_SEQID_OP_HEAD;
-				        
-	if (!nfserr) {
-		RESERVE_SPACE(sizeof(stateid_t));
-		WRITE32(od->od_stateid.si_generation);
-		WRITEMEM(&od->od_stateid.si_opaque, sizeof(stateid_opaque_t));
-		ADJUST_ARGS();
-	}
+
+	if (!nfserr)
+		nfsd4_encode_stateid(resp, &od->od_stateid);
 
 	ENCODE_SEQID_OP_TAIL(od->od_stateowner);
 	return nfserr;
-- 
1.5.6.5

[PATCH 5/7] nfsd: don't declare p in ENCODE_SEQID_OP_HEAD

[Benny Halevy]

After using the encode_stateid helper the "p" pointer declared
by ENCODE_SEQID_OP_HEAD is warned as unused.
In the single site where it is still needed it can be declared
separately using the ENCODE_HEAD macro.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 96da1aa..e102247 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1192,7 +1192,6 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
  * Header routine to setup seqid operation replay cache
  */
 #define ENCODE_SEQID_OP_HEAD					\
-	__be32 *p;						\
 	__be32 *save;						\
 								\
 	save = resp->p;
@@ -2138,6 +2137,7 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_li
 static __be32
 nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open *open)
 {
+	ENCODE_HEAD;
 	ENCODE_SEQID_OP_HEAD;
 
 	if (nfserr)
-- 
1.5.6.5

[PATCH 6/7] nfsd: properly xdr-decode NFS4_OPEN_CLAIM_DELEGATE_CUR stateid

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e102247..47ac498 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -679,7 +679,9 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 		break;
 	case NFS4_OPEN_CLAIM_DELEGATE_CUR:
 		READ_BUF(sizeof(stateid_t) + 4);
-		COPYMEM(&open->op_delegate_stateid, sizeof(stateid_t));
+		READ32(open->op_delegate_stateid.si_generation);
+		COPYMEM(&open->op_delegate_stateid.si_opaque,
+			sizeof(stateid_opaque_t));
 		READ32(open->op_fname.len);
 		READ_BUF(open->op_fname.len);
 		SAVEMEM(open->op_fname.data, open->op_fname.len);
-- 
1.5.6.5

[PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 47ac498..9570b1b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -413,6 +413,18 @@ out_nfserr:
 }
 
 static __be32
+nfsd4_decode_stateid(struct nfsd4_compoundargs *argp, stateid_t *sid)
+{
+	DECODE_HEAD;
+
+	READ_BUF(sizeof(stateid_t));
+	READ32(sid->si_generation);
+	COPYMEM(&sid->si_opaque, sizeof(stateid_opaque_t));
+
+	DECODE_TAIL;
+}
+
+static __be32
 nfsd4_decode_access(struct nfsd4_compoundargs *argp, struct nfsd4_access *access)
 {
 	DECODE_HEAD;
@@ -429,10 +441,9 @@ nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)
 	DECODE_HEAD;
 
 	close->cl_stateowner = NULL;
-	READ_BUF(4 + sizeof(stateid_t));
+	READ_BUF(4);
 	READ32(close->cl_seqid);
-	READ32(close->cl_stateid.si_generation);
-	COPYMEM(&close->cl_stateid.si_opaque, sizeof(stateid_opaque_t));
+	return nfsd4_decode_stateid(argp, &close->cl_stateid);
 
 	DECODE_TAIL;
 }
@@ -493,13 +504,7 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 static inline __be32
 nfsd4_decode_delegreturn(struct nfsd4_compoundargs *argp, struct nfsd4_delegreturn *dr)
 {
-	DECODE_HEAD;
-
-	READ_BUF(sizeof(stateid_t));
-	READ32(dr->dr_stateid.si_generation);
-	COPYMEM(&dr->dr_stateid.si_opaque, sizeof(stateid_opaque_t));
-
-	DECODE_TAIL;
+	return nfsd4_decode_stateid(argp, &dr->dr_stateid);
 }
 
 static inline __be32
@@ -542,20 +547,22 @@ nfsd4_decode_lock(struct nfsd4_compoundargs *argp, struct nfsd4_lock *lock)
 	READ32(lock->lk_is_new);
 
 	if (lock->lk_is_new) {
-		READ_BUF(36);
+		READ_BUF(4);
 		READ32(lock->lk_new_open_seqid);
-		READ32(lock->lk_new_open_stateid.si_generation);
-
-		COPYMEM(&lock->lk_new_open_stateid.si_opaque, sizeof(stateid_opaque_t));
+		status = nfsd4_decode_stateid(argp, &lock->lk_new_open_stateid);
+		if (status)
+			return status;
+		READ_BUF(8 + sizeof(clientid_t));
 		READ32(lock->lk_new_lock_seqid);
 		COPYMEM(&lock->lk_new_clientid, sizeof(clientid_t));
 		READ32(lock->lk_new_owner.len);
 		READ_BUF(lock->lk_new_owner.len);
 		READMEM(lock->lk_new_owner.data, lock->lk_new_owner.len);
 	} else {
-		READ_BUF(20);
-		READ32(lock->lk_old_lock_stateid.si_generation);
-		COPYMEM(&lock->lk_old_lock_stateid.si_opaque, sizeof(stateid_opaque_t));
+		status = nfsd4_decode_stateid(argp, &lock->lk_old_lock_stateid);
+		if (status)
+			return status;
+		READ_BUF(4);
 		READ32(lock->lk_old_lock_seqid);
 	}
 
@@ -587,13 +594,15 @@ nfsd4_decode_locku(struct nfsd4_compoundargs *argp, struct nfsd4_locku *locku)
 	DECODE_HEAD;
 
 	locku->lu_stateowner = NULL;
-	READ_BUF(24 + sizeof(stateid_t));
+	READ_BUF(8);
 	READ32(locku->lu_type);
 	if ((locku->lu_type < NFS4_READ_LT) || (locku->lu_type > NFS4_WRITEW_LT))
 		goto xdr_error;
 	READ32(locku->lu_seqid);
-	READ32(locku->lu_stateid.si_generation);
-	COPYMEM(&locku->lu_stateid.si_opaque, sizeof(stateid_opaque_t));
+	status = nfsd4_decode_stateid(argp, &locku->lu_stateid);
+	if (status)
+		return status;
+	READ_BUF(16);
 	READ64(locku->lu_offset);
 	READ64(locku->lu_length);
 
@@ -678,10 +687,10 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 		READ32(open->op_delegate_type);
 		break;
 	case NFS4_OPEN_CLAIM_DELEGATE_CUR:
-		READ_BUF(sizeof(stateid_t) + 4);
-		READ32(open->op_delegate_stateid.si_generation);
-		COPYMEM(&open->op_delegate_stateid.si_opaque,
-			sizeof(stateid_opaque_t));
+		status = nfsd4_decode_stateid(argp, &open->op_delegate_stateid);
+		if (status)
+			return status;
+		READ_BUF(4);
 		READ32(open->op_fname.len);
 		READ_BUF(open->op_fname.len);
 		SAVEMEM(open->op_fname.data, open->op_fname.len);
@@ -701,9 +710,10 @@ nfsd4_decode_open_confirm(struct nfsd4_compoundargs *argp, struct nfsd4_open_con
 	DECODE_HEAD;
 		    
 	open_conf->oc_stateowner = NULL;
-	READ_BUF(4 + sizeof(stateid_t));
-	READ32(open_conf->oc_req_stateid.si_generation);
-	COPYMEM(&open_conf->oc_req_stateid.si_opaque, sizeof(stateid_opaque_t));
+	status = nfsd4_decode_stateid(argp, &open_conf->oc_req_stateid);
+	if (status)
+		return status;
+	READ_BUF(4);
 	READ32(open_conf->oc_seqid);
 						        
 	DECODE_TAIL;
@@ -715,9 +725,10 @@ nfsd4_decode_open_downgrade(struct nfsd4_compoundargs *argp, struct nfsd4_open_d
 	DECODE_HEAD;
 		    
 	open_down->od_stateowner = NULL;
-	READ_BUF(12 + sizeof(stateid_t));
-	READ32(open_down->od_stateid.si_generation);
-	COPYMEM(&open_down->od_stateid.si_opaque, sizeof(stateid_opaque_t));
+	status = nfsd4_decode_stateid(argp, &open_down->od_stateid);
+	if (status)
+		return status;
+	READ_BUF(12);
 	READ32(open_down->od_seqid);
 	READ32(open_down->od_share_access);
 	READ32(open_down->od_share_deny);
@@ -745,9 +756,10 @@ nfsd4_decode_read(struct nfsd4_compoundargs *argp, struct nfsd4_read *read)
 {
 	DECODE_HEAD;
 
-	READ_BUF(sizeof(stateid_t) + 12);
-	READ32(read->rd_stateid.si_generation);
-	COPYMEM(&read->rd_stateid.si_opaque, sizeof(stateid_opaque_t));
+	status = nfsd4_decode_stateid(argp, &read->rd_stateid);
+	if (status)
+		return status;
+	READ_BUF(12);
 	READ64(read->rd_offset);
 	READ32(read->rd_length);
 
@@ -836,15 +848,13 @@ nfsd4_decode_secinfo(struct nfsd4_compoundargs *argp,
 static __be32
 nfsd4_decode_setattr(struct nfsd4_compoundargs *argp, struct nfsd4_setattr *setattr)
 {
-	DECODE_HEAD;
-
-	READ_BUF(sizeof(stateid_t));
-	READ32(setattr->sa_stateid.si_generation);
-	COPYMEM(&setattr->sa_stateid.si_opaque, sizeof(stateid_opaque_t));
-	if ((status = nfsd4_decode_fattr(argp, setattr->sa_bmval, &setattr->sa_iattr, &setattr->sa_acl)))
-		goto out;
+	__be32 status;
 
-	DECODE_TAIL;
+	status = nfsd4_decode_stateid(argp, &setattr->sa_stateid);
+	if (status)
+		return status;
+	return nfsd4_decode_fattr(argp, setattr->sa_bmval,
+				  &setattr->sa_iattr, &setattr->sa_acl);
 }
 
 static __be32
@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
 	int len;
 	DECODE_HEAD;
 
-	READ_BUF(sizeof(stateid_opaque_t) + 20);
-	READ32(write->wr_stateid.si_generation);
-	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
+	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
+	if (status)
+		return status;
+	READ_BUF(16);
 	READ64(write->wr_offset);
 	READ32(write->wr_stable_how);
 	if (write->wr_stable_how > 2)
-- 
1.5.6.5

Re: [PATCH 3/7] nfsd: fix nfsd4_encode_open buffer space reservation

[J. Bruce Fields]

On Tue, Aug 12, 2008 at 08:44:41PM +0300, Benny Halevy wrote:
> nfsd4_encode_open first reservation is currently for 36 + sizeof(stateid_t)
> while it writes after the stateid a cinfo (20 bytes) and 5 more 4-bytes
> words, for a total of 40 + sizeof(stateid_t).

Thanks, yep.  I wish this was all a little more automated!

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4xdr.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index cfe9d5c..4ec52f9 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2142,7 +2142,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
>  	if (nfserr)
>  		goto out;
>  
> -	RESERVE_SPACE(36 + sizeof(stateid_t));
> +	RESERVE_SPACE(40 + sizeof(stateid_t));
>  	WRITE32(open->op_stateid.si_generation);
>  	WRITEMEM(&open->op_stateid.si_opaque, sizeof(stateid_opaque_t));
>  	WRITECINFO(open->op_cinfo);
> -- 
> 1.5.6.5
> 

Re: [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function

[J. Bruce Fields]

On Tue, Aug 12, 2008 at 08:45:07PM +0300, Benny Halevy wrote:
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4xdr.c |   76 ++++++++++++++++++++++------------------------------
>  1 files changed, 32 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 4ec52f9..96da1aa 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
...
> @@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
>  {
>  	ENCODE_SEQID_OP_HEAD;
>  
> -	if (!nfserr) {
> -		RESERVE_SPACE(4 + sizeof(stateid_t));

Hm, so that was a little over before (a harmless error, but better to
have it fixed).

--b.

> -		WRITE32(lock->lk_resp_stateid.si_generation);
> -		WRITEMEM(&lock->lk_resp_stateid.si_opaque, sizeof(stateid_opaque_t));
> -		ADJUST_ARGS();

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[J. Bruce Fields]

On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
>  1 files changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 47ac498..9570b1b 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
>  static __be32
> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
>  	int len;
>  	DECODE_HEAD;
>  
> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
> -	READ32(write->wr_stateid.si_generation);
> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
> +	if (status)
> +		return status;
> +	READ_BUF(16);

How did that 20 become a 16?

OK, I guess this is another case of a preexisting arithmetic error.
Anywhere that error would have had immediate consequences, but here
thanks to the hand-coded write-data decoding, the incorrect argp->p
isn't used except to check that there's space for the incoming data.
And previously that check was more pessimistic than necessary.

--b.

>  	READ64(write->wr_offset);
>  	READ32(write->wr_stable_how);
>  	if (write->wr_stable_how > 2)
> -- 
> 1.5.6.5
> 

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

[J. Bruce Fields]

On Tue, Aug 12, 2008 at 08:40:28PM +0300, Benny Halevy wrote:
> 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

Thanks, all applied to

	git://linux-nfs.org/~bfields/linux.git for-2.6.28

--b.

Re: [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function

[Benny Halevy]

On Aug. 12, 2008, 21:39 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Tue, Aug 12, 2008 at 08:45:07PM +0300, Benny Halevy wrote:
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/nfs4xdr.c |   76 ++++++++++++++++++++++------------------------------
>>  1 files changed, 32 insertions(+), 44 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 4ec52f9..96da1aa 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
> ...
>> @@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
>>  {
>>  	ENCODE_SEQID_OP_HEAD;
>>  
>> -	if (!nfserr) {
>> -		RESERVE_SPACE(4 + sizeof(stateid_t));
> 
> Hm, so that was a little over before (a harmless error, but better to
> have it fixed).

True.  I can add a preliminary patch fixing that.

Benny

> 
> --b.
> 
>> -		WRITE32(lock->lk_resp_stateid.si_generation);
>> -		WRITEMEM(&lock->lk_resp_stateid.si_opaque, sizeof(stateid_opaque_t));
>> -		ADJUST_ARGS();

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Benny Halevy]

On Aug. 12, 2008, 22:04 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 55 insertions(+), 44 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 47ac498..9570b1b 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>>  static __be32
>> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
>>  	int len;
>>  	DECODE_HEAD;
>>  
>> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
>> -	READ32(write->wr_stateid.si_generation);
>> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
>> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
>> +	if (status)
>> +		return status;
>> +	READ_BUF(16);
> 
> How did that 20 become a 16?

It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 ==
sizeof(stateid_t) + 16.

> 
> OK, I guess this is another case of a preexisting arithmetic error.
> Anywhere that error would have had immediate consequences, but here
> thanks to the hand-coded write-data decoding, the incorrect argp->p
> isn't used except to check that there's space for the incoming data.
> And previously that check was more pessimistic than necessary.

Oddly enough, the hand coded calculation was fine, as shown above...
Note that the remaining 16 bytes correspond to one READ64
and two READ32s.

Benny

> 
> --b.
> 
>>  	READ64(write->wr_offset);
>>  	READ32(write->wr_stable_how);
>>  	if (write->wr_stable_how > 2)
>> -- 
>> 1.5.6.5
>>

Re: [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function

[J. Bruce Fields]

On Wed, Aug 13, 2008 at 10:27:37AM +0300, Benny Halevy wrote:
> On Aug. 12, 2008, 21:39 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Tue, Aug 12, 2008 at 08:45:07PM +0300, Benny Halevy wrote:
> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >> ---
> >>  fs/nfsd/nfs4xdr.c |   76 ++++++++++++++++++++++------------------------------
> >>  1 files changed, 32 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index 4ec52f9..96da1aa 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> > ...
> >> @@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
> >>  {
> >>  	ENCODE_SEQID_OP_HEAD;
> >>  
> >> -	if (!nfserr) {
> >> -		RESERVE_SPACE(4 + sizeof(stateid_t));
> > 
> > Hm, so that was a little over before (a harmless error, but better to
> > have it fixed).
> 
> True.  I can add a preliminary patch fixing that.

Eh, it's minor.  (In future, yes, I'd prefer things split out that way,
but I don't think it's worth redoing for something this small.)

--b.

> 
> Benny
> 
> > 
> > --b.
> > 
> >> -		WRITE32(lock->lk_resp_stateid.si_generation);
> >> -		WRITEMEM(&lock->lk_resp_stateid.si_opaque, sizeof(stateid_opaque_t));
> >> -		ADJUST_ARGS();

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[J. Bruce Fields]

On Wed, Aug 13, 2008 at 10:31:54AM +0300, Benny Halevy wrote:
> On Aug. 12, 2008, 22:04 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >> ---
> >>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
> >>  1 files changed, 55 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index 47ac498..9570b1b 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> >>  static __be32
> >> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
> >>  	int len;
> >>  	DECODE_HEAD;
> >>  
> >> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
> >> -	READ32(write->wr_stateid.si_generation);
> >> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
> >> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
> >> +	if (status)
> >> +		return status;
> >> +	READ_BUF(16);
> > 
> > How did that 20 become a 16?
> 
> It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 ==
> sizeof(stateid_t) + 16.

Whoops!  Even now I still fall into the stateid_opaque_t/stateid_t trap!

> > OK, I guess this is another case of a preexisting arithmetic error.
> > Anywhere that error would have had immediate consequences, but here
> > thanks to the hand-coded write-data decoding, the incorrect argp->p
> > isn't used except to check that there's space for the incoming data.
> > And previously that check was more pessimistic than necessary.
> 
> Oddly enough, the hand coded calculation was fine, as shown above...
> Note that the remaining 16 bytes correspond to one READ64
> and two READ32s.

Got it.

--b.

> 
> Benny
> 
> > 
> > --b.
> > 
> >>  	READ64(write->wr_offset);
> >>  	READ32(write->wr_stable_how);
> >>  	if (write->wr_stable_how > 2)
> >> -- 
> >> 1.5.6.5
> >>

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Trond Myklebust]

On Wed, 2008-08-13 at 11:03 -0400, J. Bruce Fields wrote:
> On Wed, Aug 13, 2008 at 10:31:54AM +0300, Benny Halevy wrote:
> > On Aug. 12, 2008, 22:04 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
> > >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> > >> ---
> > >>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
> > >>  1 files changed, 55 insertions(+), 44 deletions(-)
> > >>
> > >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > >> index 47ac498..9570b1b 100644
> > >> --- a/fs/nfsd/nfs4xdr.c
> > >> +++ b/fs/nfsd/nfs4xdr.c
> > >>  static __be32
> > >> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
> > >>  	int len;
> > >>  	DECODE_HEAD;
> > >>  
> > >> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
> > >> -	READ32(write->wr_stateid.si_generation);
> > >> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
> > >> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
> > >> +	if (status)
> > >> +		return status;
> > >> +	READ_BUF(16);
> > > 
> > > How did that 20 become a 16?
> > 
> > It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 ==
> > sizeof(stateid_t) + 16.
> 
> Whoops!  Even now I still fall into the stateid_opaque_t/stateid_t trap!

Which is a good reason for ditching the entire confusing typedef, and
replacing it with a packed structure instead:

struct stateid {
	__be32 generation;
	char opaque[12];
} __attribute__((packed));

Trond

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[J. Bruce Fields]

On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
> On Wed, 2008-08-13 at 11:03 -0400, J. Bruce Fields wrote:
> > On Wed, Aug 13, 2008 at 10:31:54AM +0300, Benny Halevy wrote:
> > > On Aug. 12, 2008, 22:04 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
> > > >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> > > >> ---
> > > >>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
> > > >>  1 files changed, 55 insertions(+), 44 deletions(-)
> > > >>
> > > >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > >> index 47ac498..9570b1b 100644
> > > >> --- a/fs/nfsd/nfs4xdr.c
> > > >> +++ b/fs/nfsd/nfs4xdr.c
> > > >>  static __be32
> > > >> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
> > > >>  	int len;
> > > >>  	DECODE_HEAD;
> > > >>  
> > > >> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
> > > >> -	READ32(write->wr_stateid.si_generation);
> > > >> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
> > > >> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
> > > >> +	if (status)
> > > >> +		return status;
> > > >> +	READ_BUF(16);
> > > > 
> > > > How did that 20 become a 16?
> > > 
> > > It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 ==
> > > sizeof(stateid_t) + 16.
> > 
> > Whoops!  Even now I still fall into the stateid_opaque_t/stateid_t trap!
> 
> Which is a good reason for ditching the entire confusing typedef, and
> replacing it with a packed structure instead:
> 
> struct stateid {
> 	__be32 generation;
> 	char opaque[12];
> } __attribute__((packed));

So without the ((packed)), all arrays get aligned to 8-byte boundaries
on 64-bit machines?  (What do I need to read to catch up here?)

Anyway, yes, I'd rather get rid of stateid_opaque_t.

--b.

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Trond Myklebust]

On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
> On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
> > Which is a good reason for ditching the entire confusing typedef, and
> > replacing it with a packed structure instead:
> > 
> > struct stateid {
> > 	__be32 generation;
> > 	char opaque[12];
> > } __attribute__((packed));
> 
> So without the ((packed)), all arrays get aligned to 8-byte boundaries
> on 64-bit machines?  (What do I need to read to catch up here?)

A quick google showed up:

	http://sig9.com/articles/gcc-packed-structures

In any case, yes, the idea behind the packed attribute is to turn off
the field alignment.

Cheers
  Trond

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[J. Bruce Fields]

On Wed, Aug 13, 2008 at 02:59:52PM -0400, Trond Myklebust wrote:
> On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
> > On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
> > > Which is a good reason for ditching the entire confusing typedef, and
> > > replacing it with a packed structure instead:
> > > 
> > > struct stateid {
> > > 	__be32 generation;
> > > 	char opaque[12];
> > > } __attribute__((packed));
> > 
> > So without the ((packed)), all arrays get aligned to 8-byte boundaries
> > on 64-bit machines?  (What do I need to read to catch up here?)
> 
> A quick google showed up:
> 
> 	http://sig9.com/articles/gcc-packed-structures
> 
> In any case, yes, the idea behind the packed attribute is to turn off
> the field alignment.

Yeah, I was more curious about how to decide when it's necessary.  (Why
didn't we need it before?  Is an embedded struct always aligned as if
the fields of the embedded struct were declared directly in the
containing struct?  Or should we really just be using the packed
attribute *any* time we depend on that alignment, even if it seems
obvious the compiler wouldn't need to add padding?)

--b.

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Trond Myklebust]

On Wed, 2008-08-13 at 15:11 -0400, J. Bruce Fields wrote:
> On Wed, Aug 13, 2008 at 02:59:52PM -0400, Trond Myklebust wrote:
> > On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
> > > On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
> > > > Which is a good reason for ditching the entire confusing typedef, and
> > > > replacing it with a packed structure instead:
> > > > 
> > > > struct stateid {
> > > > 	__be32 generation;
> > > > 	char opaque[12];
> > > > } __attribute__((packed));
> > > 
> > > So without the ((packed)), all arrays get aligned to 8-byte boundaries
> > > on 64-bit machines?  (What do I need to read to catch up here?)
> > 
> > A quick google showed up:
> > 
> > 	http://sig9.com/articles/gcc-packed-structures
> > 
> > In any case, yes, the idea behind the packed attribute is to turn off
> > the field alignment.
> 
> Yeah, I was more curious about how to decide when it's necessary.  (Why
> didn't we need it before?  Is an embedded struct always aligned as if
> the fields of the embedded struct were declared directly in the
> containing struct?  Or should we really just be using the packed
> attribute *any* time we depend on that alignment, even if it seems
> obvious the compiler wouldn't need to add padding?)

The advantage of having it packed like the above is that you can still
use WRITEMEM() to write out the whole structure in one fell swoop. If
you don't specify 'packed', then the C standard allows the compiler to
add padding between the fields in order align them. I'm not sure that
compilers will usually do that for a 'char[]' field, but they will
definitely for the integer types.

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[J. Bruce Fields]

On Wed, Aug 13, 2008 at 03:35:53PM -0400, Trond Myklebust wrote:
> On Wed, 2008-08-13 at 15:11 -0400, J. Bruce Fields wrote:
> > On Wed, Aug 13, 2008 at 02:59:52PM -0400, Trond Myklebust wrote:
> > > On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
> > > > On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
> > > > > Which is a good reason for ditching the entire confusing typedef, and
> > > > > replacing it with a packed structure instead:
> > > > > 
> > > > > struct stateid {
> > > > > 	__be32 generation;
> > > > > 	char opaque[12];
> > > > > } __attribute__((packed));
> > > > 
> > > > So without the ((packed)), all arrays get aligned to 8-byte boundaries
> > > > on 64-bit machines?  (What do I need to read to catch up here?)
> > > 
> > > A quick google showed up:
> > > 
> > > 	http://sig9.com/articles/gcc-packed-structures
> > > 
> > > In any case, yes, the idea behind the packed attribute is to turn off
> > > the field alignment.
> > 
> > Yeah, I was more curious about how to decide when it's necessary.  (Why
> > didn't we need it before?  Is an embedded struct always aligned as if
> > the fields of the embedded struct were declared directly in the
> > containing struct?  Or should we really just be using the packed
> > attribute *any* time we depend on that alignment, even if it seems
> > obvious the compiler wouldn't need to add padding?)
> 
> The advantage of having it packed like the above is that you can still
> use WRITEMEM() to write out the whole structure in one fell swoop.

Right, I understand.  But the code has been doing exactly that (a
WRITEMEM of the whole thing) since the beginning, so I wondered if there
was some reason you thought the switch to the extra char opaque[12] in
particular was something that was likely to trigger the addition of
padding.

Sounds instead like your policy would be just to declare any struct
"packed" if we might depend on the absence of padding, without making
any assumptions about what compilers might do.  Which is fine.

--b.

> If you don't specify 'packed', then the C standard allows the compiler
> to add padding between the fields in order align them. I'm not sure
> that compilers will usually do that for a 'char[]' field, but they
> will definitely for the integer types.

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Chuck Lever]

On Wed, Aug 13, 2008 at 4:17 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Aug 13, 2008 at 03:35:53PM -0400, Trond Myklebust wrote:
>> On Wed, 2008-08-13 at 15:11 -0400, J. Bruce Fields wrote:
>> > On Wed, Aug 13, 2008 at 02:59:52PM -0400, Trond Myklebust wrote:
>> > > On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
>> > > > On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
>> > > > > Which is a good reason for ditching the entire confusing typedef, and
>> > > > > replacing it with a packed structure instead:
>> > > > >
>> > > > > struct stateid {
>> > > > >       __be32 generation;
>> > > > >       char opaque[12];
>> > > > > } __attribute__((packed));
>> > > >
>> > > > So without the ((packed)), all arrays get aligned to 8-byte boundaries
>> > > > on 64-bit machines?  (What do I need to read to catch up here?)
>> > >
>> > > A quick google showed up:
>> > >
>> > >   http://sig9.com/articles/gcc-packed-structures
>> > >
>> > > In any case, yes, the idea behind the packed attribute is to turn off
>> > > the field alignment.
>> >
>> > Yeah, I was more curious about how to decide when it's necessary.  (Why
>> > didn't we need it before?  Is an embedded struct always aligned as if
>> > the fields of the embedded struct were declared directly in the
>> > containing struct?  Or should we really just be using the packed
>> > attribute *any* time we depend on that alignment, even if it seems
>> > obvious the compiler wouldn't need to add padding?)
>>
>> The advantage of having it packed like the above is that you can still
>> use WRITEMEM() to write out the whole structure in one fell swoop.
>
> Right, I understand.  But the code has been doing exactly that (a
> WRITEMEM of the whole thing) since the beginning, so I wondered if there
> was some reason you thought the switch to the extra char opaque[12] in
> particular was something that was likely to trigger the addition of
> padding.
>
> Sounds instead like your policy would be just to declare any struct
> "packed" if we might depend on the absence of padding, without making
> any assumptions about what compilers might do.  Which is fine.
>
> --b.
>
>> If you don't specify 'packed', then the C standard allows the compiler
>> to add padding between the fields in order align them. I'm not sure
>> that compilers will usually do that for a 'char[]' field, but they
>> will definitely for the integer types.

If we go down this route, I suggest we add extra pre-processor
checking to ensure that the sizeof() the structure is exactly what we
expect.  This would catch compiler bugs, platform-specific structure
packing behaviors we didn't anticipate, and incorrect human
assumptions.  An added bonus might be using sizeof() such a structure
to generate the maximum buffer size macros automatically.

I'm a little leary of using attribute(packed) structures, though.

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

Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Benny Halevy]

On Aug. 13, 2008, 23:17 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Wed, Aug 13, 2008 at 03:35:53PM -0400, Trond Myklebust wrote:
>> On Wed, 2008-08-13 at 15:11 -0400, J. Bruce Fields wrote:
>>> On Wed, Aug 13, 2008 at 02:59:52PM -0400, Trond Myklebust wrote:
>>>> On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
>>>>> On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
>>>>>> Which is a good reason for ditching the entire confusing typedef, and
>>>>>> replacing it with a packed structure instead:
>>>>>>
>>>>>> struct stateid {
>>>>>> 	__be32 generation;
>>>>>> 	char opaque[12];
>>>>>> } __attribute__((packed));
>>>>> So without the ((packed)), all arrays get aligned to 8-byte boundaries
>>>>> on 64-bit machines?  (What do I need to read to catch up here?)
>>>> A quick google showed up:
>>>>
>>>> 	http://sig9.com/articles/gcc-packed-structures
>>>>
>>>> In any case, yes, the idea behind the packed attribute is to turn off
>>>> the field alignment.
>>> Yeah, I was more curious about how to decide when it's necessary.  (Why
>>> didn't we need it before?  Is an embedded struct always aligned as if
>>> the fields of the embedded struct were declared directly in the
>>> containing struct?  Or should we really just be using the packed
>>> attribute *any* time we depend on that alignment, even if it seems
>>> obvious the compiler wouldn't need to add padding?)
>> The advantage of having it packed like the above is that you can still
>> use WRITEMEM() to write out the whole structure in one fell swoop.

True, just you need to keep generation in network order in memory
(hence Trond defined it as __be32...)

> 
> Right, I understand.  But the code has been doing exactly that (a
> WRITEMEM of the whole thing) since the beginning, so I wondered if there
> was some reason you thought the switch to the extra char opaque[12] in
> particular was something that was likely to trigger the addition of
> padding.
> 
> Sounds instead like your policy would be just to declare any struct
> "packed" if we might depend on the absence of padding, without making
> any assumptions about what compilers might do.  Which is fine.

Agreed.  If you care about how the structure is laid out in memory then
pack it.

> 
> --b.
> 
>> If you don't specify 'packed', then the C standard allows the compiler
>> to add padding between the fields in order align them. I'm not sure
>> that compilers will usually do that for a 'char[]' field, but they
>> will definitely for the integer types.

gcc (/c90)  seems to align the field based on its size and arrays based
on their element size.

Benny

Re: [pnfs] [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Boaz Harrosh]

Trond Myklebust wrote:
> On Wed, 2008-08-13 at 11:03 -0400, J. Bruce Fields wrote:
>> On Wed, Aug 13, 2008 at 10:31:54AM +0300, Benny Halevy wrote:
>>> On Aug. 12, 2008, 22:04 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>>> On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
>>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>>>> ---
>>>>>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
>>>>>  1 files changed, 55 insertions(+), 44 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>>>> index 47ac498..9570b1b 100644
>>>>> --- a/fs/nfsd/nfs4xdr.c
>>>>> +++ b/fs/nfsd/nfs4xdr.c
>>>>>  static __be32
>>>>> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
>>>>>  	int len;
>>>>>  	DECODE_HEAD;
>>>>>  
>>>>> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
>>>>> -	READ32(write->wr_stateid.si_generation);
>>>>> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
>>>>> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
>>>>> +	if (status)
>>>>> +		return status;
>>>>> +	READ_BUF(16);
>>>> How did that 20 become a 16?
>>> It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 ==
>>> sizeof(stateid_t) + 16.
>> Whoops!  Even now I still fall into the stateid_opaque_t/stateid_t trap!
> 
> Which is a good reason for ditching the entire confusing typedef, and
> replacing it with a packed structure instead:
> 
> struct stateid {
> 	__be32 generation;
> 	char opaque[12];
> } __attribute__((packed));
> 
> Trond
> 

Please use the "__packed" Kernel macro for better compilers support.

And Yes I would suggest using "__packed" where ever a struct is defined
for on-the-wire. Even just for indication to fellow programmers that this 
is going on the wire as is.

Boaz

Re: [pnfs] [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[J. Bruce Fields]

On Sun, Aug 17, 2008 at 03:02:01PM +0300, Boaz Harrosh wrote:
> Trond Myklebust wrote:
> > On Wed, 2008-08-13 at 11:03 -0400, J. Bruce Fields wrote:
> >> On Wed, Aug 13, 2008 at 10:31:54AM +0300, Benny Halevy wrote:
> >>> On Aug. 12, 2008, 22:04 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >>>> On Tue, Aug 12, 2008 at 08:46:18PM +0300, Benny Halevy wrote:
> >>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >>>>> ---
> >>>>>  fs/nfsd/nfs4xdr.c |   99 +++++++++++++++++++++++++++++-----------------------
> >>>>>  1 files changed, 55 insertions(+), 44 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >>>>> index 47ac498..9570b1b 100644
> >>>>> --- a/fs/nfsd/nfs4xdr.c
> >>>>> +++ b/fs/nfsd/nfs4xdr.c
> >>>>>  static __be32
> >>>>> @@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
> >>>>>  	int len;
> >>>>>  	DECODE_HEAD;
> >>>>>  
> >>>>> -	READ_BUF(sizeof(stateid_opaque_t) + 20);
> >>>>> -	READ32(write->wr_stateid.si_generation);
> >>>>> -	COPYMEM(&write->wr_stateid.si_opaque, sizeof(stateid_opaque_t));
> >>>>> +	status = nfsd4_decode_stateid(argp, &write->wr_stateid);
> >>>>> +	if (status)
> >>>>> +		return status;
> >>>>> +	READ_BUF(16);
> >>>> How did that 20 become a 16?
> >>> It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 ==
> >>> sizeof(stateid_t) + 16.
> >> Whoops!  Even now I still fall into the stateid_opaque_t/stateid_t trap!
> > 
> > Which is a good reason for ditching the entire confusing typedef, and
> > replacing it with a packed structure instead:
> > 
> > struct stateid {
> > 	__be32 generation;
> > 	char opaque[12];
> > } __attribute__((packed));
> > 
> > Trond
> > 
> 
> Please use the "__packed" Kernel macro for better compilers support.
> 
> And Yes I would suggest using "__packed" where ever a struct is defined
> for on-the-wire. Even just for indication to fellow programmers that this 
> is going on the wire as is.

OK.  This all sounds reasonable, but I can't volunteer at the moment,
so: patches welcomed.

--b.