Received-SPF: pass (google.com: domain of linux-kernel+bounces-82708-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240222075806.1816400-1-yukuai1@huaweicloud.com> <20240222075806.1816400-11-yukuai1@huaweicloud.com>
In-Reply-To: <20240222075806.1816400-11-yukuai1@huaweicloud.com>
From: Xiao Ni <xni@redhat.com>
Date: Tue, 27 Feb 2024 12:47:50 +0800
Message-ID: <CALTww28=sZNJESGX5fjuW5TaM0rSO6PoR_GgJZY3e+yF3KWobA@mail.gmail.com>
Subject: Re: [PATCH md-6.9 10/10] md/raid1: factor out helpers to choose the
 best rdev from read_balance()
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:06=E2=80=AFPM Yu Kuai <yukuai1@huaweicloud.com> w=
rote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> The way that best rdev is chosen:
>
> 1) If the read is sequential from one rdev:
>  - if rdev is rotational, use this rdev;
>  - if rdev is non-rotational, use this rdev until total read length
>    exceed disk opt io size;
>
> 2) If the read is not sequential:
>  - if there is idle disk, use it, otherwise:
>  - if the array has non-rotational disk, choose the rdev with minimal
>    inflight IO;
>  - if all the underlaying disks are rotational disk, choose the rdev
>    with closest IO;
>
> There are no functional changes, just to make code cleaner and prepare
> for following refactor.
>
> 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 | 171 ++++++++++++++++++++++++---------------------
>  1 file changed, 92 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 223ef8d06f67..938b0e0170df 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -730,73 +730,68 @@ static bool should_choose_next(struct r1conf *conf,=
 int disk)
