Re: [PATCH md-6.9 02/10] md: record nonrot rdevs while adding/removing rdevs to conf

From: Xiao Ni
Date: Mon Feb 26 2024 - 08:28:27 EST

Next message: Mark Brown: "Re: [PATCH v4 0/4] spi: cadence-qspi: Fix runtime PM and system-wide suspend"
Previous message: Ilpo Järvinen: "Re: [PATCH 0/2] map Fn + R key on newer Yogas and Legions"
In reply to: Yu Kuai: "Re: [PATCH md-6.9 02/10] md: record nonrot rdevs while adding/removing rdevs to conf"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 26, 2024 at 9:25 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> 在 2024/02/26 21:12, Xiao Ni 写道:
> > Hi Kuai
> >
> > I added some logs to check and add_bound_rdev can't be called when
> > creating raid device. Maybe move rdev_update_nonrot to
> > bind_rdev_to_array?
>
> bind_rdev_to_array() is used to add new rdev to the array, then
> 'pers->hot_add_disk' is used to add the rdev to conf. For new spares,
> bind_rdev_to_array() will be called while 'pers->hot_add_disk' won't.
> Hence rdev_update_nonrot() is used where 'pers->hot_add_disk' succeed
> in this patch.
>
> Perhaps it's better to move related code to raid1/raid10_add_disk() and
> the new counter in raid1/raid10 conf?

Yes, this sounds better.

