| Message ID | 20240919092302.3094725-2-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | bio_split() error handling rework | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_12-PR | fail | merge-conflict |
On 19.09.24 11:25, John Garry wrote: > - BUG_ON(sectors <= 0); > - BUG_ON(sectors >= bio_sectors(bio)); > + if (WARN_ON(sectors <= 0)) > + return ERR_PTR(-EINVAL); > + if (WARN_ON(sectors >= bio_sectors(bio))) > + return ERR_PTR(-EINVAL); Nit: WARN_ON_ONCE() otherwise it'll trigger endless amounts of stacktraces in dmesg.
This looks reasonable to me modulo the WARN_ON_ONCE comment from Johannes.
On 19/09/2024 16:50, Johannes Thumshirn wrote: > On 19.09.24 11:25, John Garry wrote: >> - BUG_ON(sectors <= 0); >> - BUG_ON(sectors >= bio_sectors(bio)); >> + if (WARN_ON(sectors <= 0)) >> + return ERR_PTR(-EINVAL); >> + if (WARN_ON(sectors >= bio_sectors(bio))) >> + return ERR_PTR(-EINVAL); > > Nit: WARN_ON_ONCE() otherwise it'll trigger endless amounts of > stacktraces in dmesg. Considering it is a BUG_ON() today, I don't expect this to be hit. And, even if it was, prob it would be some buggy corner case which occasionally occurs. Anyway, I don't feel too strongly about this and I suppose a WARN_ON_ONCE() is ok. Thanks, John
diff --git a/block/bio.c b/block/bio.c index ac4d77c88932..784ad8d35bd0 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1728,16 +1728,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(sectors <= 0)) + return ERR_PTR(-EINVAL); + if (WARN_ON(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; }
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() instead. Indeed, since almost all callers don't check the return code from bio_split(), we'll crash anyway (for those failures). 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(-)