[PATCH 0/3] btrfs: raid56: fully support stripe_len < PAGE_SIZE

[PATCH 0/3] btrfs: raid56: fully support stripe_len < PAGE_SIZE

[Qu Wenruo]

Previously the variable stripe_len support for RAID56 intentionally
skips the stripe_len < PAGE_SIZE for partial rbio page allocation and
steal.

The reason is that the existing code assumes a stripe_len can never be
smaller than PAGE_SIZE, thus a page can either either data or P/Q
blocks, not both.

But it turns out that the original code is not unnecessarily complex,
especially for steal_rbio().
We can easily calculate the number of pages to steal/allocate without
any special assumption on stripe_len and PAGE_SIZE.

So add back the proper support for steal_rbio() and partial rbio page
allocation, which also slightly cleanup the steal_rbio() related to
helpers.

Qu Wenruo (3):
  btrfs: introduce rbio_nr_data_sectors() helper
  btrfs: add stripe_len < PAGE_SIZE support for steal_rbio()
  btrfs: add stripe_len < PAGE_SIZE support for alloc_rbio_*_pages()

 fs/btrfs/raid56.c | 182 +++++++++++++---------------------------------
 1 file changed, 51 insertions(+), 131 deletions(-)

-- 
2.56.0

[PATCH 1/3] btrfs: introduce rbio_nr_data_sectors() helper

[Qu Wenruo]

The pattern "rbio->nr_data * rbio->stripe_nsectors" is utilized more
than 5 times inside raid56.c, it's worthy to extract it into a new
helper.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 59aad807f98a..0c792d85f520 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -246,6 +246,12 @@ static void memcpy_from_bio_to_stripe(struct btrfs_raid_bio *rbio, unsigned int
 	}
 }
 
+/* Return how many data blocks there is inside the rbio. */
+static unsigned int rbio_nr_data_sectors(const struct btrfs_raid_bio *rbio)
+{
+	return rbio->nr_data * rbio->stripe_nsectors;
+}
+
 /*
  * caching an rbio means to copy anything from the
  * bio_sectors array into the stripe_pages array.  We
@@ -272,7 +278,7 @@ static void cache_rbio_pages(struct btrfs_raid_bio *rbio)
 			 * a data sector it should still be uptodate as it is
 			 * read from disk.
 			 */
-			if (i < rbio->nr_data * rbio->stripe_nsectors)
+			if (i < rbio_nr_data_sectors(rbio))
 				ASSERT(test_bit(i, rbio->stripe_uptodate_bitmap));
 			continue;
 		}
@@ -402,7 +408,7 @@ static bool is_data_stripe_page(struct btrfs_raid_bio *rbio, int page_nr)
 	 * Thus if the first sector of the page belongs to data stripes, then
 	 * the full page belongs to data stripes.
 	 */
-	return (sector_nr < rbio->nr_data * rbio->stripe_nsectors);
+	return (sector_nr < rbio_nr_data_sectors(rbio));
 }
 
 /*
@@ -1559,7 +1565,7 @@ static void set_rbio_range_error(struct btrfs_raid_bio *rbio, struct bio *bio)
 		     rbio->bioc->full_stripe_logical;
 	int total_nr_sector = offset >> fs_info->sectorsize_bits;
 
-	ASSERT(total_nr_sector < rbio->nr_data * rbio->stripe_nsectors);
+	ASSERT(total_nr_sector < rbio_nr_data_sectors(rbio));
 
 	bitmap_set(rbio->error_bitmap, total_nr_sector,
 		   bio->bi_iter.bi_size >> fs_info->sectorsize_bits);
@@ -1669,7 +1675,7 @@ static void verify_bio_data_sectors(struct btrfs_raid_bio *rbio,
 		return;
 
 	/* P/Q stripes, they have no data csum to verify against. */
