diff mbox series

btrfs: merge btrfs_orig_bbio_end_io() into btrfs_bio_end_io()

Message ID 2107a0e72fa6eac67583deb421ac1247b02b7723.1724057484.git.wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: merge btrfs_orig_bbio_end_io() into btrfs_bio_end_io() | expand

Commit Message

Qu Wenruo Aug. 19, 2024, 8:51 a.m. UTC 
There is only two differences between the two functions:

- btrfs_orig_bbio_end_io() do extra error propagation
  This is mostly to allow tolerance for write errors.

- btrfs_orig_bbio_end_io() do extra pending_ios check
  This check can handle both the original bio, or the cloned one.
  (All accounting happens in the original one).

This makes btrfs_orig_bbio_end_io() a much safer call.
In fact we already had a double freeing error due to usage of
btrfs_bio_end_io() in the error path of btrfs_submit_chunk().

This patch will move the whole content of btrfs_orig_bbio_end_io() into
btrfs_bio_end_io().

For normal paths this brings no change, because they are already calling
btrfs_orig_bbio_end_io() in the first place.

For error paths (not only inside bio.c but also external callers), this
change will introduce extra checks, especially for external callers, as
they will error out without submitting the btrfs bio.

But considering it's already in the error path, such slower but much
safer checks are still an overall win.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/bio.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

Comments

David Sterba Aug. 19, 2024, 2:34 p.m. UTC | #1 
On Mon, Aug 19, 2024 at 06:21:59PM +0930, Qu Wenruo wrote:
> There is only two differences between the two functions:
> 
> - btrfs_orig_bbio_end_io() do extra error propagation
>   This is mostly to allow tolerance for write errors.
> 
> - btrfs_orig_bbio_end_io() do extra pending_ios check
>   This check can handle both the original bio, or the cloned one.
>   (All accounting happens in the original one).
> 
> This makes btrfs_orig_bbio_end_io() a much safer call.
> In fact we already had a double freeing error due to usage of
> btrfs_bio_end_io() in the error path of btrfs_submit_chunk().
> 
> This patch will move the whole content of btrfs_orig_bbio_end_io() into

"Move the ..."

> btrfs_bio_end_io().
> 
> For normal paths this brings no change, because they are already calling
> btrfs_orig_bbio_end_io() in the first place.
> 
> For error paths (not only inside bio.c but also external callers), this
> change will introduce extra checks, especially for external callers, as
> they will error out without submitting the btrfs bio.
> 
> But considering it's already in the error path, such slower but much
> safer checks are still an overall win.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Please fix the grammar in the changelog when you commit the patch.

> ---
>  fs/btrfs/bio.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 088ceaca6ab0..0e4de33515fe 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -120,12 +120,6 @@ static void __btrfs_bio_end_io(struct btrfs_bio *bbio)
>  	}
>  }
>  
> -void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
> -{
> -	bbio->bio.bi_status = status;
> -	__btrfs_bio_end_io(bbio);

This leaves one last call to __btrfs_bio_end_io() so it can be moved to
it's caller btrfs_orig_bbio_end_io().
Qu Wenruo Aug. 20, 2024, 3:25 a.m. UTC | #2 
在 2024/8/20 00:04, David Sterba 写道:
> On Mon, Aug 19, 2024 at 06:21:59PM +0930, Qu Wenruo wrote:
>> There is only two differences between the two functions:
>>
>> - btrfs_orig_bbio_end_io() do extra error propagation
>>    This is mostly to allow tolerance for write errors.
>>
>> - btrfs_orig_bbio_end_io() do extra pending_ios check
>>    This check can handle both the original bio, or the cloned one.
>>    (All accounting happens in the original one).
>>
>> This makes btrfs_orig_bbio_end_io() a much safer call.
>> In fact we already had a double freeing error due to usage of
>> btrfs_bio_end_io() in the error path of btrfs_submit_chunk().
>>
>> This patch will move the whole content of btrfs_orig_bbio_end_io() into
>
> "Move the ..."
>
>> btrfs_bio_end_io().
>>
>> For normal paths this brings no change, because they are already calling
>> btrfs_orig_bbio_end_io() in the first place.
>>
>> For error paths (not only inside bio.c but also external callers), this
>> change will introduce extra checks, especially for external callers, as
>> they will error out without submitting the btrfs bio.
>>
>> But considering it's already in the error path, such slower but much
>> safer checks are still an overall win.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Reviewed-by: David Sterba <dsterba@suse.com>
>
> Please fix the grammar in the changelog when you commit the patch.

Sure, although this depends on the use-after-free bug inside
btrfs_submit_chunk().

I'll commit when that fix got merged first.

Thanks,
Qu
>
>> ---
>>   fs/btrfs/bio.c | 26 ++++++++++----------------
>>   1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>> index 088ceaca6ab0..0e4de33515fe 100644
>> --- a/fs/btrfs/bio.c
>> +++ b/fs/btrfs/bio.c
>> @@ -120,12 +120,6 @@ static void __btrfs_bio_end_io(struct btrfs_bio *bbio)
>>   	}
>>   }
>>
>> -void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
>> -{
>> -	bbio->bio.bi_status = status;
>> -	__btrfs_bio_end_io(bbio);
>
> This leaves one last call to __btrfs_bio_end_io() so it can be moved to
> it's caller btrfs_orig_bbio_end_io().
>
diff mbox series

Patch

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 088ceaca6ab0..0e4de33515fe 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -120,12 +120,6 @@  static void __btrfs_bio_end_io(struct btrfs_bio *bbio)
 	}
 }
 
