fix up xdr_shrink_pagelen

PATCH In review Benny Halevy 3 participants 10 messages Aug 18, 2010 – Aug 19, 2010

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Benny Halevy

author

2010-08-18 20:41 UTC #0 lore raw

cover fix up xdr_shrink_pagelen

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

Benny Halevy

author

2010-08-18 20:42 UTC #1 lore raw

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

Quoted 5 lines · 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(-)

net/sunrpc/xdr.c +1 -2

index a1f82a8..91f0de9 100644
@@ -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)

Signature · 1 line

1.7.2.1

Trond Myklebust

2010-08-18 21:26 UTC #2 lore raw

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

Quoted 9 lines · Benny Halevy

Quoted 5 lines · 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;

Quoted 20 lines

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(-)

net/sunrpc/xdr.c +1 -2

index a1f82a8..91f0de9 100644
@@ -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)

Trond Myklebust

2010-08-18 22:16 UTC #3 lore raw

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

Quoted 8 lines · Trond Myklebust

Quoted 7 lines · Benny Halevy
Quoted 5 lines · 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.

Quoted 8 lines

Quoted 2 lines
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(-)

net/sunrpc/xdr.c +10 -1

index 3317db3..3bbef7f 100644
@@ -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;

Benny Halevy

author

2010-08-19 05:49 UTC #4 lore raw

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

Quoted 19 lines · Trond Myklebust

Quoted 8 lines · Trond Myklebust
Quoted 7 lines · Benny Halevy
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 2 lines
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:

Quoted 7 lines

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

Quoted 41 lines

Signed-off-by Trond Myklebust <[email protected]>

---

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

net/sunrpc/xdr.c +10 -2

index 3317db3..3bbef7f 100644
@@ -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

Benny Halevy

author

2010-08-18 20:42 UTC #5 lore raw

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

In reply to 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 <[email protected]>

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

net/sunrpc/xdr.c +1 -2

index 91f0de9..41be21f 100644
@@ -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);
 		}

Signature · 1 line

1.7.2.1

Benny Halevy

author

2010-08-18 20:42 UTC #6 lore raw

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

In reply to 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 <[email protected]>

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

net/sunrpc/xdr.c +1 -2

index 41be21f..42a7ebf 100644
@@ -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;

Signature · 1 line

1.7.2.1

Benny Halevy

author

2010-08-18 20:43 UTC #7 lore raw

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

In reply to Benny Halevy

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(-)

net/sunrpc/xdr.c +3 -3

index 42a7ebf..2ab59c3 100644
@@ -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);

Signature · 1 line

1.7.2.1

Trond Myklebust

2010-08-18 21:20 UTC #8 lore raw

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

Quoted 13 lines · Benny Halevy

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(-)
net/sunrpc/xdr.c +0 -0
index 42a7ebf..2ab59c3 100644

net/sunrpc/xdr.c xdr_shrink_pagelen(struct xdr_buf *buf, size_t len) · line 411

Quoted 10 lines

net/sunrpc/xdr.c +3 -0

@@ -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.

Quoted 7 lines

net/sunrpc/xdr.c +0 -3

 		/* 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);

Benny Halevy

author

2010-08-19 05:39 UTC #9 lore raw

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

Quoted 15 lines · Trond Myklebust

Quoted 14 lines · Benny Halevy
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(-)
net/sunrpc/xdr.c +0 -0
index 42a7ebf..2ab59c3 100644
@@ -402,14 +402,14 @@ xdr_shrink_pagelen(struct xdr_buf *buf, size_t len)
 

Quoted 11 lines

net/sunrpc/xdr.c xdr_shrink_pagelen(struct xdr_buf *buf, size_t len) · line 411
Quoted 9 lines
net/sunrpc/xdr.c +3 -0
 	/* 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

Quoted 13 lines

net/sunrpc/xdr.c xdr_shrink_pagelen(struct xdr_buf *buf, size_t len) · line 415
Quoted 7 lines
net/sunrpc/xdr.c +0 -3
 		/* 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