[PATCH 0/4] fix up xdr_shrink_pagelen

[PATCH 0/4] fix up xdr_shrink_pagelen

[Benny Halevy]

Trond, the following 3 patches fix and clean up xdr_shrink_pagelen.

The main issue is that buf->buflen -= len is potentially done
twice.  The rest are cosmetic clean ups that simplify the code a bit.

[PATCH 1/4] sunrpc: don't shorten buflen twice in xdr_shrink_pagelen
[PATCH 2/4] sunrpc: clean up xdr_shrink_pagelen use of temporary pointer
[PATCH 3/4] sunrpc: don't use the copy variable in nested block
[PATCH 4/4] sunrpc: simplify xdr_shrijk_pagelen use of "copy"

Benny

[PATCH 1/4] sunrpc: don't shorten buflen twice in xdr_shrink_pagelen

[Benny Halevy]

On Jan. 14, 2009, 2:50 +0200, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
>
> The buflen is reset for all cases at the end of xdr_shrink_pagelen.
> The data left in the tail after xdr_read_pages is not processed when the
> buflen is incorrectly set.

Note that in this case we also lose (len - tail->iov_len)
bytes from the buffered data in pages.

Reported-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 net/sunrpc/xdr.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index a1f82a8..91f0de9 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -407,8 +407,7 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 		if (tail->iov_len > len) {
 			copy = tail->iov_len - len;
 			memmove(p, tail->iov_base, copy);
-		} else
-			buf->buflen -= len;
+		}
 		/* Copy from the inlined pages into the tail */
 		copy = len;
 		if (copy > tail->iov_len)
-- 
1.7.2.1

Re: [PATCH 1/4] sunrpc: don't shorten buflen twice in xdr_shrink_pagelen

[Trond Myklebust]

On Wed, 2010-08-18 at 23:42 +0300, Benny Halevy wrote:
> On Jan. 14, 2009, 2:50 +0200, andros@netapp.com wrote:
> > From: Andy Adamson <andros@netapp.com>
> >
> > The buflen is reset for all cases at the end of xdr_shrink_pagelen.
> > The data left in the tail after xdr_read_pages is not processed when the
> > buflen is incorrectly set.
> 
> Note that in this case we also lose (len - tail->iov_len)
> bytes from the buffered data in pages.

We don't really need to do that. The amount of free space in the tail
(as opposed to space occupied by data) can be calculated as:

buf->buflen - buf->head->iov_len - buf->page_len - buf->tail->iov_len;


> Reported-by: Andy Adamson <andros@netapp.com>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  net/sunrpc/xdr.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index a1f82a8..91f0de9 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -407,8 +407,7 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
>  		if (tail->iov_len > len) {
>  			copy = tail->iov_len - len;
>  			memmove(p, tail->iov_base, copy);
> -		} else
> -			buf->buflen -= len;
> +		}
>  		/* Copy from the inlined pages into the tail */
>  		copy = len;
>  		if (copy > tail->iov_len)

Re: [PATCH 1/4] sunrpc: don't shorten buflen twice in xdr_shrink_pagelen

[Trond Myklebust]

On Wed, 2010-08-18 at 17:26 -0400, Trond Myklebust wrote:
> On Wed, 2010-08-18 at 23:42 +0300, Benny Halevy wrote:
> > On Jan. 14, 2009, 2:50 +0200, andros@netapp.com wrote:
> > > From: Andy Adamson <andros@netapp.com>
> > >
> > > The buflen is reset for all cases at the end of xdr_shrink_pagelen.
> > > The data left in the tail after xdr_read_pages is not processed when the
> > > buflen is incorrectly set.
> > 
> > Note that in this case we also lose (len - tail->iov_len)
> > bytes from the buffered data in pages.
> 
> We don't really need to do that. The amount of free space in the tail
> (as opposed to space occupied by data) can be calculated as:
> 
> buf->buflen - buf->head->iov_len - buf->page_len - buf->tail->iov_len;
> 
Something like the following:

---------------------------------------------------------------------------------------------
SUNRPC: Don't truncate tail data unnecessarily in xdr_shrink_pagelen

