diff mbox series

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

Message ID 20201208202925.597663-2-Anna.Schumaker@Netapp.com (mailing list archive)
State New
Headers show
Series Fixes for READ_PLUS | expand

Commit Message

Anna Schumaker Dec. 8, 2020, 8:29 p.m. UTC
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(-)

Comments

Chuck Lever Dec. 8, 2020, 8:56 p.m. UTC | #1
> 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
Anna Schumaker Dec. 8, 2020, 9:11 p.m. UTC | #2
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
>
>
>
Chuck Lever Dec. 9, 2020, 4:05 p.m. UTC | #3
> 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
diff mbox series

Patch

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;
 }