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
Signed-off-by: Benny Halevy <[email protected]>
---
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
Signed-off-by: Benny Halevy <[email protected]>
---
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
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
On Aug. 11, 2008, 18:58 +0300, "J. Bruce Fields" <[email protected]> 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
> [email protected]
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
Hi Benny-
On Mon, Aug 11, 2008 at 12:11 PM, Benny Halevy <[email protected]> wrote:
> On Aug. 11, 2008, 18:58 +0300, "J. Bruce Fields" <[email protected]> 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
>> [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
"Officer. Ma'am. Squeaker."
-- Mr. Incredible
On Mon, Aug 11, 2008 at 07:11:40PM +0300, Benny Halevy wrote:
> On Aug. 11, 2008, 18:58 +0300, "J. Bruce Fields" <[email protected]> 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
> > [email protected]
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
On Aug. 11, 2008, 19:27 +0300, "J. Bruce Fields" <[email protected]> wrote:
> On Mon, Aug 11, 2008 at 07:11:40PM +0300, Benny Halevy wrote:
>> On Aug. 11, 2008, 18:58 +0300, "J. Bruce Fields" <[email protected]> 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
>>> [email protected]
>>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs