| Message ID | 20240919092302.3094725-1-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bio_split() error handling rework | expand |
On 9/19/24 11:22, John Garry wrote: > 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. > > Sending as an RFC as unsure if this is the right direction. > > The motivator for this series was initial md RAID atomic write support in > https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2-a93b-0a433741fd26@oracle.com/ > > 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). > > 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 | 5 +++++ > drivers/md/raid0.c | 10 ++++++++++ > drivers/md/raid1.c | 8 ++++++++ > drivers/md/raid10.c | 18 ++++++++++++++++++ > 6 files changed, 52 insertions(+), 5 deletions(-) > You are missing '__bio_split_to_limits()' which looks as it would need to be modified, too. Cheers, Hannes
On 23/09/2024 06:53, Hannes Reinecke wrote: > On 9/19/24 11:22, John Garry wrote: >> 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. >> >> Sending as an RFC as unsure if this is the right direction. >> >> The motivator for this series was initial md RAID atomic write support in >> https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2- >> a93b-0a433741fd26@oracle.com/ >> >> 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). >> >> 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 | 5 +++++ >> drivers/md/raid0.c | 10 ++++++++++ >> drivers/md/raid1.c | 8 ++++++++ >> drivers/md/raid10.c | 18 ++++++++++++++++++ >> 6 files changed, 52 insertions(+), 5 deletions(-) >> > You are missing '__bio_split_to_limits()' which looks as it would need > to be modified, too. > In __bio_split_to_limits(), for REQ_OP_DISCARD, REQ_OP_SECURE_ERASE, and REQ_OP_WRITE_ZEROES, we indirectly call bio_split(). And bio_split() might error. But functions like bio_split_discard() can return NULL for cases where a split is not required. So I suppose we need to check IS_ERR(split) for those request types mentioned. For NULL being returned, we would still have the __bio_split_to_limits() is "if (split)" check. Thanks, John
On 9/23/24 09:19, John Garry wrote: > On 23/09/2024 06:53, Hannes Reinecke wrote: >> On 9/19/24 11:22, John Garry wrote: >>> 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. >>> >>> Sending as an RFC as unsure if this is the right direction. >>> >>> The motivator for this series was initial md RAID atomic write >>> support in >>> https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2- >>> a93b-0a433741fd26@oracle.com/ >>> >>> 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). >>> >>> 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 | 5 +++++ >>> drivers/md/raid0.c | 10 ++++++++++ >>> drivers/md/raid1.c | 8 ++++++++ >>> drivers/md/raid10.c | 18 ++++++++++++++++++ >>> 6 files changed, 52 insertions(+), 5 deletions(-) >>> >> You are missing '__bio_split_to_limits()' which looks as it would need >> to be modified, too. >> > > In __bio_split_to_limits(), for REQ_OP_DISCARD, REQ_OP_SECURE_ERASE, and > REQ_OP_WRITE_ZEROES, we indirectly call bio_split(). And bio_split() > might error. But functions like bio_split_discard() can return NULL for > cases where a split is not required. So I suppose we need to check > IS_ERR(split) for those request types mentioned. For NULL being > returned, we would still have the __bio_split_to_limits() is "if > (split)" check. > Indeed. And then you'll need to modify nvme: diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index f72c5a6a2d8e..c99f51e7730e 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -453,7 +453,7 @@ static void nvme_ns_head_submit_bio(struct bio *bio) * pool from the original queue to allocate the bvecs from. */ bio = bio_split_to_limits(bio); - if (!bio) + if (IS_ERR_OR_NULL(bio)) return; srcu_idx = srcu_read_lock(&head->srcu); Cheers, Hannes
On 23/09/2024 10:43, Hannes Reinecke wrote: > On 9/23/24 09:19, John Garry wrote: >> On 23/09/2024 06:53, Hannes Reinecke wrote: >>> On 9/19/24 11:22, John Garry wrote: >>>> 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. >>>> >>>> Sending as an RFC as unsure if this is the right direction. >>>> >>>> The motivator for this series was initial md RAID atomic write >>>> support in >>>> https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2- >>>> a93b-0a433741fd26@oracle.com/ >>>> >>>> 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). >>>> >>>> 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 | 5 +++++ >>>> drivers/md/raid0.c | 10 ++++++++++ >>>> drivers/md/raid1.c | 8 ++++++++ >>>> drivers/md/raid10.c | 18 ++++++++++++++++++ >>>> 6 files changed, 52 insertions(+), 5 deletions(-) >>>> >>> You are missing '__bio_split_to_limits()' which looks as it would >>> need to be modified, too. >>> >> >> In __bio_split_to_limits(), for REQ_OP_DISCARD, REQ_OP_SECURE_ERASE, >> and REQ_OP_WRITE_ZEROES, we indirectly call bio_split(). And >> bio_split() might error. But functions like bio_split_discard() can >> return NULL for cases where a split is not required. So I suppose we >> need to check IS_ERR(split) for those request types mentioned. For >> NULL being returned, we would still have the __bio_split_to_limits() >> is "if (split)" check. >> hold on a moment - were you looking at latest code on Jens' branch? There __bio_split_to_limits() will not return a ERR_PTR() (from changes in this series) - it will still just return NULL or a bio. In all cases there, __bio_split_to_limits() calls bio_submit_rw(), and still bio_submit_rw() will return NULL or a proper bio. This is because we translate a ERR_PTR() from bio_split() to NULL. Christoph changed this bio splitting in https://lore.kernel.org/linux-block/20240826173820.1690925-1-hch@lst.de/ I think that if my changes were based on v6.11, you were right. Thanks, John