[PATCH v2 0/2] Fixes for READ_PLUS

[PATCH v2 0/2] Fixes for READ_PLUS

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

These patches fix up hole and data segment decoding for READ_PLUS. It
turns out I wasn't handling data getting truncated off the end of the
message properly. These patches fix it up, and now xfstests generic/091
and generic/263 pass when run against servers exporting ext4 and btrfs.
These tests also pass against servers exporting xfs when the clone
operation is disabled, so it seems like there is something going on
inside the xfs filesystem causing these tests to still fail.

- Changes since v1:
  - Drop patch for allocating scratch page
  - Drop patch for disabling READ_PLUS behind a Kconfig option

Thanks,
Anna

Anna Schumaker (2):
  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
  SUNRPC: Check if the buffer has fewer bytes than requested

 net/sunrpc/xdr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.29.2

[PATCH v2 1/2] SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

Otherwise we could end up placing data a few bytes off from where we
actually want to put it.

Fixes: 84ce182ab85b "SUNRPC: Add the ability to expand holes in data pages"
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 net/sunrpc/xdr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 71e03b930b70..5b848fe65c81 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1314,6 +1314,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
 		unsigned int res = _shift_data_right_tail(buf, from + bytes - shift, shift);
 		truncated = shift - res;
 		xdr->nwords -= XDR_QUADLEN(truncated);
+		buf->len -= 4 * XDR_QUADLEN(truncated);
 		bytes -= shift;
 	}
 
@@ -1325,7 +1326,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
 					bytes);
 	_zero_pages(buf->pages, buf->page_base + offset, length);
 
-	buf->len += length - (from - offset) - truncated;
+	buf->len += length - (from - offset);
 	xdr_set_page(xdr, offset + length, PAGE_SIZE);
 	return length;
 }
-- 
2.29.2

[PATCH v2 2/2] SUNRPC: Check if the buffer has fewer bytes than requested

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

xdr_expand_hole() might truncate data off the end of the buffer. If that
happens, we need to return a short read to the NFS code to indicate that
some data has been lost.

Fixes: e6ac0accb27c "SUNRPC: Add an xdr_align_data() function"
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 net/sunrpc/xdr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 5b848fe65c81..68f470e33427 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1273,6 +1273,8 @@ uint64_t xdr_align_data(struct xdr_stream *xdr, uint64_t offset, uint32_t length
 	bytes = xdr->nwords << 2;
 	if (length < bytes)
 		bytes = length;
+	if (bytes < length)
+		length = bytes;
 
 	/* Move page data to the left */
 	if (from > offset) {
-- 
2.29.2

Re: [PATCH v2 1/2] SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes

[Chuck Lever]

> On Dec 8, 2020, at 3:29 PM, schumaker.anna@gmail.com wrote:
> 
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> Otherwise we could end up placing data a few bytes off from where we
> actually want to put it.
> 
> Fixes: 84ce182ab85b "SUNRPC: Add the ability to expand holes in data pages"
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> net/sunrpc/xdr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 71e03b930b70..5b848fe65c81 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1314,6 +1314,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
> 		unsigned int res = _shift_data_right_tail(buf, from + bytes - shift, shift);
> 		truncated = shift - res;
> 		xdr->nwords -= XDR_QUADLEN(truncated);
> +		buf->len -= 4 * XDR_QUADLEN(truncated);

If I understand what you're doing here correctly, the usual idiom
is "XDR_QUADLEN(truncated) << 2" .


> 		bytes -= shift;
> 	}
> 
> @@ -1325,7 +1326,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
> 					bytes);
> 	_zero_pages(buf->pages, buf->page_base + offset, length);
> 
> -	buf->len += length - (from - offset) - truncated;
> +	buf->len += length - (from - offset);
> 	xdr_set_page(xdr, offset + length, PAGE_SIZE);
> 	return length;
> }
> -- 
> 2.29.2
> 

--
Chuck Lever

Re: [PATCH v2 1/2] SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes

[Anna Schumaker]

On Tue, Dec 8, 2020 at 3:56 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Dec 8, 2020, at 3:29 PM, schumaker.anna@gmail.com wrote:
> >
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > Otherwise we could end up placing data a few bytes off from where we
> > actually want to put it.
> >
> > Fixes: 84ce182ab85b "SUNRPC: Add the ability to expand holes in data pages"
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > ---
> > net/sunrpc/xdr.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 71e03b930b70..5b848fe65c81 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -1314,6 +1314,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
> >               unsigned int res = _shift_data_right_tail(buf, from + bytes - shift, shift);
> >               truncated = shift - res;
> >               xdr->nwords -= XDR_QUADLEN(truncated);
> > +             buf->len -= 4 * XDR_QUADLEN(truncated);
>
> If I understand what you're doing here correctly, the usual idiom
> is "XDR_QUADLEN(truncated) << 2" .

Oh, that works too. I'll adjust the patch. Thanks for letting me know!

Anna

>
>
> >               bytes -= shift;
> >       }
> >
> > @@ -1325,7 +1326,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
> >                                       bytes);
> >       _zero_pages(buf->pages, buf->page_base + offset, length);
> >
> > -     buf->len += length - (from - offset) - truncated;
> > +     buf->len += length - (from - offset);
> >       xdr_set_page(xdr, offset + length, PAGE_SIZE);
> >       return length;
> > }
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
>

Re: [PATCH v2 1/2] SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes

[Chuck Lever]

> On Dec 8, 2020, at 4:11 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote:
> 
> On Tue, Dec 8, 2020 at 3:56 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Dec 8, 2020, at 3:29 PM, schumaker.anna@gmail.com wrote:
>>> 
>>> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>> 
>>> Otherwise we could end up placing data a few bytes off from where we
>>> actually want to put it.
>>> 
>>> Fixes: 84ce182ab85b "SUNRPC: Add the ability to expand holes in data pages"
>>> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>> ---
>>> net/sunrpc/xdr.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>> index 71e03b930b70..5b848fe65c81 100644
>>> --- a/net/sunrpc/xdr.c
>>> +++ b/net/sunrpc/xdr.c
>>> @@ -1314,6 +1314,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
>>>              unsigned int res = _shift_data_right_tail(buf, from + bytes - shift, shift);
>>>              truncated = shift - res;
>>>              xdr->nwords -= XDR_QUADLEN(truncated);
>>> +             buf->len -= 4 * XDR_QUADLEN(truncated);
>> 
>> If I understand what you're doing here correctly, the usual idiom
>> is "XDR_QUADLEN(truncated) << 2" .
> 
> Oh, that works too. I'll adjust the patch. Thanks for letting me know!
> 

Urp, sorry. These days, the preferred mechanism is xdr_align_size().
Old habits die hard, I guess.


> Anna
> 
>> 
>> 
>>>              bytes -= shift;
>>>      }
>>> 
>>> @@ -1325,7 +1326,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
>>>                                      bytes);
>>>      _zero_pages(buf->pages, buf->page_base + offset, length);
>>> 
>>> -     buf->len += length - (from - offset) - truncated;
>>> +     buf->len += length - (from - offset);
>>>      xdr_set_page(xdr, offset + length, PAGE_SIZE);
>>>      return length;
>>> }
>>> --
>>> 2.29.2
>>> 
>> 
>> --
>> Chuck Lever

--
Chuck Lever