2023-06-23 09:42:56

by Li Nan

Subject: [PATCH 1/3] md/raid10: optimize fix_read_error

From: Li Nan <[email protected]>

We dereference r10_bio->read_slot too many times in fix_read_error().
Optimize it by using a variable to store read_slot.

Signed-off-by: Li Nan <[email protected]>
---
drivers/md/raid10.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 381c21f7fb06..94ae294c8a3c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2725,10 +2725,10 @@ static int r10_sync_page_io(struct md_rdev *rdev, sector_t sector,
static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10bio *r10_bio)
{
int sect = 0; /* Offset from r10_bio->sector */
- int sectors = r10_bio->sectors;
+ int sectors = r10_bio->sectors, slot = r10_bio->read_slot;
struct md_rdev *rdev;
int max_read_errors = atomic_read(&mddev->max_corr_read_errors);
- int d = r10_bio->devs[r10_bio->read_slot].devnum;
+ int d = r10_bio->devs[slot].devnum;

/* still own a reference to this rdev, so it cannot
* have been cleared recently.
@@ -2749,13 +2749,13 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
pr_notice("md/raid10:%s: %pg: Failing raid device\n",
mdname(mddev), rdev->bdev);
md_error(mddev, rdev);
- r10_bio->devs[r10_bio->read_slot].bio = IO_BLOCKED;
+ r10_bio->devs[slot].bio = IO_BLOCKED;
return;
}

while(sectors) {
int s = sectors;
- int sl = r10_bio->read_slot;
+ int sl = slot;
int success = 0;
int start;

@@ -2790,7 +2790,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
sl++;
if (sl == conf->copies)
sl = 0;
- } while (!success && sl != r10_bio->read_slot);
+ } while (!success && sl != slot);
rcu_read_unlock();

if (!success) {
@@ -2798,16 +2798,16 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
* as bad on the first device to discourage future
* reads.
*/
- int dn = r10_bio->devs[r10_bio->read_slot].devnum;
+ int dn = r10_bio->devs[slot].devnum;
rdev = conf->mirrors[dn].rdev;

if (!rdev_set_badblocks(
rdev,
- r10_bio->devs[r10_bio->read_slot].addr
+ r10_bio->devs[slot].addr
+ sect,
s, 0)) {
md_error(mddev, rdev);
- r10_bio->devs[r10_bio->read_slot].bio
+ r10_bio->devs[slot].bio
= IO_BLOCKED;
}
break;
@@ -2816,7 +2816,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
start = sl;
/* write it back and re-read */
rcu_read_lock();
- while (sl != r10_bio->read_slot) {
+ while (sl != slot) {
if (sl==0)
sl = conf->copies;
sl--;
@@ -2850,7 +2850,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
rcu_read_lock();
}
sl = start;
- while (sl != r10_bio->read_slot) {
+ while (sl != slot) {
if (sl==0)
sl = conf->copies;
sl--;
--
2.39.2

2023-06-23 10:13:50

by Paul Menzel

Subject: Re: [PATCH 1/3] md/raid10: optimize fix_read_error

Dear Li,


Thank you for your patch.

Am 23.06.23 um 19:32 schrieb [email protected]:
> From: Li Nan <[email protected]>
>
> We dereference r10_bio->read_slot too many times in fix_read_error().
> Optimize it by using a variable to store read_slot.

I am always cautious reading about optimizations without any benchmarks
or object code analysis. Although your explanation makes sense, did you
check, that performance didn’t decrease in some way? (Maybe the compiler
even generates the same code.)


Kind regards,

Paul


> Signed-off-by: Li Nan <[email protected]>
> ---
> drivers/md/raid10.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 381c21f7fb06..94ae294c8a3c 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2725,10 +2725,10 @@ static int r10_sync_page_io(struct md_rdev *rdev, sector_t sector,
> static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10bio *r10_bio)
> {
> int sect = 0; /* Offset from r10_bio->sector */
> - int sectors = r10_bio->sectors;
> + int sectors = r10_bio->sectors, slot = r10_bio->read_slot;
> struct md_rdev *rdev;
> int max_read_errors = atomic_read(&mddev->max_corr_read_errors);
> - int d = r10_bio->devs[r10_bio->read_slot].devnum;
> + int d = r10_bio->devs[slot].devnum;
>
> /* still own a reference to this rdev, so it cannot
> * have been cleared recently.
> @@ -2749,13 +2749,13 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> pr_notice("md/raid10:%s: %pg: Failing raid device\n",
> mdname(mddev), rdev->bdev);
> md_error(mddev, rdev);
> - r10_bio->devs[r10_bio->read_slot].bio = IO_BLOCKED;
> + r10_bio->devs[slot].bio = IO_BLOCKED;
> return;
> }
>
> while(sectors) {
> int s = sectors;
> - int sl = r10_bio->read_slot;
> + int sl = slot;
> int success = 0;
> int start;
>
> @@ -2790,7 +2790,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> sl++;
> if (sl == conf->copies)
> sl = 0;
> - } while (!success && sl != r10_bio->read_slot);
> + } while (!success && sl != slot);
> rcu_read_unlock();
>
> if (!success) {
> @@ -2798,16 +2798,16 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> * as bad on the first device to discourage future
> * reads.
> */
> - int dn = r10_bio->devs[r10_bio->read_slot].devnum;
> + int dn = r10_bio->devs[slot].devnum;
> rdev = conf->mirrors[dn].rdev;
>
> if (!rdev_set_badblocks(
> rdev,
> - r10_bio->devs[r10_bio->read_slot].addr
> + r10_bio->devs[slot].addr
> + sect,
> s, 0)) {
> md_error(mddev, rdev);
> - r10_bio->devs[r10_bio->read_slot].bio
> + r10_bio->devs[slot].bio
> = IO_BLOCKED;
> }
> break;
> @@ -2816,7 +2816,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> start = sl;
> /* write it back and re-read */
> rcu_read_lock();
> - while (sl != r10_bio->read_slot) {
> + while (sl != slot) {
> if (sl==0)
> sl = conf->copies;
> sl--;
> @@ -2850,7 +2850,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> rcu_read_lock();
> }
> sl = start;
> - while (sl != r10_bio->read_slot) {
> + while (sl != slot) {
> if (sl==0)
> sl = conf->copies;
> sl--;

2023-06-25 07:15:06

by Li Nan

Subject: Re: [PATCH 1/3] md/raid10: optimize fix_read_error

在 2023/6/23 18:03, Paul Menzel 写道:
> Dear Li,
>
>
> Thank you for your patch.
>
> Am 23.06.23 um 19:32 schrieb [email protected]:
>> From: Li Nan <[email protected]>
>>
>> We dereference r10_bio->read_slot too many times in fix_read_error().
>> Optimize it by using a variable to store read_slot.
>
> I am always cautious reading about optimizations without any benchmarks
> or object code analysis. Although your explanation makes sense, did you
> check, that performance didn’t decrease in some way? (Maybe the compiler
> even generates the same code.)
>
>
> Kind regards,
>
> Paul
>
>

Compared assembly code before and after optimization:
- With gcc 8.3.0, both are consistent.
- With gcc 12.2.1, 'r10_bio->read_slot' mostly uses r10bio dirctly:
2853 while (sl != r10_bio->read_slot) {
0xffffffff8213f2a0 <+1328>: cmp %r14d,0x38(%rbp)

'slot' is mostly saved to a register individually:
2819 while (sl != slot) {
0xffffffff8213f08a <+794>: cmp %r14d,%ebx

I have not tested the performance, and it won't bring significant
performance optimization, which can also be seen from the analysis of
the assembly code. In fact, I just want to make code more readable.

--
Thanks,
Nan

2023-06-26 02:38:19

by Yu Kuai

Subject: Re: [PATCH 1/3] md/raid10: optimize fix_read_error

Hi,

?? 2023/06/24 1:32, [email protected] д??:
> From: Li Nan <[email protected]>
>
> We dereference r10_bio->read_slot too many times in fix_read_error().
> Optimize it by using a variable to store read_slot.
>

Other than a nit below, this patch LGTM.

Reviewed-by: Yu Kuai <[email protected]>

> Signed-off-by: Li Nan <[email protected]>
> ---
> drivers/md/raid10.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 381c21f7fb06..94ae294c8a3c 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2725,10 +2725,10 @@ static int r10_sync_page_io(struct md_rdev *rdev, sector_t sector,
> static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10bio *r10_bio)
> {
> int sect = 0; /* Offset from r10_bio->sector */
> - int sectors = r10_bio->sectors;
> + int sectors = r10_bio->sectors, slot = r10_bio->read_slot;
> struct md_rdev *rdev;
> int max_read_errors = atomic_read(&mddev->max_corr_read_errors);
> - int d = r10_bio->devs[r10_bio->read_slot].devnum;
> + int d = r10_bio->devs[slot].devnum;
>
> /* still own a reference to this rdev, so it cannot
> * have been cleared recently.
> @@ -2749,13 +2749,13 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> pr_notice("md/raid10:%s: %pg: Failing raid device\n",
> mdname(mddev), rdev->bdev);
> md_error(mddev, rdev);
> - r10_bio->devs[r10_bio->read_slot].bio = IO_BLOCKED;
> + r10_bio->devs[slot].bio = IO_BLOCKED;
> return;
> }
>
> while(sectors) {
> int s = sectors;
> - int sl = r10_bio->read_slot;
> + int sl = slot;
> int success = 0;
> int start;
>
> @@ -2790,7 +2790,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> sl++;
> if (sl == conf->copies)
> sl = 0;
> - } while (!success && sl != r10_bio->read_slot);
> + } while (!success && sl != slot);
> rcu_read_unlock();
>
> if (!success) {
> @@ -2798,16 +2798,16 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> * as bad on the first device to discourage future
> * reads.
> */
> - int dn = r10_bio->devs[r10_bio->read_slot].devnum;
> + int dn = r10_bio->devs[slot].devnum;
> rdev = conf->mirrors[dn].rdev;
>
> if (!rdev_set_badblocks(
> rdev,
> - r10_bio->devs[r10_bio->read_slot].addr
> + r10_bio->devs[slot].addr
> + sect,
> s, 0)) {
> md_error(mddev, rdev);
> - r10_bio->devs[r10_bio->read_slot].bio
> + r10_bio->devs[slot].bio
> = IO_BLOCKED;

There is no need to split lines now.

Thanks,
Kuai
> }
> break;
> @@ -2816,7 +2816,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> start = sl;
> /* write it back and re-read */
> rcu_read_lock();
> - while (sl != r10_bio->read_slot) {
> + while (sl != slot) {
> if (sl==0)
> sl = conf->copies;
> sl--;
> @@ -2850,7 +2850,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
> rcu_read_lock();
> }
> sl = start;
> - while (sl != r10_bio->read_slot) {
> + while (sl != slot) {
> if (sl==0)
> sl = conf->copies;
> sl--;
>