Regards
Xiao
>
> Thanks,
> Kuai
>
> >
> > Regards
> > Xiao
> >
> > On Thu, Feb 22, 2024 at 4:04 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> >>
> >> From: Yu Kuai <yukuai3@xxxxxxxxxx>
> >>
> >> For raid1/raid10, each read will iterate all the rdevs from conf and
> >> check if any rdev is non-rotational, then choose rdev with minimal IO
> >> inflight if so, or rdev with closest distance otherwise.
> >>
> >> Disk nonrot info can be changed through sysfs entry:
> >>
> >> /sys/block/[disk_name]/queue/rotational
> >>
> >> However, consider that this should only be used for testing, and user
> >> really shouldn't do this in real life. Record the number of non-rotational
> >> disks in mddev, to avoid checking each rdev in IO fast path and simplify
> >> read_balance() a little bit.
> >>
> >> Co-developed-by: Paul Luse <paul.e.luse@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Paul Luse <paul.e.luse@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> >> ---
> >> drivers/md/md.c | 28 ++++++++++++++++++++++++++--
> >> drivers/md/md.h | 2 ++
> >> drivers/md/raid1.c | 9 ++-------
> >> drivers/md/raid10.c | 8 ++------
> >> 4 files changed, 32 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index e2a5f513dbb7..9e671eec9309 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -146,6 +146,24 @@ static inline int speed_max(struct mddev *mddev)
> >> mddev->sync_speed_max : sysctl_speed_limit_max;
> >> }
> >>
> >> +static void rdev_update_nonrot(struct md_rdev *rdev)
> >> +{
> >> + if (!bdev_nonrot(rdev->bdev))
> >> + return;
> >> +
> >> + set_bit(Nonrot, &rdev->flags);
> >> + WRITE_ONCE(rdev->mddev->nonrot_disks, rdev->mddev->nonrot_disks + 1);
> >> +}
> >> +
> >> +static void rdev_clear_nonrot(struct md_rdev *rdev)
> >> +{
> >> + if (!test_bit(Nonrot, &rdev->flags))
> >> + return;
> >> +
> >> + clear_bit(Nonrot, &rdev->flags);
> >> + WRITE_ONCE(rdev->mddev->nonrot_disks, rdev->mddev->nonrot_disks - 1);
> >> +}
> >> +
> >> static void rdev_uninit_serial(struct md_rdev *rdev)
> >> {
> >> if (!test_and_clear_bit(CollisionCheck, &rdev->flags))
> >> @@ -2922,6 +2940,8 @@ static int add_bound_rdev(struct md_rdev *rdev)
> >> md_kick_rdev_from_array(rdev);
> >> return err;
> >> }
> >> +
> >> + rdev_update_nonrot(rdev);
> >> }
> >> sysfs_notify_dirent_safe(rdev->sysfs_state);
> >>
> >> @@ -3271,8 +3291,10 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len)
> >> if (err) {
> >> rdev->raid_disk = -1;
> >> return err;
> >> - } else
> >> - sysfs_notify_dirent_safe(rdev->sysfs_state);
> >> + }
> >> +
> >> + rdev_update_nonrot(rdev);
> >> + sysfs_notify_dirent_safe(rdev->sysfs_state);
> >> /* failure here is OK */;
> >> sysfs_link_rdev(rdev->mddev, rdev);
> >> /* don't wakeup anyone, leave that to userspace. */
> >> @@ -9266,6 +9288,7 @@ static int remove_and_add_spares(struct mddev *mddev,
> >> rdev_for_each(rdev, mddev) {
> >> if ((this == NULL || rdev == this) && rdev_removeable(rdev) &&
> >> !mddev->pers->hot_remove_disk(mddev, rdev)) {
> >> + rdev_clear_nonrot(rdev);
> >> sysfs_unlink_rdev(mddev, rdev);
> >> rdev->saved_raid_disk = rdev->raid_disk;
> >> rdev->raid_disk = -1;
> >> @@ -9289,6 +9312,7 @@ static int remove_and_add_spares(struct mddev *mddev,
> >> if (!test_bit(Journal, &rdev->flags))
> >> rdev->recovery_offset = 0;
> >> if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
> >> + rdev_update_nonrot(rdev);
> >> /* failure here is OK */
> >> sysfs_link_rdev(mddev, rdev);
> >> if (!test_bit(Journal, &rdev->flags))
> >> diff --git a/drivers/md/md.h b/drivers/md/md.h
> >> index a49ab04ab707..54aa951f2bba 100644
> >> --- a/drivers/md/md.h
> >> +++ b/drivers/md/md.h
> >> @@ -207,6 +207,7 @@ enum flag_bits {
> >> * check if there is collision between raid1
> >> * serial bios.
> >> */
> >> + Nonrot, /* non-rotational device (SSD) */
> >> };
> >>
> >> static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
> >> @@ -312,6 +313,7 @@ struct mddev {
> >> unsigned long flags;
> >> unsigned long sb_flags;
> >>
> >> + int nonrot_disks;
> >> int suspended;
> >> struct mutex suspend_mutex;
> >> struct percpu_ref active_io;
> >> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> >> index a145fe48b9ce..c60ea58ae8c5 100644
> >> --- a/drivers/md/raid1.c
> >> +++ b/drivers/md/raid1.c
> >> @@ -599,7 +599,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> >> int sectors;
> >> int best_good_sectors;
> >> int best_disk, best_dist_disk, best_pending_disk;
> >> - int has_nonrot_disk;
> >> int disk;
> >> sector_t best_dist;
> >> unsigned int min_pending;
> >> @@ -620,7 +619,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> >> best_pending_disk = -1;
> >> min_pending = UINT_MAX;
> >> best_good_sectors = 0;
> >> - has_nonrot_disk = 0;
> >> choose_next_idle = 0;
> >> clear_bit(R1BIO_FailFast, &r1_bio->state);
> >>
> >> @@ -637,7 +635,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> >> sector_t first_bad;
> >> int bad_sectors;
> >> unsigned int pending;
> >> - bool nonrot;
> >>
> >> rdev = conf->mirrors[disk].rdev;
> >> if (r1_bio->bios[disk] == IO_BLOCKED
> >> @@ -703,8 +700,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> >> /* At least two disks to choose from so failfast is OK */
> >> set_bit(R1BIO_FailFast, &r1_bio->state);
> >>
> >> - nonrot = bdev_nonrot(rdev->bdev);
> >> - has_nonrot_disk |= nonrot;
> >> pending = atomic_read(&rdev->nr_pending);
> >> dist = abs(this_sector - conf->mirrors[disk].head_position);
> >> if (choose_first) {
> >> @@ -731,7 +726,7 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> >> * small, but not a big deal since when the second disk
> >> * starts IO, the first disk is likely still busy.
> >> */
> >> - if (nonrot && opt_iosize > 0 &&
> >> + if (test_bit(Nonrot, &rdev->flags) && opt_iosize > 0 &&
> >> mirror->seq_start != MaxSector &&
> >> mirror->next_seq_sect > opt_iosize &&
> >> mirror->next_seq_sect - opt_iosize >=
> >> @@ -763,7 +758,7 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> >> * mixed ratation/non-rotational disks depending on workload.
> >> */
> >> if (best_disk == -1) {
> >> - if (has_nonrot_disk || min_pending == 0)
> >> + if (conf->mddev->nonrot_disks || min_pending == 0)
> >> best_disk = best_pending_disk;
> >> else
> >> best_disk = best_dist_disk;
> >> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >> index d5a7a621f0f0..1f6693e40e12 100644
> >> --- a/drivers/md/raid10.c
> >> +++ b/drivers/md/raid10.c
> >> @@ -735,7 +735,6 @@ static struct md_rdev *read_balance(struct r10conf *conf,
> >> struct md_rdev *best_dist_rdev, *best_pending_rdev, *rdev = NULL;
> >> int do_balance;
> >> int best_dist_slot, best_pending_slot;
> >> - bool has_nonrot_disk = false;
> >> unsigned int min_pending;
> >> struct geom *geo = &conf->geo;
> >>
> >> @@ -766,7 +765,6 @@ static struct md_rdev *read_balance(struct r10conf *conf,
> >> int bad_sectors;
> >> sector_t dev_sector;
> >> unsigned int pending;
> >> - bool nonrot;
> >>
> >> if (r10_bio->devs[slot].bio == IO_BLOCKED)
> >> continue;
> >> @@ -818,10 +816,8 @@ static struct md_rdev *read_balance(struct r10conf *conf,
> >> if (!do_balance)
> >> break;
> >>
> >> - nonrot = bdev_nonrot(rdev->bdev);
> >> - has_nonrot_disk |= nonrot;
> >> pending = atomic_read(&rdev->nr_pending);
> >> - if (min_pending > pending && nonrot) {
> >> + if (min_pending > pending && test_bit(Nonrot, &rdev->flags)) {
> >> min_pending = pending;
> >> best_pending_slot = slot;
> >> best_pending_rdev = rdev;
> >> @@ -851,7 +847,7 @@ static struct md_rdev *read_balance(struct r10conf *conf,
> >> }
> >> }
> >> if (slot >= conf->copies) {
> >> - if (has_nonrot_disk) {
> >> + if (conf->mddev->nonrot_disks) {
> >> slot = best_pending_slot;
> >> rdev = best_pending_rdev;
> >> } else {
> >> --
> >> 2.39.2
> >>
> >>
> >
> > .
> >
>

Next message: Mark Brown: "Re: [PATCH v4 0/4] spi: cadence-qspi: Fix runtime PM and system-wide suspend"
Previous message: Ilpo Järvinen: "Re: [PATCH 0/2] map Fn + R key on newer Yogas and Legions"
In reply to: Yu Kuai: "Re: [PATCH md-6.9 02/10] md: record nonrot rdevs while adding/removing rdevs to conf"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]