-	if (total_sector_nr >= rbio->nr_data * rbio->stripe_nsectors)
+	if (total_sector_nr >= rbio_nr_data_sectors(rbio))
 		return;
 
 	for (struct bvec_iter iter = init_bvec_iter_for_bio(bio);
@@ -2303,9 +2309,9 @@ static void fill_data_csums(struct btrfs_raid_bio *rbio)
 {
 	struct btrfs_fs_info *fs_info = rbio->bioc->fs_info;
 	struct btrfs_root *csum_root;
+	const unsigned int nr_data_sectors = rbio_nr_data_sectors(rbio);
 	const u64 start = rbio->bioc->full_stripe_logical;
-	const u32 len = (rbio->nr_data * rbio->stripe_nsectors) <<
-			fs_info->sectorsize_bits;
+	const u32 len = nr_data_sectors << fs_info->sectorsize_bits;
 	int ret;
 
 	/* The rbio should not have its csum buffer initialized. */
@@ -2326,10 +2332,8 @@ static void fill_data_csums(struct btrfs_raid_bio *rbio)
 	    rbio->bioc->map_type & BTRFS_BLOCK_GROUP_METADATA)
 		return;
 
-	rbio->csum_buf = kzalloc(rbio->nr_data * rbio->stripe_nsectors *
-				 fs_info->csum_size, GFP_NOFS);
-	rbio->csum_bitmap = bitmap_zalloc(rbio->nr_data * rbio->stripe_nsectors,
-					  GFP_NOFS);
+	rbio->csum_buf = kzalloc(nr_data_sectors * fs_info->csum_size, GFP_NOFS);
+	rbio->csum_bitmap = bitmap_zalloc(nr_data_sectors, GFP_NOFS);
 	if (!rbio->csum_buf || !rbio->csum_bitmap) {
 		ret = -ENOMEM;
 		goto error;
@@ -2445,9 +2449,10 @@ static void submit_write_bios(struct btrfs_raid_bio *rbio,
  */
 static bool need_read_stripe_sectors(struct btrfs_raid_bio *rbio)
 {
+	const unsigned int nr_data_sectors = rbio_nr_data_sectors(rbio);
 	int i;
 
-	for (i = 0; i < rbio->nr_data * rbio->stripe_nsectors; i++) {
+	for (i = 0; i < nr_data_sectors; i++) {
 		phys_addr_t paddr = rbio->stripe_paddrs[i * rbio->sector_nsteps];
 
 		/*
-- 
2.56.0

[PATCH 2/3] btrfs: add stripe_len < PAGE_SIZE support for steal_rbio()

[Qu Wenruo]

The function steal_rbio() is utilized to use the pages that belongs to
uptodate data blocks, without doing a page copy.

That is done by just updating the dest->stripe_pages[] pointer to use
the point inside @src directly.

That function previously will reject stripe_len < PAGE_SIZE cases, as a
page can contain both data and P/Q blocks.

But that shouldn't be a big deal, as long as the corresponding P/Q
blocks are not marked uptodate, we can still steal the page that also
contains P/Q blocks.

So rework steal_rbio() to support stripe_len < PAGE_SIZE by:

- Calculate the last page index correctly
  Instead of iterating all pages but skip P/Q pages, calculate the last
  page index properly.

  The last page may contain both data and P/Q blocks, but we can still
  steal the page.

- Update the bitmap in one go
  Instead of updating the stripe_uptodate_bitmap for every page, we can
  do the update in just one go, out of the main loop.

  And that update should not touch the P/Q blocks.

- Remove unused helpers
  * page_nr_to_sector_nr()
  * page_nr_to_num_sectors()
  * steal_rbio_page()
  * is_data_stripe_page()

  * full_page_sectors_uptodate()
    This one is very annoying, because of its __maybe_unused
    attribution.

  They are based on the old BTRFS_STRIPE_LEN >= PAGE_SIZE assumption,
  and no longer utilized.

- Make index_stripe_sectors() update the stripe_paddrs[] without a page
  This doesn't cause any functional change for now, as there is no one
  accessing the source stripe_paddrs[] after a steal.

  But this should still make the function a little more robust.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 0c792d85f520..c81f7a0297e4 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -307,55 +307,8 @@ static int rbio_bucket(struct btrfs_raid_bio *rbio)
 	return hash_64(num >> 16, BTRFS_STRIPE_HASH_TABLE_BITS);
 }
 
-/* Get the sector number of the first sector covered by @page_nr. */
-static u32 page_nr_to_sector_nr(struct btrfs_raid_bio *rbio, unsigned int page_nr)
-{
-	u32 sector_nr;
-
-	ASSERT(page_nr < rbio->nr_pages);
-
-	sector_nr = (page_nr << PAGE_SHIFT) >> rbio->bioc->fs_info->sectorsize_bits;
-	ASSERT(sector_nr < rbio->nr_sectors);
-	return sector_nr;
-}
-
 /*
- * Get the number of sectors covered by @page_nr.
- *
- * For bs > ps cases, the result will always be 1.
- * For bs <= ps cases, the result will be ps / bs.
- */
-static u32 page_nr_to_num_sectors(struct btrfs_raid_bio *rbio, unsigned int page_nr)
-{
-	struct btrfs_fs_info *fs_info = rbio->bioc->fs_info;
-	u32 nr_sectors;
-
-	ASSERT(page_nr < rbio->nr_pages);
-
-	nr_sectors = round_up(PAGE_SIZE, fs_info->sectorsize) >> fs_info->sectorsize_bits;
-	ASSERT(nr_sectors > 0);
-	return nr_sectors;
-}
-
-static __maybe_unused bool full_page_sectors_uptodate(struct btrfs_raid_bio *rbio,
-						      unsigned int page_nr)
-{
-	const u32 sector_nr = page_nr_to_sector_nr(rbio, page_nr);
-	const u32 nr_bits = page_nr_to_num_sectors(rbio, page_nr);
-	int i;
-
-	ASSERT(page_nr < rbio->nr_pages);
-	ASSERT(sector_nr + nr_bits < rbio->nr_sectors);
-
-	for (i = sector_nr; i < sector_nr + nr_bits; i++) {
-		if (!test_bit(i, rbio->stripe_uptodate_bitmap))
-			return false;
-	}
-	return true;
-}
-
-/*
- * Update the stripe_sectors[] array to use correct page and pgoff
+ * Update the stripe_paddrs[] array to use correct paddr.
  *
  * Should be called every time any page pointer in stripes_pages[] got modified.
  */
@@ -370,82 +323,48 @@ static void index_stripe_sectors(struct btrfs_raid_bio *rbio)
 		int page_index = offset >> PAGE_SHIFT;
 
 		ASSERT(page_index < rbio->nr_pages);
-		if (!rbio->stripe_pages[page_index])
+		if (!rbio->stripe_pages[page_index]) {
+			rbio->stripe_paddrs[i] = INVALID_PADDR;
 			continue;
+		}
 
 		rbio->stripe_paddrs[i] = page_to_phys(rbio->stripe_pages[page_index]) +
 					 offset_in_page(offset);
 	}
 }
 
-static void steal_rbio_page(struct btrfs_raid_bio *src,
-			    struct btrfs_raid_bio *dest, int page_nr)
-{
-	const u32 sector_nr = page_nr_to_sector_nr(src, page_nr);
-	const u32 nr_bits = page_nr_to_num_sectors(src, page_nr);
-
-	ASSERT(page_nr < src->nr_pages);
-	ASSERT(sector_nr + nr_bits < src->nr_sectors);
-
-	if (dest->stripe_pages[page_nr])
-		__free_page(dest->stripe_pages[page_nr]);
-	dest->stripe_pages[page_nr] = src->stripe_pages[page_nr];
-	src->stripe_pages[page_nr] = NULL;
-
-	/* Also update the stripe_uptodate_bitmap bits. */
-	bitmap_set(dest->stripe_uptodate_bitmap, sector_nr, nr_bits);
-}
-
-static bool is_data_stripe_page(struct btrfs_raid_bio *rbio, int page_nr)
-{
-	const int sector_nr = page_nr_to_sector_nr(rbio, page_nr);
-
-	/*
-	 * The caller has ensured stripe_len >= PAGE_SIZE, and PAGE_SIZE is
-	 * aligned with sectorsize, thus we won't have a page which is half
-	 * data half parity.
-	 *
-	 * Thus if the first sector of the page belongs to data stripes, then
-	 * the full page belongs to data stripes.
-	 */
-	return (sector_nr < rbio_nr_data_sectors(rbio));
-}
-
 /*
- * Stealing an rbio means taking all the uptodate pages from the stripe array
- * in the source rbio and putting them into the destination rbio.
+ * Stealing an rbio means taking all pages containing data blocks from the
+ * stripe array in the source rbio and putting them into the destination rbio.
  *
- * This will also update the involved stripe_sectors[] which are referring to
+ * This will also update the involved stripe_paddrs[] which are referring to
  * the old pages.
  */
 static void steal_rbio(struct btrfs_raid_bio *src, struct btrfs_raid_bio *dest)
 {
-	int i;
+	const u32 nr_data_blocks = rbio_nr_data_sectors(src);
+	const u32 last_data_pos = nr_data_blocks << src->bioc->fs_info->sectorsize_bits;
+	const u32 last_pindex = round_up(last_data_pos, PAGE_SIZE) >> PAGE_SHIFT;
 
 	if (!test_bit(RBIO_CACHE_READY_BIT, &src->flags))
 		return;
-	/* Data and P/Q can be mixed into a page, skip stealing. */
-	if (src->bioc->stripe_len < PAGE_SIZE)
-		return;
 
-	for (i = 0; i < dest->nr_pages; i++) {
-		struct page *p = src->stripe_pages[i];
+	/* All data blocks in the src should be uptodate. */
+	ASSERT(bitmap_full(src->stripe_uptodate_bitmap, nr_data_blocks));
 
-		/*
-		 * We don't need to steal P/Q pages as they will always be
-		 * regenerated for RMW or full write anyway.
-		 */
-		if (!is_data_stripe_page(src, i))
-			continue;
+	/* Steal all pages that contains any data block. */
+	for (int i = 0; i < last_pindex; i++) {
+		ASSERT(src->stripe_pages[i]);
 
-		/*
-		 * If @src already has RBIO_CACHE_READY_BIT, it should have
-		 * all data stripe pages present and uptodate.
-		 */
-		ASSERT(p);
-		ASSERT(full_page_sectors_uptodate(src, i));
-		steal_rbio_page(src, dest, i);
+		if (dest->stripe_pages[i])
+			__free_page(dest->stripe_pages[i]);
+		dest->stripe_pages[i] = src->stripe_pages[i];
+		src->stripe_pages[i] = NULL;
 	}
+
+	/* Only data blocks should be set uptodate. */
+	bitmap_set(dest->stripe_uptodate_bitmap, 0, nr_data_blocks);
+
 	index_stripe_sectors(dest);
 	index_stripe_sectors(src);
 }
-- 
2.56.0

[PATCH 3/3] btrfs: add stripe_len < PAGE_SIZE support for alloc_rbio_*_pages()

[Qu Wenruo]

The functions alloc_rbio_parity_pages() and alloc_rbio_data_pages()
didn't support stripe_len < PAGE_SIZE cases, and fallback to
alloc_rbio_pages() instead.

That's mostly to avoid the case where both data and P/Q blocks can exist
in the same page.

However even in that case, it's not a big deal, the existing
infrastructures like index_stripe_sectors() can handle it correctly.

Update those functions to support stripe_len < PAGE_SIZE cases, with two
points to note:

- Extra atttention needed for the round direction
  For alloc_rbio_data_pages() we need to round up, so the last data page
  gets allocated.

  For alloc_rbio_parity_pages() we need to round down, so the first
  P/Q page get allocated.

- Mixed page allocation
  We can call alloc_rbio_parity_pages() first, then
  alloc_rbio_data_pages().

  In that case, it's alloc_pages_bulk() saving us, which always skips
  allocated slot. Thus we're still safe.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index c81f7a0297e4..7dd937fa0c26 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1061,22 +1061,21 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio)
 	return 0;
 }
 
-/* only allocate pages for p/q stripes */
+/*
+ * Allocate pages that cover P/Q blocks.
+ *
+ * For stripe_len < PAGE_SIZE case, we can allocate a page that cover both
+ * data and P/Q blocks.
+ */
 static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio)
 {
-	int data_pages;
+	const unsigned int first_pq_page = round_down(rbio->nr_data *
+					rbio->bioc->stripe_len, PAGE_SIZE) >>
+					PAGE_SHIFT;
 	int ret;
 
-	/*
-	 * Page size larger than stripe size, we can not only allocate
-	 * pages for P/Q stripes.
-	 */
-	if (rbio->bioc->stripe_len < PAGE_SIZE)
-		return alloc_rbio_pages(rbio);
-
-	data_pages = rbio->nr_data * (rbio->bioc->stripe_len >> PAGE_SHIFT);
-	ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages,
-				     rbio->stripe_pages + data_pages, GFP_NOFS);
+	ret = btrfs_alloc_page_array(rbio->nr_pages - first_pq_page,
+				     rbio->stripe_pages + first_pq_page, GFP_NOFS);
 	if (ret < 0)
 		return ret;
 
@@ -1653,13 +1652,10 @@ static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
 
 static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio)
 {
-	int data_pages;
+	const unsigned int data_pages = round_up(rbio->nr_data *
+			rbio->bioc->stripe_len, PAGE_SIZE) >> PAGE_SHIFT;
 	int ret;
 
-	if (rbio->bioc->stripe_len < PAGE_SIZE)
-		return alloc_rbio_pages(rbio);
-
-	data_pages = rbio->nr_data * (rbio->bioc->stripe_len >> PAGE_SHIFT);
 	ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, GFP_NOFS);
 	if (ret < 0)
 		return ret;
-- 
2.56.0