* [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
@ 2015-04-24 22:52 Ming Lin
2015-04-25 7:51 ` Dongsu Park
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ming Lin @ 2015-04-24 22:52 UTC (permalink / raw)
To: Neil Brown
Cc: Christoph Hellwig, Jens Axboe, linux-raid, Kent Overstreet,
Dongsu Park, Ming Lin
From: Kent Overstreet <kent.overstreet@gmail.com>
Refactor sync_request_write() of md/raid10 to use bio_copy_data()
instead of open coding bio_vec iterations.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
[dpark: add more description in commit message]
Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
Signed-off-by: Ming Lin <mlin@kernel.org>
---
drivers/md/raid10.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index a7196c4..02e33f1 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -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,
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
2015-04-24 22:52 [PATCH] md/raid10: make sync_request_write() call bio_copy_data() Ming Lin
@ 2015-04-25 7:51 ` Dongsu Park
2015-04-25 9:47 ` Christoph Hellwig
2015-04-26 23:49 ` NeilBrown
2 siblings, 0 replies; 6+ messages in thread
From: Dongsu Park @ 2015-04-25 7:51 UTC (permalink / raw)
To: Ming Lin
Cc: Neil Brown, Christoph Hellwig, Jens Axboe, linux-raid,
Kent Overstreet
On 24.04.2015 15:52, Ming Lin wrote:
> From: Kent Overstreet <kent.overstreet@gmail.com>
>
> 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
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> [dpark: add more description in commit message]
> Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
> Signed-off-by: Ming Lin <mlin@kernel.org>
> ---
> drivers/md/raid10.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index a7196c4..02e33f1 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -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,
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
2015-04-24 22:52 [PATCH] md/raid10: make sync_request_write() call bio_copy_data() Ming Lin
2015-04-25 7:51 ` Dongsu Park
@ 2015-04-25 9:47 ` Christoph Hellwig
2015-04-26 5:42 ` Ming Lin
2015-04-26 23:49 ` NeilBrown
2 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2015-04-25 9:47 UTC (permalink / raw)
To: Ming Lin; +Cc: Neil Brown, Jens Axboe, linux-raid, Kent Overstreet, Dongsu Park
On Fri, Apr 24, 2015 at 03:52:10PM -0700, Ming Lin wrote:
> From: Kent Overstreet <kent.overstreet@gmail.com>
>
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
2015-04-25 9:47 ` Christoph Hellwig
@ 2015-04-26 5:42 ` Ming Lin
0 siblings, 0 replies; 6+ messages in thread
From: Ming Lin @ 2015-04-26 5:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Neil Brown, Jens Axboe, linux-raid, Kent Overstreet, Dongsu Park
On Sat, Apr 25, 2015 at 2:47 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Apr 24, 2015 at 03:52:10PM -0700, Ming Lin wrote:
>> From: Kent Overstreet <kent.overstreet@gmail.com>
>>
>> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
2015-04-24 22:52 [PATCH] md/raid10: make sync_request_write() call bio_copy_data() Ming Lin
2015-04-25 7:51 ` Dongsu Park
2015-04-25 9:47 ` Christoph Hellwig
@ 2015-04-26 23:49 ` NeilBrown
2015-04-27 7:29 ` Ming Lin
2 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2015-04-26 23:49 UTC (permalink / raw)
To: Ming Lin
Cc: Christoph Hellwig, Jens Axboe, linux-raid, Kent Overstreet,
Dongsu Park
[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]
On Fri, 24 Apr 2015 15:52:10 -0700 Ming Lin <mlin@kernel.org> wrote:
> From: Kent Overstreet <kent.overstreet@gmail.com>
>
> Refactor sync_request_write() of md/raid10 to use bio_copy_data()
> instead of open coding bio_vec iterations.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> [dpark: add more description in commit message]
> Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
> Signed-off-by: Ming Lin <mlin@kernel.org>
> ---
> drivers/md/raid10.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index a7196c4..02e33f1 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -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);
> - }
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?
> 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.
> +
> + 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,
Providing you are confident that bv_offset and bv_len don't need to be
updated:
Acked-by: NeilBrown <neilb@suse.de>
though I'd prefer the bi_private assignment was left where it was.
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
2015-04-26 23:49 ` NeilBrown
@ 2015-04-27 7:29 ` Ming Lin
0 siblings, 0 replies; 6+ messages in thread
From: Ming Lin @ 2015-04-27 7:29 UTC (permalink / raw)
To: NeilBrown
Cc: Christoph Hellwig, Jens Axboe, linux-raid, Kent Overstreet,
Dongsu Park
On Sun, Apr 26, 2015 at 4:49 PM, NeilBrown <neilb@suse.de> wrote:
> On Fri, 24 Apr 2015 15:52:10 -0700 Ming Lin <mlin@kernel.org> wrote:
>
>> From: Kent Overstreet <kent.overstreet@gmail.com>
>>
>> Refactor sync_request_write() of md/raid10 to use bio_copy_data()
>> instead of open coding bio_vec iterations.
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
>> [dpark: add more description in commit message]
>> Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
>> Signed-off-by: Ming Lin <mlin@kernel.org>
>> ---
>> drivers/md/raid10.c | 20 +++++---------------
>> 1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index a7196c4..02e33f1 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -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);
>> - }
>
> 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.
>
>> 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.
>
>
>> +
>> + 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,
>
> Providing you are confident that bv_offset and bv_len don't need to be
> updated:
>
> Acked-by: NeilBrown <neilb@suse.de>
>
> though I'd prefer the bi_private assignment was left where it was.
Will update it.
Thanks.
>
> Thanks,
> NeilBrown
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-04-27 7:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-24 22:52 [PATCH] md/raid10: make sync_request_write() call bio_copy_data() Ming Lin
2015-04-25 7:51 ` Dongsu Park
2015-04-25 9:47 ` Christoph Hellwig
2015-04-26 5:42 ` Ming Lin
2015-04-26 23:49 ` NeilBrown
2015-04-27 7:29 ` Ming Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox