Received-SPF: pass (google.com: domain of linux-kernel+bounces-82574-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240222075806.1816400-1-yukuai1@huaweicloud.com> <20240222075806.1816400-10-yukuai1@huaweicloud.com>
In-Reply-To: <20240222075806.1816400-10-yukuai1@huaweicloud.com>
From: Xiao Ni <xni@redhat.com>
Date: Tue, 27 Feb 2024 10:04:06 +0800
Message-ID: <CALTww29ajdNAXQwAYG90HC26b_hZQz=s28nCsJazwkQ+YsW53w@mail.gmail.com>
Subject: Re: [PATCH md-6.9 09/10] md/raid1: factor out the code to manage
 sequential IO
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: paul.e.luse@linux.intel.com, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 22, 2024 at 4:05=E2=80=AFPM Yu Kuai <yukuai1@huaweicloud.com> w=
rote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> There is no functional change for now, make read_balance() cleaner and
> prepare to fix problems and refactor the handler of sequential IO.
>
> Co-developed-by: Paul Luse <paul.e.luse@linux.intel.com>
> Signed-off-by: Paul Luse <paul.e.luse@linux.intel.com>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/raid1.c | 71 +++++++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4694e0e71e36..223ef8d06f67 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -705,6 +705,31 @@ static int choose_slow_rdev(struct r1conf *conf, str=
uct r1bio *r1_bio,
>         return bb_disk;
>  }
>
> +static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r=
1_bio)
> +{
> +       /* TODO: address issues with this check and concurrency. */
> +       return conf->mirrors[disk].next_seq_sect =3D=3D r1_bio->sector ||
> +              conf->mirrors[disk].head_position =3D=3D r1_bio->sector;
> +}
> +
> +/*
> + * If buffered sequential IO size exceeds optimal iosize, check if there=
 is idle
> + * disk. If yes, choose the idle disk.
> + */
> +static bool should_choose_next(struct r1conf *conf, int disk)
> +{
> +       struct raid1_info *mirror =3D &conf->mirrors[disk];
> +       int opt_iosize;
> +
> +       if (!test_bit(Nonrot, &mirror->rdev->flags))
> +               return false;
> +
> +       opt_iosize =3D bdev_io_opt(mirror->rdev->bdev) >> 9;
> +       return opt_iosize > 0 && mirror->seq_start !=3D MaxSector &&
> +              mirror->next_seq_sect > opt_iosize &&
> +              mirror->next_seq_sect - opt_iosize >=3D mirror->seq_start;
> +}
> +
>  /*
>   * This routine returns the disk from which the requested read should
>   * be done. There is a per-array 'next expected sequential IO' sector
> @@ -767,42 +792,22 @@ static int read_balance(struct r1conf *conf, struct=
 r1bio *r1_bio, int *max_sect
>                 pending =3D atomic_read(&rdev->nr_pending);
>                 dist =3D abs(this_sector - conf->mirrors[disk].head_posit=
ion);
>                 /* Don't change to another disk for sequential reads */
> -               if (conf->mirrors[disk].next_seq_sect =3D=3D this_sector
> -                   || dist =3D=3D 0) {
> -                       int opt_iosize =3D bdev_io_opt(rdev->bdev) >> 9;
> -                       struct raid1_info *mirror =3D &conf->mirrors[disk=
];
> -
> -                       /*
> -                        * If buffered sequential IO size exceeds optimal
> -                        * iosize, check if there is idle disk. If yes, c=
hoose
> -                        * the idle disk. read_balance could already choo=
se an
> -                        * idle disk before noticing it's a sequential IO=
 in
> -                        * this disk. This doesn't matter because this di=
sk
> -                        * will idle, next time it will be utilized after=
 the
> -                        * first disk has IO size exceeds optimal iosize.=
 In
> -                        * this way, iosize of the first disk will be opt=
imal
> -                        * iosize at least. iosize of the second disk mig=
ht be
> -                        * small, but not a big deal since when the secon=
d disk
> -                        * starts IO, the first disk is likely still busy=
.
> -                        */
> -                       if (test_bit(Nonrot, &rdev->flags) && opt_iosize =
> 0 &&
> -                           mirror->seq_start !=3D MaxSector &&
> -                           mirror->next_seq_sect > opt_iosize &&
> -                           mirror->next_seq_sect - opt_iosize >=3D
> -                           mirror->seq_start) {
> -                               /*
> -                                * Add 'pending' to avoid choosing this d=
isk if
> -                                * there is other idle disk.
> -                                * Set 'dist' to 0, so that if there is n=
o other
> -                                * idle disk and all disks are rotational=
, this
> -                                * disk will still be chosen.
> -                                */
> -                               pending++;
> -                               dist =3D 0;
> -                       } else {
> +               if (is_sequential(conf, disk, r1_bio)) {
> +                       if (!should_choose_next(conf, disk)) {
>                                 best_disk =3D disk;
>                                 break;
>                         }
> +
> +                       /*
> +                        * Add 'pending' to avoid choosing this disk if t=
here is
> +                        * other idle disk.
> +                        */
> +                       pending++;
> +                       /*
> +                        * Set 'dist' to 0, so that if there is no other =
idle
> +                        * disk, this disk will still be chosen.
> +                        */
> +                       dist =3D 0;
>                 }
>
>                 if (min_pending > pending) {
> --
> 2.39.2
>
>
Hi
This patch looks good to me.
Reviewed-by: Xiao Ni <xni@redhat.com>