diff mbox series

btrfs: handle bio_split() error

Message ID 20241029091121.16281-1-jth@kernel.org (mailing list archive)
State Handled Elsewhere
Headers show
Series btrfs: handle bio_split() error | expand

Checks

Context Check Description
mdraidci/vmtest-md-6_13-PR success PR summary
mdraidci/vmtest-md-6_13-VM_Test-0 success Logs for per-patch-testing

Commit Message

Johannes Thumshirn Oct. 29, 2024, 9:11 a.m. UTC
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>
---

John,
in case you have to send a v3, can you please include this one in your series?

 fs/btrfs/bio.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

John Garry Oct. 29, 2024, 10:33 a.m. UTC | #1
On 29/10/2024 09:11, 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().
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
> 
> John,
> in case you have to send a v3, can you please include this one in your series?

sure, and I think that I will be sending a v3

Cheers

> 
>   fs/btrfs/bio.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 1f216d07eff6..96cacd5c03a5 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;
> @@ -687,6 +690,10 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>   
>   	if (map_length < length) {
>   		bbio = btrfs_split_bio(fs_info, bbio, map_length);
> +		if (IS_ERR(bbio)) {
> +			ret = PTR_ERR(bbio);
> +			goto fail;
> +		}
>   		bio = &bbio->bio;
>   	}
>
kernel test robot Oct. 30, 2024, 2 p.m. UTC | #2
Hi Johannes,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on linus/master v6.12-rc5 next-20241030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johannes-Thumshirn/btrfs-handle-bio_split-error/20241029-171227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/20241029091121.16281-1-jth%40kernel.org
patch subject: [PATCH] btrfs: handle bio_split() error
config: x86_64-randconfig-121-20241030 (https://download.01.org/0day-ci/archive/20241030/202410302118.6igPxwWv-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241030/202410302118.6igPxwWv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410302118.6igPxwWv-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> fs/btrfs/bio.c:694:29: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted blk_status_t [assigned] [usertype] ret @@     got long @@
   fs/btrfs/bio.c:694:29: sparse:     expected restricted blk_status_t [assigned] [usertype] ret
   fs/btrfs/bio.c:694:29: sparse:     got long
   fs/btrfs/bio.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, ...):
   include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always evaluates to false
   include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always evaluates to false

