[PATCH] 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() go 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.

[FIX]
Revert that folio_test_dirty() back to folio_clear_dirty_for_io(), and
add a comment explaining why we need to use folio_clear_dirty_for_io().

Fixes: 095be159f3eb ("btrfs: unify folio dirty flag clearing")
Signed-off-by: Qu Wenruo <[email protected]>
---
 fs/btrfs/extent_io.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8fbb798767ca..78bd4ec541f8 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2584,8 +2584,31 @@ static int extent_write_cache_pages(struct address_space *mapping,
 				folio_wait_writeback(folio);
 			}
 
+			/*
+			 * For folios that are under writeback or no longer dirty,
+			 * we can skip the folio.
+			 *
+			 * Here we have to call folio_clear_dirty_for_io() to clear
+			 * the folio dirty flag.
+			 *
+			 * Without that call, btrfs_folio_set_writeback() will call
+			 * folio_start_writeback() for the first block to be written back.
+			 * But since the folio can still be dirty, e.g. other
+			 * blocks inside the folio are still dirty,
+			 * PAGECACHE_TAG_DIRTY will not be cleared.
+			 *
+			 * On the other hand, for btrfs_folio_set_writeback() of the
+			 * last block, although the folio is no longer dirty,
+			 * btrfs_folio_set_writeback() won't call folio_start_writeback()
+			 * as the folio already has that flag.
+			 *
+			 * This means, if we only rely one btrfs_folio_*()
+			 * helpers for both dirty and writeback flags,
+			 * PAGECACHE_TAG_DIRTY will never be cleared for subpage cases.
+			 * The only way to break out is to clear folio dirty here.
+			 */
 			if (folio_test_writeback(folio) ||
-			    !folio_test_dirty(folio)) {
+			    !folio_clear_dirty_for_io(folio)) {
 				folio_unlock(folio);
 				continue;
 			}
-- 
2.54.0