[PATCH v2] btrfs: fix a regression where PAGECACHE_TAG_DIRTY is never cleared

[BUG]
The following script (already submitted as generic/798) will report
incorrect dirty page numbers, with 64K page size systems and 4K fs block
size:

 # mkfs.btrfs -s 4k -f $dev
 # mount $dev $mnt
 # xfs_io -f -c "pwrite 0 64K" -c fsync -c "cachestat 0 64K" $mnt/foobar
 Cached: 1, Dirty: 1, Writeback: 0, Evicted: 0, Recently Evicted: 0

Note that the dirtied page number is still 1.

[CAUSE]
The cachestat() goes through the XArray of the page cache, but
instead of checking each folio's flag, it uses the
PAGECACHE_TAG_DIRTY tag to report dirty pages.

Since commit 095be159f3eb ("btrfs: unify folio dirty flag clearing"),
btrfs replaced a folio_clear_dirty_for_io() call inside
extent_write_cache_pages() with folio_test_dirty().

This will cause the following call sequence for the folio at file offset
0:

 extent_write_cache_pages()
 |- folio_test_dirty()
 |  The folio is still dirty, continue to writeback.
 |
 |- extent_writepage()
    |- extent_writepage_io()
       |- submit_one_sector() for range [0, 4K)
       |  |- btrfs_folio_clear_dirty()
       |  |- btrfs_folio_set_writeback()
       |     |- folio_start_writeback()
       |        It's the first writeback block, we set the writeback
       |	flag for the folio.
       |	But the folio is still dirty, PAGECACHE_TAG_DIRTY is
       |	kept
       |
       |- submit_one_sector() for range [4K, 8K)
       |  |- btrfs_folio_clear_dirty()
       |  |- btrfs_folio_set_writeback()
       |     The folio already has writeback flag, no need to call
       |     folio_start_writeback()
       |
       | ...
       |- submit_one_sector() for range [60K, 64K)
	  |- btrfs_folio_clear_dirty()
	  |- btrfs_folio_set_writeback()
             The folio already has writeback flag, no need to call
             folio_start_writeback()

So the PAGECACHE_TAG_DIRTY is never cleared.

Meanwhile for the old code, before that commit, the sequence looks
like:

 extent_write_cache_pages()
 |- folio_clear_dirty_for_io()
 |  The folio is still dirty, so continue to writeback.
 |  But the folio dirty flag is cleared now.
 |
 |- extent_writepage()
    |- extent_writepage_io()
       |- submit_one_sector() for range [0, 4K)
       |  |- btrfs_folio_clear_dirty()
       |  |- btrfs_folio_set_writeback()
       |     |- folio_start_writeback()
       |        |- xas_clear(PAGECACHE_TAG)
       |
       |        It's the first writeback block, we set the writeback
       |	flag for the folio.
       |	And the folio is not dirty, PAGECACHE_TAG_DIRTY is
       |        cleared
       |
       |- submit_one_sector() for range [4K, 8K)
       |  |- btrfs_folio_clear_dirty()
       |  |- btrfs_folio_set_writeback()
       |     The folio already has writeback flag, no need to call
       |     folio_start_writeback()
       |
       | ...
       |- submit_one_sector() for range [60K, 64K)
	  |- btrfs_folio_clear_dirty()
	  |- btrfs_folio_set_writeback()
             The folio already has writeback flag, no need to call
             folio_start_writeback()

Unlike the new code, old code will clear PAGECACHE_TAG_DIRTY for the
first writeback block.

There is a deeper problem, dirty and writeback folio flags are updated
at very different timing.
The dirty flag is only cleared when the last sub-folio block has dirty
flag cleared.
But the writeback flag is set when the first block starts writeback, and
later blocks that go through writeback will not call
folio_start_writeback() again.

If we rely on folio_start_writeback() to update the
PAGECACHE_TAG_DIRTY and PAGECACHE_TAG_TOWRITE, it will always be
incorrect in one way or another.

[FIX]
Do not let folio_start_writeback() do any PAGECACHE_TAG_TOWRITE
handling.

Instead, manually clear both PAGECACHE_TAG_TOWRITE and
PAGECACHE_TAG_DIRTY flags when the folio is no longer dirty during
btrfs_subpage_set_writeback().

However this is only a hot-fix, for the long term solution we will
follow iomap, by calling folio_start_writeback() immediately for the
whole folio, and folio_end_writeback() after all writeback finished
for the folio.

Fixes: 095be159f3eb ("btrfs: unify folio dirty flag clearing")
Signed-off-by: Qu Wenruo <[email protected]>
---
Changelog:
v2:
- Completely decouple the PAGECACHE_TAG_* handling from
  folio_start_writeback()
  Since folio dirty and writeback flags are only updated once, but at
  very different timings (the last dirty block cleared vs the first
  writeback block started), we have to manually handle the
  PAGECACHE_TAG_* update by ourselves.

- Add a mention about the proper fix using the same solution
  as iomap
---
 fs/btrfs/subpage.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 56060acac2e9..2a9397be8116 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -359,6 +359,23 @@ void btrfs_subpage_set_dirty(const struct btrfs_fs_info *fs_info,
 	folio_mark_dirty(folio);
 }
 
+static void folio_clear_tags(struct folio *folio)
+{
+	struct address_space *mapping = folio_mapping(folio);
+	XA_STATE(xas, &mapping->i_pages, folio->index);
+	unsigned long flags;
+
+	ASSERT(folio_test_locked(folio));
+	ASSERT(mapping);
+	ASSERT(mapping_use_writeback_tags(mapping));
+
+	xas_lock_irqsave(&xas, flags);
+	xas_load(&xas);
+	xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
+	xas_clear_mark(&xas, PAGECACHE_TAG_TOWRITE);
+	xas_unlock_irqrestore(&xas, flags);
+}
+
 /*
  * Extra clear_and_test function for subpage dirty bitmap.
  *
@@ -403,7 +420,6 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info,
 	unsigned int start_bit = subpage_calc_start_bit(fs_info, folio,
 							writeback, start, len);
 	unsigned long flags;
-	bool keep_write;
 
 	spin_lock_irqsave(&bfs->lock, flags);
 	bitmap_set(bfs->bitmaps, start_bit, len >> fs_info->sectorsize_bits);
@@ -413,10 +429,14 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info,
 	 * folio. Doing so can cause WB_SYNC_ALL writepages() to overlook it,
 	 * assume writeback is complete, and exit too early — violating sync
 	 * ordering guarantees.
+	 *
+	 * Instead we manually clear the DIRTY and TOWRITE tags after the folio
+	 * is no longer dirty.
 	 */
-	keep_write = folio_test_dirty(folio);
 	if (!folio_test_writeback(folio))
-		__folio_start_writeback(folio, keep_write);
+		__folio_start_writeback(folio, true);
+	if (!folio_test_dirty(folio))
+		folio_clear_tags(folio);
 	spin_unlock_irqrestore(&bfs->lock, flags);
 }
 
-- 
2.54.0