vim +694 fs/btrfs/bio.c

   659	
   660	static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
   661	{
   662		struct btrfs_inode *inode = bbio->inode;
   663		struct btrfs_fs_info *fs_info = bbio->fs_info;
   664		struct bio *bio = &bbio->bio;
   665		u64 logical = bio->bi_iter.bi_sector << SECTOR_SHIFT;
   666		u64 length = bio->bi_iter.bi_size;
   667		u64 map_length = length;
   668		bool use_append = btrfs_use_zone_append(bbio);
   669		struct btrfs_io_context *bioc = NULL;
   670		struct btrfs_io_stripe smap;
   671		blk_status_t ret;
   672		int error;
   673	
   674		if (!bbio->inode || btrfs_is_data_reloc_root(inode->root))
   675			smap.rst_search_commit_root = true;
   676		else
   677			smap.rst_search_commit_root = false;
   678	
   679		btrfs_bio_counter_inc_blocked(fs_info);
   680		error = btrfs_map_block(fs_info, btrfs_op(bio), logical, &map_length,
   681					&bioc, &smap, &mirror_num);
   682		if (error) {
   683			ret = errno_to_blk_status(error);
   684			goto fail;
   685		}
   686	
   687		map_length = min(map_length, length);
   688		if (use_append)
   689			map_length = btrfs_append_map_length(bbio, map_length);
   690	
   691		if (map_length < length) {
   692			bbio = btrfs_split_bio(fs_info, bbio, map_length);
   693			if (IS_ERR(bbio)) {
 > 694				ret = PTR_ERR(bbio);
   695				goto fail;
   696			}
   697			bio = &bbio->bio;
   698		}
   699	
   700		/*
   701		 * Save the iter for the end_io handler and preload the checksums for
   702		 * data reads.
   703		 */
   704		if (bio_op(bio) == REQ_OP_READ && is_data_bbio(bbio)) {
   705			bbio->saved_iter = bio->bi_iter;
   706			ret = btrfs_lookup_bio_sums(bbio);
   707			if (ret)
   708				goto fail;
   709		}
   710	
   711		if (btrfs_op(bio) == BTRFS_MAP_WRITE) {
   712			if (use_append) {
   713				bio->bi_opf &= ~REQ_OP_WRITE;
   714				bio->bi_opf |= REQ_OP_ZONE_APPEND;
   715			}
   716	
   717			if (is_data_bbio(bbio) && bioc &&
   718			    btrfs_need_stripe_tree_update(bioc->fs_info, bioc->map_type)) {
   719				/*
   720				 * No locking for the list update, as we only add to
   721				 * the list in the I/O submission path, and list
   722				 * iteration only happens in the completion path, which
   723				 * can't happen until after the last submission.
   724				 */
   725				btrfs_get_bioc(bioc);
   726				list_add_tail(&bioc->rst_ordered_entry, &bbio->ordered->bioc_list);
   727			}
   728	
   729			/*
   730			 * Csum items for reloc roots have already been cloned at this
   731			 * point, so they are handled as part of the no-checksum case.
   732			 */
   733			if (inode && !(inode->flags & BTRFS_INODE_NODATASUM) &&
   734			    !test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) &&
   735			    !btrfs_is_data_reloc_root(inode->root)) {
   736				if (should_async_write(bbio) &&
   737				    btrfs_wq_submit_bio(bbio, bioc, &smap, mirror_num))
   738					goto done;
   739	
   740				ret = btrfs_bio_csum(bbio);
   741				if (ret)
   742					goto fail;
   743			} else if (use_append ||
   744				   (btrfs_is_zoned(fs_info) && inode &&
   745				    inode->flags & BTRFS_INODE_NODATASUM)) {
   746				ret = btrfs_alloc_dummy_sum(bbio);
   747				if (ret)
   748					goto fail;
   749			}
   750		}
   751	
   752		btrfs_submit_bio(bio, bioc, &smap, mirror_num);
   753	done:
   754		return map_length == length;
   755	
   756	fail:
   757		btrfs_bio_counter_dec(fs_info);
   758		/*
   759		 * We have split the original bbio, now we have to end both the current
   760		 * @bbio and remaining one, as the remaining one will never be submitted.
   761		 */
   762		if (map_length < length) {
   763			struct btrfs_bio *remaining = bbio->private;
   764	
   765			ASSERT(bbio->bio.bi_pool == &btrfs_clone_bioset);
   766			ASSERT(remaining);
   767	
   768			btrfs_bio_end_io(remaining, ret);
   769		}
   770		btrfs_bio_end_io(bbio, ret);
   771		/* Do not submit another chunk */
   772		return true;
   773	}
   774
Johannes Thumshirn Oct. 30, 2024, 2:06 p.m. UTC | #3
On 30.10.24 15:01, kernel test robot wrote:

> sparse warnings: (new ones prefixed by >>)
>>> fs/btrfs/bio.c:694:29: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted blk_status_t [assigned] [usertype] ret @@     got long @@

