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@xxxxxxxxxx/
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
