md/raid10: make sync_request_write() call bio_copy_data()

PATCH In review Ming Lin 4 participants 6 messages Apr 24, 2015 – Apr 27, 2015 MD RAID

lore

Share & tools

lore

Ming Lin

author

2015-04-24 22:52 UTC #0 lore raw

PATCH 1/1 md/raid10: make sync_request_write() call bio_copy_data()

From: Kent Overstreet <[email protected]>

Refactor sync_request_write() of md/raid10 to use bio_copy_data() instead of open coding bio_vec iterations.

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

[dpark: add more description in commit message]

Signed-off-by Dongsu Park <[email protected]>

Signed-off-by Ming Lin <[email protected]>

---
 drivers/md/raid10.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

drivers/md/raid10.c +5 ~~-15~~

index a7196c4..02e33f1 100644
@@ -2097,18 +2097,11 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 		tbio->bi_vcnt = vcnt;
 		tbio->bi_iter.bi_size = r10_bio->sectors << 9;
 		tbio->bi_rw = WRITE;
-		tbio->bi_private = r10_bio;
 		tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
-
-		for (j=0; j < vcnt ; j++) {
-			tbio->bi_io_vec[j].bv_offset = 0;
-			tbio->bi_io_vec[j].bv_len = PAGE_SIZE;
-
-			memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-			       page_address(fbio->bi_io_vec[j].bv_page),
-			       PAGE_SIZE);
-		}
 		tbio->bi_end_io = end_sync_write;
+		tbio->bi_private = r10_bio;
+
+		bio_copy_data(tbio, fbio);
 
 		d = r10_bio->devs[i].devnum;
 		atomic_inc(&conf->mirrors[d].rdev->nr_pending);

@@ -2124,17 +2117,14 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	 * that are active
 	 */
 	for (i = 0; i < conf->copies; i++) {
-		int j, d;
+		int d;
 
 		tbio = r10_bio->devs[i].repl_bio;
 		if (!tbio || !tbio->bi_end_io)
 			continue;
 		if (r10_bio->devs[i].bio->bi_end_io != end_sync_write
 		    && r10_bio->devs[i].bio != fbio)
-			for (j = 0; j < vcnt; j++)
-				memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-				       page_address(fbio->bi_io_vec[j].bv_page),
-				       PAGE_SIZE);
+			bio_copy_data(tbio, fbio);
 		d = r10_bio->devs[i].devnum;
 		atomic_inc(&r10_bio->remaining);
 		md_sync_acct(conf->mirrors[d].replacement->bdev,

Signature · 1 line

1.9.1

Dongsu Park

2015-04-25 07:51 UTC #1 lore raw

Quoted 4 lines · Ming Lin

From: Kent Overstreet <[email protected]>
Refactor sync_request_write() of md/raid10 to use bio_copy_data() instead of open coding bio_vec iterations.

Looks good. Keep up the good work! ;-)

Thanks,
Dongsu

Quoted 58 lines

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

[dpark: add more description in commit message]

Signed-off-by Dongsu Park <[email protected]>

Signed-off-by Ming Lin <[email protected]>

---
 drivers/md/raid10.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

drivers/md/raid10.c +5 ~~-15~~

index a7196c4..02e33f1 100644
@@ -2097,18 +2097,11 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 		tbio->bi_vcnt = vcnt;
 		tbio->bi_iter.bi_size = r10_bio->sectors << 9;
 		tbio->bi_rw = WRITE;
-		tbio->bi_private = r10_bio;
 		tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
-
-		for (j=0; j < vcnt ; j++) {
-			tbio->bi_io_vec[j].bv_offset = 0;
-			tbio->bi_io_vec[j].bv_len = PAGE_SIZE;
-
-			memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-			       page_address(fbio->bi_io_vec[j].bv_page),
-			       PAGE_SIZE);
-		}
 		tbio->bi_end_io = end_sync_write;
+		tbio->bi_private = r10_bio;
+
+		bio_copy_data(tbio, fbio);
 
 		d = r10_bio->devs[i].devnum;
 		atomic_inc(&conf->mirrors[d].rdev->nr_pending);

