* [PATCH V3 0/4] md/raid10: Improve handling raid10 discard request
@ 2020-08-13 8:14 Xiao Ni
2020-08-13 8:14 ` [PATCH V3 1/4] md/raid10: Move codes related with submitting discard bio into one function Xiao Ni
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Xiao Ni @ 2020-08-13 8:14 UTC (permalink / raw
To: linux-raid, song; +Cc: colyli, guoqing.jiang, heinzm, ncroxon
Hi all
Now mkfs on raid10 which is combined with ssd/nvme disks takes a long time.
This patch set tries to resolve this problem.
v1:
Coly helps to review these patches and give some suggestions:
One bug is found. If discard bio is across one stripe but bio size is bigger
than one stripe size. After spliting, the bio will be NULL. In this version,
it checks whether discard bio size is bigger than 2*stripe_size.
In raid10_end_discard_request, it's better to check R10BIO_Uptodate is set
or not. It can avoid write memory to improve performance.
Add more comments for calculating addresses.
v2:
Fix error by checkpatch.pl
Fix one bug for offset layout. v1 calculates wrongly split size
Add more comments to explain how the discard range of each component disk
is decided.
v3:
add support for far layout
Change the patch name
Xiao Ni (4):
Move codes related with submitting discard bio into one function
extend r10bio devs to raid disks
improve raid10 discard request
Improve discard request for far layout
drivers/md/md.c | 23 ++++
drivers/md/md.h | 3 +
drivers/md/raid0.c | 15 +--
drivers/md/raid10.c | 338 +++++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/md/raid10.h | 1 +
5 files changed, 361 insertions(+), 19 deletions(-)
--
2.7.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V3 1/4] md/raid10: Move codes related with submitting discard bio into one function
2020-08-13 8:14 [PATCH V3 0/4] md/raid10: Improve handling raid10 discard request Xiao Ni
@ 2020-08-13 8:14 ` Xiao Ni
2020-08-13 8:14 ` [PATCH V3 2/4] md/raid10: extend r10bio devs to raid disks Xiao Ni
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Xiao Ni @ 2020-08-13 8:14 UTC (permalink / raw
To: linux-raid, song; +Cc: colyli, guoqing.jiang, heinzm, ncroxon
These codes can be used in raid10. So we can move these codes into
md.c. raid0 and raid10 can share these codes.
Reviewed-by: Coly Li <colyli@suse.de>
Reviewed-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
---
drivers/md/md.c | 23 +++++++++++++++++++++++
drivers/md/md.h | 3 +++
drivers/md/raid0.c | 15 ++-------------
3 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9c69084..e70f19c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8622,6 +8622,29 @@ void md_write_end(struct mddev *mddev)
EXPORT_SYMBOL(md_write_end);
+/* This is used by raid0 and raid10 */
+void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
+ struct bio *bio,
+ sector_t dev_start, sector_t dev_end)
+{
+ struct bio *discard_bio = NULL;
+
+ if (__blkdev_issue_discard(rdev->bdev,
+ dev_start + rdev->data_offset,
+ dev_end - dev_start, GFP_NOIO, 0, &discard_bio) ||
+ !discard_bio)
+ return;
+
+ bio_chain(discard_bio, bio);
+ bio_clone_blkg_association(discard_bio, bio);
+ if (mddev->gendisk)
+ trace_block_bio_remap(bdev_get_queue(rdev->bdev),
+ discard_bio, disk_devt(mddev->gendisk),
+ bio->bi_iter.bi_sector);
+ submit_bio_noacct(discard_bio);
+}
+EXPORT_SYMBOL(md_submit_discard_bio);
+
/* md_allow_write(mddev)
* Calling this ensures that the array is marked 'active' so that writes
* may proceed without blocking. It is important to call this before
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 1e172b3..64c1a18 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -716,6 +716,9 @@ extern void md_write_end(struct mddev *mddev);
extern void md_done_sync(struct mddev *mddev, int blocks, int ok);
extern void md_error(struct mddev *mddev, struct md_rdev *rdev);
extern void md_finish_reshape(struct mddev *mddev);
+extern void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
+ struct bio *bio,
+ sector_t dev_start, sector_t dev_end);
extern int mddev_congested(struct mddev *mddev, int bits);
extern bool __must_check md_flush_request(struct mddev *mddev, struct bio *bio);
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index e9e91c8..e37fe8a 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -525,7 +525,6 @@ static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
for (disk = 0; disk < zone->nb_dev; disk++) {
sector_t dev_start, dev_end;
- struct bio *discard_bio = NULL;
struct md_rdev *rdev;
if (disk < start_disk_index)
@@ -548,18 +547,8 @@ static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
rdev = conf->devlist[(zone - conf->strip_zone) *
conf->strip_zone[0].nb_dev + disk];
- if (__blkdev_issue_discard(rdev->bdev,
- dev_start + zone->dev_start + rdev->data_offset,
- dev_end - dev_start, GFP_NOIO, 0, &discard_bio) ||
- !discard_bio)
- continue;
- bio_chain(discard_bio, bio);
- bio_clone_blkg_association(discard_bio, bio);
- if (mddev->gendisk)
- trace_block_bio_remap(bdev_get_queue(rdev->bdev),
- discard_bio, disk_devt(mddev->gendisk),
- bio->bi_iter.bi_sector);
- submit_bio_noacct(discard_bio);
+ dev_start += zone->dev_start;
+ md_submit_discard_bio(mddev, rdev, bio, dev_start, dev_end);
}
bio_endio(bio);
}
--
2.7.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V3 2/4] md/raid10: extend r10bio devs to raid disks
2020-08-13 8:14 [PATCH V3 0/4] md/raid10: Improve handling raid10 discard request Xiao Ni
2020-08-13 8:14 ` [PATCH V3 1/4] md/raid10: Move codes related with submitting discard bio into one function Xiao Ni
@ 2020-08-13 8:14 ` Xiao Ni
2020-08-22 9:28 ` Xiao Ni
2020-08-13 8:14 ` [PATCH V3 3/4] md/raid10: improve raid10 discard request Xiao Ni
2020-08-13 8:14 ` [PATCH V3 4/4] md/raid10: Improve discard request for far layout Xiao Ni
3 siblings, 1 reply; 13+ messages in thread
From: Xiao Ni @ 2020-08-13 8:14 UTC (permalink / raw
To: linux-raid, song; +Cc: colyli, guoqing.jiang, heinzm, ncroxon
Now it allocs r10bio->devs[conf->copies]. Discard bio needs to submit
to all member disks and it needs to use r10bio. So extend to
r10bio->devs[geo.raid_disks].
Reviewed-by: Coly Li <colyli@suse.de>
Signed-off-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid10.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index cefda2a..cef3cb8 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -91,7 +91,7 @@ static inline struct r10bio *get_resync_r10bio(struct bio *bio)
static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
{
struct r10conf *conf = data;
- int size = offsetof(struct r10bio, devs[conf->copies]);
+ int size = offsetof(struct r10bio, devs[conf->geo.raid_disks]);
/* allocate a r10bio with room for raid_disks entries in the
* bios array */
@@ -238,7 +238,7 @@ static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
{
int i;
- for (i = 0; i < conf->copies; i++) {
+ for (i = 0; i < conf->geo.raid_disks; i++) {
struct bio **bio = & r10_bio->devs[i].bio;
if (!BIO_SPECIAL(*bio))
bio_put(*bio);
@@ -327,7 +327,7 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
int slot;
int repl = 0;
- for (slot = 0; slot < conf->copies; slot++) {
+ for (slot = 0; slot < conf->geo.raid_disks; slot++) {
if (r10_bio->devs[slot].bio == bio)
break;
if (r10_bio->devs[slot].repl_bio == bio) {
@@ -336,7 +336,7 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
}
}
- BUG_ON(slot == conf->copies);
+ BUG_ON(slot == conf->geo.raid_disks);
update_head_pos(slot, r10_bio);
if (slotp)
@@ -1518,7 +1518,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
r10_bio->mddev = mddev;
r10_bio->sector = bio->bi_iter.bi_sector;
r10_bio->state = 0;
- memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->copies);
+ memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->geo.raid_disks);
if (bio_data_dir(bio) == READ)
raid10_read_request(mddev, bio, r10_bio);
--
2.7.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V3 3/4] md/raid10: improve raid10 discard request
2020-08-13 8:14 [PATCH V3 0/4] md/raid10: Improve handling raid10 discard request Xiao Ni
2020-08-13 8:14 ` [PATCH V3 1/4] md/raid10: Move codes related with submitting discard bio into one function Xiao Ni
2020-08-13 8:14 ` [PATCH V3 2/4] md/raid10: extend r10bio devs to raid disks Xiao Ni
@ 2020-08-13 8:14 ` Xiao Ni
2020-08-19 18:36 ` Song Liu
2020-08-13 8:14 ` [PATCH V3 4/4] md/raid10: Improve discard request for far layout Xiao Ni
3 siblings, 1 reply; 13+ messages in thread
From: Xiao Ni @ 2020-08-13 8:14 UTC (permalink / raw
To: linux-raid, song; +Cc: colyli, guoqing.jiang, heinzm, ncroxon
Now the discard request is split by chunk size. So it takes a long time
to finish mkfs on disks which support discard function. This patch improve
handling raid10 discard request. It uses the similar way with patch
29efc390b (md/md0: optimize raid0 discard handling).
But it's a little complex than raid0. Because raid10 has different layout.
If raid10 is offset layout and the discard request is smaller than stripe
size. There are some holes when we submit discard bio to underlayer disks.
For example: five disks (disk1 - disk5)
D01 D02 D03 D04 D05
D05 D01 D02 D03 D04
D06 D07 D08 D09 D10
D10 D06 D07 D08 D09
The discard bio just wants to discard from D03 to D10. For disk3, there is
a hole between D03 and D08. For disk4, there is a hole between D04 and D09.
D03 is a chunk, raid10_write_request can handle one chunk perfectly. So
the part that is not aligned with stripe size is still handled by
raid10_write_request.
If reshape is running when discard bio comes and the discard bio spans the
reshape position, raid10_write_request is responsible to handle this
discard bio.
I did a test with this patch set.
Without patch:
time mkfs.xfs /dev/md0
real4m39.775s
user0m0.000s
sys0m0.298s
With patch:
time mkfs.xfs /dev/md0
real0m0.105s
user0m0.000s
sys0m0.007s
nvme3n1 259:1 0 477G 0 disk
└─nvme3n1p1 259:10 0 50G 0 part
nvme4n1 259:2 0 477G 0 disk
└─nvme4n1p1 259:11 0 50G 0 part
nvme5n1 259:6 0 477G 0 disk
└─nvme5n1p1 259:12 0 50G 0 part
nvme2n1 259:9 0 477G 0 disk
└─nvme2n1p1 259:15 0 50G 0 part
nvme0n1 259:13 0 477G 0 disk
└─nvme0n1p1 259:14 0 50G 0 part
Reviewed-by: Coly Li <colyli@suse.de>
Reviewed-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
v1:
Coly helps to review these patches and give some suggestions:
One bug is found. If discard bio is across one stripe but bio size is bigger
than one stripe size. After spliting, the bio will be NULL. In this version,
it checks whether discard bio size is bigger than 2*stripe_size.
In raid10_end_discard_request, it's better to check R10BIO_Uptodate is set
or not. It can avoid write memory to improve performance.
Add more comments for calculating addresses.
v2:
Fix error by checkpatch.pl
Fix one bug for offset layout. v1 calculates wrongly split size
Add more comments to explain how the discard range of each component disk
is decided.
---
drivers/md/raid10.c | 287 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 286 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index cef3cb8..5431e1b 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1526,6 +1526,287 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
raid10_write_request(mddev, bio, r10_bio);
}
+static void wait_blocked_dev(struct mddev *mddev, int cnt)
+{
+ int i;
+ struct r10conf *conf = mddev->private;
+ struct md_rdev *blocked_rdev = NULL;
+
+retry_wait:
+ rcu_read_lock();
+ for (i = 0; i < cnt; i++) {
+ struct md_rdev *rdev = rcu_dereference(conf->mirrors[i].rdev);
+ struct md_rdev *rrdev = rcu_dereference(
+ conf->mirrors[i].replacement);
+ if (rdev == rrdev)
+ rrdev = NULL;
+ if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
+ atomic_inc(&rdev->nr_pending);
+ blocked_rdev = rdev;
+ break;
+ }
+ if (rrdev && unlikely(test_bit(Blocked, &rrdev->flags))) {
+ atomic_inc(&rrdev->nr_pending);
+ blocked_rdev = rrdev;
+ break;
+ }
+ }
+ rcu_read_unlock();
+
+ if (unlikely(blocked_rdev)) {
+ /* Have to wait for this device to get unblocked, then retry */
+ allow_barrier(conf);
+ raid10_log(conf->mddev, "%s wait rdev %d blocked",
+ __func__, blocked_rdev->raid_disk);
+ md_wait_for_blocked_rdev(blocked_rdev, mddev);
+ wait_barrier(conf);
+ goto retry_wait;
+ }
+}
+
+static struct bio *raid10_split_bio(struct r10conf *conf,
+ struct bio *bio, sector_t sectors, bool want_first)
+{
+ struct bio *split;
+
+ split = bio_split(bio, sectors, GFP_NOIO, &conf->bio_split);
+ bio_chain(split, bio);
+ allow_barrier(conf);
+ if (want_first) {
+ submit_bio_noacct(bio);
+ bio = split;
+ } else
+ submit_bio_noacct(split);
+ wait_barrier(conf);
+
+ return bio;
+}
+
+static void raid10_end_discard_request(struct bio *bio)
+{
+ struct r10bio *r10_bio = bio->bi_private;
+ struct r10conf *conf = r10_bio->mddev->private;
+ struct md_rdev *rdev = NULL;
+ int dev;
+ int slot, repl;
+
+ /*
+ * We don't care the return value of discard bio
+ */
+ if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
+ set_bit(R10BIO_Uptodate, &r10_bio->state);
+
+ dev = find_bio_disk(conf, r10_bio, bio, &slot, &repl);
+ if (repl)
+ rdev = conf->mirrors[dev].replacement;
+ if (!rdev) {
+ smp_rmb();
+ repl = 0;
+ rdev = conf->mirrors[dev].rdev;
+ }
+
+ if (atomic_dec_and_test(&r10_bio->remaining)) {
+ md_write_end(r10_bio->mddev);
+ raid_end_bio_io(r10_bio);
+ }
+
+ rdev_dec_pending(rdev, conf->mddev);
+}
+
+/* There are some limitations to handle discard bio
+ * 1st, the discard size is bigger than stripe_size*2.
+ * 2st, if the discard bio spans reshape progress, we use the old way to
+ * handle discard bio
+ */
+static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
+{
+ struct r10conf *conf = mddev->private;
+ struct geom geo = conf->geo;
+ struct r10bio *r10_bio;
+
+ int disk;
+ sector_t chunk;
+ int stripe_size, stripe_mask;
+
+ sector_t bio_start, bio_end;
+ sector_t first_stripe_index, last_stripe_index;
+ sector_t start_disk_offset;
+ unsigned int start_disk_index;
+ sector_t end_disk_offset;
+ unsigned int end_disk_index;
+
+ wait_barrier(conf);
+
+ if (conf->reshape_progress != MaxSector &&
+ ((bio->bi_iter.bi_sector >= conf->reshape_progress) !=
+ conf->mddev->reshape_backwards))
+ geo = conf->prev;
+
+ stripe_size = (1<<geo.chunk_shift) * geo.raid_disks;
+ stripe_mask = stripe_size - 1;
+
+ bio_start = bio->bi_iter.bi_sector;
+ bio_end = bio_end_sector(bio);
+
+ /* Maybe one discard bio is smaller than strip size or across one stripe
+ * and discard region is larger than one stripe size. For far offset layout,
+ * if the discard region is not aligned with stripe size, there is hole
+ * when we submit discard bio to member disk. For simplicity, we only
+ * handle discard bio which discard region is bigger than stripe_size*2
+ */
+ if (bio_sectors(bio) < stripe_size*2)
+ goto out;
+
+ if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
+ bio_start < conf->reshape_progress &&
+ bio_end > conf->reshape_progress)
+ goto out;
+
+ /* For far offset layout, if bio is not aligned with stripe size, it splits
+ * the part that is not aligned with strip size.
+ */
+ if (geo.far_offset && (bio_start & stripe_mask)) {
+ sector_t split_size;
+ split_size = round_up(bio_start, stripe_size) - bio_start;
+ bio = raid10_split_bio(conf, bio, split_size, false);
+ }
+ if (geo.far_offset && (bio_end & stripe_mask)) {
+ sector_t split_size;
+ split_size = bio_sectors(bio) - (bio_end & stripe_mask);
+ bio = raid10_split_bio(conf, bio, split_size, true);
+ }
+
+ r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
+ r10_bio->mddev = mddev;
+ r10_bio->state = 0;
+ memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->geo.raid_disks);
+
+ wait_blocked_dev(mddev, geo.raid_disks);
+
+ r10_bio->master_bio = bio;
+
+ bio_start = bio->bi_iter.bi_sector;
+ bio_end = bio_end_sector(bio);
+
+ /* raid10 uses chunk as the unit to store data. It's similar like raid0.
+ * One stripe contains the chunks from all member disk (one chunk from
+ * one disk at the same HBA address). For layout detail, see 'man md 4'
+ */
+ chunk = bio_start >> geo.chunk_shift;
+ chunk *= geo.near_copies;
+ first_stripe_index = chunk;
+ start_disk_index = sector_div(first_stripe_index, geo.raid_disks);
+ if (geo.far_offset)
+ first_stripe_index *= geo.far_copies;
+ start_disk_offset = (bio_start & geo.chunk_mask) +
+ (first_stripe_index << geo.chunk_shift);
+
+ chunk = bio_end >> geo.chunk_shift;
+ chunk *= geo.near_copies;
+ last_stripe_index = chunk;
+ end_disk_index = sector_div(last_stripe_index, geo.raid_disks);
+ if (geo.far_offset)
+ last_stripe_index *= geo.far_copies;
+ end_disk_offset = (bio_end & geo.chunk_mask) +
+ (last_stripe_index << geo.chunk_shift);
+
+ rcu_read_lock();
+ for (disk = 0; disk < geo.raid_disks; disk++) {
+ struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
+ struct md_rdev *rrdev = rcu_dereference(
+ conf->mirrors[disk].replacement);
+
+ r10_bio->devs[disk].bio = NULL;
+ r10_bio->devs[disk].repl_bio = NULL;
+
+ if (rdev && (test_bit(Faulty, &rdev->flags)))
+ rdev = NULL;
+ if (rrdev && (test_bit(Faulty, &rrdev->flags)))
+ rrdev = NULL;
+ if (!rdev && !rrdev)
+ continue;
+
+ if (rdev) {
+ r10_bio->devs[disk].bio = bio;
+ atomic_inc(&rdev->nr_pending);
+ }
+ if (rrdev) {
+ r10_bio->devs[disk].repl_bio = bio;
+ atomic_inc(&rrdev->nr_pending);
+ }
+ }
+ rcu_read_unlock();
+
+ atomic_set(&r10_bio->remaining, 1);
+ for (disk = 0; disk < geo.raid_disks; disk++) {
+ sector_t dev_start, dev_end;
+ struct bio *mbio, *rbio = NULL;
+ struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
+ struct md_rdev *rrdev = rcu_dereference(
+ conf->mirrors[disk].replacement);
+
+ /*
+ * Now start to calculate the start and end address for each disk.
+ * The space between dev_start and dev_end is the discard region.
+ *
+ * For dev_start, it needs to consider three conditions:
+ * 1st, the disk is before start_disk, you can imagine the disk in
+ * the next stripe. So the dev_start is the start address of next
+ * stripe.
+ * 2st, the disk is after start_disk, it means the disk is at the
+ * same stripe of first disk
+ * 3st, the first disk itself, we can use start_disk_offset directly
+ */
+ if (disk < start_disk_index)
+ dev_start = (first_stripe_index + 1) * mddev->chunk_sectors;
+ else if (disk > start_disk_index)
+ dev_start = first_stripe_index * mddev->chunk_sectors;
+ else
+ dev_start = start_disk_offset;
+
+ if (disk < end_disk_index)
+ dev_end = (last_stripe_index + 1) * mddev->chunk_sectors;
+ else if (disk > end_disk_index)
+ dev_end = last_stripe_index * mddev->chunk_sectors;
+ else
+ dev_end = end_disk_offset;
+
+ /* It only handles discard bio which size is >= stripe size, so
+ * dev_end > dev_start all the time
+ */
+ if (r10_bio->devs[disk].bio) {
+ mbio = bio_clone_fast(bio, GFP_NOIO, &mddev->bio_set);
+ mbio->bi_end_io = raid10_end_discard_request;
+ mbio->bi_private = r10_bio;
+ r10_bio->devs[disk].bio = mbio;
+ r10_bio->devs[disk].devnum = disk;
+ atomic_inc(&r10_bio->remaining);
+ md_submit_discard_bio(mddev, rdev, mbio, dev_start, dev_end);
+ bio_endio(mbio);
+ }
+ if (r10_bio->devs[disk].repl_bio) {
+ rbio = bio_clone_fast(bio, GFP_NOIO, &mddev->bio_set);
+ rbio->bi_end_io = raid10_end_discard_request;
+ rbio->bi_private = r10_bio;
+ r10_bio->devs[disk].repl_bio = rbio;
+ r10_bio->devs[disk].devnum = disk;
+ atomic_inc(&r10_bio->remaining);
+ md_submit_discard_bio(mddev, rrdev, rbio, dev_start, dev_end);
+ bio_endio(rbio);
+ }
+ }
+
+ if (atomic_dec_and_test(&r10_bio->remaining)) {
+ md_write_end(r10_bio->mddev);
+ raid_end_bio_io(r10_bio);
+ }
+
+ return 0;
+out:
+ allow_barrier(conf);
+ return -EAGAIN;
+}
+
static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
{
struct r10conf *conf = mddev->private;
@@ -1540,6 +1821,10 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
if (!md_write_start(mddev, bio))
return false;
+ if (unlikely(bio_op(bio) == REQ_OP_DISCARD))
+ if (!raid10_handle_discard(mddev, bio))
+ return true;
+
/*
* If this request crosses a chunk boundary, we need to split
* it.
@@ -3770,7 +4055,7 @@ static int raid10_run(struct mddev *mddev)
chunk_size = mddev->chunk_sectors << 9;
if (mddev->queue) {
blk_queue_max_discard_sectors(mddev->queue,
- mddev->chunk_sectors);
+ UINT_MAX);
blk_queue_max_write_same_sectors(mddev->queue, 0);
blk_queue_max_write_zeroes_sectors(mddev->queue, 0);
blk_queue_io_min(mddev->queue, chunk_size);
--
2.7.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V3 4/4] md/raid10: Improve discard request for far layout
2020-08-13 8:14 [PATCH V3 0/4] md/raid10: Improve handling raid10 discard request Xiao Ni
` (2 preceding siblings ...)
2020-08-13 8:14 ` [PATCH V3 3/4] md/raid10: improve raid10 discard request Xiao Ni
@ 2020-08-13 8:14 ` Xiao Ni
2020-08-19 18:38 ` Song Liu
3 siblings, 1 reply; 13+ messages in thread
From: Xiao Ni @ 2020-08-13 8:14 UTC (permalink / raw
To: linux-raid, song; +Cc: colyli, guoqing.jiang, heinzm, ncroxon
For far layout, the discard region is not continuous on disks. So it needs
far copies r10bio to cover all regions. It needs a way to know all r10bios
have finish or not. Similar with raid10_sync_request, only the first r10bio
master_bio records the discard bio. Other r10bios master_bio record the first
r10bio. The first r10bio can finish after other r10bios finish and then return
the discard bio.
Signed-off-by: Xiao Ni <xni@redhat.com>
---
drivers/md/raid10.c | 89 ++++++++++++++++++++++++++++++++++++++---------------
drivers/md/raid10.h | 1 +
2 files changed, 66 insertions(+), 24 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 5431e1b..97f673a 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1582,6 +1582,27 @@ static struct bio *raid10_split_bio(struct r10conf *conf,
return bio;
}
+static void raid_end_discard_bio(struct r10bio *r10bio)
+{
+ struct r10conf *conf = r10bio->mddev->private;
+
+ while (atomic_dec_and_test(&r10bio->remaining)) {
+
+ allow_barrier(conf);
+
+ if (test_bit(R10BIO_Discard, &r10bio->state)) {
+ md_write_end(r10bio->mddev);
+ bio_endio(r10bio->master_bio);
+ free_r10bio(r10bio);
+ break;
+ } else {
+ struct r10bio *first_r10bio = (struct r10bio *)r10bio->master_bio;
+ free_r10bio(r10bio);
+ r10bio = first_r10bio;
+ }
+ }
+}
+
static void raid10_end_discard_request(struct bio *bio)
{
struct r10bio *r10_bio = bio->bi_private;
@@ -1605,10 +1626,7 @@ static void raid10_end_discard_request(struct bio *bio)
rdev = conf->mirrors[dev].rdev;
}
- if (atomic_dec_and_test(&r10_bio->remaining)) {
- md_write_end(r10_bio->mddev);
- raid_end_bio_io(r10_bio);
- }
+ raid_end_discard_bio(r10_bio);
rdev_dec_pending(rdev, conf->mddev);
}
@@ -1622,7 +1640,9 @@ static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
{
struct r10conf *conf = mddev->private;
struct geom geo = conf->geo;
- struct r10bio *r10_bio;
+ struct r10bio *r10_bio, *first_r10bio;
+ int far_copies = geo.far_copies;
+ bool first_copy = true;
int disk;
sector_t chunk;
@@ -1649,9 +1669,9 @@ static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
bio_end = bio_end_sector(bio);
/* Maybe one discard bio is smaller than strip size or across one stripe
- * and discard region is larger than one stripe size. For far offset layout,
- * if the discard region is not aligned with stripe size, there is hole
- * when we submit discard bio to member disk. For simplicity, we only
+ * and discard region is larger than one stripe size. For far and far offset
+ * layout, if the discard region is not aligned with stripe size, there is
+ * hole when we submit discard bio to member disk. For simplicity, we only
* handle discard bio which discard region is bigger than stripe_size*2
*/
if (bio_sectors(bio) < stripe_size*2)
@@ -1662,29 +1682,20 @@ static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
bio_end > conf->reshape_progress)
goto out;
- /* For far offset layout, if bio is not aligned with stripe size, it splits
- * the part that is not aligned with strip size.
+ /* For far and far offset layout, if bio is not aligned with stripe size,
+ * it splits the part that is not aligned with strip size.
*/
- if (geo.far_offset && (bio_start & stripe_mask)) {
+ if ((far_copies > 1) && (bio_start & stripe_mask)) {
sector_t split_size;
split_size = round_up(bio_start, stripe_size) - bio_start;
bio = raid10_split_bio(conf, bio, split_size, false);
}
- if (geo.far_offset && (bio_end & stripe_mask)) {
+ if ((far_copies > 1) && (bio_end & stripe_mask)) {
sector_t split_size;
split_size = bio_sectors(bio) - (bio_end & stripe_mask);
bio = raid10_split_bio(conf, bio, split_size, true);
}
- r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
- r10_bio->mddev = mddev;
- r10_bio->state = 0;
- memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->geo.raid_disks);
-
- wait_blocked_dev(mddev, geo.raid_disks);
-
- r10_bio->master_bio = bio;
-
bio_start = bio->bi_iter.bi_sector;
bio_end = bio_end_sector(bio);
@@ -1710,6 +1721,28 @@ static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
end_disk_offset = (bio_end & geo.chunk_mask) +
(last_stripe_index << geo.chunk_shift);
+ wait_blocked_dev(mddev, geo.raid_disks);
+
+retry_discard:
+ r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
+ r10_bio->mddev = mddev;
+ r10_bio->state = 0;
+ memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->geo.raid_disks);
+
+ /* For far layout it needs more than one r10bio to cover all regions.
+ * Inspired by raid10_sync_request, we can use the first r10bio->master_bio
+ * to record the discard bio. Other r10bio->master_bio record the first
+ * r10bio. The first r10bio only release after all other r10bios finish.
+ * The discard bio returns only first r10bio finishes
+ */
+ if (first_copy) {
+ r10_bio->master_bio = bio;
+ set_bit(R10BIO_Discard, &r10_bio->state);
+ first_copy = false;
+ first_r10bio = r10_bio;
+ } else
+ r10_bio->master_bio = (struct bio *)first_r10bio;
+
rcu_read_lock();
for (disk = 0; disk < geo.raid_disks; disk++) {
struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
@@ -1796,11 +1829,19 @@ static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
}
}
- if (atomic_dec_and_test(&r10_bio->remaining)) {
- md_write_end(r10_bio->mddev);
- raid_end_bio_io(r10_bio);
+ if (!geo.far_offset && --far_copies) {
+ first_stripe_index += geo.stride >> geo.chunk_shift;
+ start_disk_offset += geo.stride;
+ last_stripe_index += geo.stride >> geo.chunk_shift;
+ end_disk_offset += geo.stride;
+ atomic_inc(&first_r10bio->remaining);
+ raid_end_discard_bio(r10_bio);
+ wait_barrier(conf);
+ goto retry_discard;
}
+ raid_end_discard_bio(r10_bio);
+
return 0;
out:
allow_barrier(conf);
diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
index 79cd2b7..1461fd5 100644
--- a/drivers/md/raid10.h
+++ b/drivers/md/raid10.h
@@ -179,5 +179,6 @@ enum r10bio_state {
R10BIO_Previous,
/* failfast devices did receive failfast requests. */
R10BIO_FailFast,
+ R10BIO_Discard,
};
#endif
--
2.7.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V3 3/4] md/raid10: improve raid10 discard request
2020-08-13 8:14 ` [PATCH V3 3/4] md/raid10: improve raid10 discard request Xiao Ni
@ 2020-08-19 18:36 ` Song Liu
2020-08-20 7:26 ` Xiao Ni
[not found] ` <21012936-9397-40f9-115b-87dda93d9017@redhat.com>
0 siblings, 2 replies; 13+ messages in thread
From: Song Liu @ 2020-08-19 18:36 UTC (permalink / raw
To: Xiao Ni; +Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
On Thu, Aug 13, 2020 at 1:15 AM Xiao Ni <xni@redhat.com> wrote:
>
[...]
> Reviewed-by: Coly Li <colyli@suse.de>
> Reviewed-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
>
Please add "---" before "v1:..." so that this part is ignored during git am.
> v1:
> Coly helps to review these patches and give some suggestions:
> One bug is found. If discard bio is across one stripe but bio size is bigger
> than one stripe size. After spliting, the bio will be NULL. In this version,
> it checks whether discard bio size is bigger than 2*stripe_size.
> In raid10_end_discard_request, it's better to check R10BIO_Uptodate is set
> or not. It can avoid write memory to improve performance.
> Add more comments for calculating addresses.
>
> v2:
> Fix error by checkpatch.pl
> Fix one bug for offset layout. v1 calculates wrongly split size
> Add more comments to explain how the discard range of each component disk
> is decided.
> ---
> drivers/md/raid10.c | 287 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 286 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index cef3cb8..5431e1b 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1526,6 +1526,287 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
> raid10_write_request(mddev, bio, r10_bio);
> }
>
> +static void wait_blocked_dev(struct mddev *mddev, int cnt)
> +{
> + int i;
> + struct r10conf *conf = mddev->private;
> + struct md_rdev *blocked_rdev = NULL;
> +
> +retry_wait:
> + rcu_read_lock();
> + for (i = 0; i < cnt; i++) {
> + struct md_rdev *rdev = rcu_dereference(conf->mirrors[i].rdev);
> + struct md_rdev *rrdev = rcu_dereference(
> + conf->mirrors[i].replacement);
> + if (rdev == rrdev)
> + rrdev = NULL;
> + if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
> + atomic_inc(&rdev->nr_pending);
> + blocked_rdev = rdev;
> + break;
> + }
> + if (rrdev && unlikely(test_bit(Blocked, &rrdev->flags))) {
> + atomic_inc(&rrdev->nr_pending);
> + blocked_rdev = rrdev;
> + break;
> + }
> + }
> + rcu_read_unlock();
> +
> + if (unlikely(blocked_rdev)) {
> + /* Have to wait for this device to get unblocked, then retry */
> + allow_barrier(conf);
> + raid10_log(conf->mddev, "%s wait rdev %d blocked",
> + __func__, blocked_rdev->raid_disk);
> + md_wait_for_blocked_rdev(blocked_rdev, mddev);
> + wait_barrier(conf);
> + goto retry_wait;
We need to clear blocked_rdev before this goto, or put retry_wait label
before "blocked_rdev = NULL;". I guess this path is not tested...
We are duplicating a lot of logic from raid10_write_request() here. Can we
try to pull the common logic into a helper function?
[...]
> +static void raid10_end_discard_request(struct bio *bio)
> +{
> + struct r10bio *r10_bio = bio->bi_private;
> + struct r10conf *conf = r10_bio->mddev->private;
> + struct md_rdev *rdev = NULL;
> + int dev;
> + int slot, repl;
> +
> + /*
> + * We don't care the return value of discard bio
> + */
> + if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
> + set_bit(R10BIO_Uptodate, &r10_bio->state);
> +
> + dev = find_bio_disk(conf, r10_bio, bio, &slot, &repl);
> + if (repl)
> + rdev = conf->mirrors[dev].replacement;
> + if (!rdev) {
> + smp_rmb();
> + repl = 0;
> + rdev = conf->mirrors[dev].rdev;
> + }
> +
> + if (atomic_dec_and_test(&r10_bio->remaining)) {
> + md_write_end(r10_bio->mddev);
> + raid_end_bio_io(r10_bio);
> + }
> +
> + rdev_dec_pending(rdev, conf->mddev);
> +}
> +
> +/* There are some limitations to handle discard bio
> + * 1st, the discard size is bigger than stripe_size*2.
> + * 2st, if the discard bio spans reshape progress, we use the old way to
> + * handle discard bio
> + */
> +static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> +{
> + struct r10conf *conf = mddev->private;
> + struct geom geo = conf->geo;
Do we really need a full copy of conf->geo?
> + struct r10bio *r10_bio;
> +
> + int disk;
> + sector_t chunk;
> + int stripe_size, stripe_mask;
> +
> + sector_t bio_start, bio_end;
> + sector_t first_stripe_index, last_stripe_index;
> + sector_t start_disk_offset;
> + unsigned int start_disk_index;
> + sector_t end_disk_offset;
> + unsigned int end_disk_index;
> +
> + wait_barrier(conf);
> +
> + if (conf->reshape_progress != MaxSector &&
> + ((bio->bi_iter.bi_sector >= conf->reshape_progress) !=
> + conf->mddev->reshape_backwards))
> + geo = conf->prev;
> +
> + stripe_size = (1<<geo.chunk_shift) * geo.raid_disks;
This could be raid_disks << chunk_shift
> + stripe_mask = stripe_size - 1;
Does this work when raid_disks is not power of 2?
> +
> + bio_start = bio->bi_iter.bi_sector;
> + bio_end = bio_end_sector(bio);
> +
> + /* Maybe one discard bio is smaller than strip size or across one stripe
> + * and discard region is larger than one stripe size. For far offset layout,
> + * if the discard region is not aligned with stripe size, there is hole
> + * when we submit discard bio to member disk. For simplicity, we only
> + * handle discard bio which discard region is bigger than stripe_size*2
> + */
> + if (bio_sectors(bio) < stripe_size*2)
> + goto out;
> +
> + if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
> + bio_start < conf->reshape_progress &&
> + bio_end > conf->reshape_progress)
> + goto out;
> +
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 4/4] md/raid10: Improve discard request for far layout
2020-08-13 8:14 ` [PATCH V3 4/4] md/raid10: Improve discard request for far layout Xiao Ni
@ 2020-08-19 18:38 ` Song Liu
0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2020-08-19 18:38 UTC (permalink / raw
To: Xiao Ni; +Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
On Thu, Aug 13, 2020 at 1:15 AM Xiao Ni <xni@redhat.com> wrote:
>
> For far layout, the discard region is not continuous on disks. So it needs
> far copies r10bio to cover all regions. It needs a way to know all r10bios
> have finish or not. Similar with raid10_sync_request, only the first r10bio
> master_bio records the discard bio. Other r10bios master_bio record the first
> r10bio. The first r10bio can finish after other r10bios finish and then return
> the discard bio.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
1/4 and 2/4 of the set looks good to me. But we will need more work on 3
and 4. Please refer to comments in 3/4.
Please also rebase on top of the latest md-next branch, and try fix warnings
from checkpatch.pl.
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 3/4] md/raid10: improve raid10 discard request
2020-08-19 18:36 ` Song Liu
@ 2020-08-20 7:26 ` Xiao Ni
2020-08-20 9:06 ` Xiao Ni
[not found] ` <21012936-9397-40f9-115b-87dda93d9017@redhat.com>
1 sibling, 1 reply; 13+ messages in thread
From: Xiao Ni @ 2020-08-20 7:26 UTC (permalink / raw
To: Song Liu
Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
Hi Song
On 08/20/2020 02:36 AM, Song Liu wrote:
> On Thu, Aug 13, 2020 at 1:15 AM Xiao Ni <xni@redhat.com> wrote:
> [...]
>> Reviewed-by: Coly Li <colyli@suse.de>
>> Reviewed-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>
> Please add "---" before "v1:..." so that this part is ignored during git am.
Ok
>
>> v1:
>> Coly helps to review these patches and give some suggestions:
>> One bug is found. If discard bio is across one stripe but bio size is bigger
>> than one stripe size. After spliting, the bio will be NULL. In this version,
>> it checks whether discard bio size is bigger than 2*stripe_size.
>> In raid10_end_discard_request, it's better to check R10BIO_Uptodate is set
>> or not. It can avoid write memory to improve performance.
>> Add more comments for calculating addresses.
>>
>> v2:
>> Fix error by checkpatch.pl
>> Fix one bug for offset layout. v1 calculates wrongly split size
>> Add more comments to explain how the discard range of each component disk
>> is decided.
>> ---
>> drivers/md/raid10.c | 287 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 286 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index cef3cb8..5431e1b 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1526,6 +1526,287 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>> raid10_write_request(mddev, bio, r10_bio);
>> }
>>
>> +static void wait_blocked_dev(struct mddev *mddev, int cnt)
>> +{
>> + int i;
>> + struct r10conf *conf = mddev->private;
>> + struct md_rdev *blocked_rdev = NULL;
>> +
>> +retry_wait:
>> + rcu_read_lock();
>> + for (i = 0; i < cnt; i++) {
>> + struct md_rdev *rdev = rcu_dereference(conf->mirrors[i].rdev);
>> + struct md_rdev *rrdev = rcu_dereference(
>> + conf->mirrors[i].replacement);
>> + if (rdev == rrdev)
>> + rrdev = NULL;
>> + if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
>> + atomic_inc(&rdev->nr_pending);
>> + blocked_rdev = rdev;
>> + break;
>> + }
>> + if (rrdev && unlikely(test_bit(Blocked, &rrdev->flags))) {
>> + atomic_inc(&rrdev->nr_pending);
>> + blocked_rdev = rrdev;
>> + break;
>> + }
>> + }
>> + rcu_read_unlock();
>> +
>> + if (unlikely(blocked_rdev)) {
>> + /* Have to wait for this device to get unblocked, then retry */
>> + allow_barrier(conf);
>> + raid10_log(conf->mddev, "%s wait rdev %d blocked",
>> + __func__, blocked_rdev->raid_disk);
>> + md_wait_for_blocked_rdev(blocked_rdev, mddev);
>> + wait_barrier(conf);
>> + goto retry_wait;
> We need to clear blocked_rdev before this goto, or put retry_wait label
> before "blocked_rdev = NULL;". I guess this path is not tested...
I did test for this patch with mkfs with/without resync and wrote some
files to device.
And ran fstrim after writing some files. The patch worked well during
the test.
For blocked device situation, I didn't test. I'll add this test.
>
> We are duplicating a lot of logic from raid10_write_request() here. Can we
> try to pull the common logic into a helper function?
I'll do this.
>
> [...]
>
>> +static void raid10_end_discard_request(struct bio *bio)
>> +{
>> + struct r10bio *r10_bio = bio->bi_private;
>> + struct r10conf *conf = r10_bio->mddev->private;
>> + struct md_rdev *rdev = NULL;
>> + int dev;
>> + int slot, repl;
>> +
>> + /*
>> + * We don't care the return value of discard bio
>> + */
>> + if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
>> + set_bit(R10BIO_Uptodate, &r10_bio->state);
>> +
>> + dev = find_bio_disk(conf, r10_bio, bio, &slot, &repl);
>> + if (repl)
>> + rdev = conf->mirrors[dev].replacement;
>> + if (!rdev) {
>> + smp_rmb();
>> + repl = 0;
>> + rdev = conf->mirrors[dev].rdev;
>> + }
>> +
>> + if (atomic_dec_and_test(&r10_bio->remaining)) {
>> + md_write_end(r10_bio->mddev);
>> + raid_end_bio_io(r10_bio);
>> + }
>> +
>> + rdev_dec_pending(rdev, conf->mddev);
>> +}
>> +
>> +/* There are some limitations to handle discard bio
>> + * 1st, the discard size is bigger than stripe_size*2.
>> + * 2st, if the discard bio spans reshape progress, we use the old way to
>> + * handle discard bio
>> + */
>> +static bool raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>> +{
>> + struct r10conf *conf = mddev->private;
>> + struct geom geo = conf->geo;
> Do we really need a full copy of conf->geo?
It doesn't need a full copy. Thanks for pointing about this.
>
>> + struct r10bio *r10_bio;
>> +
>> + int disk;
>> + sector_t chunk;
>> + int stripe_size, stripe_mask;
>> +
>> + sector_t bio_start, bio_end;
>> + sector_t first_stripe_index, last_stripe_index;
>> + sector_t start_disk_offset;
>> + unsigned int start_disk_index;
>> + sector_t end_disk_offset;
>> + unsigned int end_disk_index;
>> +
>> + wait_barrier(conf);
>> +
>> + if (conf->reshape_progress != MaxSector &&
>> + ((bio->bi_iter.bi_sector >= conf->reshape_progress) !=
>> + conf->mddev->reshape_backwards))
>> + geo = conf->prev;
>> +
>> + stripe_size = (1<<geo.chunk_shift) * geo.raid_disks;
> This could be raid_disks << chunk_shift
ok
>
>> + stripe_mask = stripe_size - 1;
> Does this work when raid_disks is not power of 2?
In test I used 5 disks to create the raid10 too, it worked well. Could
you explain what
you worried in detail?
Regards
Xiao
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 3/4] md/raid10: improve raid10 discard request
2020-08-20 7:26 ` Xiao Ni
@ 2020-08-20 9:06 ` Xiao Ni
2020-08-20 22:45 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Xiao Ni @ 2020-08-20 9:06 UTC (permalink / raw
To: Song Liu
Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
On 08/20/2020 03:26 PM, Xiao Ni wrote:
> Hi Song
>
>
> On 08/20/2020 02:36 AM, Song Liu wrote:
>> On Thu, Aug 13, 2020 at 1:15 AM Xiao Ni <xni@redhat.com> wrote:
>> [...]
>>> Reviewed-by: Coly Li <colyli@suse.de>
>>> Reviewed-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>>
>> Please add "---" before "v1:..." so that this part is ignored during
>> git am.
> Ok
>>
>>> v1:
>>> Coly helps to review these patches and give some suggestions:
>>> One bug is found. If discard bio is across one stripe but bio size
>>> is bigger
>>> than one stripe size. After spliting, the bio will be NULL. In this
>>> version,
>>> it checks whether discard bio size is bigger than 2*stripe_size.
>>> In raid10_end_discard_request, it's better to check R10BIO_Uptodate
>>> is set
>>> or not. It can avoid write memory to improve performance.
>>> Add more comments for calculating addresses.
>>>
>>> v2:
>>> Fix error by checkpatch.pl
>>> Fix one bug for offset layout. v1 calculates wrongly split size
>>> Add more comments to explain how the discard range of each component
>>> disk
>>> is decided.
>>> ---
>>> drivers/md/raid10.c | 287
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 286 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>> index cef3cb8..5431e1b 100644
>>> --- a/drivers/md/raid10.c
>>> +++ b/drivers/md/raid10.c
>>> @@ -1526,6 +1526,287 @@ static void __make_request(struct mddev
>>> *mddev, struct bio *bio, int sectors)
>>> raid10_write_request(mddev, bio, r10_bio);
>>> }
>>>
>>> +static void wait_blocked_dev(struct mddev *mddev, int cnt)
>>> +{
>>> + int i;
>>> + struct r10conf *conf = mddev->private;
>>> + struct md_rdev *blocked_rdev = NULL;
>>> +
>>> +retry_wait:
>>> + rcu_read_lock();
>>> + for (i = 0; i < cnt; i++) {
>>> + struct md_rdev *rdev =
>>> rcu_dereference(conf->mirrors[i].rdev);
>>> + struct md_rdev *rrdev = rcu_dereference(
>>> + conf->mirrors[i].replacement);
>>> + if (rdev == rrdev)
>>> + rrdev = NULL;
>>> + if (rdev && unlikely(test_bit(Blocked,
>>> &rdev->flags))) {
>>> + atomic_inc(&rdev->nr_pending);
>>> + blocked_rdev = rdev;
>>> + break;
>>> + }
>>> + if (rrdev && unlikely(test_bit(Blocked,
>>> &rrdev->flags))) {
>>> + atomic_inc(&rrdev->nr_pending);
>>> + blocked_rdev = rrdev;
>>> + break;
>>> + }
>>> + }
>>> + rcu_read_unlock();
>>> +
>>> + if (unlikely(blocked_rdev)) {
>>> + /* Have to wait for this device to get unblocked,
>>> then retry */
>>> + allow_barrier(conf);
>>> + raid10_log(conf->mddev, "%s wait rdev %d blocked",
>>> + __func__, blocked_rdev->raid_disk);
>>> + md_wait_for_blocked_rdev(blocked_rdev, mddev);
>>> + wait_barrier(conf);
>>> + goto retry_wait;
>> We need to clear blocked_rdev before this goto, or put retry_wait label
>> before "blocked_rdev = NULL;". I guess this path is not tested...
> I did test for this patch with mkfs with/without resync and wrote some
> files to device.
> And ran fstrim after writing some files. The patch worked well during
> the test.
> For blocked device situation, I didn't test. I'll add this test.
>>
>> We are duplicating a lot of logic from raid10_write_request() here.
>> Can we
>> try to pull the common logic into a helper function?
> I'll do this.
Hi Song
At first I had thought about this. In raid10_write_request, it checks
blocked rdev, badblock and sets
r10_bio->devs[].bio in one loop. Can we move the codes that check
blocked rdev into a separate loop,
similar like this:
wait_block:
for (i = 0; i < conf->copies; i++) {
/* check rdev/rrdev is block or not
* If it's block, goto wait_block
*/
}
for (i = 0; i < conf->copies; i++) {
/* check bad block
* sets r10_bio->devs[i].bio
*/
}
This way it waits until all rdev devices are not blocked. There is a
possibility that some rdev devices change
to blocked again after checking. But the way raid10_write_request uses
now has this risk too. If you think
the method mentioned above is OK, I'll try to use this way.
Best Regards
Xiao
>>
>> [...]
>>
>>> +static void raid10_end_discard_request(struct bio *bio)
>>> +{
>>> + struct r10bio *r10_bio = bio->bi_private;
>>> + struct r10conf *conf = r10_bio->mddev->private;
>>> + struct md_rdev *rdev = NULL;
>>> + int dev;
>>> + int slot, repl;
>>> +
>>> + /*
>>> + * We don't care the return value of discard bio
>>> + */
>>> + if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
>>> + set_bit(R10BIO_Uptodate, &r10_bio->state);
>>> +
>>> + dev = find_bio_disk(conf, r10_bio, bio, &slot, &repl);
>>> + if (repl)
>>> + rdev = conf->mirrors[dev].replacement;
>>> + if (!rdev) {
>>> + smp_rmb();
>>> + repl = 0;
>>> + rdev = conf->mirrors[dev].rdev;
>>> + }
>>> +
>>> + if (atomic_dec_and_test(&r10_bio->remaining)) {
>>> + md_write_end(r10_bio->mddev);
>>> + raid_end_bio_io(r10_bio);
>>> + }
>>> +
>>> + rdev_dec_pending(rdev, conf->mddev);
>>> +}
>>> +
>>> +/* There are some limitations to handle discard bio
>>> + * 1st, the discard size is bigger than stripe_size*2.
>>> + * 2st, if the discard bio spans reshape progress, we use the old
>>> way to
>>> + * handle discard bio
>>> + */
>>> +static bool raid10_handle_discard(struct mddev *mddev, struct bio
>>> *bio)
>>> +{
>>> + struct r10conf *conf = mddev->private;
>>> + struct geom geo = conf->geo;
>> Do we really need a full copy of conf->geo?
> It doesn't need a full copy. Thanks for pointing about this.
>>
>>> + struct r10bio *r10_bio;
>>> +
>>> + int disk;
>>> + sector_t chunk;
>>> + int stripe_size, stripe_mask;
>>> +
>>> + sector_t bio_start, bio_end;
>>> + sector_t first_stripe_index, last_stripe_index;
>>> + sector_t start_disk_offset;
>>> + unsigned int start_disk_index;
>>> + sector_t end_disk_offset;
>>> + unsigned int end_disk_index;
>>> +
>>> + wait_barrier(conf);
>>> +
>>> + if (conf->reshape_progress != MaxSector &&
>>> + ((bio->bi_iter.bi_sector >= conf->reshape_progress) !=
>>> + conf->mddev->reshape_backwards))
>>> + geo = conf->prev;
>>> +
>>> + stripe_size = (1<<geo.chunk_shift) * geo.raid_disks;
>> This could be raid_disks << chunk_shift
> ok
>>
>>> + stripe_mask = stripe_size - 1;
>> Does this work when raid_disks is not power of 2?
> In test I used 5 disks to create the raid10 too, it worked well. Could
> you explain what
> you worried in detail?
>
> Regards
> Xiao
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 3/4] md/raid10: improve raid10 discard request
[not found] ` <21012936-9397-40f9-115b-87dda93d9017@redhat.com>
@ 2020-08-20 22:13 ` Song Liu
2020-08-20 22:39 ` Xiao Ni
0 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2020-08-20 22:13 UTC (permalink / raw
To: Xiao Ni; +Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
On Thu, Aug 20, 2020 at 12:22 AM Xiao Ni <xni@redhat.com> wrote:
[...]
> +
> + if (conf->reshape_progress != MaxSector &&
> + ((bio->bi_iter.bi_sector >= conf->reshape_progress) !=
> + conf->mddev->reshape_backwards))
> + geo = conf->prev;
> +
> + stripe_size = (1<<geo.chunk_shift) * geo.raid_disks;
>
> This could be raid_disks << chunk_shift
>
> + stripe_mask = stripe_size - 1;
>
> Does this work when raid_disks is not power of 2?
>
> In test I used 5 disks to create the raid10 too, it worked well. Could you explain what
> you worried in detail?
Say we have geo.raid_disks == 5, and geo.chunk_shift == 8. Then we get
stripe_size == 0x500 and stripe_mask == 0x4ff == b'100 1111 1111
Is this the proper stripe_mask?
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 3/4] md/raid10: improve raid10 discard request
2020-08-20 22:13 ` Song Liu
@ 2020-08-20 22:39 ` Xiao Ni
0 siblings, 0 replies; 13+ messages in thread
From: Xiao Ni @ 2020-08-20 22:39 UTC (permalink / raw
To: Song Liu
Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
On 08/21/2020 06:13 AM, Song Liu wrote:
> On Thu, Aug 20, 2020 at 12:22 AM Xiao Ni <xni@redhat.com> wrote:
> [...]
>
>> +
>> + if (conf->reshape_progress != MaxSector &&
>> + ((bio->bi_iter.bi_sector >= conf->reshape_progress) !=
>> + conf->mddev->reshape_backwards))
>> + geo = conf->prev;
>> +
>> + stripe_size = (1<<geo.chunk_shift) * geo.raid_disks;
>>
>> This could be raid_disks << chunk_shift
>>
>> + stripe_mask = stripe_size - 1;
>>
>> Does this work when raid_disks is not power of 2?
>>
>> In test I used 5 disks to create the raid10 too, it worked well. Could you explain what
>> you worried in detail?
> Say we have geo.raid_disks == 5, and geo.chunk_shift == 8. Then we get
> stripe_size == 0x500 and stripe_mask == 0x4ff == b'100 1111 1111
> Is this the proper stripe_mask?
>
> Thanks,
> Song
>
I got it. Thanks very much. It can't use mask here to justify whether
start/end address is aligned
with stripe size. It should check whether it's a multiple of stripe
size. I'll send patch again after
fixing all problems.
Regards
Xiao
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 3/4] md/raid10: improve raid10 discard request
2020-08-20 9:06 ` Xiao Ni
@ 2020-08-20 22:45 ` Song Liu
0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2020-08-20 22:45 UTC (permalink / raw
To: Xiao Ni; +Cc: linux-raid, Coly Li, Guoqing Jiang, Heinz Mauelshagen,
Nigel Croxon
On Thu, Aug 20, 2020 at 2:06 AM Xiao Ni <xni@redhat.com> wrote:
[...]
> >> We need to clear blocked_rdev before this goto, or put retry_wait label
> >> before "blocked_rdev = NULL;". I guess this path is not tested...
> > I did test for this patch with mkfs with/without resync and wrote some
> > files to device.
> > And ran fstrim after writing some files. The patch worked well during
> > the test.
> > For blocked device situation, I didn't test. I'll add this test.
> >>
> >> We are duplicating a lot of logic from raid10_write_request() here.
> >> Can we
> >> try to pull the common logic into a helper function?
> > I'll do this.
>
> Hi Song
>
> At first I had thought about this. In raid10_write_request, it checks
> blocked rdev, badblock and sets
> r10_bio->devs[].bio in one loop. Can we move the codes that check
> blocked rdev into a separate loop,
> similar like this:
>
> wait_block:
> for (i = 0; i < conf->copies; i++) {
> /* check rdev/rrdev is block or not
> * If it's block, goto wait_block
> */
> }
>
> for (i = 0; i < conf->copies; i++) {
> /* check bad block
> * sets r10_bio->devs[i].bio
> */
> }
>
> This way it waits until all rdev devices are not blocked. There is a
> possibility that some rdev devices change
> to blocked again after checking. But the way raid10_write_request uses
> now has this risk too. If you think
> the method mentioned above is OK, I'll try to use this way.
I think it is possible to make the code work this way. Let's try refactor
raid10_write_request() like this. If it doesn't work for some reason, we
can revisit this part.
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V3 2/4] md/raid10: extend r10bio devs to raid disks
2020-08-13 8:14 ` [PATCH V3 2/4] md/raid10: extend r10bio devs to raid disks Xiao Ni
@ 2020-08-22 9:28 ` Xiao Ni
0 siblings, 0 replies; 13+ messages in thread
From: Xiao Ni @ 2020-08-22 9:28 UTC (permalink / raw
To: linux-raid, song; +Cc: colyli, guoqing.jiang, heinzm, ncroxon
On 08/13/2020 04:14 PM, Xiao Ni wrote:
> Now it allocs r10bio->devs[conf->copies]. Discard bio needs to submit
> to all member disks and it needs to use r10bio. So extend to
> r10bio->devs[geo.raid_disks].
>
> Reviewed-by: Coly Li <colyli@suse.de>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
> drivers/md/raid10.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index cefda2a..cef3cb8 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -91,7 +91,7 @@ static inline struct r10bio *get_resync_r10bio(struct bio *bio)
> static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
> {
> struct r10conf *conf = data;
> - int size = offsetof(struct r10bio, devs[conf->copies]);
> + int size = offsetof(struct r10bio, devs[conf->geo.raid_disks]);
>
> /* allocate a r10bio with room for raid_disks entries in the
> * bios array */
> @@ -238,7 +238,7 @@ static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
> {
> int i;
>
> - for (i = 0; i < conf->copies; i++) {
> + for (i = 0; i < conf->geo.raid_disks; i++) {
> struct bio **bio = & r10_bio->devs[i].bio;
> if (!BIO_SPECIAL(*bio))
> bio_put(*bio);
> @@ -327,7 +327,7 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
> int slot;
> int repl = 0;
>
> - for (slot = 0; slot < conf->copies; slot++) {
> + for (slot = 0; slot < conf->geo.raid_disks; slot++) {
> if (r10_bio->devs[slot].bio == bio)
> break;
> if (r10_bio->devs[slot].repl_bio == bio) {
> @@ -336,7 +336,7 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
> }
> }
>
> - BUG_ON(slot == conf->copies);
> + BUG_ON(slot == conf->geo.raid_disks);
Hi Song
I'm trying to fix the warning messages for these patches. What's your
suggestion for this kind of things?
Is it ok to remove BUG_ON from the code? Or change BUG_ON to WARN_ON?
Regards
Xiao
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-08-22 9:28 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-13 8:14 [PATCH V3 0/4] md/raid10: Improve handling raid10 discard request Xiao Ni
2020-08-13 8:14 ` [PATCH V3 1/4] md/raid10: Move codes related with submitting discard bio into one function Xiao Ni
2020-08-13 8:14 ` [PATCH V3 2/4] md/raid10: extend r10bio devs to raid disks Xiao Ni
2020-08-22 9:28 ` Xiao Ni
2020-08-13 8:14 ` [PATCH V3 3/4] md/raid10: improve raid10 discard request Xiao Ni
2020-08-19 18:36 ` Song Liu
2020-08-20 7:26 ` Xiao Ni
2020-08-20 9:06 ` Xiao Ni
2020-08-20 22:45 ` Song Liu
[not found] ` <21012936-9397-40f9-115b-87dda93d9017@redhat.com>
2020-08-20 22:13 ` Song Liu
2020-08-20 22:39 ` Xiao Ni
2020-08-13 8:14 ` [PATCH V3 4/4] md/raid10: Improve discard request for far layout Xiao Ni
2020-08-19 18:38 ` 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.