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(-)
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
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(-) >
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(-) >
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
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
© 2016 - 2024 Red Hat, Inc.