@@ -2124,17 +2117,14 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	 * that are active
 	 */
 	for (i = 0; i < conf->copies; i++) {
-		int j, d;
+		int d;
 
 		tbio = r10_bio->devs[i].repl_bio;
 		if (!tbio || !tbio->bi_end_io)
 			continue;
 		if (r10_bio->devs[i].bio->bi_end_io != end_sync_write
 		    && r10_bio->devs[i].bio != fbio)
-			for (j = 0; j < vcnt; j++)
-				memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-				       page_address(fbio->bi_io_vec[j].bv_page),
-				       PAGE_SIZE);
+			bio_copy_data(tbio, fbio);
 		d = r10_bio->devs[i].devnum;
 		atomic_inc(&r10_bio->remaining);
 		md_sync_acct(conf->mirrors[d].replacement->bdev,

Signature · 1 line

1.9.1

Christoph Hellwig

2015-04-25 09:47 UTC #2 lore raw

In reply to Ming Lin

Quoted 4 lines · Ming Lin

From: Kent Overstreet <[email protected]>
Refactor sync_request_write() of md/raid10 to use bio_copy_data() instead of open coding bio_vec iterations.

Do you plan to submit an updated version of the immutable biovecs set soon? In that case it would be good to merge this patch through the block tree, as it's a requirement for it AFAICS. Might be worth to repost them together.

Ming Lin

author

2015-04-26 05:42 UTC #3 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 4 lines · Ming Lin
From: Kent Overstreet <[email protected]>
Refactor sync_request_write() of md/raid10 to use bio_copy_data() instead of open coding bio_vec iterations.
Do you plan to submit an updated version of the immutable biovecs set soon? In that case it would be good to merge this patch through the block tree, as it's a requirement for it AFAICS. Might be worth to repost them together.

I have put the immutable biovecs set here:
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req
Will add this patch also.

I'll submit the patch set soon after running some tests.

NeilBrown

Acked-by on v1 1/1

2015-04-26 23:49 UTC #4 lore raw

In reply to Ming Lin

Quoted 23 lines · On Fri, 24 Apr 2015 15:52:10 -0700 Ming Lin <[email protected]> wrote:

From: Kent Overstreet <[email protected]>
Refactor sync_request_write() of md/raid10 to use bio_copy_data() instead of open coding bio_vec iterations.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Kent Overstreet <[email protected]>
[dpark: add more description in commit message]
Signed-off-by Dongsu Park <[email protected]>
Signed-off-by Ming Lin <[email protected]>
---
 drivers/md/raid10.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)
drivers/md/raid10.c +0 -1
index a7196c4..02e33f1 100644
@@ -2097,18 +2097,11 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 		tbio->bi_vcnt = vcnt;
 		tbio->bi_iter.bi_size = r10_bio->sectors << 9;
 		tbio->bi_rw = WRITE;
-		tbio->bi_private = r10_bio;

drivers/md/raid10.c static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) · line 2110

Quoted 10 lines

drivers/md/raid10.c +0 -9

 		tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
-
-		for (j=0; j < vcnt ; j++) {
-			tbio->bi_io_vec[j].bv_offset = 0;
-			tbio->bi_io_vec[j].bv_len = PAGE_SIZE;
-
-			memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-			       page_address(fbio->bi_io_vec[j].bv_page),
-			       PAGE_SIZE);
-		}

You removed the resetting of bv_offset and bv_len.
So I assume this is being applied in a context where these things are now immutable - is that correct?

drivers/md/raid10.c static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) · line 2102

Quoted 2 lines

drivers/md/raid10.c +1 -0

 		tbio->bi_end_io = end_sync_write;
+		tbio->bi_private = r10_bio;

Any reason you are moving this assignment to bi_private? It doesn't hurt, but it doesn't seem to be necessary.

Quoted 15 lines

drivers/md/raid10.c +3 -1

+
+		bio_copy_data(tbio, fbio);
 
 		d = r10_bio->devs[i].devnum;
 		atomic_inc(&conf->mirrors[d].rdev->nr_pending);

@@ -2124,17 +2117,14 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	 * that are active
 	 */
 	for (i = 0; i < conf->copies; i++) {
-		int j, d;
+		int d;
 
 		tbio = r10_bio->devs[i].repl_bio;
 		if (!tbio || !tbio->bi_end_io)
 			continue;

drivers/md/raid10.c static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) · line 2130

Quoted 10 lines

