Received-SPF: pass (google.com: domain of linux-kernel+bounces-82564-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-9-yukuai1@huaweicloud.com>
In-Reply-To: <20240222075806.1816400-9-yukuai1@huaweicloud.com>
From: Xiao Ni <xni@redhat.com>
Date: Tue, 27 Feb 2024 09:50:23 +0800
Message-ID: <CALTww285P14E8oucJuLunNL8H+hGeVa4LRpjurP1is3xjqTLQg@mail.gmail.com>
Subject: Re: [PATCH md-6.9 08/10] md/raid1: factor out choose_bb_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>
>
> read_balance() is hard to understand because there are too many status
> and branches, and it's overlong.
>
> This patch factor out the case to read the rdev with bad blocks from
> read_balance(), there are no functional changes.
>
> 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 | 79 ++++++++++++++++++++++++++++------------------
>  1 file changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index bc2f8fcbe5b3..4694e0e71e36 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -620,6 +620,44 @@ static int choose_first_rdev(struct r1conf *conf, st=
ruct r1bio *r1_bio,
>         return -1;
>  }
>
> +static int choose_bb_rdev(struct r1conf *conf, struct r1bio *r1_bio,
> +                         int *max_sectors)
> +{
> +       sector_t this_sector =3D r1_bio->sector;
> +       int best_disk =3D -1;
> +       int best_len =3D 0;
> +       int disk;
> +
> +       for (disk =3D 0 ; disk < conf->raid_disks * 2 ; disk++) {
> +               struct md_rdev *rdev;
> +               int len;
> +               int read_len;
> +
> +               if (r1_bio->bios[disk] =3D=3D IO_BLOCKED)
> +                       continue;
> +
> +               rdev =3D conf->mirrors[disk].rdev;
> +               if (!rdev || test_bit(Faulty, &rdev->flags) ||
> +                   test_bit(WriteMostly, &rdev->flags))
> +                       continue;
> +
> +               /* keep track of the disk with the most readable sectors.=
 */
> +               len =3D r1_bio->sectors;
> +               read_len =3D raid1_check_read_range(rdev, this_sector, &l=
en);
> +               if (read_len > best_len) {
> +                       best_disk =3D disk;
> +                       best_len =3D read_len;
> +               }
> +       }
> +
> +       if (best_disk !=3D -1) {
> +               *max_sectors =3D best_len;
> +               update_read_sectors(conf, best_disk, this_sector, best_le=
n);
> +       }
> +
> +       return best_disk;
> +}
> +
>  static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>                             int *max_sectors)
>  {
> @@ -707,8 +745,6 @@ static int read_balance(struct r1conf *conf, struct r=
1bio *r1_bio, int *max_sect
>
>         for (disk =3D 0 ; disk < conf->raid_disks * 2 ; disk++) {
>                 sector_t dist;
> -               sector_t first_bad;
> -               int bad_sectors;
>                 unsigned int pending;
>
>                 rdev =3D conf->mirrors[disk].rdev;
> @@ -721,36 +757,8 @@ static int read_balance(struct r1conf *conf, struct =
r1bio *r1_bio, int *max_sect
>                         continue;
>                 if (test_bit(WriteMostly, &rdev->flags))
>                         continue;
> -               /* This is a reasonable device to use.  It might
> -                * even be best.
> -                */
> -               if (is_badblock(rdev, this_sector, sectors,
> -                               &first_bad, &bad_sectors)) {
> -                       if (best_dist < MaxSector)
> -                               /* already have a better device */
> -                               continue;
> -                       if (first_bad <=3D this_sector) {
> -                               /* cannot read here. If this is the 'prim=
ary'
> -                                * device, then we must not read beyond
> -                                * bad_sectors from another device..
> -                                */
> -                               bad_sectors -=3D (this_sector - first_bad=
);
> -                               if (best_good_sectors > sectors)
> -                                       best_good_sectors =3D sectors;
> -
> -                       } else {
> -                               sector_t good_sectors =3D first_bad - thi=
s_sector;
> -                               if (good_sectors > best_good_sectors) {
> -                                       best_good_sectors =3D good_sector=
s;
> -                                       best_disk =3D disk;
> -                               }
> -                       }
> +               if (rdev_has_badblock(rdev, this_sector, sectors))
>                         continue;
> -               } else {
> -                       if ((sectors > best_good_sectors) && (best_disk >=
=3D 0))
> -                               best_disk =3D -1;
> -                       best_good_sectors =3D sectors;
> -               }
>
>                 if (best_disk >=3D 0)
>                         /* At least two disks to choose from so failfast =
is OK */
> @@ -834,6 +842,15 @@ static int read_balance(struct r1conf *conf, struct =
r1bio *r1_bio, int *max_sect
>         if (best_disk >=3D 0)
>                 return best_disk;
>
> +       /*
> +        * If we are here it means we didn't find a perfectly good disk s=
o
> +        * now spend a bit more time trying to find one with the most goo=
d
> +        * sectors.
> +        */
> +       disk =3D choose_bb_rdev(conf, r1_bio, max_sectors);
> +       if (disk >=3D 0)
> +               return disk;
> +
>         return choose_slow_rdev(conf, r1_bio, max_sectors);
>  }
>
> --
> 2.39.2
>
>
Hi
This patch looks good to me.
Reviewed-by: Xiao Ni <xni@redhat.com>