On 13.11.24 10:51, John Garry wrote:
> On 04/11/2024 12:13, Johannes Thumshirn wrote:
>> From: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
>>
>> Now that bio_split() can return errors, add error handling for it in
>> btrfs_split_bio() and ultimately btrfs_submit_chunk().
>
> I have a couple of comments, below; However, since I am not familiar
> with the code, maybe they are invalid.
>
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
>> ---
>>
>> This is based on top of John Garry's series "bio_split() error handling
>> rework" explicitly on the patch titled "block: Rework bio_split() return
>> value", which are as of now (Tue Oct 29 10:02:16 2024) not yet merged into
>> any tree.
>>
>> Changes to v2:
>> - assign the split bbio to a new variable, so we can keep the old error
>> paths and end the original bbio
>>
>> Changes to v1:
>> - convert ERR_PTR to blk_status_t
>> - correctly fail already split bbios
>> ---
>> fs/btrfs/bio.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>> index 1f216d07eff6..7a0998d0abe3 100644
>> --- a/fs/btrfs/bio.c
>> +++ b/fs/btrfs/bio.c
>> @@ -81,6 +81,9 @@ static struct btrfs_bio *btrfs_split_bio(struct btrfs_fs_info *fs_info,
>>
>> bio = bio_split(&orig_bbio->bio, map_length >> SECTOR_SHIFT, GFP_NOFS,
>> &btrfs_clone_bioset);
>> + if (IS_ERR(bio))
>> + return ERR_CAST(bio);
>> +
>> bbio = btrfs_bio(bio);
>> btrfs_bio_init(bbio, fs_info, NULL, orig_bbio);
>> bbio->inode = orig_bbio->inode;
>> @@ -678,7 +681,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>> &bioc, &smap, &mirror_num);
>> if (error) {
>> ret = errno_to_blk_status(error);
>> - goto fail;
>> + goto end_bbio;
>
> eh, I am not sure why this has changed (and we now skip the "fail" label
> actions)
Because we want to skip the 'if (map_length < length) {' part below the
'fail' label and directly go to btrfs_bio_end_io().
>> }
>>
>> map_length = min(map_length, length);
>> @@ -686,7 +689,14 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>> map_length = btrfs_append_map_length(bbio, map_length);
>>
>> if (map_length < length) {
>> - bbio = btrfs_split_bio(fs_info, bbio, map_length);
>> + struct btrfs_bio *split;
>> +
>> + split = btrfs_split_bio(fs_info, bbio, map_length);
>> + if (IS_ERR(split)) {
>> + ret = errno_to_blk_status(PTR_ERR(split));
>> + goto end_bbio;
>
> Do we need to undo the btrfs_bio_counter_inc() (not shown)?
Yes we do.
>
>> + }
>> + bbio = split;
>> bio = &bbio->bio;
>> }
>>
>> @@ -760,6 +770,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>>
>> btrfs_bio_end_io(remaining, ret);
>> }
>> +end_bbio:
>> btrfs_bio_end_io(bbio, ret);
>> /* Do not submit another chunk */
>> return true;
>
>
