btrfs: fix a compiling error if DEBUG is defined

Commit Message

Qu Wenruo Aug. 22, 2023, 5:50 a.m. UTC

[BUG]
After commit 72a69cd03082 ("btrfs: subpage: pack all subpage bitmaps
into a larger bitmap"), the DEBUG section of btree_dirty_folio() would
no longer compile.

[CAUSE]
If DEBUG is defined, we would do extra checks for btree_dirty_folio(),
mostly to make sure the range we marked dirty has an extent buffer and
that extent buffer is dirty.

For subpage, we need to iterate through all the extent buffers covered
by that page range, and make sure they all matches the criteria.

However commit 72a69cd03082 ("btrfs: subpage: pack all subpage bitmaps
into a larger bitmap") changes how we store the bitmap, we pack all the
16 bits bitmaps into a larger bitmap, which would save some space.

This means we no longer have btrfs_subpage::dirty_bitmap, instead the
dirty bitmap is starting at btrfs_subpage_info::dirty_offset, and has a
length of btrfs_subpage_info::bitmap_nr_bits.

[FIX]
Although I'm not sure if it still makes sense to maintain such code, at
least let it compile.

This patch would let us test the bits one by one through the bitmaps.

Signed-off-by: Qu Wenruo <[e-mail]>
---
 fs/btrfs/disk-io.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

David Sterba Sept. 5, 2023, 11:22 a.m. UTC | #1

On Tue, Aug 22, 2023 at 01:50:51PM +0800, Qu Wenruo wrote:
> [BUG]
> After commit 72a69cd03082 ("btrfs: subpage: pack all subpage bitmaps
> into a larger bitmap"), the DEBUG section of btree_dirty_folio() would
> no longer compile.
> 
> [CAUSE]
> If DEBUG is defined, we would do extra checks for btree_dirty_folio(),
> mostly to make sure the range we marked dirty has an extent buffer and
> that extent buffer is dirty.
> 
> For subpage, we need to iterate through all the extent buffers covered
> by that page range, and make sure they all matches the criteria.
> 
> However commit 72a69cd03082 ("btrfs: subpage: pack all subpage bitmaps
> into a larger bitmap") changes how we store the bitmap, we pack all the
> 16 bits bitmaps into a larger bitmap, which would save some space.
> 
> This means we no longer have btrfs_subpage::dirty_bitmap, instead the
> dirty bitmap is starting at btrfs_subpage_info::dirty_offset, and has a
> length of btrfs_subpage_info::bitmap_nr_bits.
> 
> [FIX]
> Although I'm not sure if it still makes sense to maintain such code, at
> least let it compile.

I'm not sure either, making it compile again makes sense. I vaguely
remember that the DEBUG macro is set by some option externally and there
are many ifdefs in other code so I assume it's still a valid feature.
There's a reference to -DDEBUG in
Documentation/admin-guide/dynamic-debug-howto.rst .

> This patch would let us test the bits one by one through the bitmaps.
> 
> Signed-off-by: Qu Wenruo <[e-mail]>

Added to misc-next, thanks.

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0a96ea8c1d3a..6599c5a206e9 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -520,6 +520,7 @@  static bool btree_dirty_folio(struct address_space *mapping,
 		struct folio *folio)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(mapping->host->i_sb);
+	struct btrfs_subpage_info *spi = fs_info->subpage_info;
 	struct btrfs_subpage *subpage;
 	struct extent_buffer *eb;
 	int cur_bit = 0;
@@ -533,18 +534,18 @@  static bool btree_dirty_folio(struct address_space *mapping,
 		btrfs_assert_tree_write_locked(eb);
 		return filemap_dirty_folio(mapping, folio);
 	}
+
+	ASSERT(spi);
 	subpage = folio_get_private(folio);
 
-	ASSERT(subpage->dirty_bitmap);
-	while (cur_bit < BTRFS_SUBPAGE_BITMAP_SIZE) {
+	for (cur_bit = spi->dirty_offset;
+	     cur_bit < spi->dirty_offset + spi->bitmap_nr_bits; cur_bit++) {
 		unsigned long flags;
 		u64 cur;
-		u16 tmp = (1 << cur_bit);
 
 		spin_lock_irqsave(&subpage->lock, flags);
-		if (!(tmp & subpage->dirty_bitmap)) {
+		if (!test_bit(cur_bit, subpage->bitmaps)) {
 			spin_unlock_irqrestore(&subpage->lock, flags);
-			cur_bit++;
 			continue;
 		}
 		spin_unlock_irqrestore(&subpage->lock, flags);
@@ -557,7 +558,7 @@  static bool btree_dirty_folio(struct address_space *mapping,
 		btrfs_assert_tree_write_locked(eb);
 		free_extent_buffer(eb);
 
-		cur_bit += (fs_info->nodesize >> fs_info->sectorsize_bits);
+		cur_bit += (fs_info->nodesize >> fs_info->sectorsize_bits) - 1;
 	}
 	return filemap_dirty_folio(mapping, folio);
 }