* [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* 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 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
* [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* 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
* [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* 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
* [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* 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
* [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* 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 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
* [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 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 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 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