-void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
-{
-	bbio->bio.bi_status = status;
-	__btrfs_bio_end_io(bbio);
-}
-
 static void btrfs_orig_write_end_io(struct bio *bio);
 
 static void btrfs_bbio_propagate_error(struct btrfs_bio *bbio,
@@ -147,8 +141,9 @@  static void btrfs_bbio_propagate_error(struct btrfs_bio *bbio,
 	}
 }
 
-static void btrfs_orig_bbio_end_io(struct btrfs_bio *bbio)
+void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
 {
+	bbio->bio.bi_status = status;
 	if (bbio->bio.bi_pool == &btrfs_clone_bioset) {
 		struct btrfs_bio *orig_bbio = bbio->private;
 
@@ -179,7 +174,7 @@  static int prev_repair_mirror(struct btrfs_failed_bio *fbio, int cur_mirror)
 static void btrfs_repair_done(struct btrfs_failed_bio *fbio)
 {
 	if (atomic_dec_and_test(&fbio->repair_count)) {
-		btrfs_orig_bbio_end_io(fbio->bbio);
+		btrfs_bio_end_io(fbio->bbio, fbio->bbio->bio.bi_status);
 		mempool_free(fbio, &btrfs_failed_bio_pool);
 	}
 }
@@ -326,7 +321,7 @@  static void btrfs_check_read_bio(struct btrfs_bio *bbio, struct btrfs_device *de
 	if (fbio)
 		btrfs_repair_done(fbio);
 	else
-		btrfs_orig_bbio_end_io(bbio);
+		btrfs_bio_end_io(bbio, bbio->bio.bi_status);
 }
 
 static void btrfs_log_dev_io_error(struct bio *bio, struct btrfs_device *dev)
@@ -360,7 +355,7 @@  static void btrfs_end_bio_work(struct work_struct *work)
 	if (is_data_bbio(bbio))
 		btrfs_check_read_bio(bbio, bbio->bio.bi_private);
 	else
-		btrfs_orig_bbio_end_io(bbio);
+		btrfs_bio_end_io(bbio, bbio->bio.bi_status);
 }
 
 static void btrfs_simple_end_io(struct bio *bio)
@@ -380,7 +375,7 @@  static void btrfs_simple_end_io(struct bio *bio)
 	} else {
 		if (bio_op(bio) == REQ_OP_ZONE_APPEND && !bio->bi_status)
 			btrfs_record_physical_zoned(bbio);
-		btrfs_orig_bbio_end_io(bbio);
+		btrfs_bio_end_io(bbio, bbio->bio.bi_status);
 	}
 }
 
@@ -394,7 +389,7 @@  static void btrfs_raid56_end_io(struct bio *bio)
 	if (bio_op(bio) == REQ_OP_READ && is_data_bbio(bbio))
 		btrfs_check_read_bio(bbio, NULL);
 	else
-		btrfs_orig_bbio_end_io(bbio);
+		btrfs_bio_end_io(bbio, bbio->bio.bi_status);
 
 	btrfs_put_bioc(bioc);
 }
@@ -424,7 +419,7 @@  static void btrfs_orig_write_end_io(struct bio *bio)
 	if (bio_op(bio) == REQ_OP_ZONE_APPEND && !bio->bi_status)
 		stripe->physical = bio->bi_iter.bi_sector << SECTOR_SHIFT;
 
-	btrfs_orig_bbio_end_io(bbio);
+	btrfs_bio_end_io(bbio, bbio->bio.bi_status);
 	btrfs_put_bioc(bioc);
 }
 
@@ -593,7 +588,7 @@  static void run_one_async_done(struct btrfs_work *work, bool do_free)
 
 	/* If an error occurred we just want to clean up the bio and move on. */
 	if (bio->bi_status) {
-		btrfs_orig_bbio_end_io(async->bbio);
+		btrfs_bio_end_io(async->bbio, async->bbio->bio.bi_status);
 		return;
 	}
 
@@ -761,8 +756,7 @@  static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
 		btrfs_cleanup_bio(bbio);
 fail:
 	btrfs_bio_counter_dec(fs_info);
-	bbio->bio.bi_status = ret;
-	btrfs_orig_bbio_end_io(bbio);
+	btrfs_bio_end_io(bbio, ret);
 	/* Do not submit another chunk */
 	return true;
 }