[PATCH v2 0/3] btrfs: make subpage reader/writer counter to be sector aware

[PATCH v2 0/3] btrfs: make subpage reader/writer counter to be sector aware

[Qu Wenruo]

[CHANGELOG]
v2:
- Add btrfs_subpage_dump_bitmap() support for locked bitmap

- Add spinlock to protect the bitmap and locked bitmap operation
  In theory, this opeartion should always be single threaded, since the
  page is locked.
  But to keep the behavior consistent, use spin lock to protect bitmap
  and atomic reader/write updates.

This can be fetched from github, and the branch would be utilized for
all newer subpage delalloc update to support full sector sized
compression and zoned:
https://github.com/adam900710/linux/tree/subpage_delalloc

Currently we just trace subpage reader/writer counter using an atomic.

It's fine for the current subpage usage, but for the future, we want to
be aware of which subpage sector is locked inside a page, for proper
compression (we only support full page compression for now) and zoned support.

So here we introduce a new bitmap, called locked bitmap, to trace which
sector is locked for read/write.

And since reader/writer are both exclusive (to each other and to the same
type of lock), we can safely use the same bitmap for both reader and
writer.

In theory we can use the bitmap (the weight of the locked bitmap) to
indicate how many bytes are under reader/write lock, but it's not
possible yet:

- No weight support for bitmap range
  The bitmap API only provides bitmap_weight(), which always starts at
  bit 0.

- Need to distinguish read/write lock

Thus we still keep the reader/writer atomic counter.

Qu Wenruo (3):
  btrfs: unexport btrfs_subpage_start_writer() and
    btrfs_subpage_end_and_test_writer()
  btrfs: subpage: make reader lock to utilize bitmap
  btrfs: subpage: make writer lock to utilize bitmap

 fs/btrfs/subpage.c | 77 ++++++++++++++++++++++++++++++++++++----------
 fs/btrfs/subpage.h | 16 +++++++---
 2 files changed, 72 insertions(+), 21 deletions(-)

-- 
2.43.1

[PATCH v2 1/3] btrfs: unexport btrfs_subpage_start_writer() and btrfs_subpage_end_and_test_writer()

[Qu Wenruo]

Both functions are introduced in commit 1e1de38792e0 ("btrfs: make
process_one_page() to handle subpage locking"), but they are never
utilized out of subpage code.

So just unexport them.

Fixes: 1e1de38792e0 ("btrfs: make process_one_page() to handle subpage locking")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/subpage.c | 8 ++++----
 fs/btrfs/subpage.h | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 0e49dab8dad2..24f8be565a61 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -290,8 +290,8 @@ static void btrfs_subpage_clamp_range(struct folio *folio, u64 *start, u32 *len)
 			     orig_start + orig_len) - *start;
 }
 
