[PATCH v3] btrfs: handle bio_split() error

[PATCH v3] btrfs: handle bio_split() error

[Johannes Thumshirn]

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().

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;
 	}
 
 	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;
+		}
+		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;
-- 
2.43.0

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

[John Garry]

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)

>   	}
>   
>   	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)?

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

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

[Johannes Thumshirn]

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

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

[John Garry]

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