| Message ID | 20220901183341.1543827-2-anna@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] NFSD: Simplify READ_PLUS | expand |
Good to see this! I'll study it over the next few days. > On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > Change the implementation to return a single DATA segment covering the > requested read range. The discussion in your cover letter should go in this patch description. A good patch description explains "why"; the diff below already explains "what". I harp on that because the patch description is important information that I often consult when conducting archaeology during troubleshooting. "Why the f... did we do that?" > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > fs/nfsd/nfs4xdr.c | 122 ++++++++-------------------------------------- > 1 file changed, 20 insertions(+), 102 deletions(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 1e9690a061ec..adbff7737c14 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -4731,79 +4731,30 @@ nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr, > > static __be32 > nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > - struct nfsd4_read *read, > - unsigned long *maxcount, u32 *eof, > - loff_t *pos) > + struct nfsd4_read *read) > { > + unsigned long maxcount; > struct xdr_stream *xdr = resp->xdr; > struct file *file = read->rd_nf->nf_file; > - int starting_len = xdr->buf->len; > - loff_t hole_pos; > __be32 nfserr; > - __be32 *p, tmp; > - __be64 tmp64; > - > - hole_pos = pos ? *pos : vfs_llseek(file, read->rd_offset, SEEK_HOLE); > - if (hole_pos > read->rd_offset) > - *maxcount = min_t(unsigned long, *maxcount, hole_pos - read->rd_offset); > - *maxcount = min_t(unsigned long, *maxcount, (xdr->buf->buflen - xdr->buf->len)); > + __be32 *p; > > /* Content type, offset, byte count */ > p = xdr_reserve_space(xdr, 4 + 8 + 4); > if (!p) > return nfserr_resource; > > - read->rd_vlen = xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, *maxcount); > - if (read->rd_vlen < 0) > - return nfserr_resource; > + maxcount = min_t(unsigned long, read->rd_length, > + (xdr->buf->buflen - xdr->buf->len)); > > - nfserr = nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset, > - resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); > + nfserr = nfsd4_encode_readv(resp, read, file, maxcount); > 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); > - tmp64 = cpu_to_be64(read->rd_offset); > - 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; > -} > - > -static __be32 > -nfsd4_encode_read_plus_hole(struct nfsd4_compoundres *resp, > - struct nfsd4_read *read, > - unsigned long *maxcount, u32 *eof) > -{ > - struct file *file = read->rd_nf->nf_file; > - loff_t data_pos = vfs_llseek(file, read->rd_offset, SEEK_DATA); > - loff_t f_size = i_size_read(file_inode(file)); > - unsigned long count; > - __be32 *p; > - > - if (data_pos == -ENXIO) > - data_pos = f_size; > - else if (data_pos <= read->rd_offset || (data_pos < f_size && data_pos % PAGE_SIZE)) > - return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size); > - count = data_pos - read->rd_offset; > - > - /* Content type, offset, byte count */ > - p = xdr_reserve_space(resp->xdr, 4 + 8 + 8); > - if (!p) > - return nfserr_resource; > - > - *p++ = htonl(NFS4_CONTENT_HOLE); > + *p++ = cpu_to_be32(NFS4_CONTENT_DATA); > p = xdr_encode_hyper(p, read->rd_offset); > - p = xdr_encode_hyper(p, count); > + *p = cpu_to_be32(read->rd_length); > > - *eof = (read->rd_offset + count) >= f_size; > - *maxcount = min_t(unsigned long, count, *maxcount); > return nfs_ok; > } > > @@ -4811,20 +4762,14 @@ static __be32 > nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, > struct nfsd4_read *read) > { > - unsigned long maxcount, count; > struct xdr_stream *xdr = resp->xdr; > - struct file *file; > + struct file *file = read->rd_nf->nf_file; > int starting_len = xdr->buf->len; > - int last_segment = xdr->buf->len; > int segments = 0; > - __be32 *p, tmp; > - bool is_data; > - loff_t pos; > - u32 eof; > + __be32 *p; > > if (nfserr) > return nfserr; > - file = read->rd_nf->nf_file; > > /* eof flag, segment count */ > p = xdr_reserve_space(xdr, 4 + 4); > @@ -4832,48 +4777,21 @@ nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, > return nfserr_resource; > xdr_commit_encode(xdr); > > - maxcount = min_t(unsigned long, read->rd_length, > - (xdr->buf->buflen - xdr->buf->len)); > - count = maxcount; > - > - eof = read->rd_offset >= i_size_read(file_inode(file)); > - if (eof) > + read->rd_eof = read->rd_offset >= i_size_read(file_inode(file)); > + if (read->rd_eof) > goto out; > > - pos = vfs_llseek(file, read->rd_offset, SEEK_HOLE); > - is_data = pos > read->rd_offset; > - > - while (count > 0 && !eof) { > - maxcount = count; > - if (is_data) > - nfserr = nfsd4_encode_read_plus_data(resp, read, &maxcount, &eof, > - segments == 0 ? &pos : NULL); > - else > - nfserr = nfsd4_encode_read_plus_hole(resp, read, &maxcount, &eof); > - if (nfserr) > - goto out; > - count -= maxcount; > - read->rd_offset += maxcount; > - is_data = !is_data; > - last_segment = xdr->buf->len; > - segments++; > - } > - > -out: > - if (nfserr && segments == 0) > + nfserr = nfsd4_encode_read_plus_data(resp, read); > + if (nfserr) { > xdr_truncate_encode(xdr, starting_len); > - else { > - if (nfserr) { > - xdr_truncate_encode(xdr, last_segment); > - nfserr = nfs_ok; > - eof = 0; > - } > - tmp = htonl(eof); > - write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); > - tmp = htonl(segments); > - write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4); > + return nfserr; > } > > + segments++; > + > +out: > + p = xdr_encode_bool(p, read->rd_eof); > + *p = cpu_to_be32(segments); > return nfserr; > } > > -- > 2.37.2 > -- Chuck Lever
On Thu, Sep 1, 2022 at 3:05 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > Good to see this! I'll study it over the next few days. > > > > On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: > > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > Change the implementation to return a single DATA segment covering the > > requested read range. > > The discussion in your cover letter should go in this patch > description. A good patch description explains "why"; the diff > below already explains "what". > > I harp on that because the patch description is important > information that I often consult when conducting archaeology > during troubleshooting. "Why the f... did we do that?" Makes sense! Do you want me to resubmit now as a v2 with some of this moved over to the patch description? Anna > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > > fs/nfsd/nfs4xdr.c | 122 ++++++++-------------------------------------- > > 1 file changed, 20 insertions(+), 102 deletions(-) > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index 1e9690a061ec..adbff7737c14 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -4731,79 +4731,30 @@ nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr, > > > > static __be32 > > nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > > - struct nfsd4_read *read, > > - unsigned long *maxcount, u32 *eof, > > - loff_t *pos) > > + struct nfsd4_read *read) > > { > > + unsigned long maxcount; > > struct xdr_stream *xdr = resp->xdr; > > struct file *file = read->rd_nf->nf_file; > > - int starting_len = xdr->buf->len; > > - loff_t hole_pos; > > __be32 nfserr; > > - __be32 *p, tmp; > > - __be64 tmp64; > > - > > - hole_pos = pos ? *pos : vfs_llseek(file, read->rd_offset, SEEK_HOLE); > > - if (hole_pos > read->rd_offset) > > - *maxcount = min_t(unsigned long, *maxcount, hole_pos - read->rd_offset); > > - *maxcount = min_t(unsigned long, *maxcount, (xdr->buf->buflen - xdr->buf->len)); > > + __be32 *p; > > > > /* Content type, offset, byte count */ > > p = xdr_reserve_space(xdr, 4 + 8 + 4); > > if (!p) > > return nfserr_resource; > > > > - read->rd_vlen = xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, *maxcount); > > - if (read->rd_vlen < 0) > > - return nfserr_resource; > > + maxcount = min_t(unsigned long, read->rd_length, > > + (xdr->buf->buflen - xdr->buf->len)); > > > > - nfserr = nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset, > > - resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); > > + nfserr = nfsd4_encode_readv(resp, read, file, maxcount); > > 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); > > - tmp64 = cpu_to_be64(read->rd_offset); > > - 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; > > -} > > - > > -static __be32 > > -nfsd4_encode_read_plus_hole(struct nfsd4_compoundres *resp, > > - struct nfsd4_read *read, > > - unsigned long *maxcount, u32 *eof) > > -{ > > - struct file *file = read->rd_nf->nf_file; > > - loff_t data_pos = vfs_llseek(file, read->rd_offset, SEEK_DATA); > > - loff_t f_size = i_size_read(file_inode(file)); > > - unsigned long count; > > - __be32 *p; > > - > > - if (data_pos == -ENXIO) > > - data_pos = f_size; > > - else if (data_pos <= read->rd_offset || (data_pos < f_size && data_pos % PAGE_SIZE)) > > - return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size); > > - count = data_pos - read->rd_offset; > > - > > - /* Content type, offset, byte count */ > > - p = xdr_reserve_space(resp->xdr, 4 + 8 + 8); > > - if (!p) > > - return nfserr_resource; > > - > > - *p++ = htonl(NFS4_CONTENT_HOLE); > > + *p++ = cpu_to_be32(NFS4_CONTENT_DATA); > > p = xdr_encode_hyper(p, read->rd_offset); > > - p = xdr_encode_hyper(p, count); > > + *p = cpu_to_be32(read->rd_length); > > > > - *eof = (read->rd_offset + count) >= f_size; > > - *maxcount = min_t(unsigned long, count, *maxcount); > > return nfs_ok; > > } > > > > @@ -4811,20 +4762,14 @@ static __be32 > > nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, > > struct nfsd4_read *read) > > { > > - unsigned long maxcount, count; > > struct xdr_stream *xdr = resp->xdr; > > - struct file *file; > > + struct file *file = read->rd_nf->nf_file; > > int starting_len = xdr->buf->len; > > - int last_segment = xdr->buf->len; > > int segments = 0; > > - __be32 *p, tmp; > > - bool is_data; > > - loff_t pos; > > - u32 eof; > > + __be32 *p; > > > > if (nfserr) > > return nfserr; > > - file = read->rd_nf->nf_file; > > > > /* eof flag, segment count */ > > p = xdr_reserve_space(xdr, 4 + 4); > > @@ -4832,48 +4777,21 @@ nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, > > return nfserr_resource; > > xdr_commit_encode(xdr); > > > > - maxcount = min_t(unsigned long, read->rd_length, > > - (xdr->buf->buflen - xdr->buf->len)); > > - count = maxcount; > > - > > - eof = read->rd_offset >= i_size_read(file_inode(file)); > > - if (eof) > > + read->rd_eof = read->rd_offset >= i_size_read(file_inode(file)); > > + if (read->rd_eof) > > goto out; > > > > - pos = vfs_llseek(file, read->rd_offset, SEEK_HOLE); > > - is_data = pos > read->rd_offset; > > - > > - while (count > 0 && !eof) { > > - maxcount = count; > > - if (is_data) > > - nfserr = nfsd4_encode_read_plus_data(resp, read, &maxcount, &eof, > > - segments == 0 ? &pos : NULL); > > - else > > - nfserr = nfsd4_encode_read_plus_hole(resp, read, &maxcount, &eof); > > - if (nfserr) > > - goto out; > > - count -= maxcount; > > - read->rd_offset += maxcount; > > - is_data = !is_data; > > - last_segment = xdr->buf->len; > > - segments++; > > - } > > - > > -out: > > - if (nfserr && segments == 0) > > + nfserr = nfsd4_encode_read_plus_data(resp, read); > > + if (nfserr) { > > xdr_truncate_encode(xdr, starting_len); > > - else { > > - if (nfserr) { > > - xdr_truncate_encode(xdr, last_segment); > > - nfserr = nfs_ok; > > - eof = 0; > > - } > > - tmp = htonl(eof); > > - write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); > > - tmp = htonl(segments); > > - write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4); > > + return nfserr; > > } > > > > + segments++; > > + > > +out: > > + p = xdr_encode_bool(p, read->rd_eof); > > + *p = cpu_to_be32(segments); > > return nfserr; > > } > > > > -- > > 2.37.2 > > > > -- > Chuck Lever > > >
> On Sep 1, 2022, at 3:44 PM, Anna Schumaker <anna@kernel.org> wrote: > > On Thu, Sep 1, 2022 at 3:05 PM Chuck Lever III <chuck.lever@oracle.com> wrote: >> >> Good to see this! I'll study it over the next few days. >> >> >>> On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: >>> >>> From: Anna Schumaker <Anna.Schumaker@Netapp.com> >>> >>> Change the implementation to return a single DATA segment covering the >>> requested read range. >> >> The discussion in your cover letter should go in this patch >> description. A good patch description explains "why"; the diff >> below already explains "what". >> >> I harp on that because the patch description is important >> information that I often consult when conducting archaeology >> during troubleshooting. "Why the f... did we do that?" > > Makes sense! Do you want me to resubmit now as a v2 with some of this > moved over to the patch description? Let me have a careful look at the diff -- give me a couple days -- so you can incorporate any review comments in v2. Also I'll try to answer the splice question you posed in 0/1. > Anna >> >> >>> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> >>> --- >>> fs/nfsd/nfs4xdr.c | 122 ++++++++-------------------------------------- >>> 1 file changed, 20 insertions(+), 102 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>> index 1e9690a061ec..adbff7737c14 100644 >>> --- a/fs/nfsd/nfs4xdr.c >>> +++ b/fs/nfsd/nfs4xdr.c >>> @@ -4731,79 +4731,30 @@ nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr, >>> >>> static __be32 >>> nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, >>> - struct nfsd4_read *read, >>> - unsigned long *maxcount, u32 *eof, >>> - loff_t *pos) >>> + struct nfsd4_read *read) >>> { >>> + unsigned long maxcount; >>> struct xdr_stream *xdr = resp->xdr; >>> struct file *file = read->rd_nf->nf_file; >>> - int starting_len = xdr->buf->len; >>> - loff_t hole_pos; >>> __be32 nfserr; >>> - __be32 *p, tmp; >>> - __be64 tmp64; >>> - >>> - hole_pos = pos ? *pos : vfs_llseek(file, read->rd_offset, SEEK_HOLE); >>> - if (hole_pos > read->rd_offset) >>> - *maxcount = min_t(unsigned long, *maxcount, hole_pos - read->rd_offset); >>> - *maxcount = min_t(unsigned long, *maxcount, (xdr->buf->buflen - xdr->buf->len)); >>> + __be32 *p; >>> >>> /* Content type, offset, byte count */ >>> p = xdr_reserve_space(xdr, 4 + 8 + 4); >>> if (!p) >>> return nfserr_resource; >>> >>> - read->rd_vlen = xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, *maxcount); >>> - if (read->rd_vlen < 0) >>> - return nfserr_resource; >>> + maxcount = min_t(unsigned long, read->rd_length, >>> + (xdr->buf->buflen - xdr->buf->len)); >>> >>> - nfserr = nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset, >>> - resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); >>> + nfserr = nfsd4_encode_readv(resp, read, file, maxcount); >>> 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); >>> - tmp64 = cpu_to_be64(read->rd_offset); >>> - 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; >>> -} >>> - >>> -static __be32 >>> -nfsd4_encode_read_plus_hole(struct nfsd4_compoundres *resp, >>> - struct nfsd4_read *read, >>> - unsigned long *maxcount, u32 *eof) >>> -{ >>> - struct file *file = read->rd_nf->nf_file; >>> - loff_t data_pos = vfs_llseek(file, read->rd_offset, SEEK_DATA); >>> - loff_t f_size = i_size_read(file_inode(file)); >>> - unsigned long count; >>> - __be32 *p; >>> - >>> - if (data_pos == -ENXIO) >>> - data_pos = f_size; >>> - else if (data_pos <= read->rd_offset || (data_pos < f_size && data_pos % PAGE_SIZE)) >>> - return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size); >>> - count = data_pos - read->rd_offset; >>> - >>> - /* Content type, offset, byte count */ >>> - p = xdr_reserve_space(resp->xdr, 4 + 8 + 8); >>> - if (!p) >>> - return nfserr_resource; >>> - >>> - *p++ = htonl(NFS4_CONTENT_HOLE); >>> + *p++ = cpu_to_be32(NFS4_CONTENT_DATA); >>> p = xdr_encode_hyper(p, read->rd_offset); >>> - p = xdr_encode_hyper(p, count); >>> + *p = cpu_to_be32(read->rd_length); >>> >>> - *eof = (read->rd_offset + count) >= f_size; >>> - *maxcount = min_t(unsigned long, count, *maxcount); >>> return nfs_ok; >>> } >>> >>> @@ -4811,20 +4762,14 @@ static __be32 >>> nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, >>> struct nfsd4_read *read) >>> { >>> - unsigned long maxcount, count; >>> struct xdr_stream *xdr = resp->xdr; >>> - struct file *file; >>> + struct file *file = read->rd_nf->nf_file; >>> int starting_len = xdr->buf->len; >>> - int last_segment = xdr->buf->len; >>> int segments = 0; >>> - __be32 *p, tmp; >>> - bool is_data; >>> - loff_t pos; >>> - u32 eof; >>> + __be32 *p; >>> >>> if (nfserr) >>> return nfserr; >>> - file = read->rd_nf->nf_file; >>> >>> /* eof flag, segment count */ >>> p = xdr_reserve_space(xdr, 4 + 4); >>> @@ -4832,48 +4777,21 @@ nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, >>> return nfserr_resource; >>> xdr_commit_encode(xdr); >>> >>> - maxcount = min_t(unsigned long, read->rd_length, >>> - (xdr->buf->buflen - xdr->buf->len)); >>> - count = maxcount; >>> - >>> - eof = read->rd_offset >= i_size_read(file_inode(file)); >>> - if (eof) >>> + read->rd_eof = read->rd_offset >= i_size_read(file_inode(file)); >>> + if (read->rd_eof) >>> goto out; >>> >>> - pos = vfs_llseek(file, read->rd_offset, SEEK_HOLE); >>> - is_data = pos > read->rd_offset; >>> - >>> - while (count > 0 && !eof) { >>> - maxcount = count; >>> - if (is_data) >>> - nfserr = nfsd4_encode_read_plus_data(resp, read, &maxcount, &eof, >>> - segments == 0 ? &pos : NULL); >>> - else >>> - nfserr = nfsd4_encode_read_plus_hole(resp, read, &maxcount, &eof); >>> - if (nfserr) >>> - goto out; >>> - count -= maxcount; >>> - read->rd_offset += maxcount; >>> - is_data = !is_data; >>> - last_segment = xdr->buf->len; >>> - segments++; >>> - } >>> - >>> -out: >>> - if (nfserr && segments == 0) >>> + nfserr = nfsd4_encode_read_plus_data(resp, read); >>> + if (nfserr) { >>> xdr_truncate_encode(xdr, starting_len); >>> - else { >>> - if (nfserr) { >>> - xdr_truncate_encode(xdr, last_segment); >>> - nfserr = nfs_ok; >>> - eof = 0; >>> - } >>> - tmp = htonl(eof); >>> - write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); >>> - tmp = htonl(segments); >>> - write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4); >>> + return nfserr; >>> } >>> >>> + segments++; >>> + >>> +out: >>> + p = xdr_encode_bool(p, read->rd_eof); >>> + *p = cpu_to_be32(segments); >>> return nfserr; >>> } >>> >>> -- >>> 2.37.2 >>> >> >> -- >> Chuck Lever -- Chuck Lever
> On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > Change the implementation to return a single DATA segment covering the > requested read range. > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > fs/nfsd/nfs4xdr.c | 122 ++++++++-------------------------------------- > 1 file changed, 20 insertions(+), 102 deletions(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 1e9690a061ec..adbff7737c14 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -4731,79 +4731,30 @@ nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr, > > static __be32 > nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > - struct nfsd4_read *read, > - unsigned long *maxcount, u32 *eof, > - loff_t *pos) > + struct nfsd4_read *read) Now eventually (but not right at the moment) I don't think we can use the rd_length and rd_offset fields in @read directly in this function ... won't those arguments be different for each data segment? Not a problem yet, just thinking out loud. I think using @read directly here makes it easy to re-use nfsd4_encode_readv(), so I'm OK with this strategy for the moment. > { > + unsigned long maxcount; Nit: reverse christmas tree, please. The declaration of maxcount goes on the line after "struct file *file ...". > struct xdr_stream *xdr = resp->xdr; > struct file *file = read->rd_nf->nf_file; > - int starting_len = xdr->buf->len; > - loff_t hole_pos; > __be32 nfserr; > - __be32 *p, tmp; > - __be64 tmp64; > - > - hole_pos = pos ? *pos : vfs_llseek(file, read->rd_offset, SEEK_HOLE); > - if (hole_pos > read->rd_offset) > - *maxcount = min_t(unsigned long, *maxcount, hole_pos - read->rd_offset); > - *maxcount = min_t(unsigned long, *maxcount, (xdr->buf->buflen - xdr->buf->len)); > + __be32 *p; > > /* Content type, offset, byte count */ > p = xdr_reserve_space(xdr, 4 + 8 + 4); > if (!p) > return nfserr_resource; > > - read->rd_vlen = xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, *maxcount); > - if (read->rd_vlen < 0) > - return nfserr_resource; > + maxcount = min_t(unsigned long, read->rd_length, > + (xdr->buf->buflen - xdr->buf->len)); > > - nfserr = nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset, > - resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); > + nfserr = nfsd4_encode_readv(resp, read, file, maxcount); > if (nfserr) > return nfserr; The only problem I see with this so far is that nfsd4_encode_readv() can return nfserr_resource, which isn't allowed after NFSv4.0, and is not listed in RFC 7862's table of allowed errors for READ_PLUS. I see that READ_PLUS code remaining after this patch might make the same mistake. I think this patch needs to correct that at least for the code that isn't shared with READ. Consult RFC 7862 to figure out the correct status codes to return. (I'll file a bug to audit the other encoders and come up with a plan to ensure NFSD returns an appropriate error code for the minorversion in use). To help answer the "merge readiness" question and to know if we need splice read support or not, can you run some performance comparison tests with NFSv4.2 READ (with and without a splice-enabled filesystem) and this READ_PLUS implementation? Nothing elaborate, let's just check our assumptions. I've also applied the 0/1 splice change here to do some testing to see if I can work out what's giving xfstests heartburn. > - 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); > - tmp64 = cpu_to_be64(read->rd_offset); > - 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; > -} > - > -static __be32 > -nfsd4_encode_read_plus_hole(struct nfsd4_compoundres *resp, > - struct nfsd4_read *read, > - unsigned long *maxcount, u32 *eof) > -{ > - struct file *file = read->rd_nf->nf_file; > - loff_t data_pos = vfs_llseek(file, read->rd_offset, SEEK_DATA); > - loff_t f_size = i_size_read(file_inode(file)); > - unsigned long count; > - __be32 *p; > - > - if (data_pos == -ENXIO) > - data_pos = f_size; > - else if (data_pos <= read->rd_offset || (data_pos < f_size && data_pos % PAGE_SIZE)) > - return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size); > - count = data_pos - read->rd_offset; > - > - /* Content type, offset, byte count */ > - p = xdr_reserve_space(resp->xdr, 4 + 8 + 8); > - if (!p) > - return nfserr_resource; > - > - *p++ = htonl(NFS4_CONTENT_HOLE); > + *p++ = cpu_to_be32(NFS4_CONTENT_DATA); > p = xdr_encode_hyper(p, read->rd_offset); > - p = xdr_encode_hyper(p, count); > + *p = cpu_to_be32(read->rd_length); > > - *eof = (read->rd_offset + count) >= f_size; > - *maxcount = min_t(unsigned long, count, *maxcount); > return nfs_ok; > } > > @@ -4811,20 +4762,14 @@ static __be32 > nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, > struct nfsd4_read *read) > { > - unsigned long maxcount, count; > struct xdr_stream *xdr = resp->xdr; > - struct file *file; > + struct file *file = read->rd_nf->nf_file; Nit: Reverse christmas tree style means this one goes at the top. > int starting_len = xdr->buf->len; > - int last_segment = xdr->buf->len; > int segments = 0; I would also say that @segments should be u32, but that's not relevant to this changeset. Optional if you want to add that change. > - __be32 *p, tmp; > - bool is_data; > - loff_t pos; > - u32 eof; > + __be32 *p; > > if (nfserr) > return nfserr; > - file = read->rd_nf->nf_file; > > /* eof flag, segment count */ > p = xdr_reserve_space(xdr, 4 + 4); > @@ -4832,48 +4777,21 @@ nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, > return nfserr_resource; > xdr_commit_encode(xdr); > > - maxcount = min_t(unsigned long, read->rd_length, > - (xdr->buf->buflen - xdr->buf->len)); > - count = maxcount; > - > - eof = read->rd_offset >= i_size_read(file_inode(file)); > - if (eof) > + read->rd_eof = read->rd_offset >= i_size_read(file_inode(file)); > + if (read->rd_eof) > goto out; > > - pos = vfs_llseek(file, read->rd_offset, SEEK_HOLE); > - is_data = pos > read->rd_offset; > - > - while (count > 0 && !eof) { > - maxcount = count; > - if (is_data) > - nfserr = nfsd4_encode_read_plus_data(resp, read, &maxcount, &eof, > - segments == 0 ? &pos : NULL); > - else > - nfserr = nfsd4_encode_read_plus_hole(resp, read, &maxcount, &eof); > - if (nfserr) > - goto out; > - count -= maxcount; > - read->rd_offset += maxcount; > - is_data = !is_data; > - last_segment = xdr->buf->len; > - segments++; > - } > - > -out: > - if (nfserr && segments == 0) > + nfserr = nfsd4_encode_read_plus_data(resp, read); > + if (nfserr) { > xdr_truncate_encode(xdr, starting_len); > - else { > - if (nfserr) { > - xdr_truncate_encode(xdr, last_segment); > - nfserr = nfs_ok; > - eof = 0; > - } > - tmp = htonl(eof); > - write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); > - tmp = htonl(segments); > - write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4); > + return nfserr; > } > > + segments++; > + > +out: > + p = xdr_encode_bool(p, read->rd_eof); > + *p = cpu_to_be32(segments); > return nfserr; > } > > -- > 2.37.2 > -- Chuck Lever
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 1e9690a061ec..adbff7737c14 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -4731,79 +4731,30 @@ nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr, static __be32 nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, - struct nfsd4_read *read, - unsigned long *maxcount, u32 *eof, - loff_t *pos) + struct nfsd4_read *read) { + unsigned long maxcount; struct xdr_stream *xdr = resp->xdr; struct file *file = read->rd_nf->nf_file; - int starting_len = xdr->buf->len; - loff_t hole_pos; __be32 nfserr; - __be32 *p, tmp; - __be64 tmp64; - - hole_pos = pos ? *pos : vfs_llseek(file, read->rd_offset, SEEK_HOLE); - if (hole_pos > read->rd_offset) - *maxcount = min_t(unsigned long, *maxcount, hole_pos - read->rd_offset); - *maxcount = min_t(unsigned long, *maxcount, (xdr->buf->buflen - xdr->buf->len)); + __be32 *p; /* Content type, offset, byte count */ p = xdr_reserve_space(xdr, 4 + 8 + 4); if (!p) return nfserr_resource; - read->rd_vlen = xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, *maxcount); - if (read->rd_vlen < 0) - return nfserr_resource; + maxcount = min_t(unsigned long, read->rd_length, + (xdr->buf->buflen - xdr->buf->len)); - nfserr = nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset, - resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof); + nfserr = nfsd4_encode_readv(resp, read, file, maxcount); 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); - tmp64 = cpu_to_be64(read->rd_offset); - 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; -} - -static __be32 -nfsd4_encode_read_plus_hole(struct nfsd4_compoundres *resp, - struct nfsd4_read *read, - unsigned long *maxcount, u32 *eof) -{ - struct file *file = read->rd_nf->nf_file; - loff_t data_pos = vfs_llseek(file, read->rd_offset, SEEK_DATA); - loff_t f_size = i_size_read(file_inode(file)); - unsigned long count; - __be32 *p; - - if (data_pos == -ENXIO) - data_pos = f_size; - else if (data_pos <= read->rd_offset || (data_pos < f_size && data_pos % PAGE_SIZE)) - return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size); - count = data_pos - read->rd_offset; - - /* Content type, offset, byte count */ - p = xdr_reserve_space(resp->xdr, 4 + 8 + 8); - if (!p) - return nfserr_resource; - - *p++ = htonl(NFS4_CONTENT_HOLE); + *p++ = cpu_to_be32(NFS4_CONTENT_DATA); p = xdr_encode_hyper(p, read->rd_offset); - p = xdr_encode_hyper(p, count); + *p = cpu_to_be32(read->rd_length); - *eof = (read->rd_offset + count) >= f_size; - *maxcount = min_t(unsigned long, count, *maxcount); return nfs_ok; } @@ -4811,20 +4762,14 @@ static __be32 nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_read *read) { - unsigned long maxcount, count; struct xdr_stream *xdr = resp->xdr; - struct file *file; + struct file *file = read->rd_nf->nf_file; int starting_len = xdr->buf->len; - int last_segment = xdr->buf->len; int segments = 0; - __be32 *p, tmp; - bool is_data; - loff_t pos; - u32 eof; + __be32 *p; if (nfserr) return nfserr; - file = read->rd_nf->nf_file; /* eof flag, segment count */ p = xdr_reserve_space(xdr, 4 + 4); @@ -4832,48 +4777,21 @@ nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr, return nfserr_resource; xdr_commit_encode(xdr); - maxcount = min_t(unsigned long, read->rd_length, - (xdr->buf->buflen - xdr->buf->len)); - count = maxcount; - - eof = read->rd_offset >= i_size_read(file_inode(file)); - if (eof) + read->rd_eof = read->rd_offset >= i_size_read(file_inode(file)); + if (read->rd_eof) goto out; - pos = vfs_llseek(file, read->rd_offset, SEEK_HOLE); - is_data = pos > read->rd_offset; - - while (count > 0 && !eof) { - maxcount = count; - if (is_data) - nfserr = nfsd4_encode_read_plus_data(resp, read, &maxcount, &eof, - segments == 0 ? &pos : NULL); - else - nfserr = nfsd4_encode_read_plus_hole(resp, read, &maxcount, &eof); - if (nfserr) - goto out; - count -= maxcount; - read->rd_offset += maxcount; - is_data = !is_data; - last_segment = xdr->buf->len; - segments++; - } - -out: - if (nfserr && segments == 0) + nfserr = nfsd4_encode_read_plus_data(resp, read); + if (nfserr) { xdr_truncate_encode(xdr, starting_len); - else { - if (nfserr) { - xdr_truncate_encode(xdr, last_segment); - nfserr = nfs_ok; - eof = 0; - } - tmp = htonl(eof); - write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4); - tmp = htonl(segments); - write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4); + return nfserr; } + segments++; + +out: + p = xdr_encode_bool(p, read->rd_eof); + *p = cpu_to_be32(segments); return nfserr; }