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")
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>

1 files 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);
 }