Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 1/3] md/raid10: optimize fix_read_error
To:     linan666@huaweicloud.com, song@kernel.org
Cc:     linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org,
        linan122@huawei.com, yi.zhang@huawei.com, houtao1@huawei.com,
        yangerkun@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20230623173236.2513554-1-linan666@huaweicloud.com>
 <20230623173236.2513554-2-linan666@huaweicloud.com>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <292862ac-f0e0-6068-eb16-511a01f185b2@huaweicloud.com>
Date:   Mon, 26 Jun 2023 10:28:07 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20230623173236.2513554-2-linan666@huaweicloud.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

?? 2023/06/24 1:32, linan666@huaweicloud.com д??:
> From: Li Nan <linan122@huawei.com>
> 
> 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 <yukuai3@huawei.com>

> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   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--;
>