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
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 <[email protected]>
---
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
On Aug. 19, 2010, 1:16 +0300, Trond Myklebust <[email protected]> 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, [email protected] wrote:
>>>> From: Andy Adamson <[email protected]>
>>>>
>>>> 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 <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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, [email protected] wrote:
> > > From: Andy Adamson <[email protected]>
> > >
> > > 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 <[email protected]>
If we have unused buffer space, then we should make use of that rather
than unnecessarily truncating the message.
Signed-off-by: Trond Myklebust <[email protected]>
---
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;
On Jan. 14, 2009, 2:50 +0200, [email protected] wrote:
> From: Andy Adamson <[email protected]>
>
> 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 <[email protected]>
Signed-off-by: Benny Halevy <[email protected]>
---
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
On Aug. 19, 2010, 0:19 +0300, Trond Myklebust <[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-08-18 at 23:42 +0300, Benny Halevy wrote:
> On Jan. 14, 2009, 2:50 +0200, [email protected] wrote:
> > From: Andy Adamson <[email protected]>
> >
> > 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 <[email protected]>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> 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)
The "copy" variable value can be computed using the existing
logic rather than repeating it.
Signed-off-by: Benny Halevy <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);