* [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix
@ 2024-02-29 9:57 Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 01/11] md: add a new helper rdev_has_badblock() Yu Kuai
` (11 more replies)
0 siblings, 12 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
Changes in v4:
- fix a problem in v2, that replacement rdev->raid_disk is set to
raid_disk + conf->mirros, which will cause test 01replace to run
forever, and mdadm tests looks good now(no new regression);
Changes in v3:
- add patch 2, and fix that setup_conf() is missing in patch3;
- add some review tag from Xiao Ni(other than patch 2,3);
Changes in v2:
- add new conter in conf for patch 2;
- fix the case choose next idle while there is no other idle disk in
patch 3;
- add some review tag from Xiao Ni for patch 1, 4-8
The original idea is that Paul want to optimize raid1 read
performance([1]), however, we think that the original code for
read_balance() is quite complex, and we don't want to add more
complexity. Hence we decide to refactor read_balance() first, to make
code cleaner and easier for follow up.
Before this patchset, read_balance() has many local variables and many
branches, it want to consider all the scenarios in one iteration. The
idea of this patch is to divide them into 4 different steps:
1) If resync is in progress, find the first usable disk, patch 5;
Otherwise:
2) Loop through all disks and skipping slow disks and disks with bad
blocks, choose the best disk, patch 10. If no disk is found:
3) Look for disks with bad blocks and choose the one with most number of
sectors, patch 8. If no disk is found:
4) Choose first found slow disk with no bad blocks, or slow disk with
most number of sectors, patch 7.
Note that step 3) and step 4) are super code path, and performance
should not be considered.
And after this patchset, we'll continue to optimize read_balance for
step 2), specifically how to choose the best rdev to read.
[1] https://lore.kernel.org/all/20240102125115.129261-1-paul.e.luse@linux.intel.com/
Yu Kuai (11):
md: add a new helper rdev_has_badblock()
md/raid1: factor out helpers to add rdev to conf
md/raid1: record nonrot rdevs while adding/removing rdevs to conf
md/raid1: fix choose next idle in read_balance()
md/raid1-10: add a helper raid1_check_read_range()
md/raid1-10: factor out a new helper raid1_should_read_first()
md/raid1: factor out read_first_rdev() from read_balance()
md/raid1: factor out choose_slow_rdev() from read_balance()
md/raid1: factor out choose_bb_rdev() from read_balance()
md/raid1: factor out the code to manage sequential IO
md/raid1: factor out helpers to choose the best rdev from
read_balance()
drivers/md/md.h | 11 +
drivers/md/raid1-10.c | 69 ++++++
drivers/md/raid1.c | 550 +++++++++++++++++++++++++-----------------
drivers/md/raid1.h | 1 +
drivers/md/raid10.c | 58 ++---
drivers/md/raid5.c | 35 +--
6 files changed, 444 insertions(+), 280 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 01/11] md: add a new helper rdev_has_badblock()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 02/11] md/raid1: factor out helpers to add rdev to conf Yu Kuai
` (10 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
The current api is_badblock() must pass in 'first_bad' and
'bad_sectors', however, many caller just want to know if there are
badblocks or not, and these caller must define two local variable that
will never be used.
Add a new helper rdev_has_badblock() that will only return if there are
badblocks or not, remove unnecessary local variables and replace
is_badblock() with the new helper in many places.
There are no functional changes, and the new helper will also be used
later to refactor read_balance().
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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/md.h | 10 ++++++++++
drivers/md/raid1.c | 26 +++++++-------------------
drivers/md/raid10.c | 45 ++++++++++++++-------------------------------
drivers/md/raid5.c | 35 +++++++++++++----------------------
4 files changed, 44 insertions(+), 72 deletions(-)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 8d881cc59799..a49ab04ab707 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -222,6 +222,16 @@ static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
}
return 0;
}
+
+static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
+ int sectors)
+{
+ sector_t first_bad;
+ int bad_sectors;
+
+ return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
+}
+
extern int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
int is_new);
extern int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 286f8b16c7bd..a145fe48b9ce 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -498,9 +498,6 @@ static void raid1_end_write_request(struct bio *bio)
* to user-side. So if something waits for IO, then it
* will wait for the 'master' bio.
*/
- sector_t first_bad;
- int bad_sectors;
-
r1_bio->bios[mirror] = NULL;
to_put = bio;
/*
@@ -516,8 +513,8 @@ static void raid1_end_write_request(struct bio *bio)
set_bit(R1BIO_Uptodate, &r1_bio->state);
/* Maybe we can clear some bad blocks. */
- if (is_badblock(rdev, r1_bio->sector, r1_bio->sectors,
- &first_bad, &bad_sectors) && !discard_error) {
+ if (rdev_has_badblock(rdev, r1_bio->sector, r1_bio->sectors) &&
+ !discard_error) {
r1_bio->bios[mirror] = IO_MADE_GOOD;
set_bit(R1BIO_MadeGood, &r1_bio->state);
}
@@ -1944,8 +1941,6 @@ static void end_sync_write(struct bio *bio)
struct r1bio *r1_bio = get_resync_r1bio(bio);
struct mddev *mddev = r1_bio->mddev;
struct r1conf *conf = mddev->private;
- sector_t first_bad;
- int bad_sectors;
struct md_rdev *rdev = conf->mirrors[find_bio_disk(r1_bio, bio)].rdev;
if (!uptodate) {
@@ -1955,14 +1950,11 @@ static void end_sync_write(struct bio *bio)
set_bit(MD_RECOVERY_NEEDED, &
mddev->recovery);
set_bit(R1BIO_WriteError, &r1_bio->state);
- } else if (is_badblock(rdev, r1_bio->sector, r1_bio->sectors,
- &first_bad, &bad_sectors) &&
- !is_badblock(conf->mirrors[r1_bio->read_disk].rdev,
- r1_bio->sector,
- r1_bio->sectors,
- &first_bad, &bad_sectors)
- )
+ } else if (rdev_has_badblock(rdev, r1_bio->sector, r1_bio->sectors) &&
+ !rdev_has_badblock(conf->mirrors[r1_bio->read_disk].rdev,
+ r1_bio->sector, r1_bio->sectors)) {
set_bit(R1BIO_MadeGood, &r1_bio->state);
+ }
put_sync_write_buf(r1_bio, uptodate);
}
@@ -2279,16 +2271,12 @@ static void fix_read_error(struct r1conf *conf, struct r1bio *r1_bio)
s = PAGE_SIZE >> 9;
do {
- sector_t first_bad;
- int bad_sectors;
-
rdev = conf->mirrors[d].rdev;
if (rdev &&
(test_bit(In_sync, &rdev->flags) ||
(!test_bit(Faulty, &rdev->flags) &&
rdev->recovery_offset >= sect + s)) &&
- is_badblock(rdev, sect, s,
- &first_bad, &bad_sectors) == 0) {
+ rdev_has_badblock(rdev, sect, s) == 0) {
atomic_inc(&rdev->nr_pending);
if (sync_page_io(rdev, sect, s<<9,
conf->tmppage, REQ_OP_READ, false))
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 7412066ea22c..d5a7a621f0f0 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -518,11 +518,7 @@ static void raid10_end_write_request(struct bio *bio)
* The 'master' represents the composite IO operation to
* user-side. So if something waits for IO, then it will
* wait for the 'master' bio.
- */
- sector_t first_bad;
- int bad_sectors;
-
- /*
+ *
* Do not set R10BIO_Uptodate if the current device is
* rebuilding or Faulty. This is because we cannot use
* such device for properly reading the data back (we could
@@ -535,10 +531,9 @@ static void raid10_end_write_request(struct bio *bio)
set_bit(R10BIO_Uptodate, &r10_bio->state);
/* Maybe we can clear some bad blocks. */
- if (is_badblock(rdev,
- r10_bio->devs[slot].addr,
- r10_bio->sectors,
- &first_bad, &bad_sectors) && !discard_error) {
+ if (rdev_has_badblock(rdev, r10_bio->devs[slot].addr,
+ r10_bio->sectors) &&
+ !discard_error) {
bio_put(bio);
if (repl)
r10_bio->devs[slot].repl_bio = IO_MADE_GOOD;
@@ -1330,10 +1325,7 @@ static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
}
if (rdev && test_bit(WriteErrorSeen, &rdev->flags)) {
- sector_t first_bad;
sector_t dev_sector = r10_bio->devs[i].addr;
- int bad_sectors;
- int is_bad;
/*
* Discard request doesn't care the write result
@@ -1342,9 +1334,8 @@ static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
if (!r10_bio->sectors)
continue;
- is_bad = is_badblock(rdev, dev_sector, r10_bio->sectors,
- &first_bad, &bad_sectors);
- if (is_bad < 0) {
+ if (rdev_has_badblock(rdev, dev_sector,
+ r10_bio->sectors) < 0) {
/*
* Mustn't write here until the bad block
* is acknowledged
@@ -2290,8 +2281,6 @@ static void end_sync_write(struct bio *bio)
struct mddev *mddev = r10_bio->mddev;
struct r10conf *conf = mddev->private;
int d;
- sector_t first_bad;
- int bad_sectors;
int slot;
int repl;
struct md_rdev *rdev = NULL;
@@ -2312,11 +2301,10 @@ static void end_sync_write(struct bio *bio)
&rdev->mddev->recovery);
set_bit(R10BIO_WriteError, &r10_bio->state);
}
- } else if (is_badblock(rdev,
- r10_bio->devs[slot].addr,
- r10_bio->sectors,
- &first_bad, &bad_sectors))
+ } else if (rdev_has_badblock(rdev, r10_bio->devs[slot].addr,
+ r10_bio->sectors)) {
set_bit(R10BIO_MadeGood, &r10_bio->state);
+ }
rdev_dec_pending(rdev, mddev);
@@ -2597,11 +2585,8 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
static int r10_sync_page_io(struct md_rdev *rdev, sector_t sector,
int sectors, struct page *page, enum req_op op)
{
- sector_t first_bad;
- int bad_sectors;
-
- if (is_badblock(rdev, sector, sectors, &first_bad, &bad_sectors)
- && (op == REQ_OP_READ || test_bit(WriteErrorSeen, &rdev->flags)))
+ if (rdev_has_badblock(rdev, sector, sectors) &&
+ (op == REQ_OP_READ || test_bit(WriteErrorSeen, &rdev->flags)))
return -1;
if (sync_page_io(rdev, sector, sectors << 9, page, op, false))
/* success */
@@ -2658,16 +2643,14 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
s = PAGE_SIZE >> 9;
do {
- sector_t first_bad;
- int bad_sectors;
-
d = r10_bio->devs[sl].devnum;
rdev = conf->mirrors[d].rdev;
if (rdev &&
test_bit(In_sync, &rdev->flags) &&
!test_bit(Faulty, &rdev->flags) &&
- is_badblock(rdev, r10_bio->devs[sl].addr + sect, s,
- &first_bad, &bad_sectors) == 0) {
+ rdev_has_badblock(rdev,
+ r10_bio->devs[sl].addr + sect,
+ s) == 0) {
atomic_inc(&rdev->nr_pending);
success = sync_page_io(rdev,
r10_bio->devs[sl].addr +
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 14f2cf75abbd..9241e95ef55c 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -1210,10 +1210,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
*/
while (op_is_write(op) && rdev &&
test_bit(WriteErrorSeen, &rdev->flags)) {
- sector_t first_bad;
- int bad_sectors;
- int bad = is_badblock(rdev, sh->sector, RAID5_STRIPE_SECTORS(conf),
- &first_bad, &bad_sectors);
+ int bad = rdev_has_badblock(rdev, sh->sector,
+ RAID5_STRIPE_SECTORS(conf));
if (!bad)
break;
@@ -2855,8 +2853,6 @@ static void raid5_end_write_request(struct bio *bi)
struct r5conf *conf = sh->raid_conf;
int disks = sh->disks, i;
struct md_rdev *rdev;
- sector_t first_bad;
- int bad_sectors;
int replacement = 0;
for (i = 0 ; i < disks; i++) {
@@ -2888,9 +2884,8 @@ static void raid5_end_write_request(struct bio *bi)
if (replacement) {
if (bi->bi_status)
md_error(conf->mddev, rdev);
- else if (is_badblock(rdev, sh->sector,
- RAID5_STRIPE_SECTORS(conf),
- &first_bad, &bad_sectors))
+ else if (rdev_has_badblock(rdev, sh->sector,
+ RAID5_STRIPE_SECTORS(conf)))
set_bit(R5_MadeGoodRepl, &sh->dev[i].flags);
} else {
if (bi->bi_status) {
@@ -2900,9 +2895,8 @@ static void raid5_end_write_request(struct bio *bi)
if (!test_and_set_bit(WantReplacement, &rdev->flags))
set_bit(MD_RECOVERY_NEEDED,
&rdev->mddev->recovery);
- } else if (is_badblock(rdev, sh->sector,
- RAID5_STRIPE_SECTORS(conf),
- &first_bad, &bad_sectors)) {
+ } else if (rdev_has_badblock(rdev, sh->sector,
+ RAID5_STRIPE_SECTORS(conf))) {
set_bit(R5_MadeGood, &sh->dev[i].flags);
if (test_bit(R5_ReadError, &sh->dev[i].flags))
/* That was a successful write so make
@@ -4674,8 +4668,6 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
/* Now to look around and see what can be done */
for (i=disks; i--; ) {
struct md_rdev *rdev;
- sector_t first_bad;
- int bad_sectors;
int is_bad = 0;
dev = &sh->dev[i];
@@ -4719,8 +4711,8 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
rdev = conf->disks[i].replacement;
if (rdev && !test_bit(Faulty, &rdev->flags) &&
rdev->recovery_offset >= sh->sector + RAID5_STRIPE_SECTORS(conf) &&
- !is_badblock(rdev, sh->sector, RAID5_STRIPE_SECTORS(conf),
- &first_bad, &bad_sectors))
+ !rdev_has_badblock(rdev, sh->sector,
+ RAID5_STRIPE_SECTORS(conf)))
set_bit(R5_ReadRepl, &dev->flags);
else {
if (rdev && !test_bit(Faulty, &rdev->flags))
@@ -4733,8 +4725,8 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
if (rdev && test_bit(Faulty, &rdev->flags))
rdev = NULL;
if (rdev) {
- is_bad = is_badblock(rdev, sh->sector, RAID5_STRIPE_SECTORS(conf),
- &first_bad, &bad_sectors);
+ is_bad = rdev_has_badblock(rdev, sh->sector,
+ RAID5_STRIPE_SECTORS(conf));
if (s->blocked_rdev == NULL
&& (test_bit(Blocked, &rdev->flags)
|| is_bad < 0)) {
@@ -5463,8 +5455,8 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
struct r5conf *conf = mddev->private;
struct bio *align_bio;
struct md_rdev *rdev;
- sector_t sector, end_sector, first_bad;
- int bad_sectors, dd_idx;
+ sector_t sector, end_sector;
+ int dd_idx;
bool did_inc;
if (!in_chunk_boundary(mddev, raid_bio)) {
@@ -5493,8 +5485,7 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
atomic_inc(&rdev->nr_pending);
- if (is_badblock(rdev, sector, bio_sectors(raid_bio), &first_bad,
- &bad_sectors)) {
+ if (rdev_has_badblock(rdev, sector, bio_sectors(raid_bio))) {
rdev_dec_pending(rdev, mddev);
return 0;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 02/11] md/raid1: factor out helpers to add rdev to conf
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 01/11] md: add a new helper rdev_has_badblock() Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs " Yu Kuai
` (9 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
There are no functional changes, just make code cleaner and prepare to
record disk non-rotational information while adding and removing rdev to
conf
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
drivers/md/raid1.c | 85 +++++++++++++++++++++++++++++-----------------
1 file changed, 53 insertions(+), 32 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index a145fe48b9ce..6ec9998f6257 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1757,6 +1757,44 @@ static int raid1_spare_active(struct mddev *mddev)
return count;
}
+static bool raid1_add_conf(struct r1conf *conf, struct md_rdev *rdev, int disk,
+ bool replacement)
+{
+ struct raid1_info *info = conf->mirrors + disk;
+
+ if (replacement)
+ info += conf->raid_disks;
+
+ if (info->rdev)
+ return false;
+
+ rdev->raid_disk = disk;
+ info->head_position = 0;
+ info->seq_start = MaxSector;
+ WRITE_ONCE(info->rdev, rdev);
+
+ return true;
+}
+
+static bool raid1_remove_conf(struct r1conf *conf, int disk)
+{
+ struct raid1_info *info = conf->mirrors + disk;
+ struct md_rdev *rdev = info->rdev;
+
+ if (!rdev || test_bit(In_sync, &rdev->flags) ||
+ atomic_read(&rdev->nr_pending))
+ return false;
+
+ /* Only remove non-faulty devices if recovery is not possible. */
+ if (!test_bit(Faulty, &rdev->flags) &&
+ rdev->mddev->recovery_disabled != conf->recovery_disabled &&
+ rdev->mddev->degraded < conf->raid_disks)
+ return false;
+
+ WRITE_ONCE(info->rdev, NULL);
+ return true;
+}
+
static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
{
struct r1conf *conf = mddev->private;
@@ -1792,15 +1830,13 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
disk_stack_limits(mddev->gendisk, rdev->bdev,
rdev->data_offset << 9);
- p->head_position = 0;
- rdev->raid_disk = mirror;
+ raid1_add_conf(conf, rdev, mirror, false);
err = 0;
/* As all devices are equivalent, we don't need a full recovery
* if this was recently any drive of the array
*/
if (rdev->saved_raid_disk < 0)
conf->fullsync = 1;
- WRITE_ONCE(p->rdev, rdev);
break;
}
if (test_bit(WantReplacement, &p->rdev->flags) &&
@@ -1810,13 +1846,11 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
if (err && repl_slot >= 0) {
/* Add this device as a replacement */
- p = conf->mirrors + repl_slot;
clear_bit(In_sync, &rdev->flags);
set_bit(Replacement, &rdev->flags);
- rdev->raid_disk = repl_slot;
+ raid1_add_conf(conf, rdev, repl_slot, true);
err = 0;
conf->fullsync = 1;
- WRITE_ONCE(p[conf->raid_disks].rdev, rdev);
}
print_conf(conf);
@@ -1833,27 +1867,20 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
if (unlikely(number >= conf->raid_disks))
goto abort;
- if (rdev != p->rdev)
- p = conf->mirrors + conf->raid_disks + number;
+ if (rdev != p->rdev) {
+ number += conf->raid_disks;
+ p = conf->mirrors + number;
+ }
print_conf(conf);
if (rdev == p->rdev) {
- if (test_bit(In_sync, &rdev->flags) ||
- atomic_read(&rdev->nr_pending)) {
- err = -EBUSY;
- goto abort;
- }
- /* Only remove non-faulty devices if recovery
- * is not possible.
- */
- if (!test_bit(Faulty, &rdev->flags) &&
- mddev->recovery_disabled != conf->recovery_disabled &&
- mddev->degraded < conf->raid_disks) {
+ if (!raid1_remove_conf(conf, number)) {
err = -EBUSY;
goto abort;
}
- WRITE_ONCE(p->rdev, NULL);
- if (conf->mirrors[conf->raid_disks + number].rdev) {
+
+ if (number < conf->raid_disks &&
+ conf->mirrors[conf->raid_disks + number].rdev) {
/* We just removed a device that is being replaced.
* Move down the replacement. We drain all IO before
* doing this to avoid confusion.
@@ -2994,23 +3021,17 @@ static struct r1conf *setup_conf(struct mddev *mddev)
err = -EINVAL;
spin_lock_init(&conf->device_lock);
+ conf->raid_disks = mddev->raid_disks;
rdev_for_each(rdev, mddev) {
int disk_idx = rdev->raid_disk;
- if (disk_idx >= mddev->raid_disks
- || disk_idx < 0)
+
+ if (disk_idx >= conf->raid_disks || disk_idx < 0)
continue;
- if (test_bit(Replacement, &rdev->flags))
- disk = conf->mirrors + mddev->raid_disks + disk_idx;
- else
- disk = conf->mirrors + disk_idx;
- if (disk->rdev)
+ if (!raid1_add_conf(conf, rdev, disk_idx,
+ test_bit(Replacement, &rdev->flags)))
goto abort;
- disk->rdev = rdev;
- disk->head_position = 0;
- disk->seq_start = MaxSector;
}
- conf->raid_disks = mddev->raid_disks;
conf->mddev = mddev;
INIT_LIST_HEAD(&conf->retry_list);
INIT_LIST_HEAD(&conf->bio_end_io_list);
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs to conf
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 01/11] md: add a new helper rdev_has_badblock() Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 02/11] md/raid1: factor out helpers to add rdev to conf Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 16:37 ` Paul Menzel
2024-02-29 9:57 ` [PATCH md-6.9 v4 04/11] md/raid1: fix choose next idle in read_balance() Yu Kuai
` (8 subsequent siblings)
11 siblings, 1 reply; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
For raid1, 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 conf, to avoid checking each rdev in IO fast path and simplify
read_balance() a little bit.
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/md.h | 1 +
drivers/md/raid1.c | 17 ++++++++++-------
drivers/md/raid1.h | 1 +
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index a49ab04ab707..b2076a165c10 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,
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 6ec9998f6257..de6ea87d4d24 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 (READ_ONCE(conf->nonrot_disks) || min_pending == 0)
best_disk = best_pending_disk;
else
best_disk = best_dist_disk;
@@ -1768,6 +1763,11 @@ static bool raid1_add_conf(struct r1conf *conf, struct md_rdev *rdev, int disk,
if (info->rdev)
return false;
+ if (bdev_nonrot(rdev->bdev)) {
+ set_bit(Nonrot, &rdev->flags);
+ WRITE_ONCE(conf->nonrot_disks, conf->nonrot_disks + 1);
+ }
+
rdev->raid_disk = disk;
info->head_position = 0;
info->seq_start = MaxSector;
@@ -1791,6 +1791,9 @@ static bool raid1_remove_conf(struct r1conf *conf, int disk)
rdev->mddev->degraded < conf->raid_disks)
return false;
+ if (test_and_clear_bit(Nonrot, &rdev->flags))
+ WRITE_ONCE(conf->nonrot_disks, conf->nonrot_disks - 1);
+
WRITE_ONCE(info->rdev, NULL);
return true;
}
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index 14d4211a123a..5300cbaa58a4 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -71,6 +71,7 @@ struct r1conf {
* allow for replacements.
*/
int raid_disks;
+ int nonrot_disks;
spinlock_t device_lock;
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 04/11] md/raid1: fix choose next idle in read_balance()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (2 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs " Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 05/11] md/raid1-10: add a helper raid1_check_read_range() Yu Kuai
` (7 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
Commit 12cee5a8a29e ("md/raid1: prevent merging too large request") add
the case choose next idle in read_balance():
read_balance:
for_each_rdev
if(next_seq_sect == this_sector || dist == 0)
-> sequential reads
best_disk = disk;
if (...)
choose_next_idle = 1
continue;
for_each_rdev
-> iterate next rdev
if (pending == 0)
best_disk = disk;
-> choose the next idle disk
break;
if (choose_next_idle)
-> keep using this rdev if there are no other idle disk
contine
However, commit 2e52d449bcec ("md/raid1: add failfast handling for reads.")
remove the code:
- /* If device is idle, use it */
- if (pending == 0) {
- best_disk = disk;
- break;
- }
Hence choose next idle will never work now, fix this problem by
following:
1) don't set best_disk in this case, read_balance() will choose the best
disk after iterating all the disks;
2) add 'pending' so that other idle disk will be chosen;
3) add a new local variable 'sequential_disk' to record the disk, and if
there is no other idle disk, 'sequential_disk' will be chosen;
Fixes: 2e52d449bcec ("md/raid1: add failfast handling for reads.")
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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid1.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index de6ea87d4d24..fa86d9fdb16f 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -598,13 +598,12 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
const sector_t this_sector = r1_bio->sector;
int sectors;
int best_good_sectors;
- int best_disk, best_dist_disk, best_pending_disk;
+ int best_disk, best_dist_disk, best_pending_disk, sequential_disk;
int disk;
sector_t best_dist;
unsigned int min_pending;
struct md_rdev *rdev;
int choose_first;
- int choose_next_idle;
/*
* Check if we can balance. We can balance on the whole
@@ -615,11 +614,11 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
sectors = r1_bio->sectors;
best_disk = -1;
best_dist_disk = -1;
+ sequential_disk = -1;
best_dist = MaxSector;
best_pending_disk = -1;
min_pending = UINT_MAX;
best_good_sectors = 0;
- choose_next_idle = 0;
clear_bit(R1BIO_FailFast, &r1_bio->state);
if ((conf->mddev->recovery_cp < this_sector + sectors) ||
@@ -712,7 +711,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
int opt_iosize = bdev_io_opt(rdev->bdev) >> 9;
struct raid1_info *mirror = &conf->mirrors[disk];
- best_disk = disk;
/*
* If buffered sequential IO size exceeds optimal
* iosize, check if there is idle disk. If yes, choose
@@ -731,15 +729,22 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
mirror->next_seq_sect > opt_iosize &&
mirror->next_seq_sect - opt_iosize >=
mirror->seq_start) {
- choose_next_idle = 1;
- continue;
+ /*
+ * Add 'pending' to avoid choosing this disk if
+ * there is other idle disk.
+ */
+ pending++;
+ /*
+ * If there is no other idle disk, this disk
+ * will be chosen.
+ */
+ sequential_disk = disk;
+ } else {
+ best_disk = disk;
+ break;
}
- break;
}
- if (choose_next_idle)
- continue;
-
if (min_pending > pending) {
min_pending = pending;
best_pending_disk = disk;
@@ -751,6 +756,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
}
}
+ /*
+ * sequential IO size exceeds optimal iosize, however, there is no other
+ * idle disk, so choose the sequential disk.
+ */
+ if (best_disk == -1 && min_pending != 0)
+ best_disk = sequential_disk;
+
/*
* If all disks are rotational, choose the closest disk. If any disk is
* non-rotational, choose the disk with less pending request even the
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 05/11] md/raid1-10: add a helper raid1_check_read_range()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (3 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 04/11] md/raid1: fix choose next idle in read_balance() Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 06/11] md/raid1-10: factor out a new helper raid1_should_read_first() Yu Kuai
` (6 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
The checking and handler of bad blocks appear many timers during
read_balance() in raid1 and raid10. This helper will be used in later
patches to simplify read_balance() a lot.
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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid1-10.c | 49 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 512746551f36..9bc0f0022a6c 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -227,3 +227,52 @@ static inline bool exceed_read_errors(struct mddev *mddev, struct md_rdev *rdev)
return false;
}
+
+/**
+ * raid1_check_read_range() - check a given read range for bad blocks,
+ * available read length is returned;
+ * @rdev: the rdev to read;
+ * @this_sector: read position;
+ * @len: read length;
+ *
+ * helper function for read_balance()
+ *
+ * 1) If there are no bad blocks in the range, @len is returned;
+ * 2) If the range are all bad blocks, 0 is returned;
+ * 3) If there are partial bad blocks:
+ * - If the bad block range starts after @this_sector, the length of first
+ * good region is returned;
+ * - If the bad block range starts before @this_sector, 0 is returned and
+ * the @len is updated to the offset into the region before we get to the
+ * good blocks;
+ */
+static inline int raid1_check_read_range(struct md_rdev *rdev,
+ sector_t this_sector, int *len)
+{
+ sector_t first_bad;
+ int bad_sectors;
+
+ /* no bad block overlap */
+ if (!is_badblock(rdev, this_sector, *len, &first_bad, &bad_sectors))
+ return *len;
+
+ /*
+ * bad block range starts offset into our range so we can return the
+ * number of sectors before the bad blocks start.
+ */
+ if (first_bad > this_sector)
+ return first_bad - this_sector;
+
+ /* read range is fully consumed by bad blocks. */
+ if (this_sector + *len <= first_bad + bad_sectors)
+ return 0;
+
+ /*
+ * final case, bad block range starts before or at the start of our
+ * range but does not cover our entire range so we still return 0 but
+ * update the length with the number of sectors before we get to the
+ * good ones.
+ */
+ *len = first_bad + bad_sectors - this_sector;
+ return 0;
+}
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 06/11] md/raid1-10: factor out a new helper raid1_should_read_first()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (4 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 05/11] md/raid1-10: add a helper raid1_check_read_range() Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 07/11] md/raid1: factor out read_first_rdev() from read_balance() Yu Kuai
` (5 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
If resync is in progress, read_balance() should find the first usable
disk, otherwise, data could be inconsistent after resync is done. raid1
and raid10 implement the same checking, hence factor out the checking
to make code cleaner.
Noted that raid1 is using 'mddev->recovery_cp', which is updated after
all resync IO is done, while raid10 is using 'conf->next_resync', which
is inaccurate because raid10 update it before submitting resync IO.
Fortunately, raid10 read IO can't concurrent with resync IO, hence there
is no problem. And this patch also switch raid10 to use
'mddev->recovery_cp'.
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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid1-10.c | 20 ++++++++++++++++++++
drivers/md/raid1.c | 15 ++-------------
drivers/md/raid10.c | 13 ++-----------
3 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 9bc0f0022a6c..2ea1710a3b70 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -276,3 +276,23 @@ static inline int raid1_check_read_range(struct md_rdev *rdev,
*len = first_bad + bad_sectors - this_sector;
return 0;
}
+
+/*
+ * Check if read should choose the first rdev.
+ *
+ * Balance on the whole device if no resync is going on (recovery is ok) or
+ * below the resync window. Otherwise, take the first readable disk.
+ */
+static inline bool raid1_should_read_first(struct mddev *mddev,
+ sector_t this_sector, int len)
+{
+ if ((mddev->recovery_cp < this_sector + len))
+ return true;
+
+ if (mddev_is_clustered(mddev) &&
+ md_cluster_ops->area_resyncing(mddev, READ, this_sector,
+ this_sector + len))
+ return true;
+
+ return false;
+}
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index fa86d9fdb16f..30f467bb48fd 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -605,11 +605,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
struct md_rdev *rdev;
int choose_first;
- /*
- * Check if we can balance. We can balance on the whole
- * device if no resync is going on, or below the resync window.
- * We take the first readable disk when above the resync window.
- */
retry:
sectors = r1_bio->sectors;
best_disk = -1;
@@ -619,16 +614,10 @@ 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;
+ choose_first = raid1_should_read_first(conf->mddev, this_sector,
+ sectors);
clear_bit(R1BIO_FailFast, &r1_bio->state);
- if ((conf->mddev->recovery_cp < this_sector + sectors) ||
- (mddev_is_clustered(conf->mddev) &&
- md_cluster_ops->area_resyncing(conf->mddev, READ, this_sector,
- this_sector + sectors)))
- choose_first = 1;
- else
- choose_first = 0;
-
for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
sector_t dist;
sector_t first_bad;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d5a7a621f0f0..8aecdb1ccc16 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -748,17 +748,8 @@ static struct md_rdev *read_balance(struct r10conf *conf,
best_good_sectors = 0;
do_balance = 1;
clear_bit(R10BIO_FailFast, &r10_bio->state);
- /*
- * Check if we can balance. We can balance on the whole
- * device if no resync is going on (recovery is ok), or below
- * the resync window. We take the first readable disk when
- * above the resync window.
- */
- if ((conf->mddev->recovery_cp < MaxSector
- && (this_sector + sectors >= conf->next_resync)) ||
- (mddev_is_clustered(conf->mddev) &&
- md_cluster_ops->area_resyncing(conf->mddev, READ, this_sector,
- this_sector + sectors)))
+
+ if (raid1_should_read_first(conf->mddev, this_sector, sectors))
do_balance = 0;
for (slot = 0; slot < conf->copies ; slot++) {
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 07/11] md/raid1: factor out read_first_rdev() from read_balance()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (5 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 06/11] md/raid1-10: factor out a new helper raid1_should_read_first() Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 08/11] md/raid1: factor out choose_slow_rdev() " Yu Kuai
` (4 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
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 first rdev 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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid1.c | 63 +++++++++++++++++++++++++++++++++-------------
1 file changed, 46 insertions(+), 17 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 30f467bb48fd..3149f22f1155 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -579,6 +579,47 @@ static sector_t align_to_barrier_unit_end(sector_t start_sector,
return len;
}
+static void update_read_sectors(struct r1conf *conf, int disk,
+ sector_t this_sector, int len)
+{
+ struct raid1_info *info = &conf->mirrors[disk];
+
+ atomic_inc(&info->rdev->nr_pending);
+ if (info->next_seq_sect != this_sector)
+ info->seq_start = this_sector;
+ info->next_seq_sect = this_sector + len;
+}
+
+static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
+ int *max_sectors)
+{
+ sector_t this_sector = r1_bio->sector;
+ int len = r1_bio->sectors;
+ int disk;
+
+ for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
+ struct md_rdev *rdev;
+ int read_len;
+
+ if (r1_bio->bios[disk] == IO_BLOCKED)
+ continue;
+
+ rdev = conf->mirrors[disk].rdev;
+ if (!rdev || test_bit(Faulty, &rdev->flags))
+ continue;
+
+ /* choose the first disk even if it has some bad blocks. */
+ read_len = raid1_check_read_range(rdev, this_sector, &len);
+ if (read_len > 0) {
+ update_read_sectors(conf, disk, this_sector, read_len);
+ *max_sectors = read_len;
+ return disk;
+ }
+ }
+
+ return -1;
+}
+
/*
* This routine returns the disk from which the requested read should
* be done. There is a per-array 'next expected sequential IO' sector
@@ -603,7 +644,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
sector_t best_dist;
unsigned int min_pending;
struct md_rdev *rdev;
- int choose_first;
retry:
sectors = r1_bio->sectors;
@@ -614,10 +654,11 @@ 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;
- choose_first = raid1_should_read_first(conf->mddev, this_sector,
- sectors);
clear_bit(R1BIO_FailFast, &r1_bio->state);
+ if (raid1_should_read_first(conf->mddev, this_sector, sectors))
+ return choose_first_rdev(conf, r1_bio, max_sectors);
+
for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
sector_t dist;
sector_t first_bad;
@@ -663,8 +704,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
* bad_sectors from another device..
*/
bad_sectors -= (this_sector - first_bad);
- if (choose_first && sectors > bad_sectors)
- sectors = bad_sectors;
if (best_good_sectors > sectors)
best_good_sectors = sectors;
@@ -674,8 +713,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
best_good_sectors = good_sectors;
best_disk = disk;
}
- if (choose_first)
- break;
}
continue;
} else {
@@ -690,10 +727,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
pending = atomic_read(&rdev->nr_pending);
dist = abs(this_sector - conf->mirrors[disk].head_position);
- if (choose_first) {
- best_disk = disk;
- break;
- }
/* Don't change to another disk for sequential reads */
if (conf->mirrors[disk].next_seq_sect == this_sector
|| dist == 0) {
@@ -769,13 +802,9 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
rdev = conf->mirrors[best_disk].rdev;
if (!rdev)
goto retry;
- atomic_inc(&rdev->nr_pending);
- sectors = best_good_sectors;
-
- if (conf->mirrors[best_disk].next_seq_sect != this_sector)
- conf->mirrors[best_disk].seq_start = this_sector;
- conf->mirrors[best_disk].next_seq_sect = this_sector + sectors;
+ sectors = best_good_sectors;
+ update_read_sectors(conf, disk, this_sector, sectors);
}
*max_sectors = sectors;
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 08/11] md/raid1: factor out choose_slow_rdev() from read_balance()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (6 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 07/11] md/raid1: factor out read_first_rdev() from read_balance() Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 09/11] md/raid1: factor out choose_bb_rdev() " Yu Kuai
` (3 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
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 slow rdev 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>
Reviewed-by: Xiao Ni <xni@redhat.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 3149f22f1155..09b7e93a54b5 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
@@ -673,23 +720,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.
*/
@@ -808,7 +840,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 09/11] md/raid1: factor out choose_bb_rdev() from read_balance()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (7 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 08/11] md/raid1: factor out choose_slow_rdev() " Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 10/11] md/raid1: factor out the code to manage sequential IO Yu Kuai
` (2 subsequent siblings)
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
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>
Reviewed-by: Xiao Ni <xni@redhat.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 09b7e93a54b5..f6e75c123e5a 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -620,6 +620,44 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
return -1;
}
+static int choose_bb_rdev(struct r1conf *conf, struct r1bio *r1_bio,
+ int *max_sectors)
+{
+ sector_t this_sector = r1_bio->sector;
+ int best_disk = -1;
+ int best_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;
+
+ /* keep track of the disk with the most readable sectors. */
+ len = r1_bio->sectors;
+ read_len = raid1_check_read_range(rdev, this_sector, &len);
+ if (read_len > best_len) {
+ best_disk = disk;
+ best_len = read_len;
+ }
+ }
+
+ if (best_disk != -1) {
+ *max_sectors = best_len;
+ update_read_sectors(conf, best_disk, this_sector, best_len);
+ }
+
+ return best_disk;
+}
+
static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
int *max_sectors)
{
@@ -708,8 +746,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
sector_t dist;
- sector_t first_bad;
- int bad_sectors;
unsigned int pending;
rdev = conf->mirrors[disk].rdev;
@@ -722,36 +758,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 <= this_sector) {
- /* cannot read here. If this is the 'primary'
- * device, then we must not read beyond
- * bad_sectors from another device..
- */
- bad_sectors -= (this_sector - first_bad);
- if (best_good_sectors > sectors)
- best_good_sectors = sectors;
-
- } else {
- sector_t good_sectors = first_bad - this_sector;
- if (good_sectors > best_good_sectors) {
- best_good_sectors = good_sectors;
- best_disk = disk;
- }
- }
+ if (rdev_has_badblock(rdev, this_sector, sectors))
continue;
- } else {
- if ((sectors > best_good_sectors) && (best_disk >= 0))
- best_disk = -1;
- best_good_sectors = sectors;
- }
if (best_disk >= 0)
/* At least two disks to choose from so failfast is OK */
@@ -843,6 +851,15 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
if (best_disk >= 0)
return best_disk;
+ /*
+ * If we are here it means we didn't find a perfectly good disk so
+ * now spend a bit more time trying to find one with the most good
+ * sectors.
+ */
+ disk = choose_bb_rdev(conf, r1_bio, max_sectors);
+ if (disk >= 0)
+ return disk;
+
return choose_slow_rdev(conf, r1_bio, max_sectors);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 10/11] md/raid1: factor out the code to manage sequential IO
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (8 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 09/11] md/raid1: factor out choose_bb_rdev() " Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 11/11] md/raid1: factor out helpers to choose the best rdev from read_balance() Yu Kuai
2024-03-01 7:16 ` [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Song Liu
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid1.c | 71 ++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 34 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f6e75c123e5a..17c2201d5d2b 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -705,6 +705,31 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
return bb_disk;
}
+static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r1_bio)
+{
+ /* TODO: address issues with this check and concurrency. */
+ return conf->mirrors[disk].next_seq_sect == r1_bio->sector ||
+ conf->mirrors[disk].head_position == 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 = &conf->mirrors[disk];
+ int opt_iosize;
+
+ if (!test_bit(Nonrot, &mirror->rdev->flags))
+ return false;
+
+ opt_iosize = bdev_io_opt(mirror->rdev->bdev) >> 9;
+ return opt_iosize > 0 && mirror->seq_start != MaxSector &&
+ mirror->next_seq_sect > opt_iosize &&
+ mirror->next_seq_sect - opt_iosize >= 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
@@ -768,43 +793,21 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
pending = atomic_read(&rdev->nr_pending);
dist = abs(this_sector - conf->mirrors[disk].head_position);
/* Don't change to another disk for sequential reads */
- if (conf->mirrors[disk].next_seq_sect == this_sector
- || dist == 0) {
- int opt_iosize = bdev_io_opt(rdev->bdev) >> 9;
- struct raid1_info *mirror = &conf->mirrors[disk];
-
- /*
- * If buffered sequential IO size exceeds optimal
- * iosize, check if there is idle disk. If yes, choose
- * the idle disk. read_balance could already choose an
- * idle disk before noticing it's a sequential IO in
- * this disk. This doesn't matter because this disk
- * 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 optimal
- * iosize at least. iosize of the second disk might be
- * small, but not a big deal since when the second disk
- * starts IO, the first disk is likely still busy.
- */
- 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 >=
- mirror->seq_start) {
- /*
- * Add 'pending' to avoid choosing this disk if
- * there is other idle disk.
- */
- pending++;
- /*
- * If there is no other idle disk, this disk
- * will be chosen.
- */
- sequential_disk = disk;
- } else {
+ if (is_sequential(conf, disk, r1_bio)) {
+ if (!should_choose_next(conf, disk)) {
best_disk = disk;
break;
}
+ /*
+ * Add 'pending' to avoid choosing this disk if
+ * there is other idle disk.
+ */
+ pending++;
+ /*
+ * If there is no other idle disk, this disk
+ * will be chosen.
+ */
+ sequential_disk = disk;
}
if (min_pending > pending) {
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH md-6.9 v4 11/11] md/raid1: factor out helpers to choose the best rdev from read_balance()
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (9 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 10/11] md/raid1: factor out the code to manage sequential IO Yu Kuai
@ 2024-02-29 9:57 ` Yu Kuai
2024-03-01 7:16 ` [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Song Liu
11 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-02-29 9:57 UTC (permalink / raw)
To: xni, paul.e.luse, song, neilb, shli
Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
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>
Reviewed-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid1.c | 175 +++++++++++++++++++++++++--------------------
1 file changed, 98 insertions(+), 77 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 17c2201d5d2b..afca975ec7f3 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -730,74 +730,71 @@ static bool should_choose_next(struct r1conf *conf, int disk)
mirror->next_seq_sect - opt_iosize >= 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 = r1_bio->sector;
- int sectors;
- int best_good_sectors;
- int best_disk, best_dist_disk, best_pending_disk, sequential_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 = r1_bio->sectors;
- best_disk = -1;
- best_dist_disk = -1;
- sequential_disk = -1;
- best_dist = MaxSector;
- best_pending_disk = -1;
- min_pending = UINT_MAX;
- best_good_sectors = 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 sequential_disk;
+ int readable_disks;
+};
+
+static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
+{
+ int disk;
+ struct read_balance_ctl ctl = {
+ .closest_dist_disk = -1,
+ .closest_dist = MaxSector,
+ .min_pending_disk = -1,
+ .min_pending = UINT_MAX,
+ .sequential_disk = -1,
+ };
for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
+ struct md_rdev *rdev;
sector_t dist;
unsigned int pending;
- rdev = conf->mirrors[disk].rdev;
- if (r1_bio->bios[disk] == IO_BLOCKED
- || rdev == 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] == IO_BLOCKED)
continue;
- if (rdev_has_badblock(rdev, this_sector, sectors))
+
+ rdev = conf->mirrors[disk].rdev;
+ if (!rdev_readable(rdev, r1_bio))
continue;
- if (best_disk >= 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++ == 1)
set_bit(R1BIO_FailFast, &r1_bio->state);
pending = atomic_read(&rdev->nr_pending);
- dist = abs(this_sector - conf->mirrors[disk].head_position);
+ dist = abs(r1_bio->sector - conf->mirrors[disk].head_position);
+
/* Don't change to another disk for sequential reads */
if (is_sequential(conf, disk, r1_bio)) {
- if (!should_choose_next(conf, disk)) {
- best_disk = disk;
- break;
- }
+ if (!should_choose_next(conf, disk))
+ return disk;
+
/*
* Add 'pending' to avoid choosing this disk if
* there is other idle disk.
@@ -807,17 +804,17 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
* If there is no other idle disk, this disk
* will be chosen.
*/
- sequential_disk = disk;
+ ctl.sequential_disk = disk;
}
- if (min_pending > pending) {
- min_pending = pending;
- best_pending_disk = disk;
+ if (ctl.min_pending > pending) {
+ ctl.min_pending = pending;
+ ctl.min_pending_disk = disk;
}
- if (dist < best_dist) {
- best_dist = dist;
- best_dist_disk = disk;
+ if (ctl.closest_dist > dist) {
+ ctl.closest_dist = dist;
+ ctl.closest_dist_disk = disk;
}
}
@@ -825,8 +822,8 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
* sequential IO size exceeds optimal iosize, however, there is no other
* idle disk, so choose the sequential disk.
*/
- if (best_disk == -1 && min_pending != 0)
- best_disk = sequential_disk;
+ if (ctl.sequential_disk != -1 && ctl.min_pending != 0)
+ return ctl.sequential_disk;
/*
* If all disks are rotational, choose the closest disk. If any disk is
@@ -834,25 +831,49 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
* disk is rotational, which might/might not be optimal for raids with
* mixed ratation/non-rotational disks depending on workload.
*/
- if (best_disk == -1) {
- if (READ_ONCE(conf->nonrot_disks) || min_pending == 0)
- best_disk = best_pending_disk;
- else
- best_disk = best_dist_disk;
- }
+ if (ctl.min_pending_disk != -1 &&
+ (READ_ONCE(conf->nonrot_disks) || ctl.min_pending == 0))
+ return ctl.min_pending_disk;
+ else
+ return ctl.closest_dist_disk;
+}
- if (best_disk >= 0) {
- rdev = conf->mirrors[best_disk].rdev;
- if (!rdev)
- goto retry;
+/*
+ * 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 slow
+ * disks and disks with bad blocks for now. Only pay attention to key disk
+ * 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 disk 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;
- sectors = best_good_sectors;
- update_read_sectors(conf, disk, this_sector, sectors);
- }
- *max_sectors = sectors;
+ clear_bit(R1BIO_FailFast, &r1_bio->state);
+
+ if (raid1_should_read_first(conf->mddev, r1_bio->sector,
+ r1_bio->sectors))
+ return choose_first_rdev(conf, r1_bio, max_sectors);
- if (best_disk >= 0)
- return best_disk;
+ disk = choose_best_rdev(conf, r1_bio);
+ if (disk >= 0) {
+ *max_sectors = 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 so
--
2.39.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs to conf
2024-02-29 9:57 ` [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs " Yu Kuai
@ 2024-02-29 16:37 ` Paul Menzel
2024-03-01 1:59 ` Yu Kuai
0 siblings, 1 reply; 15+ messages in thread
From: Paul Menzel @ 2024-02-29 16:37 UTC (permalink / raw)
To: Yu Kuai
Cc: xni, paul.e.luse, song, neilb, shli, linux-raid, linux-kernel,
yukuai3, yi.zhang, yangerkun
Dear Yu,
Thank you for your patch.
Am 29.02.24 um 10:57 schrieb Yu Kuai:
> From: Yu Kuai <yukuai3@huawei.com>
>
> For raid1, 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 conf, to avoid checking each rdev in IO fast path and simplify
The comma is not needed.
> read_balance() a little bit.
Just to make sure, I understood correctly. Changing
`/sys/block/[disk_name]/queue/rotational` will now not be considered
anymore, right?
For the summary, maybe you could also say “cache”. Maybe:
Cache attribute rotational while adding/removing rdevs to conf
> 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/md.h | 1 +
> drivers/md/raid1.c | 17 ++++++++++-------
> drivers/md/raid1.h | 1 +
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index a49ab04ab707..b2076a165c10 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,
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 6ec9998f6257..de6ea87d4d24 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 (READ_ONCE(conf->nonrot_disks) || min_pending == 0)
> best_disk = best_pending_disk;
> else
> best_disk = best_dist_disk;
> @@ -1768,6 +1763,11 @@ static bool raid1_add_conf(struct r1conf *conf, struct md_rdev *rdev, int disk,
> if (info->rdev)
> return false;
>
> + if (bdev_nonrot(rdev->bdev)) {
> + set_bit(Nonrot, &rdev->flags);
> + WRITE_ONCE(conf->nonrot_disks, conf->nonrot_disks + 1);
> + }
> +
> rdev->raid_disk = disk;
> info->head_position = 0;
> info->seq_start = MaxSector;
> @@ -1791,6 +1791,9 @@ static bool raid1_remove_conf(struct r1conf *conf, int disk)
> rdev->mddev->degraded < conf->raid_disks)
> return false;
>
> + if (test_and_clear_bit(Nonrot, &rdev->flags))
> + WRITE_ONCE(conf->nonrot_disks, conf->nonrot_disks - 1);
> +
> WRITE_ONCE(info->rdev, NULL);
> return true;
> }
> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> index 14d4211a123a..5300cbaa58a4 100644
> --- a/drivers/md/raid1.h
> +++ b/drivers/md/raid1.h
> @@ -71,6 +71,7 @@ struct r1conf {
> * allow for replacements.
> */
> int raid_disks;
> + int nonrot_disks;
>
> spinlock_t device_lock;
>
As you meant “fastpath” in the commit message, if I remember correctly,
this does not improve the performance in benchmarks, right?
Kind regards,
Paul
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs to conf
2024-02-29 16:37 ` Paul Menzel
@ 2024-03-01 1:59 ` Yu Kuai
0 siblings, 0 replies; 15+ messages in thread
From: Yu Kuai @ 2024-03-01 1:59 UTC (permalink / raw)
To: Paul Menzel, Yu Kuai
Cc: xni, paul.e.luse, song, neilb, shli, linux-raid, linux-kernel,
yi.zhang, yangerkun, yukuai (C)
Hi,
在 2024/03/01 0:37, Paul Menzel 写道:
> Dear Yu,
>
>
> Thank you for your patch.
>
>
> Am 29.02.24 um 10:57 schrieb Yu Kuai:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> For raid1, 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 conf, to avoid checking each rdev in IO fast path and simplify
>
> The comma is not needed.
>
>> read_balance() a little bit.
>
> Just to make sure, I understood correctly. Changing
> `/sys/block/[disk_name]/queue/rotational` will now not be considered
> anymore, right?
Yes, and I think this will case performance to be worse in real life.
>
> For the summary, maybe you could also say “cache”. Maybe:
>
> Cache attribute rotational while adding/removing rdevs to conf
>
>> 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/md.h | 1 +
>> drivers/md/raid1.c | 17 ++++++++++-------
>> drivers/md/raid1.h | 1 +
>> 3 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index a49ab04ab707..b2076a165c10 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,
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 6ec9998f6257..de6ea87d4d24 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 (READ_ONCE(conf->nonrot_disks) || min_pending == 0)
>> best_disk = best_pending_disk;
>> else
>> best_disk = best_dist_disk;
>> @@ -1768,6 +1763,11 @@ static bool raid1_add_conf(struct r1conf *conf,
>> struct md_rdev *rdev, int disk,
>> if (info->rdev)
>> return false;
>> + if (bdev_nonrot(rdev->bdev)) {
>> + set_bit(Nonrot, &rdev->flags);
>> + WRITE_ONCE(conf->nonrot_disks, conf->nonrot_disks + 1);
>> + }
>> +
>> rdev->raid_disk = disk;
>> info->head_position = 0;
>> info->seq_start = MaxSector;
>> @@ -1791,6 +1791,9 @@ static bool raid1_remove_conf(struct r1conf
>> *conf, int disk)
>> rdev->mddev->degraded < conf->raid_disks)
>> return false;
>> + if (test_and_clear_bit(Nonrot, &rdev->flags))
>> + WRITE_ONCE(conf->nonrot_disks, conf->nonrot_disks - 1);
>> +
>> WRITE_ONCE(info->rdev, NULL);
>> return true;
>> }
>> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
>> index 14d4211a123a..5300cbaa58a4 100644
>> --- a/drivers/md/raid1.h
>> +++ b/drivers/md/raid1.h
>> @@ -71,6 +71,7 @@ struct r1conf {
>> * allow for replacements.
>> */
>> int raid_disks;
>> + int nonrot_disks;
>> spinlock_t device_lock;
>
> As you meant “fastpath” in the commit message, if I remember correctly,
> this does not improve the performance in benchmarks, right?
Yest, this just safe some memory load command, this is to little to
affect performance benchmarks. Main ideal here is make read_balance()
cleaner.
Thanks,
Kuai
>
>
> Kind regards,
>
> Paul
> .
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
` (10 preceding siblings ...)
2024-02-29 9:57 ` [PATCH md-6.9 v4 11/11] md/raid1: factor out helpers to choose the best rdev from read_balance() Yu Kuai
@ 2024-03-01 7:16 ` Song Liu
11 siblings, 0 replies; 15+ messages in thread
From: Song Liu @ 2024-03-01 7:16 UTC (permalink / raw)
To: Yu Kuai
Cc: xni, paul.e.luse, neilb, shli, linux-raid, linux-kernel, yukuai3,
yi.zhang, yangerkun
On Thu, Feb 29, 2024 at 2:03 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> Changes in v4:
> - fix a problem in v2, that replacement rdev->raid_disk is set to
> raid_disk + conf->mirros, which will cause test 01replace to run
> forever, and mdadm tests looks good now(no new regression);
> Changes in v3:
> - add patch 2, and fix that setup_conf() is missing in patch3;
> - add some review tag from Xiao Ni(other than patch 2,3);
> Changes in v2:
> - add new conter in conf for patch 2;
> - fix the case choose next idle while there is no other idle disk in
> patch 3;
> - add some review tag from Xiao Ni for patch 1, 4-8
>
> The original idea is that Paul want to optimize raid1 read
> performance([1]), however, we think that the original code for
> read_balance() is quite complex, and we don't want to add more
> complexity. Hence we decide to refactor read_balance() first, to make
> code cleaner and easier for follow up.
>
> Before this patchset, read_balance() has many local variables and many
> branches, it want to consider all the scenarios in one iteration. The
> idea of this patch is to divide them into 4 different steps:
>
> 1) If resync is in progress, find the first usable disk, patch 5;
> Otherwise:
> 2) Loop through all disks and skipping slow disks and disks with bad
> blocks, choose the best disk, patch 10. If no disk is found:
> 3) Look for disks with bad blocks and choose the one with most number of
> sectors, patch 8. If no disk is found:
> 4) Choose first found slow disk with no bad blocks, or slow disk with
> most number of sectors, patch 7.
>
> Note that step 3) and step 4) are super code path, and performance
> should not be considered.
>
> And after this patchset, we'll continue to optimize read_balance for
> step 2), specifically how to choose the best rdev to read.
>
> [1] https://lore.kernel.org/all/20240102125115.129261-1-paul.e.luse@linux.intel.com/
>
> Yu Kuai (11):
> md: add a new helper rdev_has_badblock()
> md/raid1: factor out helpers to add rdev to conf
> md/raid1: record nonrot rdevs while adding/removing rdevs to conf
> md/raid1: fix choose next idle in read_balance()
> md/raid1-10: add a helper raid1_check_read_range()
> md/raid1-10: factor out a new helper raid1_should_read_first()
> md/raid1: factor out read_first_rdev() from read_balance()
> md/raid1: factor out choose_slow_rdev() from read_balance()
> md/raid1: factor out choose_bb_rdev() from read_balance()
> md/raid1: factor out the code to manage sequential IO
> md/raid1: factor out helpers to choose the best rdev from
> read_balance()
Applied v4 of the set to md-6.9 branch.
Thanks,
Song
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-03-01 7:16 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-29 9:57 [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 01/11] md: add a new helper rdev_has_badblock() Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 02/11] md/raid1: factor out helpers to add rdev to conf Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 03/11] md/raid1: record nonrot rdevs while adding/removing rdevs " Yu Kuai
2024-02-29 16:37 ` Paul Menzel
2024-03-01 1:59 ` Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 04/11] md/raid1: fix choose next idle in read_balance() Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 05/11] md/raid1-10: add a helper raid1_check_read_range() Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 06/11] md/raid1-10: factor out a new helper raid1_should_read_first() Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 07/11] md/raid1: factor out read_first_rdev() from read_balance() Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 08/11] md/raid1: factor out choose_slow_rdev() " Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 09/11] md/raid1: factor out choose_bb_rdev() " Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 10/11] md/raid1: factor out the code to manage sequential IO Yu Kuai
2024-02-29 9:57 ` [PATCH md-6.9 v4 11/11] md/raid1: factor out helpers to choose the best rdev from read_balance() Yu Kuai
2024-03-01 7:16 ` [PATCH md-6.9 v4 00/11] md/raid1: refactor read_balance() and some minor fix Song Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.