-void btrfs_subpage_start_writer(const struct btrfs_fs_info *fs_info,
-				struct folio *folio, u64 start, u32 len)
+static void btrfs_subpage_start_writer(const struct btrfs_fs_info *fs_info,
+				       struct folio *folio, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = folio_get_private(folio);
 	const int nbits = (len >> fs_info->sectorsize_bits);
@@ -304,8 +304,8 @@ void btrfs_subpage_start_writer(const struct btrfs_fs_info *fs_info,
 	ASSERT(ret == nbits);
 }
 
-bool btrfs_subpage_end_and_test_writer(const struct btrfs_fs_info *fs_info,
-				       struct folio *folio, u64 start, u32 len)
+static bool btrfs_subpage_end_and_test_writer(const struct btrfs_fs_info *fs_info,
+					      struct folio *folio, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = folio_get_private(folio);
 	const int nbits = (len >> fs_info->sectorsize_bits);
diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
index 55fc42db707e..97ba2c100b0b 100644
--- a/fs/btrfs/subpage.h
+++ b/fs/btrfs/subpage.h
@@ -98,10 +98,6 @@ void btrfs_subpage_start_reader(const struct btrfs_fs_info *fs_info,
 void btrfs_subpage_end_reader(const struct btrfs_fs_info *fs_info,
 			      struct folio *folio, u64 start, u32 len);
 
-void btrfs_subpage_start_writer(const struct btrfs_fs_info *fs_info,
-				struct folio *folio, u64 start, u32 len);
-bool btrfs_subpage_end_and_test_writer(const struct btrfs_fs_info *fs_info,
-				       struct folio *folio, u64 start, u32 len);
 int btrfs_folio_start_writer_lock(const struct btrfs_fs_info *fs_info,
 				  struct folio *folio, u64 start, u32 len);
 void btrfs_folio_end_writer_lock(const struct btrfs_fs_info *fs_info,
-- 
2.43.1

[PATCH v2 2/3] btrfs: subpage: make reader lock to utilize bitmap

[Qu Wenruo]

Currently btrfs_subpage utilized its atomic member @reader to manage the
reader counter.

However the reader counter is only utilized to prevent the page got
released/unlocked when we still have reads underway.

In that use case, we don't really allow multiple readers on the same
subpage sector.

So here we can introduce a new locked bitmap to represent exactly which
subpage range is locked for read.

In theory we can remove btrfs_subpage::reader as it's just the set bits
of the new locked bitmap.
But unfortunately bitmap doesn't provide such handy API yet, so we still
keep the reader counter.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/subpage.c | 46 ++++++++++++++++++++++++++++++++++++----------
 fs/btrfs/subpage.h | 12 +++++++++++-
 2 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 24f8be565a61..715b6df90117 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -111,6 +111,9 @@ void btrfs_init_subpage_info(struct btrfs_subpage_info *subpage_info, u32 sector
 	subpage_info->checked_offset = cur;
 	cur += nr_bits;
 
+	subpage_info->locked_offset = cur;
+	cur += nr_bits;
+
 	subpage_info->total_nr_bits = cur;
 }
 
@@ -237,28 +240,59 @@ static void btrfs_subpage_assert(const struct btrfs_fs_info *fs_info,
 		       start + len <= folio_pos(folio) + PAGE_SIZE);
 }
 
+#define subpage_calc_start_bit(fs_info, folio, name, start, len)	\
+({									\
+	unsigned int start_bit;						\
+									\
+	btrfs_subpage_assert(fs_info, folio, start, len);		\
+	start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
+	start_bit += fs_info->subpage_info->name##_offset;		\
+	start_bit;							\
+})
+
 void btrfs_subpage_start_reader(const struct btrfs_fs_info *fs_info,
 				struct folio *folio, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = folio_get_private(folio);
+	const int start_bit = subpage_calc_start_bit(fs_info, folio, locked, start, len);
 	const int nbits = len >> fs_info->sectorsize_bits;
+	unsigned long flags;
+
 
 	btrfs_subpage_assert(fs_info, folio, start, len);
 
+	spin_lock_irqsave(&subpage->lock, flags);
+	/*
+	 * Even it's just for reading the page, no one should have locked the subpage
+	 * range.
+	 */
+	ASSERT(bitmap_test_range_all_zero(subpage->bitmaps, start_bit, nbits));
+	bitmap_set(subpage->bitmaps, start_bit, nbits);
 	atomic_add(nbits, &subpage->readers);
+	spin_unlock_irqrestore(&subpage->lock, flags);
 }
 
 void btrfs_subpage_end_reader(const struct btrfs_fs_info *fs_info,
 			      struct folio *folio, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = folio_get_private(folio);
+	const int start_bit = subpage_calc_start_bit(fs_info, folio, locked,
+						     start, len);
 	const int nbits = len >> fs_info->sectorsize_bits;
+	unsigned long flags;
 	bool is_data;
 	bool last;
 
 	btrfs_subpage_assert(fs_info, folio, start, len);
 	is_data = is_data_inode(folio->mapping->host);
+
+	spin_lock_irqsave(&subpage->lock, flags);
+
+	/* The range should have already be locked. */
+	ASSERT(bitmap_test_range_all_set(subpage->bitmaps, start_bit, nbits));
 	ASSERT(atomic_read(&subpage->readers) >= nbits);
+
+	bitmap_clear(subpage->bitmaps, start_bit, nbits);
 	last = atomic_sub_and_test(nbits, &subpage->readers);
 
 	/*
@@ -270,6 +304,7 @@ void btrfs_subpage_end_reader(const struct btrfs_fs_info *fs_info,
 	 */
 	if (is_data && last)
 		folio_unlock(folio);
+	spin_unlock_irqrestore(&subpage->lock, flags);
 }
 
 static void btrfs_subpage_clamp_range(struct folio *folio, u64 *start, u32 *len)
@@ -365,16 +400,6 @@ void btrfs_folio_end_writer_lock(const struct btrfs_fs_info *fs_info,
 		folio_unlock(folio);
 }
 
-#define subpage_calc_start_bit(fs_info, folio, name, start, len)	\
-({									\
-	unsigned int start_bit;						\
-									\
-	btrfs_subpage_assert(fs_info, folio, start, len);		\
-	start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
-	start_bit += fs_info->subpage_info->name##_offset;		\
-	start_bit;							\
-})
-
 #define subpage_test_bitmap_all_set(fs_info, subpage, name)		\
 	bitmap_test_range_all_set(subpage->bitmaps,			\
 			fs_info->subpage_info->name##_offset,		\
@@ -751,6 +776,7 @@ void __cold btrfs_subpage_dump_bitmap(const struct btrfs_fs_info *fs_info,
 	GET_SUBPAGE_BITMAP(subpage, subpage_info, writeback, &writeback_bitmap);
 	GET_SUBPAGE_BITMAP(subpage, subpage_info, ordered, &ordered_bitmap);
 	GET_SUBPAGE_BITMAP(subpage, subpage_info, checked, &checked_bitmap);
+	GET_SUBPAGE_BITMAP(subpage, subpage_info, locked, &checked_bitmap);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 
 	dump_page(folio_page(folio, 0), "btrfs subpage dump");
diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
index 97ba2c100b0b..b6dc013b0fdc 100644
--- a/fs/btrfs/subpage.h
+++ b/fs/btrfs/subpage.h
@@ -33,7 +33,7 @@ struct btrfs_subpage_info {
 	unsigned int total_nr_bits;
 
 	/*
-	 * *_start indicates where the bitmap starts, the length is always
+	 * *_offset indicates where the bitmap starts, the length is always
 	 * @bitmap_size, which is calculated from PAGE_SIZE / sectorsize.
 	 */
 	unsigned int uptodate_offset;
@@ -41,6 +41,16 @@ struct btrfs_subpage_info {
 	unsigned int writeback_offset;
 	unsigned int ordered_offset;
 	unsigned int checked_offset;
+
+	/*
+	 * For locked bitmaps, normally it's subpage representation for folio
+	 * Locked flag, but metadata is different:
+	 *
+	 * - Metadata doesn't really lock the folio
+	 *   It's just to prevent page::private get cleared before the last
+	 *   end_page_read().
+	 */
+	unsigned int locked_offset;
 };
 
 /*
-- 
2.43.1

[PATCH v2 3/3] btrfs: subpage: make writer lock to utilize bitmap

[Qu Wenruo]

For the writer counter, it's pretty much the same as the reader counter,
and they are exclusive.

So it's pretty reasonable to move them to the new locked bitmap.

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

diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 715b6df90117..3b3ef8bffe05 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -329,24 +329,36 @@ static void btrfs_subpage_start_writer(const struct btrfs_fs_info *fs_info,
 				       struct folio *folio, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = folio_get_private(folio);
+	const int start_bit = subpage_calc_start_bit(fs_info, folio, locked,
+						     start, len);
 	const int nbits = (len >> fs_info->sectorsize_bits);
+	unsigned long flags;
 	int ret;
 
 	btrfs_subpage_assert(fs_info, folio, start, len);
 
+	spin_lock_irqsave(&subpage->lock, flags);
 	ASSERT(atomic_read(&subpage->readers) == 0);
+	ASSERT(bitmap_test_range_all_zero(subpage->bitmaps, start_bit, nbits));
+	bitmap_set(subpage->bitmaps, start_bit, nbits);
 	ret = atomic_add_return(nbits, &subpage->writers);
 	ASSERT(ret == nbits);
+	spin_unlock_irqrestore(&subpage->lock, flags);
 }
 
 static bool btrfs_subpage_end_and_test_writer(const struct btrfs_fs_info *fs_info,
 					      struct folio *folio, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = folio_get_private(folio);
+	const int start_bit = subpage_calc_start_bit(fs_info, folio, locked,
+						     start, len);
 	const int nbits = (len >> fs_info->sectorsize_bits);
+	unsigned long flags;
+	bool last;
 
 	btrfs_subpage_assert(fs_info, folio, start, len);
 
+	spin_lock_irqsave(&subpage->lock, flags);
 	/*
 	 * We have call sites passing @lock_page into
 	 * extent_clear_unlock_delalloc() for compression path.
@@ -354,11 +366,18 @@ static bool btrfs_subpage_end_and_test_writer(const struct btrfs_fs_info *fs_inf
 	 * This @locked_page is locked by plain lock_page(), thus its
 	 * subpage::writers is 0.  Handle them in a special way.
 	 */
-	if (atomic_read(&subpage->writers) == 0)
+	if (atomic_read(&subpage->writers) == 0) {
+		spin_unlock_irqrestore(&subpage->lock, flags);
 		return true;
+	}
 
 	ASSERT(atomic_read(&subpage->writers) >= nbits);
-	return atomic_sub_and_test(nbits, &subpage->writers);
+	/* The target range should have been locked. */
+	ASSERT(bitmap_test_range_all_set(subpage->bitmaps, start_bit, nbits));
+	bitmap_clear(subpage->bitmaps, start_bit, nbits);
+	last = atomic_sub_and_test(nbits, &subpage->writers);
+	spin_unlock_irqrestore(&subpage->lock, flags);
+	return last;
 }
 
 /*
-- 
2.43.1

Re: [PATCH v2 0/3] btrfs: make subpage reader/writer counter to be sector aware

[David Sterba]

On Sat, Feb 17, 2024 at 04:59:47PM +1030, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Add btrfs_subpage_dump_bitmap() support for locked bitmap
> 
> - Add spinlock to protect the bitmap and locked bitmap operation
>   In theory, this opeartion should always be single threaded, since the
>   page is locked.
>   But to keep the behavior consistent, use spin lock to protect bitmap
>   and atomic reader/write updates.
> 
> This can be fetched from github, and the branch would be utilized for
> all newer subpage delalloc update to support full sector sized
> compression and zoned:
> https://github.com/adam900710/linux/tree/subpage_delalloc
> 
> Currently we just trace subpage reader/writer counter using an atomic.
> 
> It's fine for the current subpage usage, but for the future, we want to
> be aware of which subpage sector is locked inside a page, for proper
> compression (we only support full page compression for now) and zoned support.
> 
> So here we introduce a new bitmap, called locked bitmap, to trace which
> sector is locked for read/write.
> 
> And since reader/writer are both exclusive (to each other and to the same
> type of lock), we can safely use the same bitmap for both reader and
> writer.
> 
> In theory we can use the bitmap (the weight of the locked bitmap) to
> indicate how many bytes are under reader/write lock, but it's not
> possible yet:
> 
> - No weight support for bitmap range
>   The bitmap API only provides bitmap_weight(), which always starts at
>   bit 0.
> 
> - Need to distinguish read/write lock
> 
> Thus we still keep the reader/writer atomic counter.
> 
> Qu Wenruo (3):
>   btrfs: unexport btrfs_subpage_start_writer() and
>     btrfs_subpage_end_and_test_writer()
>   btrfs: subpage: make reader lock to utilize bitmap
>   btrfs: subpage: make writer lock to utilize bitmap

So this is preparatory work and looks safe to me, only adding the
bitmaps to counters, you can add it to for-next.

Re: [PATCH v2 0/3] btrfs: make subpage reader/writer counter to be sector aware

[Qu Wenruo]

在 2024/2/22 22:24, David Sterba 写道:
> On Sat, Feb 17, 2024 at 04:59:47PM +1030, Qu Wenruo wrote:
>> [CHANGELOG]
>> v2:
>> - Add btrfs_subpage_dump_bitmap() support for locked bitmap
>>
>> - Add spinlock to protect the bitmap and locked bitmap operation
>>    In theory, this opeartion should always be single threaded, since the
>>    page is locked.
>>    But to keep the behavior consistent, use spin lock to protect bitmap
>>    and atomic reader/write updates.
>>
>> This can be fetched from github, and the branch would be utilized for
>> all newer subpage delalloc update to support full sector sized
>> compression and zoned:
>> https://github.com/adam900710/linux/tree/subpage_delalloc
>>
>> Currently we just trace subpage reader/writer counter using an atomic.
>>
>> It's fine for the current subpage usage, but for the future, we want to
>> be aware of which subpage sector is locked inside a page, for proper
>> compression (we only support full page compression for now) and zoned support.
>>
>> So here we introduce a new bitmap, called locked bitmap, to trace which
>> sector is locked for read/write.
>>
>> And since reader/writer are both exclusive (to each other and to the same
>> type of lock), we can safely use the same bitmap for both reader and
>> writer.
>>
>> In theory we can use the bitmap (the weight of the locked bitmap) to
>> indicate how many bytes are under reader/write lock, but it's not
>> possible yet:
>>
>> - No weight support for bitmap range
>>    The bitmap API only provides bitmap_weight(), which always starts at
>>    bit 0.
>>
>> - Need to distinguish read/write lock
>>
>> Thus we still keep the reader/writer atomic counter.
>>
>> Qu Wenruo (3):
>>    btrfs: unexport btrfs_subpage_start_writer() and
>>      btrfs_subpage_end_and_test_writer()
>>    btrfs: subpage: make reader lock to utilize bitmap
>>    btrfs: subpage: make writer lock to utilize bitmap
> 
> So this is preparatory work and looks safe to me, only adding the
> bitmaps to counters, you can add it to for-next.

Got it.

BTW, this series is only stable enough after testing the whole series 
(so that no new extra changes required by later subpage delalloc behavior),
which ran fine locally at least.