Re: [PATCH v3] btrfs: handle bio_split() error

[John Garry <john.g.garry@oracle.com>]

On 13/11/2024 10:08, Johannes Thumshirn wrote:

Hi Johannes,

Could you kindly repost this patch? Sorry for not paying attention to it 
further previously.

BTW, on a related topic, should we check for a negative error code in 
btrfs_append_map_length() -> bio_split_rw_at() result?

Thanks,
John


> On 13.11.24 10:51, John Garry wrote:
>> On 04/11/2024 12:13, Johannes Thumshirn wrote:
>>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>
>>> 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@wdc.com>
>>> ---
>>>
>>> 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;
>>
>>
>