[PATCH v2 0/2] btrfs: raid56: concurrency fix and a very tiny optimization

[PATCH v2 0/2] btrfs: raid56: concurrency fix and a very tiny optimization

[Qu Wenruo]

We have a unprotected concurrency updateing rbio::error_bitmap.
The first patch is going to fix it.

While we are at rbio_update_error_bitmap(), there is also a tiny
optimization we can do for calculating the bio size.
Since we only care about the size of the bio, bio_for_each_bvec_all() is
much better than bio_for_each_segment_all(), as the former one can
handle multi-page bvec directly to reduce the loop.

Changelog:
v2:
- Use set_bit() in a loop instead of an asymmetrical spinlock.
  Since only endio can have concurrency accessing the bitmap, while
  the main thread only access them in a single thread, we will have
  asymmetrical spinlock schema, which is not ideal.
  Instead go set_bit() in a loop.

- Add a tiny optimization to calculate bio length in
  rbio_update_error_bitmap()
  

Qu Wenruo (2):
  btrfs: raid56: make error_bitmap update atomic
  btrfs: raid56: reduce the overhead to calculate the bio length

 fs/btrfs/raid56.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.39.1

[PATCH v2 1/2] btrfs: raid56: make error_bitmap update atomic

[Qu Wenruo]

In the rework of raid56 code, there is very limited concurrency in the
endio context.

Most of works is done inside the sectors arrays, which different bios
will never touch the same sector.

But there is a concurrency here for error_bitmap. Both read and write
endio functions need to touch them, and we can have multiple write bios
touching the same error bitmap if they all hit some errors.

Here we fix the unprotected bitmap operation by going set_bit() in a
loop.

Since we have a very small ceiling of the sectors (at most 16 sectors),
such set_bit() in a loop should be very acceptable.

Fixes: 2942a50dea74 ("btrfs: raid56: introduce btrfs_raid_bio::error_bitmap")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/raid56.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 65ed4f326fb9..6f91a78d2e8d 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1426,12 +1426,20 @@ static void rbio_update_error_bitmap(struct btrfs_raid_bio *rbio, struct bio *bi
 	u32 bio_size = 0;
 	struct bio_vec *bvec;
 	struct bvec_iter_all iter_all;
+	int i;
 
 	bio_for_each_segment_all(bvec, bio, iter_all)
 		bio_size += bvec->bv_len;
 
-	bitmap_set(rbio->error_bitmap, total_sector_nr,
-		   bio_size >> rbio->bioc->fs_info->sectorsize_bits);
+	/*
+	 * Since we can have multiple bios touching the error_bitmap, we can not
+	 * call bitmap_set() without protection.
+	 *
+	 * Instead use set_bit() for each bit, as set_bit() itself is atomic.
+	 */
+	for (i = total_sector_nr; i < total_sector_nr +
+	     (bio_size >> rbio->bioc->fs_info->sectorsize_bits); i++)
+		set_bit(i, rbio->error_bitmap);
 }
 
 /* Verify the data sectors at read time. */
-- 
2.39.1

Re: [PATCH v2 1/2] btrfs: raid56: make error_bitmap update atomic

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 2/2] btrfs: raid56: reduce the overhead to calculate the bio length

[Qu Wenruo]

In rbio_update_error_bitmap(), we need to calculate the length of the
rbio.
As since it's called in the endio function, we can not directly grab the
length from bi_iter.

Currently we call bio_for_each_segment_all(), which will always return a
range inside a page.
But that's not necessary as we don't really care anything inside the
page.

So this patch will use bio_for_each_bvec_all(), which can return a bvec
across multiple continuous pages thus reduce the loops.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/raid56.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 6f91a78d2e8d..1ed9e07833a7 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1425,10 +1425,9 @@ static void rbio_update_error_bitmap(struct btrfs_raid_bio *rbio, struct bio *bi
 	int total_sector_nr = get_bio_sector_nr(rbio, bio);
 	u32 bio_size = 0;
 	struct bio_vec *bvec;
-	struct bvec_iter_all iter_all;
 	int i;
 
-	bio_for_each_segment_all(bvec, bio, iter_all)
+	bio_for_each_bvec_all(bvec, bio, i)
 		bio_size += bvec->bv_len;
 
 	/*
-- 
2.39.1

Re: [PATCH v2 2/2] btrfs: raid56: reduce the overhead to calculate the bio length

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 0/2] btrfs: raid56: concurrency fix and a very tiny optimization

[David Sterba]

On Sat, Jan 21, 2023 at 04:06:10PM +0800, Qu Wenruo wrote:
> We have a unprotected concurrency updateing rbio::error_bitmap.
> The first patch is going to fix it.
> 
> While we are at rbio_update_error_bitmap(), there is also a tiny
> optimization we can do for calculating the bio size.
> Since we only care about the size of the bio, bio_for_each_bvec_all() is
> much better than bio_for_each_segment_all(), as the former one can
> handle multi-page bvec directly to reduce the loop.
> 
> Changelog:
> v2:
> - Use set_bit() in a loop instead of an asymmetrical spinlock.
>   Since only endio can have concurrency accessing the bitmap, while
>   the main thread only access them in a single thread, we will have
>   asymmetrical spinlock schema, which is not ideal.
>   Instead go set_bit() in a loop.
> 
> - Add a tiny optimization to calculate bio length in
>   rbio_update_error_bitmap()
>   
> 
> Qu Wenruo (2):
>   btrfs: raid56: make error_bitmap update atomic
>   btrfs: raid56: reduce the overhead to calculate the bio length

Added to misc-next, thank.