>                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
> - * number - if this matches on the next IO then we use the last disk.
> - * There is also a per-disk 'last know head position' sector that is
> - * maintained from IRQ contexts, both the normal and the resync IO
> - * completion handlers update this position correctly. If there is no
> - * perfect sequential match then we pick the disk whose head is closest.
> - *
> - * If there are 2 mirrors in the same 2 devices, performance degrades
> - * because position is mirror, not device based.
> - *
> - * The rdev for the device selected will have nr_pending incremented.
> - */
> -static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *=
max_sectors)
> +static bool rdev_readable(struct md_rdev *rdev, struct r1bio *r1_bio)
>  {
> -       const sector_t this_sector =3D r1_bio->sector;
> -       int sectors;
> -       int best_good_sectors;
> -       int best_disk, best_dist_disk, best_pending_disk;
> -       int disk;
> -       sector_t best_dist;
> -       unsigned int min_pending;
> -       struct md_rdev *rdev;
> +       if (!rdev || test_bit(Faulty, &rdev->flags))
> +               return false;
>
> - retry:
> -       sectors =3D r1_bio->sectors;
> -       best_disk =3D -1;
> -       best_dist_disk =3D -1;
> -       best_dist =3D MaxSector;
> -       best_pending_disk =3D -1;
> -       min_pending =3D UINT_MAX;
> -       best_good_sectors =3D 0;
> -       clear_bit(R1BIO_FailFast, &r1_bio->state);
> +       /* still in recovery */
> +       if (!test_bit(In_sync, &rdev->flags) &&
> +           rdev->recovery_offset < r1_bio->sector + r1_bio->sectors)
> +               return false;
>
> -       if (raid1_should_read_first(conf->mddev, this_sector, sectors))
> -               return choose_first_rdev(conf, r1_bio, max_sectors);
> +       /* don't read from slow disk unless have to */
> +       if (test_bit(WriteMostly, &rdev->flags))
> +               return false;
> +
> +       /* don't split IO for bad blocks unless have to */
> +       if (rdev_has_badblock(rdev, r1_bio->sector, r1_bio->sectors))
> +               return false;
> +
> +       return true;
> +}
> +
> +struct read_balance_ctl {
> +       sector_t closest_dist;
> +       int closest_dist_disk;
> +       int min_pending;
> +       int min_pending_disk;
> +       int readable_disks;
> +};
> +
> +static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
> +{
> +       int disk;
> +       struct read_balance_ctl ctl =3D {
> +               .closest_dist_disk      =3D -1,
> +               .closest_dist           =3D MaxSector,
> +               .min_pending_disk       =3D -1,
> +               .min_pending            =3D UINT_MAX,
> +       };
>
>         for (disk =3D 0 ; disk < conf->raid_disks * 2 ; disk++) {
> +               struct md_rdev *rdev;
>                 sector_t dist;
>                 unsigned int pending;
>
> -               rdev =3D conf->mirrors[disk].rdev;
> -               if (r1_bio->bios[disk] =3D=3D IO_BLOCKED
> -                   || rdev =3D=3D NULL
> -                   || test_bit(Faulty, &rdev->flags))
> -                       continue;
> -               if (!test_bit(In_sync, &rdev->flags) &&
> -                   rdev->recovery_offset < this_sector + sectors)
> -                       continue;
> -               if (test_bit(WriteMostly, &rdev->flags))
> +               if (r1_bio->bios[disk] =3D=3D IO_BLOCKED)
>                         continue;
> -               if (rdev_has_badblock(rdev, this_sector, sectors))
> +
> +               rdev =3D conf->mirrors[disk].rdev;
> +               if (!rdev_readable(rdev, r1_bio))
>                         continue;
>
> -               if (best_disk >=3D 0)
> -                       /* At least two disks to choose from so failfast =
is OK */
> +               /* At least two disks to choose from so failfast is OK */
> +               if (ctl.readable_disks++ =3D=3D 1)
>                         set_bit(R1BIO_FailFast, &r1_bio->state);
>
>                 pending =3D atomic_read(&rdev->nr_pending);
> -               dist =3D abs(this_sector - conf->mirrors[disk].head_posit=
ion);
> +               dist =3D abs(r1_bio->sector - conf->mirrors[disk].head_po=
sition);
> +
>                 /* Don't change to another disk for sequential reads */
>                 if (is_sequential(conf, disk, r1_bio)) {
> -                       if (!should_choose_next(conf, disk)) {
> -                               best_disk =3D disk;
> -                               break;
> -                       }
> +                       if (!should_choose_next(conf, disk))
> +                               return disk;
>
>                         /*
>                          * Add 'pending' to avoid choosing this disk if t=
here is
> @@ -810,42 +805,60 @@ static int read_balance(struct r1conf *conf, struct=
 r1bio *r1_bio, int *max_sect
>                         dist =3D 0;
>                 }
>
> -               if (min_pending > pending) {
> -                       min_pending =3D pending;
> -                       best_pending_disk =3D disk;
> +               if (ctl.min_pending > pending) {
> +                       ctl.min_pending =3D pending;
> +                       ctl.min_pending_disk =3D disk;
>                 }
>
> -               if (dist < best_dist) {
> -                       best_dist =3D dist;
> -                       best_dist_disk =3D disk;
> +               if (dist < ctl.closest_dist) {
> +                       ctl.closest_dist =3D dist;
> +                       ctl.closest_dist_disk =3D disk;
>                 }
>         }
>
> -       /*
> -        * If all disks are rotational, choose the closest disk. If any d=
isk is
> -        * non-rotational, choose the disk with less pending request even=
 the
> -        * disk is rotational, which might/might not be optimal for raids=
 with
> -        * mixed ratation/non-rotational disks depending on workload.
> -        */
> -       if (best_disk =3D=3D -1) {
> -               if (conf->mddev->nonrot_disks || min_pending =3D=3D 0)
> -                       best_disk =3D best_pending_disk;
> -               else
> -                       best_disk =3D best_dist_disk;
> -       }
>
> -       if (best_disk >=3D 0) {
> -               rdev =3D conf->mirrors[best_disk].rdev;
> -               if (!rdev)
> -                       goto retry;
> +       if (ctl.min_pending_disk !=3D -1 &&
> +           (conf->mddev->nonrot_disks || ctl.min_pending =3D=3D 0))
> +               return ctl.min_pending_disk;
> +       else
> +               return ctl.closest_dist_disk;
> +}
>
> -               sectors =3D best_good_sectors;
> -               update_read_sectors(conf, disk, this_sector, sectors);
> -       }
> -       *max_sectors =3D sectors;
> +/*
> + * This routine returns the disk from which the requested read should be=
 done.
> + *
> + * 1) If resync is in progress, find the first usable disk and use
> + * it even if it has some bad blocks.
> + *
> + * 2) Now that there is no resync, loop through all disks and skipping s=
low
> + * disks and disks with bad blocks for now. Only pay attention to key di=
sk
> + * choice.
> + *
> + * 3) If we've made it this far, now look for disks with bad blocks and =
choose
> + * the one with most number of sectors.
> + *
> + * 4) If we are all the way at the end, we have no choice but to use a d=
isk even
> + * if it is write mostly.
> +
> + * The rdev for the device selected will have nr_pending incremented.
> + */
> +static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *=
max_sectors)
> +{
> +       int disk;
> +
> +       clear_bit(R1BIO_FailFast, &r1_bio->state);
>
> -       if (best_disk >=3D 0)
> -               return best_disk;
> +       if (raid1_should_read_first(conf->mddev, r1_bio->sector,
> +                                   r1_bio->sectors))
> +               return choose_first_rdev(conf, r1_bio, max_sectors);
> +
> +       disk =3D choose_best_rdev(conf, r1_bio);
> +       if (disk >=3D 0) {
> +               *max_sectors =3D r1_bio->sectors;
> +               update_read_sectors(conf, disk, r1_bio->sector,
> +                                   r1_bio->sectors);
> +               return disk;
> +       }
>
>         /*
>          * If we are here it means we didn't find a perfectly good disk s=
o
> --
> 2.39.2
>
>

Hi
This patch looks good to me. Thanks very much for the effort. Now the
read_balance is more easy to read and understand.
Reviewed-by: Xiao Ni <xni@redhat.com>