From: Trond Myklebust <Trond.Myklebust@netapp.com>

If we have unused buffer space, then we should make use of that rather
than unnecessarily truncating the message.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 net/sunrpc/xdr.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)


diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 3317db3..3bbef7f 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -396,12 +396,21 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 	struct kvec *tail;
 	size_t copy;
 	unsigned int pglen = buf->page_len;
+	unsigned int tailbuf_len;
 
 	tail = buf->tail;
 	BUG_ON (len > pglen);
 
+	tailbuf_len = buf->buflen - buf->head->iov_len - buf->page_len;
+
 	/* Shift the tail first */
-	if (tail->iov_len != 0) {
+	if (tailbuf_len != 0) {
+		unsigned int free_space = tailbuf_len - tail->iov_len;
+
+		if (len < free_space)
+			free_space = len;
+		tail->iov_len += free_space;
+
 		copy = len;
 		if (tail->iov_len > len) {
 			char *p = (char *)tail->iov_base + len;

Re: [PATCH 1/4] sunrpc: don't shorten buflen twice in xdr_shrink_pagelen

[Benny Halevy]

On Aug. 19, 2010, 1:16 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> On Wed, 2010-08-18 at 17:26 -0400, Trond Myklebust wrote:
>> On Wed, 2010-08-18 at 23:42 +0300, Benny Halevy wrote:
>>> On Jan. 14, 2009, 2:50 +0200, andros@netapp.com wrote:
>>>> From: Andy Adamson <andros@netapp.com>
>>>>
>>>> The buflen is reset for all cases at the end of xdr_shrink_pagelen.
>>>> The data left in the tail after xdr_read_pages is not processed when the
>>>> buflen is incorrectly set.
>>>
>>> Note that in this case we also lose (len - tail->iov_len)
>>> bytes from the buffered data in pages.
>>
>> We don't really need to do that. The amount of free space in the tail
>> (as opposed to space occupied by data) can be calculated as:
>>
>> buf->buflen - buf->head->iov_len - buf->page_len - buf->tail->iov_len;
>>
> Something like the following:
> 
> ---------------------------------------------------------------------------------------------
> SUNRPC: Don't truncate tail data unnecessarily in xdr_shrink_pagelen
> 
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> If we have unused buffer space, then we should make use of that rather
> than unnecessarily truncating the message.

Makes sense.
The patch looks good to me.

Benny

> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  net/sunrpc/xdr.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 3317db3..3bbef7f 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -396,12 +396,21 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
>  	struct kvec *tail;
>  	size_t copy;
>  	unsigned int pglen = buf->page_len;
> +	unsigned int tailbuf_len;
>  
>  	tail = buf->tail;
>  	BUG_ON (len > pglen);
>  
> +	tailbuf_len = buf->buflen - buf->head->iov_len - buf->page_len;
> +
>  	/* Shift the tail first */
> -	if (tail->iov_len != 0) {
> +	if (tailbuf_len != 0) {
> +		unsigned int free_space = tailbuf_len - tail->iov_len;
> +
> +		if (len < free_space)
> +			free_space = len;
> +		tail->iov_len += free_space;
> +
>  		copy = len;
>  		if (tail->iov_len > len) {
>  			char *p = (char *)tail->iov_base + len;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] sunrpc: clean up xdr_shrink_pagelen use of temporary pointer

[Benny Halevy]

char *p is used only as a shorthand for tail->iov_base + len in a nested
block.  Move it there.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 net/sunrpc/xdr.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 91f0de9..41be21f 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -395,7 +395,6 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 {
 	struct kvec *tail;
 	size_t copy;
-	char *p;
 	unsigned int pglen = buf->page_len;
 
 	tail = buf->tail;
@@ -403,8 +402,8 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 
 	/* Shift the tail first */
 	if (tail->iov_len != 0) {
-		p = (char *)tail->iov_base + len;
 		if (tail->iov_len > len) {
+			char *p = (char *)tail->iov_base + len;
 			copy = tail->iov_len - len;
 			memmove(p, tail->iov_base, copy);
 		}
-- 
1.7.2.1

[PATCH 3/4] sunrpc: don't use the copy variable in nested block

[Benny Halevy]

to clean up the code "copy" will be set prior to the block
hence it mustn't be used there.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 net/sunrpc/xdr.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 41be21f..42a7ebf 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -404,8 +404,7 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 	if (tail->iov_len != 0) {
 		if (tail->iov_len > len) {
 			char *p = (char *)tail->iov_base + len;
-			copy = tail->iov_len - len;
-			memmove(p, tail->iov_base, copy);
+			memmove(p, tail->iov_base, tail->iov_len - len);
 		}
 		/* Copy from the inlined pages into the tail */
 		copy = len;
-- 
1.7.2.1

[PATCH 4/4] sunrpc: simplify xdr_shrijk_pagelen use of "copy"

[Benny Halevy]

The "copy" variable value can be computed using the existing
logic rather than repeating it.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 net/sunrpc/xdr.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 42a7ebf..2ab59c3 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -402,14 +402,14 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 
 	/* Shift the tail first */
 	if (tail->iov_len != 0) {
+		copy = len;
 		if (tail->iov_len > len) {
 			char *p = (char *)tail->iov_base + len;
 			memmove(p, tail->iov_base, tail->iov_len - len);
+		} else {
+			copy = tail->iov_len;
 		}
 		/* Copy from the inlined pages into the tail */
-		copy = len;
-		if (copy > tail->iov_len)
-			copy = tail->iov_len;
 		_copy_from_pages((char *)tail->iov_base,
 				buf->pages, buf->page_base + pglen - len,
 				copy);
-- 
1.7.2.1

Re: [PATCH 4/4] sunrpc: simplify xdr_shrijk_pagelen use of "copy"

[Trond Myklebust]

On Wed, 2010-08-18 at 23:43 +0300, Benny Halevy wrote:
> The "copy" variable value can be computed using the existing
> logic rather than repeating it.
> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  net/sunrpc/xdr.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 42a7ebf..2ab59c3 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -402,14 +402,14 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
>  
>  	/* Shift the tail first */
>  	if (tail->iov_len != 0) {
> +		copy = len;
>  		if (tail->iov_len > len) {
>  			char *p = (char *)tail->iov_base + len;
>  			memmove(p, tail->iov_base, tail->iov_len - len);
> +		} else {
> +			copy = tail->iov_len;
>  		}
                 ^^^ We don't need a C block for the 'else' case.

>  		/* Copy from the inlined pages into the tail */
> -		copy = len;
> -		if (copy > tail->iov_len)
> -			copy = tail->iov_len;
>  		_copy_from_pages((char *)tail->iov_base,
>  				buf->pages, buf->page_base + pglen - len,
>  				copy);

Re: [PATCH 4/4] sunrpc: simplify xdr_shrijk_pagelen use of "copy"

[Benny Halevy]

On Aug. 19, 2010, 0:19 +0300, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> On Wed, 2010-08-18 at 23:43 +0300, Benny Halevy wrote:
>> The "copy" variable value can be computed using the existing
>> logic rather than repeating it.
>>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  net/sunrpc/xdr.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 42a7ebf..2ab59c3 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -402,14 +402,14 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
>>  
>>  	/* Shift the tail first */
>>  	if (tail->iov_len != 0) {
>> +		copy = len;
>>  		if (tail->iov_len > len) {
>>  			char *p = (char *)tail->iov_base + len;
>>  			memmove(p, tail->iov_base, tail->iov_len - len);
>> +		} else {
>> +			copy = tail->iov_len;
>>  		}
>                  ^^^ We don't need a C block for the 'else' case.
> 

Yeah. I just followed Documentation/CodingStyle...
I don't care either way.

Benny

>>  		/* Copy from the inlined pages into the tail */
>> -		copy = len;
>> -		if (copy > tail->iov_len)
>> -			copy = tail->iov_len;
>>  		_copy_from_pages((char *)tail->iov_base,
>>  				buf->pages, buf->page_base + pglen - len,
>>  				copy);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html