2022-11-26 21:01:28

by Chuck Lever III

[permalink] [raw]
Subject: [PATCH 2/4] SUNRPC: Clean up xdr_write_pages()

Make it more evident how xdr_write_pages() updates the tail buffer
by using the convention of naming the iov pointer variable "tail".
I spent more than a couple of hours chasing through code to
understand this, so someone is likely to find this useful later.

Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/xdr.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 336a7c7833e4..f7767bf22406 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1224,30 +1224,34 @@ EXPORT_SYMBOL(xdr_restrict_buflen);
/**
* xdr_write_pages - Insert a list of pages into an XDR buffer for sending
* @xdr: pointer to xdr_stream
- * @pages: list of pages
- * @base: offset of first byte
- * @len: length of data in bytes
+ * @pages: array of pages to insert
+ * @base: starting offset of first data byte in @pages
+ * @len: number of data bytes in @pages to insert
*
+ * After the @pages are added, the tail iovec is instantiated pointing to
+ * end of the head buffer, and the stream is set up to encode subsequent
+ * items into the tail.
*/
void xdr_write_pages(struct xdr_stream *xdr, struct page **pages, unsigned int base,
unsigned int len)
{
struct xdr_buf *buf = xdr->buf;
- struct kvec *iov = buf->tail;
+ struct kvec *tail = buf->tail;
+
buf->pages = pages;
buf->page_base = base;
buf->page_len = len;

- iov->iov_base = (char *)xdr->p;
- iov->iov_len = 0;
- xdr->iov = iov;
+ tail->iov_base = xdr->p;
+ tail->iov_len = 0;
+ xdr->iov = tail;

if (len & 3) {
unsigned int pad = 4 - (len & 3);

BUG_ON(xdr->p >= xdr->end);
- iov->iov_base = (char *)xdr->p + (len & 3);
- iov->iov_len += pad;
+ tail->iov_base = (char *)xdr->p + (len & 3);
+ tail->iov_len += pad;
len += pad;
*xdr->p++ = 0;
}



2022-12-12 17:04:59

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH 2/4] SUNRPC: Clean up xdr_write_pages()

On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
> Make it more evident how xdr_write_pages() updates the tail buffer
> by using the convention of naming the iov pointer variable "tail".
> I spent more than a couple of hours chasing through code to
> understand this, so someone is likely to find this useful later.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> net/sunrpc/xdr.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 336a7c7833e4..f7767bf22406 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1224,30 +1224,34 @@ EXPORT_SYMBOL(xdr_restrict_buflen);
> /**
> * xdr_write_pages - Insert a list of pages into an XDR buffer for sending
> * @xdr: pointer to xdr_stream
> - * @pages: list of pages
> - * @base: offset of first byte
> - * @len: length of data in bytes
> + * @pages: array of pages to insert
> + * @base: starting offset of first data byte in @pages
> + * @len: number of data bytes in @pages to insert
> *
> + * After the @pages are added, the tail iovec is instantiated pointing to
> + * end of the head buffer, and the stream is set up to encode subsequent
> + * items into the tail.
> */
> void xdr_write_pages(struct xdr_stream *xdr, struct page **pages, unsigned int base,
> unsigned int len)
> {
> struct xdr_buf *buf = xdr->buf;
> - struct kvec *iov = buf->tail;
> + struct kvec *tail = buf->tail;
> +
> buf->pages = pages;
> buf->page_base = base;
> buf->page_len = len;
>
> - iov->iov_base = (char *)xdr->p;
> - iov->iov_len = 0;
> - xdr->iov = iov;
> + tail->iov_base = xdr->p;
> + tail->iov_len = 0;
> + xdr->iov = tail;
>
> if (len & 3) {
> unsigned int pad = 4 - (len & 3);
>
> BUG_ON(xdr->p >= xdr->end);
> - iov->iov_base = (char *)xdr->p + (len & 3);
> - iov->iov_len += pad;
> + tail->iov_base = (char *)xdr->p + (len & 3);
> + tail->iov_len += pad;
> len += pad;
> *xdr->p++ = 0;
> }
>
>

Reviewed-by: Jeff Layton <[email protected]>