linux-kernel - Re: [PATCH md-6.9 07/10] md/raid1: factor out choose_slow_rdev() from read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALTww2-jdEos2HHgBHYHc0VOh1tWYuWTztmvW4iaBi6bS8B-YQ@mail.gmail.com>
Date: Mon, 26 Feb 2024 22:44:35 +0800
From: Xiao Ni <xni@...hat.com>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: paul.e.luse@...ux.intel.com, song@...nel.org, neilb@...e.com, shli@...com, 
	linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org, yukuai3@...wei.com, 
	yi.zhang@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH md-6.9 07/10] md/raid1: factor out choose_slow_rdev() from read_balance()

On Thu, Feb 22, 2024 at 4:04 PM Yu Kuai <yukuai1@...weicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@...wei.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 slow rdev from
> read_balance(), there are no functional changes.
>
> Co-developed-by: Paul Luse <paul.e.luse@...ux.intel.com>
> Signed-off-by: Paul Luse <paul.e.luse@...ux.intel.com>
> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
> ---
>  drivers/md/raid1.c | 69 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 08c45ca55a7e..bc2f8fcbe5b3 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -620,6 +620,53 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>         return -1;
>  }
>
> +static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
> +                           int *max_sectors)
> +{
> +       sector_t this_sector = r1_bio->sector;
> +       int bb_disk = -1;
> +       int bb_read_len = 0;
> +       int disk;
> +
> +       for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
> +               struct md_rdev *rdev;
> +               int len;
> +               int read_len;
> +
> +               if (r1_bio->bios[disk] == IO_BLOCKED)
> +                       continue;
> +
> +               rdev = conf->mirrors[disk].rdev;
> +               if (!rdev || test_bit(Faulty, &rdev->flags) ||
> +                   !test_bit(WriteMostly, &rdev->flags))
> +                       continue;
> +
> +               /* there are no bad blocks, we can use this disk */
> +               len = r1_bio->sectors;
> +               read_len = raid1_check_read_range(rdev, this_sector, &len);
> +               if (read_len == r1_bio->sectors) {
> +                       update_read_sectors(conf, disk, this_sector, read_len);
> +                       return disk;
> +               }
> +
> +               /*
> +                * there are partial bad blocks, choose the rdev with largest
> +                * read length.
> +                */
> +               if (read_len > bb_read_len) {
> +                       bb_disk = disk;
> +                       bb_read_len = read_len;
> +               }
> +       }
> +
> +       if (bb_disk != -1) {
> +               *max_sectors = bb_read_len;
> +               update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
> +       }
> +
> +       return bb_disk;
> +}
> +
>  /*
>   * This routine returns the disk from which the requested read should
>   * be done. There is a per-array 'next expected sequential IO' sector
> @@ -672,23 +719,8 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
>                 if (!test_bit(In_sync, &rdev->flags) &&
>                     rdev->recovery_offset < this_sector + sectors)
>                         continue;
> -               if (test_bit(WriteMostly, &rdev->flags)) {
> -                       /* Don't balance among write-mostly, just
> -                        * use the first as a last resort */
> -                       if (best_dist_disk < 0) {
> -                               if (is_badblock(rdev, this_sector, sectors,
> -                                               &first_bad, &bad_sectors)) {
> -                                       if (first_bad <= this_sector)
> -                                               /* Cannot use this */
> -                                               continue;
> -                                       best_good_sectors = first_bad - this_sector;
> -                               } else
> -                                       best_good_sectors = sectors;
> -                               best_dist_disk = disk;
> -                               best_pending_disk = disk;
> -                       }
> +               if (test_bit(WriteMostly, &rdev->flags))
>                         continue;
> -               }
>                 /* This is a reasonable device to use.  It might
>                  * even be best.
>                  */
> @@ -799,7 +831,10 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
>         }
>         *max_sectors = sectors;
>
> -       return best_disk;
> +       if (best_disk >= 0)
> +               return best_disk;
> +
> +       return choose_slow_rdev(conf, r1_bio, max_sectors);
>  }
>
>  static void wake_up_barrier(struct r1conf *conf)
> --
> 2.39.2
>
>

This patch looks good to me.
Reviewed-by: Xiao Ni <xni@...hat.com>