Yep that definitively needs to be:

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 96cacd5c03a5..847af28ecff9 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -691,7 +691,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio 
*bbio, int mirror_num)
         if (map_length < length) {
                 bbio = btrfs_split_bio(fs_info, bbio, map_length);
                 if (IS_ERR(bbio)) {
-                       ret = PTR_ERR(bbio);
+                       ret = errno_to_blk_status(PTR_ERR(bbio));
                         goto fail;
                 }
                 bio = &bbio->bio;

Can you fold that in John or do you want me to send a new version?
John Garry Oct. 30, 2024, 2:20 p.m. UTC | #4
On 30/10/2024 14:06, Johannes Thumshirn wrote:
>>>> ret @@     got long @@
> Yep that definitively needs to be:
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 96cacd5c03a5..847af28ecff9 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -691,7 +691,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio
> *bbio, int mirror_num)
>           if (map_length < length) {
>                   bbio = btrfs_split_bio(fs_info, bbio, map_length);
>                   if (IS_ERR(bbio)) {
> -                       ret = PTR_ERR(bbio);
> +                       ret = errno_to_blk_status(PTR_ERR(bbio));
>                           goto fail;
>                   }
>                   bio = &bbio->bio;
> 
> Can you fold that in John or do you want me to send a new version?

Sure, no problem.

But I would have suggested to not use variable name "ret" for holding a 
blk_status_t in original code, especially when it is mixed with 
PTR_ERR() usage ...

Thanks,
John
Johannes Thumshirn Oct. 30, 2024, 2:29 p.m. UTC | #5
On 30.10.24 15:21, John Garry wrote:
> On 30/10/2024 14:06, Johannes Thumshirn wrote:
>>>>> ret @@     got long @@
>> Yep that definitively needs to be:
>>
>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>> index 96cacd5c03a5..847af28ecff9 100644
>> --- a/fs/btrfs/bio.c
>> +++ b/fs/btrfs/bio.c
>> @@ -691,7 +691,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio
>> *bbio, int mirror_num)
>>            if (map_length < length) {
>>                    bbio = btrfs_split_bio(fs_info, bbio, map_length);
>>                    if (IS_ERR(bbio)) {
>> -                       ret = PTR_ERR(bbio);
>> +                       ret = errno_to_blk_status(PTR_ERR(bbio));
>>                            goto fail;
>>                    }
>>                    bio = &bbio->bio;
>>
>> Can you fold that in John or do you want me to send a new version?
> 
> Sure, no problem.
> 
> But I would have suggested to not use variable name "ret" for holding a
> blk_status_t in original code, especially when it is mixed with
> PTR_ERR() usage ...

Yeah but the original code is using "blk_status_t ret", otherwise it 
would be sth. like:

error = PTR_ERR(bbio);
ret = errno_to_blk_status(error);
goto fail;

[...]

fail:
btrfs_bio_end_io(bbio, ret);

Or changing the original code, which we could do but there's way more 
urgent problems :)
Dan Carpenter Oct. 30, 2024, 8:05 p.m. UTC | #6
Hi Johannes,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johannes-Thumshirn/btrfs-handle-bio_split-error/20241029-171227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/20241029091121.16281-1-jth%40kernel.org
patch subject: [PATCH] btrfs: handle bio_split() error
config: openrisc-randconfig-r072-20241030 (https://download.01.org/0day-ci/archive/20241031/202410310231.WMcRwBhG-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202410310231.WMcRwBhG-lkp@intel.com/

smatch warnings:
fs/btrfs/bio.c:763 btrfs_submit_chunk() error: 'bbio' dereferencing possible ERR_PTR()

vim +/bbio +763 fs/btrfs/bio.c

ae42a154ca8972 Christoph Hellwig  2023-03-07  660  static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
103c19723c80bf Christoph Hellwig  2022-11-15  661  {
d5e4377d505189 Christoph Hellwig  2023-01-21  662  	struct btrfs_inode *inode = bbio->inode;
4317ff0056bedf Qu Wenruo          2023-03-23  663  	struct btrfs_fs_info *fs_info = bbio->fs_info;
ae42a154ca8972 Christoph Hellwig  2023-03-07  664  	struct bio *bio = &bbio->bio;
adbe7e388e4239 Anand Jain         2023-04-15  665  	u64 logical = bio->bi_iter.bi_sector << SECTOR_SHIFT;
103c19723c80bf Christoph Hellwig  2022-11-15  666  	u64 length = bio->bi_iter.bi_size;
103c19723c80bf Christoph Hellwig  2022-11-15  667  	u64 map_length = length;
921603c76246a7 Christoph Hellwig  2022-12-12  668  	bool use_append = btrfs_use_zone_append(bbio);
103c19723c80bf Christoph Hellwig  2022-11-15  669  	struct btrfs_io_context *bioc = NULL;
103c19723c80bf Christoph Hellwig  2022-11-15  670  	struct btrfs_io_stripe smap;
9ba0004bd95e05 Christoph Hellwig  2023-01-21  671  	blk_status_t ret;
9ba0004bd95e05 Christoph Hellwig  2023-01-21  672  	int error;
103c19723c80bf Christoph Hellwig  2022-11-15  673  
f4d39cf1cebfb8 Johannes Thumshirn 2024-07-31  674  	if (!bbio->inode || btrfs_is_data_reloc_root(inode->root))
f4d39cf1cebfb8 Johannes Thumshirn 2024-07-31  675  		smap.rst_search_commit_root = true;
f4d39cf1cebfb8 Johannes Thumshirn 2024-07-31  676  	else
f4d39cf1cebfb8 Johannes Thumshirn 2024-07-31  677  		smap.rst_search_commit_root = false;
9acaa64187f9b4 Johannes Thumshirn 2023-09-14  678  
103c19723c80bf Christoph Hellwig  2022-11-15  679  	btrfs_bio_counter_inc_blocked(fs_info);
cd4efd210edfb3 Christoph Hellwig  2023-05-31  680  	error = btrfs_map_block(fs_info, btrfs_op(bio), logical, &map_length,
9fb2acc2fe07f1 Qu Wenruo          2023-09-17  681  				&bioc, &smap, &mirror_num);
9ba0004bd95e05 Christoph Hellwig  2023-01-21  682  	if (error) {
9ba0004bd95e05 Christoph Hellwig  2023-01-21  683  		ret = errno_to_blk_status(error);
9ba0004bd95e05 Christoph Hellwig  2023-01-21  684  		goto fail;
103c19723c80bf Christoph Hellwig  2022-11-15  685  	}
103c19723c80bf Christoph Hellwig  2022-11-15  686  
852eee62d31abd Christoph Hellwig  2023-01-21  687  	map_length = min(map_length, length);
d5e4377d505189 Christoph Hellwig  2023-01-21  688  	if (use_append)
b35243a447b9fe Christoph Hellwig  2024-08-26  689  		map_length = btrfs_append_map_length(bbio, map_length);
d5e4377d505189 Christoph Hellwig  2023-01-21  690  
103c19723c80bf Christoph Hellwig  2022-11-15  691  	if (map_length < length) {
b35243a447b9fe Christoph Hellwig  2024-08-26  692  		bbio = btrfs_split_bio(fs_info, bbio, map_length);
28c02a018d50ae Johannes Thumshirn 2024-10-29  693  		if (IS_ERR(bbio)) {
28c02a018d50ae Johannes Thumshirn 2024-10-29  694  			ret = PTR_ERR(bbio);
28c02a018d50ae Johannes Thumshirn 2024-10-29  695  			goto fail;

We hit this goto.  We know from the if statement that map_length < length.

28c02a018d50ae Johannes Thumshirn 2024-10-29  696  		}
2cef0c79bb81d8 Christoph Hellwig  2023-03-07  697  		bio = &bbio->bio;
103c19723c80bf Christoph Hellwig  2022-11-15  698  	}
103c19723c80bf Christoph Hellwig  2022-11-15  699  
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  700  	/*
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  701  	 * Save the iter for the end_io handler and preload the checksums for
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  702  	 * data reads.
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  703  	 */
fbe960877b6f43 Christoph Hellwig  2023-05-31  704  	if (bio_op(bio) == REQ_OP_READ && is_data_bbio(bbio)) {
0d3acb25e70d5f Christoph Hellwig  2023-01-21  705  		bbio->saved_iter = bio->bi_iter;
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  706  		ret = btrfs_lookup_bio_sums(bbio);
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  707  		if (ret)
10d9d8c3512f16 Qu Wenruo          2024-08-17  708  			goto fail;
1c2b3ee3b0ec4b Christoph Hellwig  2023-01-21  709  	}
7276aa7d38255b Christoph Hellwig  2023-01-21  710  
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  711  	if (btrfs_op(bio) == BTRFS_MAP_WRITE) {
d5e4377d505189 Christoph Hellwig  2023-01-21  712  		if (use_append) {
d5e4377d505189 Christoph Hellwig  2023-01-21  713  			bio->bi_opf &= ~REQ_OP_WRITE;
d5e4377d505189 Christoph Hellwig  2023-01-21  714  			bio->bi_opf |= REQ_OP_ZONE_APPEND;
69ccf3f4244abc Christoph Hellwig  2023-01-21  715  		}
69ccf3f4244abc Christoph Hellwig  2023-01-21  716  
02c372e1f016e5 Johannes Thumshirn 2023-09-14  717  		if (is_data_bbio(bbio) && bioc &&
02c372e1f016e5 Johannes Thumshirn 2023-09-14  718  		    btrfs_need_stripe_tree_update(bioc->fs_info, bioc->map_type)) {
02c372e1f016e5 Johannes Thumshirn 2023-09-14  719  			/*
02c372e1f016e5 Johannes Thumshirn 2023-09-14  720  			 * No locking for the list update, as we only add to
02c372e1f016e5 Johannes Thumshirn 2023-09-14  721  			 * the list in the I/O submission path, and list
02c372e1f016e5 Johannes Thumshirn 2023-09-14  722  			 * iteration only happens in the completion path, which
02c372e1f016e5 Johannes Thumshirn 2023-09-14  723  			 * can't happen until after the last submission.
02c372e1f016e5 Johannes Thumshirn 2023-09-14  724  			 */
02c372e1f016e5 Johannes Thumshirn 2023-09-14  725  			btrfs_get_bioc(bioc);
02c372e1f016e5 Johannes Thumshirn 2023-09-14  726  			list_add_tail(&bioc->rst_ordered_entry, &bbio->ordered->bioc_list);
02c372e1f016e5 Johannes Thumshirn 2023-09-14  727  		}
02c372e1f016e5 Johannes Thumshirn 2023-09-14  728  
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  729  		/*
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  730  		 * Csum items for reloc roots have already been cloned at this
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  731  		 * point, so they are handled as part of the no-checksum case.
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  732  		 */
4317ff0056bedf Qu Wenruo          2023-03-23  733  		if (inode && !(inode->flags & BTRFS_INODE_NODATASUM) &&
169aaaf2e0be61 Qu Wenruo          2024-06-14  734  		    !test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) &&
d5e4377d505189 Christoph Hellwig  2023-01-21  735  		    !btrfs_is_data_reloc_root(inode->root)) {
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  736  			if (should_async_write(bbio) &&
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  737  			    btrfs_wq_submit_bio(bbio, bioc, &smap, mirror_num))
852eee62d31abd Christoph Hellwig  2023-01-21  738  				goto done;
103c19723c80bf Christoph Hellwig  2022-11-15  739  
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  740  			ret = btrfs_bio_csum(bbio);
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  741  			if (ret)
10d9d8c3512f16 Qu Wenruo          2024-08-17  742  				goto fail;
cebae292e0c32a Johannes Thumshirn 2024-06-07  743  		} else if (use_append ||
cebae292e0c32a Johannes Thumshirn 2024-06-07  744  			   (btrfs_is_zoned(fs_info) && inode &&
cebae292e0c32a Johannes Thumshirn 2024-06-07  745  			    inode->flags & BTRFS_INODE_NODATASUM)) {
cbfce4c7fbde23 Christoph Hellwig  2023-05-24  746  			ret = btrfs_alloc_dummy_sum(bbio);
cbfce4c7fbde23 Christoph Hellwig  2023-05-24  747  			if (ret)
10d9d8c3512f16 Qu Wenruo          2024-08-17  748  				goto fail;
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  749  		}
103c19723c80bf Christoph Hellwig  2022-11-15  750  	}
f8a53bb58ec7e2 Christoph Hellwig  2023-01-21  751  
22b4ef50dc1d11 David Sterba       2024-08-27  752  	btrfs_submit_bio(bio, bioc, &smap, mirror_num);
852eee62d31abd Christoph Hellwig  2023-01-21  753  done:
852eee62d31abd Christoph Hellwig  2023-01-21  754  	return map_length == length;
9ba0004bd95e05 Christoph Hellwig  2023-01-21  755  
9ba0004bd95e05 Christoph Hellwig  2023-01-21  756  fail:
9ba0004bd95e05 Christoph Hellwig  2023-01-21  757  	btrfs_bio_counter_dec(fs_info);
10d9d8c3512f16 Qu Wenruo          2024-08-17  758  	/*
10d9d8c3512f16 Qu Wenruo          2024-08-17  759  	 * We have split the original bbio, now we have to end both the current
10d9d8c3512f16 Qu Wenruo          2024-08-17  760  	 * @bbio and remaining one, as the remaining one will never be submitted.
10d9d8c3512f16 Qu Wenruo          2024-08-17  761  	 */
10d9d8c3512f16 Qu Wenruo          2024-08-17  762  	if (map_length < length) {
10d9d8c3512f16 Qu Wenruo          2024-08-17 @763  		struct btrfs_bio *remaining = bbio->private;
                                                                                              ^^^^^^^^^^^^^
Error pointer dereference

10d9d8c3512f16 Qu Wenruo          2024-08-17  764  
10d9d8c3512f16 Qu Wenruo          2024-08-17  765  		ASSERT(bbio->bio.bi_pool == &btrfs_clone_bioset);
10d9d8c3512f16 Qu Wenruo          2024-08-17  766  		ASSERT(remaining);
10d9d8c3512f16 Qu Wenruo          2024-08-17  767  
9ca0e58cb752b0 Qu Wenruo          2024-08-24  768  		btrfs_bio_end_io(remaining, ret);
10d9d8c3512f16 Qu Wenruo          2024-08-17  769  	}
9ca0e58cb752b0 Qu Wenruo          2024-08-24  770  	btrfs_bio_end_io(bbio, ret);
852eee62d31abd Christoph Hellwig  2023-01-21  771  	/* Do not submit another chunk */
852eee62d31abd Christoph Hellwig  2023-01-21  772  	return true;
852eee62d31abd Christoph Hellwig  2023-01-21  773  }
John Garry Oct. 31, 2024, 9:29 a.m. UTC | #7
> 852eee62d31abd Christoph Hellwig  2023-01-21  687  	map_length = min(map_length, length);
> d5e4377d505189 Christoph Hellwig  2023-01-21  688  	if (use_append)
> b35243a447b9fe Christoph Hellwig  2024-08-26  689  		map_length = btrfs_append_map_length(bbio, map_length);
> d5e4377d505189 Christoph Hellwig  2023-01-21  690
> 103c19723c80bf Christoph Hellwig  2022-11-15  691  	if (map_length < length) {
> b35243a447b9fe Christoph Hellwig  2024-08-26  692  		bbio = btrfs_split_bio(fs_info, bbio, map_length);
> 28c02a018d50ae Johannes Thumshirn 2024-10-29  693  		if (IS_ERR(bbio)) {
> 28c02a018d50ae Johannes Thumshirn 2024-10-29  694  			ret = PTR_ERR(bbio);
> 28c02a018d50ae Johannes Thumshirn 2024-10-29  695  			goto fail;
> 
> We hit this goto.  We know from the if statement that map_length < length.
> 
> 28c02a018d50ae Johannes Thumshirn 2024-10-29  696  		}
> 2cef0c79bb81d8 Christoph Hellwig  2023-03-07  697  		bio = &bbio->bio;
> 103c19723c80bf Christoph Hellwig  2022-11-15  698  	}
> 103c19723c80bf Christoph Hellwig  2022-11-15  699

