Received-SPF: pass (google.com: domain of linux-kernel+bounces-76122-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <34b04a5f-7ef8-4e00-b1c6-c4e692281499@molgen.mpg.de>
Date: Thu, 22 Feb 2024 09:40:07 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH md-6.9 00/10] md/raid1: refactor read_balance() and some
 minor fix
Content-Language: en-US
To: Kuai Yu <yukuai1@huaweicloud.com>,
 Paul E Luse <paul.e.luse@linux.intel.com>
Cc: song@kernel.org, neilb@suse.com, shli@fb.com, linux-raid@vger.kernel.org,
 linux-kernel@vger.kernel.org, yukuai3@huawei.com, yi.zhang@huawei.com,
 yangerkun@huawei.com
References: <20240222075806.1816400-1-yukuai1@huaweicloud.com>
From: Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20240222075806.1816400-1-yukuai1@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Dear Kuai, dear Paul,


Thank you for your work. Some nits and request for benchmarks below.


Am 22.02.24 um 08:57 schrieb Yu Kuai:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> The orignial idea is that Paul want to optimize raid1 read

original

> performance([1]), however, we think that the orignial code for

original

> read_balance() is quite complex, and we don't want to add more
> complexity. Hence we decide to refactor read_balance() first, to make
> code cleaner and easier for follow up.
> 
> Before this patchset, read_balance() has many local variables and many
> braches, it want to consider all the scenarios in one iteration. The

branches

> idea of this patch is to devide them into 4 different steps:

divide

> 1) If resync is in progress, find the first usable disk, patch 5;
> Otherwise:
> 2) Loop through all disks and skipping slow disks and disks with bad
> blocks, choose the best disk, patch 10. If no disk is found:
> 3) Look for disks with bad blocks and choose the one with most number of
> sectors, patch 8. If no disk is found:
> 4) Choose first found slow disk with no bad blocks, or slow disk with
> most number of sectors, patch 7.
> 
> Note that step 3) and step 4) are super code path, and performance
> should not be considered.
> 
> And after this patchset, we'll continue to optimize read_balance for
> step 2), specifically how to choose the best rdev to read.

Is there a change in performance with the current patch set? Is radi1 
well enough covered by the test suite?


Kind regards,

Paul


> [1] https://lore.kernel.org/all/20240102125115.129261-1-paul.e.luse@linux.intel.com/
> 
> Yu Kuai (10):
>    md: add a new helper rdev_has_badblock()
>    md: record nonrot rdevs while adding/removing rdevs to conf
>    md/raid1: fix choose next idle in read_balance()
>    md/raid1-10: add a helper raid1_check_read_range()
>    md/raid1-10: factor out a new helper raid1_should_read_first()
>    md/raid1: factor out read_first_rdev() from read_balance()
>    md/raid1: factor out choose_slow_rdev() from read_balance()
>    md/raid1: factor out choose_bb_rdev() from read_balance()
>    md/raid1: factor out the code to manage sequential IO
>    md/raid1: factor out helpers to choose the best rdev from
>      read_balance()
> 
>   drivers/md/md.c       |  28 ++-
>   drivers/md/md.h       |  12 ++
>   drivers/md/raid1-10.c |  69 +++++++
>   drivers/md/raid1.c    | 454 ++++++++++++++++++++++++------------------
>   drivers/md/raid10.c   |  66 ++----
>   drivers/md/raid5.c    |  35 ++--
>   6 files changed, 402 insertions(+), 262 deletions(-)