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

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

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;
> 
>