drivers/md/raid10.c +1 -4

 		if (r10_bio->devs[i].bio->bi_end_io != end_sync_write
 		    && r10_bio->devs[i].bio != fbio)
-			for (j = 0; j < vcnt; j++)
-				memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-				       page_address(fbio->bi_io_vec[j].bv_page),
-				       PAGE_SIZE);
+			bio_copy_data(tbio, fbio);
 		d = r10_bio->devs[i].devnum;
 		atomic_inc(&r10_bio->remaining);
 		md_sync_acct(conf->mirrors[d].replacement->bdev,

Providing you are confident that bv_offset and bv_len don't need to be updated:

Acked-by NeilBrown <[email protected]>

though I'd prefer the bi_private assignment was left where it was.

Thanks,
NeilBrown

Ming Lin

author

2015-04-27 07:29 UTC #5 lore raw

Quoted 31 lines · NeilBrown

Quoted 29 lines · On Fri, 24 Apr 2015 15:52:10 -0700 Ming Lin <[email protected]> wrote:
From: Kent Overstreet <[email protected]>
Refactor sync_request_write() of md/raid10 to use bio_copy_data() instead of open coding bio_vec iterations.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Kent Overstreet <[email protected]>
[dpark: add more description in commit message]
Signed-off-by Dongsu Park <[email protected]>
Signed-off-by Ming Lin <[email protected]>
---
 drivers/md/raid10.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)
drivers/md/raid10.c +0 -6
index a7196c4..02e33f1 100644
@@ -2097,18 +2097,11 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
              tbio->bi_vcnt = vcnt;
              tbio->bi_iter.bi_size = r10_bio->sectors << 9;
              tbio->bi_rw = WRITE;
-             tbio->bi_private = r10_bio;
              tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
-
-             for (j=0; j < vcnt ; j++) {
-                     tbio->bi_io_vec[j].bv_offset = 0;
-                     tbio->bi_io_vec[j].bv_len = PAGE_SIZE;
-

Quoted 8 lines

drivers/md/raid10.c static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) · line 2110
Quoted 4 lines
drivers/md/raid10.c +0 -4
-                     memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-                            page_address(fbio->bi_io_vec[j].bv_page),
-                            PAGE_SIZE);
-             }
You removed the resetting of bv_offset and bv_len.
So I assume this is being applied in a context where these things are now immutable - is that correct?

Correct. The full patchset is here:
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req

I'll rebase it on top of 4.1-rc1, then post it.

Quoted 6 lines

drivers/md/raid10.c static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) · line 2102
Quoted 3 lines
drivers/md/raid10.c +1 -0
              tbio->bi_end_io = end_sync_write;
+             tbio->bi_private = r10_bio;
Any reason you are moving this assignment to bi_private? It doesn't hurt, but it doesn't seem to be necessary.

I'll update it to where it was.

Quoted 31 lines

Quoted 17 lines

drivers/md/raid10.c +3 -1

+
+             bio_copy_data(tbio, fbio);
 
              d = r10_bio->devs[i].devnum;
              atomic_inc(&conf->mirrors[d].rdev->nr_pending);

@@ -2124,17 +2117,14 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
       * that are active
       */
      for (i = 0; i < conf->copies; i++) {
-             int j, d;
+             int d;
 
              tbio = r10_bio->devs[i].repl_bio;
              if (!tbio || !tbio->bi_end_io)
                      continue;

drivers/md/raid10.c static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) · line 2130

Quoted 10 lines

drivers/md/raid10.c +1 -4

              if (r10_bio->devs[i].bio->bi_end_io != end_sync_write
                  && r10_bio->devs[i].bio != fbio)
-                     for (j = 0; j < vcnt; j++)
-                             memcpy(page_address(tbio->bi_io_vec[j].bv_page),
-                                    page_address(fbio->bi_io_vec[j].bv_page),
-                                    PAGE_SIZE);
+                     bio_copy_data(tbio, fbio);
              d = r10_bio->devs[i].devnum;
              atomic_inc(&r10_bio->remaining);
              md_sync_acct(conf->mirrors[d].replacement->bdev,

Providing you are confident that bv_offset and bv_len don't need to be updated:

Quoted 3 lines

Acked-by NeilBrown <[email protected]>
though I'd prefer the bi_private assignment was left where it was.

Will update it.

Thanks.

Quoted 4 lines

Thanks,
NeilBrown