* [PATCH v3 0/6] bio_split() error handling rework
@ 2024-10-31 9:59 John Garry
2024-10-31 9:59 ` [PATCH v3 1/6] block: Rework bio_split() return value John Garry
` (7 more replies)
0 siblings, 8 replies; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry
bio_split() error handling could be improved as follows:
- Instead of returning NULL for an error - which is vague - return a
PTR_ERR, which may hint what went wrong.
- Remove BUG_ON() calls - which are generally not preferred - and instead
WARN and pass an error code back to the caller. Many callers of
bio_split() don't check the return code. As such, for an error we would
be getting a crash still from an invalid pointer dereference.
Most bio_split() callers don't check the return value. However, it could
be argued the bio_split() calls should not fail. So far I have just
fixed up the md RAID code to handle these errors, as that is my interest
now.
The motivator for this series was initial md RAID atomic write support in
https://lore.kernel.org/linux-block/20241030094912.3960234-1-john.g.garry@oracle.com/T/#m5859ee900de8e6554d5bb027c0558f0147c32df8
There I wanted to ensure that we don't split an atomic write bio, and it
made more sense to handle this in bio_split() (instead of the bio_split()
caller).
Based on 133008e84b99 (block/for-6.13/block) blk-integrity: remove
seed for user mapped buffers
Changes since v2:
- Drop "block: Use BLK_STS_OK in bio_init()" change (Christoph)
- Use proper rdev indexing in raid10_write_request() (Kuai)
- Decrement rdev nr_pending in raid1 read error path (Kuai)
- Add RB tags from Christoph, Johannes, and Kuai (thanks!)
Changes since RFC:
- proper handling to end the raid bio in all cases, and also pass back
proper error code (Kuai)
- Add WARN_ON_ERROR in bio_split() (Johannes, Christoph)
- Add small patch to use BLK_STS_OK in bio_init()
- Change bio_submit_split() error path (Christoph)
John Garry (6):
block: Rework bio_split() return value
block: Error an attempt to split an atomic write in bio_split()
block: Handle bio_split() errors in bio_submit_split()
md/raid0: Handle bio_split() errors
md/raid1: Handle bio_split() errors
md/raid10: Handle bio_split() errors
block/bio.c | 14 +++++++----
block/blk-crypto-fallback.c | 2 +-
block/blk-merge.c | 15 ++++++++----
drivers/md/raid0.c | 12 ++++++++++
drivers/md/raid1.c | 33 ++++++++++++++++++++++++--
drivers/md/raid10.c | 47 ++++++++++++++++++++++++++++++++++++-
6 files changed, 110 insertions(+), 13 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/6] block: Rework bio_split() return value
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
@ 2024-10-31 9:59 ` John Garry
2024-11-05 7:21 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 2/6] block: Error an attempt to split an atomic write in bio_split() John Garry
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry, Johannes Thumshirn
Instead of returning an inconclusive value of NULL for an error in calling
bio_split(), return a ERR_PTR() always.
Also remove the BUG_ON() calls, and WARN_ON_ONCE() instead. Indeed, since
almost all callers don't check the return code from bio_split(), we'll
crash anyway (for those failures).
Fix up the only user which checks bio_split() return code today (directly
or indirectly), blk_crypto_fallback_split_bio_if_needed(). The md/bcache
code does check the return code in cached_dev_cache_miss() ->
bio_next_split() -> bio_split(), but only to see if there was a split, so
there would be no change in behaviour here (when returning a ERR_PTR()).
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
block/bio.c | 10 ++++++----
block/blk-crypto-fallback.c | 2 +-
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 95e2ee14cea2..7a93724e4a49 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1740,16 +1740,18 @@ struct bio *bio_split(struct bio *bio, int sectors,
{
struct bio *split;
- BUG_ON(sectors <= 0);
- BUG_ON(sectors >= bio_sectors(bio));
+ if (WARN_ON_ONCE(sectors <= 0))
+ return ERR_PTR(-EINVAL);
+ if (WARN_ON_ONCE(sectors >= bio_sectors(bio)))
+ return ERR_PTR(-EINVAL);
/* Zone append commands cannot be split */
if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
- return NULL;
+ return ERR_PTR(-EINVAL);
split = bio_alloc_clone(bio->bi_bdev, bio, gfp, bs);
if (!split)
- return NULL;
+ return ERR_PTR(-ENOMEM);
split->bi_iter.bi_size = sectors << 9;
diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index b1e7415f8439..29a205482617 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -226,7 +226,7 @@ static bool blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
split_bio = bio_split(bio, num_sectors, GFP_NOIO,
&crypto_bio_split);
- if (!split_bio) {
+ if (IS_ERR(split_bio)) {
bio->bi_status = BLK_STS_RESOURCE;
return false;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/6] block: Error an attempt to split an atomic write in bio_split()
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
2024-10-31 9:59 ` [PATCH v3 1/6] block: Rework bio_split() return value John Garry
@ 2024-10-31 9:59 ` John Garry
2024-11-05 7:22 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 3/6] block: Handle bio_split() errors in bio_submit_split() John Garry
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry, Johannes Thumshirn
This is disallowed.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
block/bio.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/block/bio.c b/block/bio.c
index 7a93724e4a49..07b971853768 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1749,6 +1749,10 @@ struct bio *bio_split(struct bio *bio, int sectors,
if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
return ERR_PTR(-EINVAL);
+ /* atomic writes cannot be split */
+ if (bio->bi_opf & REQ_ATOMIC)
+ return ERR_PTR(-EINVAL);
+
split = bio_alloc_clone(bio->bi_bdev, bio, gfp, bs);
if (!split)
return ERR_PTR(-ENOMEM);
--
2.31.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 3/6] block: Handle bio_split() errors in bio_submit_split()
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
2024-10-31 9:59 ` [PATCH v3 1/6] block: Rework bio_split() return value John Garry
2024-10-31 9:59 ` [PATCH v3 2/6] block: Error an attempt to split an atomic write in bio_split() John Garry
@ 2024-10-31 9:59 ` John Garry
2024-11-05 7:23 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 4/6] md/raid0: Handle bio_split() errors John Garry
` (4 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry, Johannes Thumshirn
bio_split() may error, so check this.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
block/blk-merge.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index a16abf7b65dc..4d7b6bc8814c 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -107,11 +107,8 @@ static unsigned int bio_allowed_max_sectors(const struct queue_limits *lim)
static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
{
- if (unlikely(split_sectors < 0)) {
- bio->bi_status = errno_to_blk_status(split_sectors);
- bio_endio(bio);
- return NULL;
- }
+ if (unlikely(split_sectors < 0))
+ goto error;
if (unlikely((bio_op(bio) == REQ_OP_DISCARD)))
@@ -123,6 +120,10 @@ static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
split = bio_split(bio, split_sectors, GFP_NOIO,
&bio->bi_bdev->bd_disk->bio_split);
+ if (IS_ERR(split)) {
+ split_sectors = PTR_ERR(split);
+ goto error;
+ }
split->bi_opf |= REQ_NOMERGE;
blkcg_bio_issue_init(split);
bio_chain(split, bio);
@@ -133,6 +134,10 @@ static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
}
return bio;
+error:
+ bio->bi_status = errno_to_blk_status(split_sectors);
+ bio_endio(bio);
+ return NULL;
}
struct bio *bio_split_discard(struct bio *bio, const struct queue_limits *lim,
--
2.31.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 4/6] md/raid0: Handle bio_split() errors
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
` (2 preceding siblings ...)
2024-10-31 9:59 ` [PATCH v3 3/6] block: Handle bio_split() errors in bio_submit_split() John Garry
@ 2024-10-31 9:59 ` John Garry
2024-11-05 7:24 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 5/6] md/raid1: " John Garry
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry
Add proper bio_split() error handling. For any error, set bi_status, end
the bio, and return.
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
drivers/md/raid0.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index 32d587524778..baaf5f8b80ae 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -466,6 +466,12 @@ static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
struct bio *split = bio_split(bio,
zone->zone_end - bio->bi_iter.bi_sector, GFP_NOIO,
&mddev->bio_set);
+
+ if (IS_ERR(split)) {
+ bio->bi_status = errno_to_blk_status(PTR_ERR(split));
+ bio_endio(bio);
+ return;
+ }
bio_chain(split, bio);
submit_bio_noacct(bio);
bio = split;
@@ -608,6 +614,12 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
if (sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, sectors, GFP_NOIO,
&mddev->bio_set);
+
+ if (IS_ERR(split)) {
+ bio->bi_status = errno_to_blk_status(PTR_ERR(split));
+ bio_endio(bio);
+ return true;
+ }
bio_chain(split, bio);
raid0_map_submit_bio(mddev, bio);
bio = split;
--
2.31.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 5/6] md/raid1: Handle bio_split() errors
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
` (3 preceding siblings ...)
2024-10-31 9:59 ` [PATCH v3 4/6] md/raid0: Handle bio_split() errors John Garry
@ 2024-10-31 9:59 ` John Garry
2024-10-31 11:07 ` Yu Kuai
2024-11-05 7:26 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 6/6] md/raid10: " John Garry
` (2 subsequent siblings)
7 siblings, 2 replies; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry
Add proper bio_split() error handling. For any error, call
raid_end_bio_io() and return.
For the case of an in the write path, we need to undo the increment in
the rdev pending count and NULLify the r1_bio->bios[] pointers.
For read path failure, we need to undo rdev pending count increment from
the earlier read_balance() call.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
drivers/md/raid1.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 6c9d24203f39..7e023e9303c8 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1322,7 +1322,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
const enum req_op op = bio_op(bio);
const blk_opf_t do_sync = bio->bi_opf & REQ_SYNC;
int max_sectors;
- int rdisk;
+ int rdisk, error;
bool r1bio_existed = !!r1_bio;
/*
@@ -1383,6 +1383,11 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
gfp, &conf->bio_split);
+
+ if (IS_ERR(split)) {
+ error = PTR_ERR(split);
+ goto err_handle;
+ }
bio_chain(split, bio);
submit_bio_noacct(bio);
bio = split;
@@ -1410,6 +1415,13 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
read_bio->bi_private = r1_bio;
mddev_trace_remap(mddev, read_bio, r1_bio->sector);
submit_bio_noacct(read_bio);
+ return;
+
+err_handle:
+ atomic_dec(&mirror->rdev->nr_pending);
+ bio->bi_status = errno_to_blk_status(error);
+ set_bit(R1BIO_Uptodate, &r1_bio->state);
+ raid_end_bio_io(r1_bio);
}
static void raid1_write_request(struct mddev *mddev, struct bio *bio,
@@ -1417,7 +1429,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
{
struct r1conf *conf = mddev->private;
struct r1bio *r1_bio;
- int i, disks;
+ int i, disks, k, error;
unsigned long flags;
struct md_rdev *blocked_rdev;
int first_clone;
@@ -1576,6 +1588,11 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
GFP_NOIO, &conf->bio_split);
+
+ if (IS_ERR(split)) {
+ error = PTR_ERR(split);
+ goto err_handle;
+ }
bio_chain(split, bio);
submit_bio_noacct(bio);
bio = split;
@@ -1660,6 +1677,18 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
/* In case raid1d snuck in to freeze_array */
wake_up_barrier(conf);
+ return;
+err_handle:
+ for (k = 0; k < i; k++) {
+ if (r1_bio->bios[k]) {
+ rdev_dec_pending(conf->mirrors[k].rdev, mddev);
+ r1_bio->bios[k] = NULL;
+ }
+ }
+
+ bio->bi_status = errno_to_blk_status(error);
+ set_bit(R1BIO_Uptodate, &r1_bio->state);
+ raid_end_bio_io(r1_bio);
}
static bool raid1_make_request(struct mddev *mddev, struct bio *bio)
--
2.31.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 6/6] md/raid10: Handle bio_split() errors
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
` (4 preceding siblings ...)
2024-10-31 9:59 ` [PATCH v3 5/6] md/raid1: " John Garry
@ 2024-10-31 9:59 ` John Garry
2024-10-31 11:11 ` Yu Kuai
2024-11-05 7:27 ` Hannes Reinecke
2024-11-07 6:49 ` [PATCH v3 0/6] bio_split() error handling rework John Garry
2024-11-07 6:49 ` John Garry
7 siblings, 2 replies; 20+ messages in thread
From: John Garry @ 2024-10-31 9:59 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, John Garry
Add proper bio_split() error handling. For any error, call
raid_end_bio_io() and return. Except for discard, where we end the bio
directly.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
drivers/md/raid10.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index f3bf1116794a..ccd95459b192 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1159,6 +1159,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
int slot = r10_bio->read_slot;
struct md_rdev *err_rdev = NULL;
gfp_t gfp = GFP_NOIO;
+ int error;
if (slot >= 0 && r10_bio->devs[slot].rdev) {
/*
@@ -1206,6 +1207,10 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
gfp, &conf->bio_split);
+ if (IS_ERR(split)) {
+ error = PTR_ERR(split);
+ goto err_handle;
+ }
bio_chain(split, bio);
allow_barrier(conf);
submit_bio_noacct(bio);
@@ -1236,6 +1241,11 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
mddev_trace_remap(mddev, read_bio, r10_bio->sector);
submit_bio_noacct(read_bio);
return;
+err_handle:
+ atomic_dec(&rdev->nr_pending);
+ bio->bi_status = errno_to_blk_status(error);
+ set_bit(R10BIO_Uptodate, &r10_bio->state);
+ raid_end_bio_io(r10_bio);
}
static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
@@ -1347,9 +1357,10 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
struct r10bio *r10_bio)
{
struct r10conf *conf = mddev->private;
- int i;
+ int i, k;
sector_t sectors;
int max_sectors;
+ int error;
if ((mddev_is_clustered(mddev) &&
md_cluster_ops->area_resyncing(mddev, WRITE,
@@ -1482,6 +1493,10 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
if (r10_bio->sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, r10_bio->sectors,
GFP_NOIO, &conf->bio_split);
+ if (IS_ERR(split)) {
+ error = PTR_ERR(split);
+ goto err_handle;
+ }
bio_chain(split, bio);
allow_barrier(conf);
submit_bio_noacct(bio);
@@ -1503,6 +1518,26 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
raid10_write_one_disk(mddev, r10_bio, bio, true, i);
}
one_write_done(r10_bio);
+ return;
+err_handle:
+ for (k = 0; k < i; k++) {
+ int d = r10_bio->devs[k].devnum;
+ struct md_rdev *rdev = conf->mirrors[d].rdev;
+ struct md_rdev *rrdev = conf->mirrors[d].replacement;
+
+ if (r10_bio->devs[k].bio) {
+ rdev_dec_pending(rdev, mddev);
+ r10_bio->devs[k].bio = NULL;
+ }
+ if (r10_bio->devs[k].repl_bio) {
+ rdev_dec_pending(rrdev, mddev);
+ r10_bio->devs[k].repl_bio = NULL;
+ }
+ }
+
+ bio->bi_status = errno_to_blk_status(error);
+ set_bit(R10BIO_Uptodate, &r10_bio->state);
+ raid_end_bio_io(r10_bio);
}
static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
@@ -1644,6 +1679,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
if (remainder) {
split_size = stripe_size - remainder;
split = bio_split(bio, split_size, GFP_NOIO, &conf->bio_split);
+ if (IS_ERR(split)) {
+ bio->bi_status = errno_to_blk_status(PTR_ERR(split));
+ bio_endio(bio);
+ return 0;
+ }
bio_chain(split, bio);
allow_barrier(conf);
/* Resend the fist split part */
@@ -1654,6 +1694,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
if (remainder) {
split_size = bio_sectors(bio) - remainder;
split = bio_split(bio, split_size, GFP_NOIO, &conf->bio_split);
+ if (IS_ERR(split)) {
+ bio->bi_status = errno_to_blk_status(PTR_ERR(split));
+ bio_endio(bio);
+ return 0;
+ }
bio_chain(split, bio);
allow_barrier(conf);
/* Resend the second split part */
--
2.31.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 5/6] md/raid1: Handle bio_split() errors
2024-10-31 9:59 ` [PATCH v3 5/6] md/raid1: " John Garry
@ 2024-10-31 11:07 ` Yu Kuai
2024-11-05 7:26 ` Hannes Reinecke
1 sibling, 0 replies; 20+ messages in thread
From: Yu Kuai @ 2024-10-31 11:07 UTC (permalink / raw)
To: John Garry, axboe, song, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, yukuai (C)
在 2024/10/31 17:59, John Garry 写道:
> Add proper bio_split() error handling. For any error, call
> raid_end_bio_io() and return.
>
> For the case of an in the write path, we need to undo the increment in
> the rdev pending count and NULLify the r1_bio->bios[] pointers.
>
> For read path failure, we need to undo rdev pending count increment from
> the earlier read_balance() call.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> drivers/md/raid1.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 6c9d24203f39..7e023e9303c8 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1322,7 +1322,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> const enum req_op op = bio_op(bio);
> const blk_opf_t do_sync = bio->bi_opf & REQ_SYNC;
> int max_sectors;
> - int rdisk;
> + int rdisk, error;
> bool r1bio_existed = !!r1_bio;
>
> /*
> @@ -1383,6 +1383,11 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> if (max_sectors < bio_sectors(bio)) {
> struct bio *split = bio_split(bio, max_sectors,
> gfp, &conf->bio_split);
> +
> + if (IS_ERR(split)) {
> + error = PTR_ERR(split);
> + goto err_handle;
> + }
> bio_chain(split, bio);
> submit_bio_noacct(bio);
> bio = split;
> @@ -1410,6 +1415,13 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> read_bio->bi_private = r1_bio;
> mddev_trace_remap(mddev, read_bio, r1_bio->sector);
> submit_bio_noacct(read_bio);
> + return;
> +
> +err_handle:
> + atomic_dec(&mirror->rdev->nr_pending);
> + bio->bi_status = errno_to_blk_status(error);
> + set_bit(R1BIO_Uptodate, &r1_bio->state);
> + raid_end_bio_io(r1_bio);
> }
>
> static void raid1_write_request(struct mddev *mddev, struct bio *bio,
> @@ -1417,7 +1429,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
> {
> struct r1conf *conf = mddev->private;
> struct r1bio *r1_bio;
> - int i, disks;
> + int i, disks, k, error;
> unsigned long flags;
> struct md_rdev *blocked_rdev;
> int first_clone;
> @@ -1576,6 +1588,11 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
> if (max_sectors < bio_sectors(bio)) {
> struct bio *split = bio_split(bio, max_sectors,
> GFP_NOIO, &conf->bio_split);
> +
> + if (IS_ERR(split)) {
> + error = PTR_ERR(split);
> + goto err_handle;
> + }
> bio_chain(split, bio);
> submit_bio_noacct(bio);
> bio = split;
> @@ -1660,6 +1677,18 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>
> /* In case raid1d snuck in to freeze_array */
> wake_up_barrier(conf);
> + return;
> +err_handle:
> + for (k = 0; k < i; k++) {
> + if (r1_bio->bios[k]) {
> + rdev_dec_pending(conf->mirrors[k].rdev, mddev);
> + r1_bio->bios[k] = NULL;
> + }
> + }
> +
> + bio->bi_status = errno_to_blk_status(error);
> + set_bit(R1BIO_Uptodate, &r1_bio->state);
> + raid_end_bio_io(r1_bio);
> }
>
> static bool raid1_make_request(struct mddev *mddev, struct bio *bio)
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 6/6] md/raid10: Handle bio_split() errors
2024-10-31 9:59 ` [PATCH v3 6/6] md/raid10: " John Garry
@ 2024-10-31 11:11 ` Yu Kuai
2024-11-05 7:27 ` Hannes Reinecke
1 sibling, 0 replies; 20+ messages in thread
From: Yu Kuai @ 2024-10-31 11:11 UTC (permalink / raw)
To: John Garry, axboe, song, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn, yukuai (C)
在 2024/10/31 17:59, John Garry 写道:
> Add proper bio_split() error handling. For any error, call
> raid_end_bio_io() and return. Except for discard, where we end the bio
> directly.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> drivers/md/raid10.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 46 insertions(+), 1 deletion(-)
>
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index f3bf1116794a..ccd95459b192 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1159,6 +1159,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
> int slot = r10_bio->read_slot;
> struct md_rdev *err_rdev = NULL;
> gfp_t gfp = GFP_NOIO;
> + int error;
>
> if (slot >= 0 && r10_bio->devs[slot].rdev) {
> /*
> @@ -1206,6 +1207,10 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
> if (max_sectors < bio_sectors(bio)) {
> struct bio *split = bio_split(bio, max_sectors,
> gfp, &conf->bio_split);
> + if (IS_ERR(split)) {
> + error = PTR_ERR(split);
> + goto err_handle;
> + }
> bio_chain(split, bio);
> allow_barrier(conf);
> submit_bio_noacct(bio);
> @@ -1236,6 +1241,11 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
> mddev_trace_remap(mddev, read_bio, r10_bio->sector);
> submit_bio_noacct(read_bio);
> return;
> +err_handle:
> + atomic_dec(&rdev->nr_pending);
> + bio->bi_status = errno_to_blk_status(error);
> + set_bit(R10BIO_Uptodate, &r10_bio->state);
> + raid_end_bio_io(r10_bio);
> }
>
> static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
> @@ -1347,9 +1357,10 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
> struct r10bio *r10_bio)
> {
> struct r10conf *conf = mddev->private;
> - int i;
> + int i, k;
> sector_t sectors;
> int max_sectors;
> + int error;
>
> if ((mddev_is_clustered(mddev) &&
> md_cluster_ops->area_resyncing(mddev, WRITE,
> @@ -1482,6 +1493,10 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
> if (r10_bio->sectors < bio_sectors(bio)) {
> struct bio *split = bio_split(bio, r10_bio->sectors,
> GFP_NOIO, &conf->bio_split);
> + if (IS_ERR(split)) {
> + error = PTR_ERR(split);
> + goto err_handle;
> + }
> bio_chain(split, bio);
> allow_barrier(conf);
> submit_bio_noacct(bio);
> @@ -1503,6 +1518,26 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
> raid10_write_one_disk(mddev, r10_bio, bio, true, i);
> }
> one_write_done(r10_bio);
> + return;
> +err_handle:
> + for (k = 0; k < i; k++) {
> + int d = r10_bio->devs[k].devnum;
> + struct md_rdev *rdev = conf->mirrors[d].rdev;
> + struct md_rdev *rrdev = conf->mirrors[d].replacement;
> +
> + if (r10_bio->devs[k].bio) {
> + rdev_dec_pending(rdev, mddev);
> + r10_bio->devs[k].bio = NULL;
> + }
> + if (r10_bio->devs[k].repl_bio) {
> + rdev_dec_pending(rrdev, mddev);
> + r10_bio->devs[k].repl_bio = NULL;
> + }
> + }
> +
> + bio->bi_status = errno_to_blk_status(error);
> + set_bit(R10BIO_Uptodate, &r10_bio->state);
> + raid_end_bio_io(r10_bio);
> }
>
> static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
> @@ -1644,6 +1679,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> if (remainder) {
> split_size = stripe_size - remainder;
> split = bio_split(bio, split_size, GFP_NOIO, &conf->bio_split);
> + if (IS_ERR(split)) {
> + bio->bi_status = errno_to_blk_status(PTR_ERR(split));
> + bio_endio(bio);
> + return 0;
> + }
> bio_chain(split, bio);
> allow_barrier(conf);
> /* Resend the fist split part */
> @@ -1654,6 +1694,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> if (remainder) {
> split_size = bio_sectors(bio) - remainder;
> split = bio_split(bio, split_size, GFP_NOIO, &conf->bio_split);
> + if (IS_ERR(split)) {
> + bio->bi_status = errno_to_blk_status(PTR_ERR(split));
> + bio_endio(bio);
> + return 0;
> + }
> bio_chain(split, bio);
> allow_barrier(conf);
> /* Resend the second split part */
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/6] block: Rework bio_split() return value
2024-10-31 9:59 ` [PATCH v3 1/6] block: Rework bio_split() return value John Garry
@ 2024-11-05 7:21 ` Hannes Reinecke
2024-11-05 17:16 ` John Garry
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2024-11-05 7:21 UTC (permalink / raw)
To: John Garry, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 10/31/24 10:59, John Garry wrote:
> Instead of returning an inconclusive value of NULL for an error in calling
> bio_split(), return a ERR_PTR() always.
>
> Also remove the BUG_ON() calls, and WARN_ON_ONCE() instead. Indeed, since
> almost all callers don't check the return code from bio_split(), we'll
> crash anyway (for those failures).
>
> Fix up the only user which checks bio_split() return code today (directly
> or indirectly), blk_crypto_fallback_split_bio_if_needed(). The md/bcache
> code does check the return code in cached_dev_cache_miss() ->
> bio_next_split() -> bio_split(), but only to see if there was a split, so
> there would be no change in behaviour here (when returning a ERR_PTR()).
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> block/bio.c | 10 ++++++----
> block/blk-crypto-fallback.c | 2 +-
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 95e2ee14cea2..7a93724e4a49 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1740,16 +1740,18 @@ struct bio *bio_split(struct bio *bio, int sectors,
> {
> struct bio *split;
>
> - BUG_ON(sectors <= 0);
> - BUG_ON(sectors >= bio_sectors(bio));
> + if (WARN_ON_ONCE(sectors <= 0))
> + return ERR_PTR(-EINVAL);
> + if (WARN_ON_ONCE(sectors >= bio_sectors(bio)))
> + return ERR_PTR(-EINVAL);
>
> /* Zone append commands cannot be split */
> if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
> - return NULL;
> + return ERR_PTR(-EINVAL);
>
> split = bio_alloc_clone(bio->bi_bdev, bio, gfp, bs);
> if (!split)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> split->bi_iter.bi_size = sectors << 9;
>
> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index b1e7415f8439..29a205482617 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
> @@ -226,7 +226,7 @@ static bool blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
>
> split_bio = bio_split(bio, num_sectors, GFP_NOIO,
> &crypto_bio_split);
> - if (!split_bio) {
> + if (IS_ERR(split_bio)) {
> bio->bi_status = BLK_STS_RESOURCE;
> return false;
> }
Don't you need to modify block/bounce.c, too?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/6] block: Error an attempt to split an atomic write in bio_split()
2024-10-31 9:59 ` [PATCH v3 2/6] block: Error an attempt to split an atomic write in bio_split() John Garry
@ 2024-11-05 7:22 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2024-11-05 7:22 UTC (permalink / raw)
To: John Garry, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 10/31/24 10:59, John Garry wrote:
> This is disallowed.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> block/bio.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/block/bio.c b/block/bio.c
> index 7a93724e4a49..07b971853768 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1749,6 +1749,10 @@ struct bio *bio_split(struct bio *bio, int sectors,
> if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
> return ERR_PTR(-EINVAL);
>
> + /* atomic writes cannot be split */
> + if (bio->bi_opf & REQ_ATOMIC)
> + return ERR_PTR(-EINVAL);
> +
> split = bio_alloc_clone(bio->bi_bdev, bio, gfp, bs);
> if (!split)
> return ERR_PTR(-ENOMEM);
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/6] block: Handle bio_split() errors in bio_submit_split()
2024-10-31 9:59 ` [PATCH v3 3/6] block: Handle bio_split() errors in bio_submit_split() John Garry
@ 2024-11-05 7:23 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2024-11-05 7:23 UTC (permalink / raw)
To: John Garry, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 10/31/24 10:59, John Garry wrote:
> bio_split() may error, so check this.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> block/blk-merge.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index a16abf7b65dc..4d7b6bc8814c 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -107,11 +107,8 @@ static unsigned int bio_allowed_max_sectors(const struct queue_limits *lim)
>
> static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
> {
> - if (unlikely(split_sectors < 0)) {
> - bio->bi_status = errno_to_blk_status(split_sectors);
> - bio_endio(bio);
> - return NULL;
> - }
> + if (unlikely(split_sectors < 0))
> + goto error;
>
>
> if (unlikely((bio_op(bio) == REQ_OP_DISCARD)))
> @@ -123,6 +120,10 @@ static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
>
> split = bio_split(bio, split_sectors, GFP_NOIO,
> &bio->bi_bdev->bd_disk->bio_split);
> + if (IS_ERR(split)) {
> + split_sectors = PTR_ERR(split);
> + goto error;
> + }
> split->bi_opf |= REQ_NOMERGE;
> blkcg_bio_issue_init(split);
> bio_chain(split, bio);
> @@ -133,6 +134,10 @@ static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
> }
>
> return bio;
> +error:
> + bio->bi_status = errno_to_blk_status(split_sectors);
> + bio_endio(bio);
> + return NULL;
> }
>
> struct bio *bio_split_discard(struct bio *bio, const struct queue_limits *lim,
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 4/6] md/raid0: Handle bio_split() errors
2024-10-31 9:59 ` [PATCH v3 4/6] md/raid0: Handle bio_split() errors John Garry
@ 2024-11-05 7:24 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2024-11-05 7:24 UTC (permalink / raw)
To: John Garry, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 10/31/24 10:59, John Garry wrote:
> Add proper bio_split() error handling. For any error, set bi_status, end
> the bio, and return.
>
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> drivers/md/raid0.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 5/6] md/raid1: Handle bio_split() errors
2024-10-31 9:59 ` [PATCH v3 5/6] md/raid1: " John Garry
2024-10-31 11:07 ` Yu Kuai
@ 2024-11-05 7:26 ` Hannes Reinecke
1 sibling, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2024-11-05 7:26 UTC (permalink / raw)
To: John Garry, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 10/31/24 10:59, John Garry wrote:
> Add proper bio_split() error handling. For any error, call
> raid_end_bio_io() and return.
>
> For the case of an in the write path, we need to undo the increment in
> the rdev pending count and NULLify the r1_bio->bios[] pointers.
>
> For read path failure, we need to undo rdev pending count increment from
> the earlier read_balance() call.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> drivers/md/raid1.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 6/6] md/raid10: Handle bio_split() errors
2024-10-31 9:59 ` [PATCH v3 6/6] md/raid10: " John Garry
2024-10-31 11:11 ` Yu Kuai
@ 2024-11-05 7:27 ` Hannes Reinecke
1 sibling, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2024-11-05 7:27 UTC (permalink / raw)
To: John Garry, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 10/31/24 10:59, John Garry wrote:
> Add proper bio_split() error handling. For any error, call
> raid_end_bio_io() and return. Except for discard, where we end the bio
> directly.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> drivers/md/raid10.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 46 insertions(+), 1 deletion(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/6] block: Rework bio_split() return value
2024-11-05 7:21 ` Hannes Reinecke
@ 2024-11-05 17:16 ` John Garry
0 siblings, 0 replies; 20+ messages in thread
From: John Garry @ 2024-11-05 17:16 UTC (permalink / raw)
To: Hannes Reinecke, axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid,
Johannes.Thumshirn
On 05/11/2024 07:21, Hannes Reinecke wrote:
> On 10/31/24 10:59, John Garry wrote:
>> Instead of returning an inconclusive value of NULL for an error in
>> calling
>> bio_split(), return a ERR_PTR() always.
>>
>> Also remove the BUG_ON() calls, and WARN_ON_ONCE() instead. Indeed, since
>> almost all callers don't check the return code from bio_split(), we'll
>> crash anyway (for those failures).
>>
>> Fix up the only user which checks bio_split() return code today (directly
>> or indirectly), blk_crypto_fallback_split_bio_if_needed(). The md/bcache
>> code does check the return code in cached_dev_cache_miss() ->
>> bio_next_split() -> bio_split(), but only to see if there was a split, so
>> there would be no change in behaviour here (when returning a ERR_PTR()).
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>> block/bio.c | 10 ++++++----
>> block/blk-crypto-fallback.c | 2 +-
>> 2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 95e2ee14cea2..7a93724e4a49 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -1740,16 +1740,18 @@ struct bio *bio_split(struct bio *bio, int
>> sectors,
>> {
>> struct bio *split;
>> - BUG_ON(sectors <= 0);
>> - BUG_ON(sectors >= bio_sectors(bio));
>> + if (WARN_ON_ONCE(sectors <= 0))
>> + return ERR_PTR(-EINVAL);
>> + if (WARN_ON_ONCE(sectors >= bio_sectors(bio)))
>> + return ERR_PTR(-EINVAL);
>> /* Zone append commands cannot be split */
>> if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
>> - return NULL;
>> + return ERR_PTR(-EINVAL);
>> split = bio_alloc_clone(bio->bi_bdev, bio, gfp, bs);
>> if (!split)
>> - return NULL;
>> + return ERR_PTR(-ENOMEM);
>> split->bi_iter.bi_size = sectors << 9;
>> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
>> index b1e7415f8439..29a205482617 100644
>> --- a/block/blk-crypto-fallback.c
>> +++ b/block/blk-crypto-fallback.c
>> @@ -226,7 +226,7 @@ static bool
>> blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
>> split_bio = bio_split(bio, num_sectors, GFP_NOIO,
>> &crypto_bio_split);
>> - if (!split_bio) {
>> + if (IS_ERR(split_bio)) {
>> bio->bi_status = BLK_STS_RESOURCE;
>> return false;
>> }
>
> Don't you need to modify block/bounce.c, too?
Today we have __blk_queue_bounce() -> bio_split(), but the return value
from bio_split() is not checked for errors (NULL) there, so it is
already in a poor state.
I will look to remedy that and other callsites which don't check
bio_split() return value for errors in next phase.
Thanks,
John
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/6] bio_split() error handling rework
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
` (5 preceding siblings ...)
2024-10-31 9:59 ` [PATCH v3 6/6] md/raid10: " John Garry
@ 2024-11-07 6:49 ` John Garry
2024-11-07 6:49 ` John Garry
7 siblings, 0 replies; 20+ messages in thread
From: John Garry @ 2024-11-07 6:49 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn
On 31/10/2024 09:59, John Garry wrote:
Hi Jens,
Could you kindly consider picking up this series via the block tree?
Please note that the raid 0/1/10 atomic write support in
https://lore.kernel.org/linux-block/20241101144616.497602-1-john.g.garry@oracle.com/
depends on this series, so maybe you would also be willing to pick that
one up (when it's fully reviewed). Or create a branch with all the block
changes, like which was done for the atomic writes XFS support.
Thanks very much,
John
> bio_split() error handling could be improved as follows:
> - Instead of returning NULL for an error - which is vague - return a
> PTR_ERR, which may hint what went wrong.
> - Remove BUG_ON() calls - which are generally not preferred - and instead
> WARN and pass an error code back to the caller. Many callers of
> bio_split() don't check the return code. As such, for an error we would
> be getting a crash still from an invalid pointer dereference.
>
> Most bio_split() callers don't check the return value. However, it could
> be argued the bio_split() calls should not fail. So far I have just
> fixed up the md RAID code to handle these errors, as that is my interest
> now.
>
> The motivator for this series was initial md RAID atomic write support in
> https://lore.kernel.org/linux-block/20241030094912.3960234-1-john.g.garry@oracle.com/T/#m5859ee900de8e6554d5bb027c0558f0147c32df8
>
> There I wanted to ensure that we don't split an atomic write bio, and it
> made more sense to handle this in bio_split() (instead of the bio_split()
> caller).
>
> Based on 133008e84b99 (block/for-6.13/block) blk-integrity: remove
> seed for user mapped buffers
>
> Changes since v2:
> - Drop "block: Use BLK_STS_OK in bio_init()" change (Christoph)
> - Use proper rdev indexing in raid10_write_request() (Kuai)
> - Decrement rdev nr_pending in raid1 read error path (Kuai)
> - Add RB tags from Christoph, Johannes, and Kuai (thanks!)
>
> Changes since RFC:
> - proper handling to end the raid bio in all cases, and also pass back
> proper error code (Kuai)
> - Add WARN_ON_ERROR in bio_split() (Johannes, Christoph)
> - Add small patch to use BLK_STS_OK in bio_init()
> - Change bio_submit_split() error path (Christoph)
>
> John Garry (6):
> block: Rework bio_split() return value
> block: Error an attempt to split an atomic write in bio_split()
> block: Handle bio_split() errors in bio_submit_split()
> md/raid0: Handle bio_split() errors
> md/raid1: Handle bio_split() errors
> md/raid10: Handle bio_split() errors
>
> block/bio.c | 14 +++++++----
> block/blk-crypto-fallback.c | 2 +-
> block/blk-merge.c | 15 ++++++++----
> drivers/md/raid0.c | 12 ++++++++++
> drivers/md/raid1.c | 33 ++++++++++++++++++++++++--
> drivers/md/raid10.c | 47 ++++++++++++++++++++++++++++++++++++-
> 6 files changed, 110 insertions(+), 13 deletions(-)
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/6] bio_split() error handling rework
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
` (6 preceding siblings ...)
2024-11-07 6:49 ` [PATCH v3 0/6] bio_split() error handling rework John Garry
@ 2024-11-07 6:49 ` John Garry
2024-11-07 18:27 ` Jens Axboe
7 siblings, 1 reply; 20+ messages in thread
From: John Garry @ 2024-11-07 6:49 UTC (permalink / raw)
To: axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn
On 31/10/2024 09:59, John Garry wrote:
Hi Jens,
Could you kindly consider picking up this series via the block tree?
Please note that the raid 0/1/10 atomic write support in
https://lore.kernel.org/linux-block/20241101144616.497602-1-john.g.garry@oracle.com/
depends on this series, so maybe you would also be willing to pick that
one up (when it's fully reviewed). Or create a branch with all the block
changes, like which was done for the atomic writes XFS support.
Thanks very much,
John
> bio_split() error handling could be improved as follows:
> - Instead of returning NULL for an error - which is vague - return a
> PTR_ERR, which may hint what went wrong.
> - Remove BUG_ON() calls - which are generally not preferred - and instead
> WARN and pass an error code back to the caller. Many callers of
> bio_split() don't check the return code. As such, for an error we would
> be getting a crash still from an invalid pointer dereference.
>
> Most bio_split() callers don't check the return value. However, it could
> be argued the bio_split() calls should not fail. So far I have just
> fixed up the md RAID code to handle these errors, as that is my interest
> now.
>
> The motivator for this series was initial md RAID atomic write support in
> https://lore.kernel.org/linux-block/20241030094912.3960234-1-john.g.garry@oracle.com/T/#m5859ee900de8e6554d5bb027c0558f0147c32df8
>
> There I wanted to ensure that we don't split an atomic write bio, and it
> made more sense to handle this in bio_split() (instead of the bio_split()
> caller).
>
> Based on 133008e84b99 (block/for-6.13/block) blk-integrity: remove
> seed for user mapped buffers
>
> Changes since v2:
> - Drop "block: Use BLK_STS_OK in bio_init()" change (Christoph)
> - Use proper rdev indexing in raid10_write_request() (Kuai)
> - Decrement rdev nr_pending in raid1 read error path (Kuai)
> - Add RB tags from Christoph, Johannes, and Kuai (thanks!)
>
> Changes since RFC:
> - proper handling to end the raid bio in all cases, and also pass back
> proper error code (Kuai)
> - Add WARN_ON_ERROR in bio_split() (Johannes, Christoph)
> - Add small patch to use BLK_STS_OK in bio_init()
> - Change bio_submit_split() error path (Christoph)
>
> John Garry (6):
> block: Rework bio_split() return value
> block: Error an attempt to split an atomic write in bio_split()
> block: Handle bio_split() errors in bio_submit_split()
> md/raid0: Handle bio_split() errors
> md/raid1: Handle bio_split() errors
> md/raid10: Handle bio_split() errors
>
> block/bio.c | 14 +++++++----
> block/blk-crypto-fallback.c | 2 +-
> block/blk-merge.c | 15 ++++++++----
> drivers/md/raid0.c | 12 ++++++++++
> drivers/md/raid1.c | 33 ++++++++++++++++++++++++--
> drivers/md/raid10.c | 47 ++++++++++++++++++++++++++++++++++++-
> 6 files changed, 110 insertions(+), 13 deletions(-)
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/6] bio_split() error handling rework
2024-11-07 6:49 ` John Garry
@ 2024-11-07 18:27 ` Jens Axboe
2024-11-07 19:54 ` John Garry
0 siblings, 1 reply; 20+ messages in thread
From: Jens Axboe @ 2024-11-07 18:27 UTC (permalink / raw)
To: John Garry, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn
On 11/6/24 11:49 PM, John Garry wrote:
> On 31/10/2024 09:59, John Garry wrote:
>
> Hi Jens,
>
> Could you kindly consider picking up this series via the block tree?
>
> Please note that the raid 0/1/10 atomic write support in https://lore.kernel.org/linux-block/20241101144616.497602-1-john.g.garry@oracle.com/ depends on this series, so maybe you would also be willing to pick that one up (when it's fully reviewed). Or create a branch with all the block changes, like which was done for the atomic writes XFS support.
The series doesn't apply to for-6.13/block - the 3rd patch for bio
splitting conflicts with:
commit b35243a447b9fe6457fa8e1352152b818436ba5a
Author: Christoph Hellwig <hch@lst.de>
Date: Mon Aug 26 19:37:54 2024 +0200
block: rework bio splitting
which was in long before for-6.13/block, which it's supposed to be based
on?
Please double check and resend a v4.
--
Jens Axboe
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/6] bio_split() error handling rework
2024-11-07 18:27 ` Jens Axboe
@ 2024-11-07 19:54 ` John Garry
0 siblings, 0 replies; 20+ messages in thread
From: John Garry @ 2024-11-07 19:54 UTC (permalink / raw)
To: Jens Axboe, song, yukuai3, hch
Cc: martin.petersen, linux-block, linux-kernel, linux-raid, hare,
Johannes.Thumshirn
On 07/11/2024 18:27, Jens Axboe wrote:
> The series doesn't apply to for-6.13/block - the 3rd patch for bio
> splitting conflicts with:
>
> commit b35243a447b9fe6457fa8e1352152b818436ba5a
> Author: Christoph Hellwig<hch@lst.de>
> Date: Mon Aug 26 19:37:54 2024 +0200
>
> block: rework bio splitting
>
> which was in long before for-6.13/block, which it's supposed to be based
> on?
>
> Please double check and resend a v4.
Hi Jens,
I was based on a recent commit, 133008e84b99 (block/for-6.13/block)
blk-integrity: remove seed for user mapped buffers
Anyway, it is a week old, so I will rebase and repost.
Thanks,
John
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-11-07 19:54 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-31 9:59 [PATCH v3 0/6] bio_split() error handling rework John Garry
2024-10-31 9:59 ` [PATCH v3 1/6] block: Rework bio_split() return value John Garry
2024-11-05 7:21 ` Hannes Reinecke
2024-11-05 17:16 ` John Garry
2024-10-31 9:59 ` [PATCH v3 2/6] block: Error an attempt to split an atomic write in bio_split() John Garry
2024-11-05 7:22 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 3/6] block: Handle bio_split() errors in bio_submit_split() John Garry
2024-11-05 7:23 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 4/6] md/raid0: Handle bio_split() errors John Garry
2024-11-05 7:24 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 5/6] md/raid1: " John Garry
2024-10-31 11:07 ` Yu Kuai
2024-11-05 7:26 ` Hannes Reinecke
2024-10-31 9:59 ` [PATCH v3 6/6] md/raid10: " John Garry
2024-10-31 11:11 ` Yu Kuai
2024-11-05 7:27 ` Hannes Reinecke
2024-11-07 6:49 ` [PATCH v3 0/6] bio_split() error handling rework John Garry
2024-11-07 6:49 ` John Garry
2024-11-07 18:27 ` Jens Axboe
2024-11-07 19:54 ` John Garry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox