| Message ID | 1374598847.12943.0.camel@leira.trondhjem.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 23, 2013 at 7:00 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote: > On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote: >> Trond, >> >> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust >> <Trond.Myklebust@netapp.com> wrote: >> > The attribute length is already calculated in advance. There is no >> > reason why we cannot calculate the bitmap in advance too so that >> > we don't have to play pointer games. >> >> I'm sorry to report that this patch seems to be more than just a cleanup. >> >> I just tested 3.11-rc2 against my FreeBSD server, and with just patch >> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with >> or without Rick's server patch. >> >> Applying this one on top of -rc2 fixes it. > > How about the attached instead of the cleanup? Much better ;) Thanks again for the quick fix! Feel free to add: Tested-by: Andre Heider <a.heider@gmail.com> -- 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
On Jul 23, 2013, at 1:00 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote: >> Trond, >> >> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust >> <Trond.Myklebust@netapp.com> wrote: >>> The attribute length is already calculated in advance. There is no >>> reason why we cannot calculate the bitmap in advance too so that >>> we don't have to play pointer games. >> >> I'm sorry to report that this patch seems to be more than just a cleanup. >> >> I just tested 3.11-rc2 against my FreeBSD server, and with just patch >> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with >> or without Rick's server patch. >> >> Applying this one on top of -rc2 fixes it. > > How about the attached instead of the cleanup? Even with this patch applied, cthon basic tests fail immediately for me. The mkdir to create the test directory fails with EIO. The wire trace shows that the CREATE operation is malformed: the client sends a length of 8 for the 4-octet mode attribute value at the end of the operation. This causes the server to skip over the following GETFH operation -- it thinks the client has sent 3 operations, when the client told it to expect 4 in this compound. Here: 1064 *p++ = cpu_to_be32(bmval_len); 1065 q = p; 1066 /* Skip bitmap entries + attrlen */ 1067 p += bmval_len + 1; You've skipped over the 4-octet attrlen field, but here: 1125 *q++ = htonl(bmval0); 1126 *q++ = htonl(bmval1); 1127 if (bmval_len == 3) 1128 *q++ = htonl(bmval2); 1129 len = (char *)p - (char *)q; 1130 *q = htonl(len); have you failed to take the attrlen field into account when computing the length of the attribute values?
On Tue, 2013-07-23 at 16:14 -0400, Chuck Lever wrote: > On Jul 23, 2013, at 1:00 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > > On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote: > >> Trond, > >> > >> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust > >> <Trond.Myklebust@netapp.com> wrote: > >>> The attribute length is already calculated in advance. There is no > >>> reason why we cannot calculate the bitmap in advance too so that > >>> we don't have to play pointer games. > >> > >> I'm sorry to report that this patch seems to be more than just a cleanup. > >> > >> I just tested 3.11-rc2 against my FreeBSD server, and with just patch > >> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with > >> or without Rick's server patch. > >> > >> Applying this one on top of -rc2 fixes it. > > > > How about the attached instead of the cleanup? > > Even with this patch applied, cthon basic tests fail immediately for me. The mkdir to create the test directory fails with EIO. > > The wire trace shows that the CREATE operation is malformed: the client sends a length of 8 for the 4-octet mode attribute value at the end of the operation. > > This causes the server to skip over the following GETFH operation -- it thinks the client has sent 3 operations, when the client told it to expect 4 in this compound. > > Here: > > 1064 *p++ = cpu_to_be32(bmval_len); > 1065 q = p; > 1066 /* Skip bitmap entries + attrlen */ > 1067 p += bmval_len + 1; > > You've skipped over the 4-octet attrlen field, but here: > > 1125 *q++ = htonl(bmval0); > 1126 *q++ = htonl(bmval1); > 1127 if (bmval_len == 3) > 1128 *q++ = htonl(bmval2); > 1129 len = (char *)p - (char *)q; In the patch I sent out, the above should read (q+1)... > 1130 *q = htonl(len); > > have you failed to take the attrlen field into account when computing the length of the attribute values? > -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Jul 23, 2013, at 5:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > On Tue, 2013-07-23 at 16:14 -0400, Chuck Lever wrote: >> On Jul 23, 2013, at 1:00 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: >> >>> On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote: >>>> Trond, >>>> >>>> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust >>>> <Trond.Myklebust@netapp.com> wrote: >>>>> The attribute length is already calculated in advance. There is no >>>>> reason why we cannot calculate the bitmap in advance too so that >>>>> we don't have to play pointer games. >>>> >>>> I'm sorry to report that this patch seems to be more than just a cleanup. >>>> >>>> I just tested 3.11-rc2 against my FreeBSD server, and with just patch >>>> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with >>>> or without Rick's server patch. >>>> >>>> Applying this one on top of -rc2 fixes it. >>> >>> How about the attached instead of the cleanup? >> >> Even with this patch applied, cthon basic tests fail immediately for me. The mkdir to create the test directory fails with EIO. >> >> The wire trace shows that the CREATE operation is malformed: the client sends a length of 8 for the 4-octet mode attribute value at the end of the operation. >> >> This causes the server to skip over the following GETFH operation -- it thinks the client has sent 3 operations, when the client told it to expect 4 in this compound. >> >> Here: >> >> 1064 *p++ = cpu_to_be32(bmval_len); >> 1065 q = p; >> 1066 /* Skip bitmap entries + attrlen */ >> 1067 p += bmval_len + 1; >> >> You've skipped over the 4-octet attrlen field, but here: >> >> 1125 *q++ = htonl(bmval0); >> 1126 *q++ = htonl(bmval1); >> 1127 if (bmval_len == 3) >> 1128 *q++ = htonl(bmval2); >> 1129 len = (char *)p - (char *)q; > > In the patch I sent out, the above should read (q+1)... D'OH! > >> 1130 *q = htonl(len); >> >> have you failed to take the attrlen field into account when computing the length of the attribute values? >>
From bd00a23bba91f54ab45e8788a8306733d8ca3062 Mon Sep 17 00:00:00 2001 From: Trond Myklebust <Trond.Myklebust@netapp.com> Date: Tue, 23 Jul 2013 12:53:39 -0400 Subject: [PATCH] NFSv4: Fix brainfart in attribute length calculation The calculation of the attribute length was 4 bytes off. Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> --- fs/nfs/nfs4xdr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index c74d616..3850b01 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -1118,11 +1118,11 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, len, ((char *)p - (char *)q) + 4); BUG(); } - len = (char *)p - (char *)q - (bmval_len << 2); *q++ = htonl(bmval0); *q++ = htonl(bmval1); if (bmval_len == 3) *q++ = htonl(bmval2); + len = (char *)p - (char *)(q + 1); *q = htonl(len); /* out: */ -- 1.8.3.1