...

> 852eee62d31abd Christoph Hellwig  2023-01-21  753  done:
> 852eee62d31abd Christoph Hellwig  2023-01-21  754  	return map_length == length;
> 9ba0004bd95e05 Christoph Hellwig  2023-01-21  755
> 9ba0004bd95e05 Christoph Hellwig  2023-01-21  756  fail:
> 9ba0004bd95e05 Christoph Hellwig  2023-01-21  757  	btrfs_bio_counter_dec(fs_info);
> 10d9d8c3512f16 Qu Wenruo          2024-08-17  758  	/*
> 10d9d8c3512f16 Qu Wenruo          2024-08-17  759  	 * We have split the original bbio, now we have to end both the current
> 10d9d8c3512f16 Qu Wenruo          2024-08-17  760  	 * @bbio and remaining one, as the remaining one will never be submitted.
> 10d9d8c3512f16 Qu Wenruo          2024-08-17  761  	 */
> 10d9d8c3512f16 Qu Wenruo          2024-08-17  762  	if (map_length < length) {
> 10d9d8c3512f16 Qu Wenruo          2024-08-17 @763  		struct btrfs_bio *remaining = bbio->private;
>                                                                                                ^^^^^^^^^^^^^
> Error pointer dereference

This analysis looks correct.

I want to send a new version of the bio_split() rework series this 
morning, so I will not pick up this change for now.

John
diff mbox series

Patch

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 1f216d07eff6..96cacd5c03a5 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;
@@ -687,6 +690,10 @@  static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
 
 	if (map_length < length) {
 		bbio = btrfs_split_bio(fs_info, bbio, map_length);
+		if (IS_ERR(bbio)) {
+			ret = PTR_ERR(bbio);
+			goto fail;
+		}
 		bio = &bbio->bio;
 	}