| Message ID | 20201209144801.700778-16-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for the NFSv4.2 READ_PLUS operation | expand |
Hey- > On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > Ensure that we encode the data payload + padding, and that we truncate > the preallocated buffer to the actual read size. Did you intend to merge 15/16 and 16/16 through your tree? Can the patch descriptions say a little more about why these changes are necessary? If they fix a misbehavior, describe the problem. > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfsd/nfs4xdr.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 833a2c64dfe8..26f6e277101d 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); > if (nfserr) > return nfserr; > + xdr_truncate_encode(xdr, starting_len + 16 + xdr_align_size(*maxcount)); > > tmp = htonl(NFS4_CONTENT_DATA); > write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); > @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64, 8); > tmp = htonl(*maxcount); > write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp, 4); > + > + tmp = xdr_zero; > + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + *maxcount, &tmp, > + xdr_pad_size(*maxcount)); > return nfs_ok; > } > > -- > 2.29.2 > -- Chuck Lever chucklever@gmail.com
On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote: > Hey- > > > On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote: > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > Ensure that we encode the data payload + padding, and that we > > truncate > > the preallocated buffer to the actual read size. > > Did you intend to merge 15/16 and 16/16 through your tree? No. They can go through the nfsd tree. I included them here because they are necessary in order to pass the xfstests. > Can the patch descriptions say a little more about why these > changes are necessary? If they fix a misbehavior, describe > the problem. It's the same problem and solution that exists in the READ code. nfsd_readv() doesn't necessarily return the same number of bytes that we requested and preallocated buffer space for. So to deal with that, we have to truncate the preallocated buffer. Finally, we have to write zeros into the padding bytes after the read buffer. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfsd/nfs4xdr.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index 833a2c64dfe8..26f6e277101d 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct > > nfsd4_compoundres *resp, > > resp->rqstp->rq_vec, read->rd_vlen, > > maxcount, eof); > > if (nfserr) > > return nfserr; > > + xdr_truncate_encode(xdr, starting_len + 16 + > > xdr_align_size(*maxcount)); > > > > tmp = htonl(NFS4_CONTENT_DATA); > > write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, > > 4); > > @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct > > nfsd4_compoundres *resp, > > write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64, > > 8); > > tmp = htonl(*maxcount); > > write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp, > > 4); > > + > > + tmp = xdr_zero; > > + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + > > *maxcount, &tmp, > > + xdr_pad_size(*maxcount)); > > return nfs_ok; > > }
> On Dec 9, 2020, at 11:39 AM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote: >> Hey- >> >>> On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote: >>> >>> From: Trond Myklebust <trond.myklebust@hammerspace.com> >>> >>> Ensure that we encode the data payload + padding, and that we >>> truncate >>> the preallocated buffer to the actual read size. >> >> Did you intend to merge 15/16 and 16/16 through your tree? > > No. They can go through the nfsd tree. I included them here because > they are necessary in order to pass the xfstests. Would it be OK if they went in 5.11-rc? I've got the initial merge tag prepared already. If they can't wait, let me know. >> Can the patch descriptions say a little more about why these >> changes are necessary? If they fix a misbehavior, describe >> the problem. > > It's the same problem and solution that exists in the READ code. > > nfsd_readv() doesn't necessarily return the same number of bytes that > we requested and preallocated buffer space for. So to deal with that, > we have to truncate the preallocated buffer. Huh. I thought it was doing that already? Oh, that's just for the cases where the server returns an error status. The READ_PLUS encoder incorrectly does not do that truncation for short READs... got it. > Finally, we have to write zeros into the padding bytes after the read > buffer. Right. Then the problem statement is that the server's READ_PLUS XDR encoder isn't padding the read buffer properly. Quibble: perhaps these are two separate issues that each deserve their own patches with Fixes: tags (and if you re-post these, please add a Fixes: tag to 16/16 too). Thanks! >>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> >>> --- >>> fs/nfsd/nfs4xdr.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>> index 833a2c64dfe8..26f6e277101d 100644 >>> --- a/fs/nfsd/nfs4xdr.c >>> +++ b/fs/nfsd/nfs4xdr.c >>> @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct >>> nfsd4_compoundres *resp, >>> resp->rqstp->rq_vec, read->rd_vlen, >>> maxcount, eof); >>> if (nfserr) >>> return nfserr; >>> + xdr_truncate_encode(xdr, starting_len + 16 + >>> xdr_align_size(*maxcount)); >>> >>> tmp = htonl(NFS4_CONTENT_DATA); >>> write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, >>> 4); >>> @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct >>> nfsd4_compoundres *resp, >>> write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64, >>> 8); >>> tmp = htonl(*maxcount); >>> write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp, >>> 4); >>> + >>> + tmp = xdr_zero; >>> + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + >>> *maxcount, &tmp, >>> + xdr_pad_size(*maxcount)); >>> return nfs_ok; >>> } > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com -- Chuck Lever chucklever@gmail.com
On Wed, 2020-12-09 at 11:57 -0500, Chuck Lever wrote: > > > > On Dec 9, 2020, at 11:39 AM, Trond Myklebust < > > trondmy@hammerspace.com> wrote: > > > > On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote: > > > Hey- > > > > > > > On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote: > > > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > Ensure that we encode the data payload + padding, and that we > > > > truncate > > > > the preallocated buffer to the actual read size. > > > > > > Did you intend to merge 15/16 and 16/16 through your tree? > > > > No. They can go through the nfsd tree. I included them here because > > they are necessary in order to pass the xfstests. > > Would it be OK if they went in 5.11-rc? I've got the initial > merge tag prepared already. If they can't wait, let me know. I'm fine with that. > > > > > Can the patch descriptions say a little more about why these > > > changes are necessary? If they fix a misbehavior, describe > > > the problem. > > > > It's the same problem and solution that exists in the READ code. > > > > nfsd_readv() doesn't necessarily return the same number of bytes > > that > > we requested and preallocated buffer space for. So to deal with > > that, > > we have to truncate the preallocated buffer. > > Huh. I thought it was doing that already? Oh, that's just for > the cases where the server returns an error status. The > READ_PLUS encoder incorrectly does not do that truncation for > short READs... got it. > > > > Finally, we have to write zeros into the padding bytes after the > > read > > buffer. > > Right. Then the problem statement is that the server's READ_PLUS > XDR encoder isn't padding the read buffer properly. > > Quibble: perhaps these are two separate issues that each deserve > their own patches with Fixes: tags (and if you re-post these, > please add a Fixes: tag to 16/16 too). I'm not planning on reposting.
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 833a2c64dfe8..26f6e277101d 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); if (nfserr) return nfserr; + xdr_truncate_encode(xdr, starting_len + 16 + xdr_align_size(*maxcount)); tmp = htonl(NFS4_CONTENT_DATA); write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64, 8); tmp = htonl(*maxcount); write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp, 4); + + tmp = xdr_zero; + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + *maxcount, &tmp, + xdr_pad_size(*maxcount)); return nfs_ok; }