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

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

[Qu Wenruo]

[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 <wqu@suse.com>
---
 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

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

[Boris Burkov]

On Tue, Jul 07, 2026 at 12:10:25PM +0930, Qu Wenruo wrote:
> [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 is an interesting coincidence. I have been debugging this exact
change from a different, scarier perspective as well. Because we stopped
calling folio_clear_dirty_for_io() we stopped calling folio_mkclean()
which meant the folio remained writeable via mmap during writeback. We
observed a significant uptick of csum errors with large folios, as a
result. However, just this fix I think is not sufficient, and has two
serious bugs of its own.

First of all, now that your patch does clear dirty here, the keep_write
check in btrfs_subpage_start_writeback() is not correct anymore, which will
result in clearing TOWRITE on a non-sync writeback and losing it for
other pages in the folio on a sync writeback. So if we do clear dirty,
we need to also add a check for the subpage dirty bits for keep_write
like we did for extent_buffers before moving to the eb xarray.
Incidentally since that means not using __folio_start_writeback, it does
fix this exact bug too.

Second, and worse, it results in a deadlock which should show on btrfs/062
and btrfs/070, I believe. The problem is that clearing dirty at this point
results in a truncate being allowed to clean pages past the end of the
new file size without calling btrfs_mark_ordered_io_finished() so we
leak that OE unfinished and get stuck. This is caused by the other
patches which removed the Ordered bitmap tracking to reduce the bitmap
size, I believe, as we now use dirty to track the existence of OEs.

Can you confirm whether btrfs/062 and btrfs/070 finish for you every
time under this patch? If not, something about my test setup and yours
is different, or something is different on the branch I was working on.
I will try to re-test with your patches and my patches on for-next as
well.

> 
> 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 <wqu@suse.com>
> ---
>  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)) {


So far, the best solution I have come up with is to call a raw
"folio_mkclean" here, as opposed to folio_clear_dirty_for_io().

a second place it is needed is in extent_range_clear_dirty_for_io()
which also used to call folio_clear_dirty_for_io() and needs stable
pages during the compression, for which we need to call folio_mkclean().

Sorry to derail your fix with a whole different problem.. But unless I
am missing something I think we will have to fix all of it to fix any of
it.
Either by fixing forward with folio_mkclean() or by reverting and/or
rethinking all of the patches in the series:
https://lore.kernel.org/linux-btrfs/cover.1778131118.git.wqu@suse.com/

specifically:
095be159f3eb ("btrfs: unify folio dirty flag clearing")
and
7d97bdca4bcb ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")

Thanks,
Boris

>  				folio_unlock(folio);
>  				continue;
>  			}
> -- 
> 2.54.0
> 

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

[Boris Burkov]

On Mon, Jul 06, 2026 at 09:14:51PM -0700, Boris Burkov wrote:
> On Tue, Jul 07, 2026 at 12:10:25PM +0930, Qu Wenruo wrote:
> > [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 is an interesting coincidence. I have been debugging this exact
> change from a different, scarier perspective as well. Because we stopped
> calling folio_clear_dirty_for_io() we stopped calling folio_mkclean()
> which meant the folio remained writeable via mmap during writeback. We
> observed a significant uptick of csum errors with large folios, as a
> result. However, just this fix I think is not sufficient, and has two
> serious bugs of its own.
> 
> First of all, now that your patch does clear dirty here, the keep_write
> check in btrfs_subpage_start_writeback() is not correct anymore, which will
> result in clearing TOWRITE on a non-sync writeback and losing it for
> other pages in the folio on a sync writeback. So if we do clear dirty,
> we need to also add a check for the subpage dirty bits for keep_write
> like we did for extent_buffers before moving to the eb xarray.
> Incidentally since that means not using __folio_start_writeback, it does
> fix this exact bug too.
> 
> Second, and worse, it results in a deadlock which should show on btrfs/062
> and btrfs/070, I believe. The problem is that clearing dirty at this point
> results in a truncate being allowed to clean pages past the end of the
> new file size without calling btrfs_mark_ordered_io_finished() so we
> leak that OE unfinished and get stuck. This is caused by the other
> patches which removed the Ordered bitmap tracking to reduce the bitmap
> size, I believe, as we now use dirty to track the existence of OEs.
> 
> Can you confirm whether btrfs/062 and btrfs/070 finish for you every
> time under this patch? If not, something about my test setup and yours
> is different, or something is different on the branch I was working on.
> I will try to re-test with your patches and my patches on for-next as
> well.
> 

I re-tested by applying your patch to my for-next and I do believe b/062
hangs indefinitely, while it passes in ~45s for me normally.

> > 
> > 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 <wqu@suse.com>
> > ---
> >  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)) {
> 
> 
> So far, the best solution I have come up with is to call a raw
> "folio_mkclean" here, as opposed to folio_clear_dirty_for_io().
> 
> a second place it is needed is in extent_range_clear_dirty_for_io()
> which also used to call folio_clear_dirty_for_io() and needs stable
> pages during the compression, for which we need to call folio_mkclean().
> 
> Sorry to derail your fix with a whole different problem.. But unless I
> am missing something I think we will have to fix all of it to fix any of
> it.
> Either by fixing forward with folio_mkclean() or by reverting and/or
> rethinking all of the patches in the series:
> https://lore.kernel.org/linux-btrfs/cover.1778131118.git.wqu@suse.com/
> 
> specifically:
> 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> and
> 7d97bdca4bcb ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> 
> Thanks,
> Boris
> 
> >  				folio_unlock(folio);
> >  				continue;
> >  			}
> > -- 
> > 2.54.0
> > 

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

[Qu Wenruo]

在 2026/7/7 13:44, Boris Burkov 写道:
> On Tue, Jul 07, 2026 at 12:10:25PM +0930, Qu Wenruo wrote:
>> [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 is an interesting coincidence. I have been debugging this exact
> change from a different, scarier perspective as well. Because we stopped
> calling folio_clear_dirty_for_io() we stopped calling folio_mkclean()
> which meant the folio remained writeable via mmap during writeback. We
> observed a significant uptick of csum errors with large folios, as a
> result. However, just this fix I think is not sufficient, and has two
> serious bugs of its own.
> 
> First of all, now that your patch does clear dirty here, the keep_write
> check in btrfs_subpage_start_writeback() is not correct anymore, which will
> result in clearing TOWRITE on a non-sync writeback and losing it for
> other pages in the folio on a sync writeback. So if we do clear dirty,
> we need to also add a check for the subpage dirty bits for keep_write
> like we did for extent_buffers before moving to the eb xarray.
> Incidentally since that means not using __folio_start_writeback, it does
> fix this exact bug too.

You're right on this changed btrfs_subpage_start_writeback() change.

But there is a deeper problem, the timing of folio dirty and writeback 
flags change is not synced anyway.

If we do not clear the folio dirty early, then the timing of folio dirty 
clearing is always on the last dirty block clearing.
But at that time, we may have already called folio_start_writeback(), 
thus for the last block we do not clear the TOWRITE tag.

If we do clear the folio dirty early, then it's exactly the problem 
you're describing.

So I believe we should not rely on btrfs_start/end_writeback() to handle 
the pagecache tags at all.

> 
> Second, and worse, it results in a deadlock which should show on btrfs/062
> and btrfs/070, I believe. The problem is that clearing dirty at this point
> results in a truncate being allowed to clean pages past the end of the
> new file size without calling btrfs_mark_ordered_io_finished() so we
> leak that OE unfinished and get stuck.

Mind to explain exactly where the missing 
btrfs_mark_ordered_io_finished() is?

The point here is, we always have the folio locked already, the 
difference is just in the timing where the folio dirty flag is cleared.

I didn't see a problem that the changed timing can cause problem related 
to btrfs_mark_ordered_io_finished().
> This is caused by the other
> patches which removed the Ordered bitmap tracking to reduce the bitmap
> size, I believe, as we now use dirty to track the existence of OEs.
> 
> Can you confirm whether btrfs/062 and btrfs/070 finish for you every
> time under this patch? If not, something about my test setup and yours
> is different, or something is different on the branch I was working on.
> I will try to re-test with your patches and my patches on for-next as
> well.

I have not yet experienced btrfs/062 nor btrfs/070 hang.

[...]
>>   			if (folio_test_writeback(folio) ||
>> -			    !folio_test_dirty(folio)) {
>> +			    !folio_clear_dirty_for_io(folio)) {
> 
> 
> So far, the best solution I have come up with is to call a raw
> "folio_mkclean" here, as opposed to folio_clear_dirty_for_io().

I believe the root cause is that we can not rely on folio_*_writeback() 
calls to do the sub-folio level handling correctly.

I think the long term solution would be the iomap solution by tracking 
how many write bytes are still pending, and only call 
folio_end_writeback() once.

But for now, I think a quicker fix is, to de-couple the 
PAGECACHE_TAG_DIRTY/TOWRITE from folio_*_writeback().

The idea is, we clear PAGECACHE_TAG_DIRTY when the last subblock dirty 
flag is cleared.

Then do the same for PAGECACHE_TAG_TOWRITE, this time do not rely on 
__folio_end_writeback(), but manually do it inside 
btrfs_subpage_set_writeback().


I think this would be much smaller, and completely free us from the 
special handling of folio_*_writeback().

Thanks,
Qu
> 
> a second place it is needed is in extent_range_clear_dirty_for_io()
> which also used to call folio_clear_dirty_for_io() and needs stable
> pages during the compression, for which we need to call folio_mkclean().
> 
> Sorry to derail your fix with a whole different problem.. But unless I
> am missing something I think we will have to fix all of it to fix any of
> it.
> Either by fixing forward with folio_mkclean() or by reverting and/or
> rethinking all of the patches in the series:
> https://lore.kernel.org/linux-btrfs/cover.1778131118.git.wqu@suse.com/
> 
> specifically:
> 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> and
> 7d97bdca4bcb ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> 
> Thanks,
> Boris
> 
>>   				folio_unlock(folio);
>>   				continue;
>>   			}
>> -- 
>> 2.54.0
>>
> 

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

[Boris Burkov]

On Tue, Jul 07, 2026 at 03:10:20PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/7/7 13:44, Boris Burkov 写道:
> > On Tue, Jul 07, 2026 at 12:10:25PM +0930, Qu Wenruo wrote:
> > > [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 is an interesting coincidence. I have been debugging this exact
> > change from a different, scarier perspective as well. Because we stopped
> > calling folio_clear_dirty_for_io() we stopped calling folio_mkclean()
> > which meant the folio remained writeable via mmap during writeback. We
> > observed a significant uptick of csum errors with large folios, as a
> > result. However, just this fix I think is not sufficient, and has two
> > serious bugs of its own.
> > 
> > First of all, now that your patch does clear dirty here, the keep_write
> > check in btrfs_subpage_start_writeback() is not correct anymore, which will
> > result in clearing TOWRITE on a non-sync writeback and losing it for
> > other pages in the folio on a sync writeback. So if we do clear dirty,
> > we need to also add a check for the subpage dirty bits for keep_write
> > like we did for extent_buffers before moving to the eb xarray.
> > Incidentally since that means not using __folio_start_writeback, it does
> > fix this exact bug too.
> 
> You're right on this changed btrfs_subpage_start_writeback() change.
> 
> But there is a deeper problem, the timing of folio dirty and writeback flags
> change is not synced anyway.
> 
> If we do not clear the folio dirty early, then the timing of folio dirty
> clearing is always on the last dirty block clearing.
> But at that time, we may have already called folio_start_writeback(), thus
> for the last block we do not clear the TOWRITE tag.
> 
> If we do clear the folio dirty early, then it's exactly the problem you're
> describing.
> 
> So I believe we should not rely on btrfs_start/end_writeback() to handle the
> pagecache tags at all.
> 
> > 
> > Second, and worse, it results in a deadlock which should show on btrfs/062
> > and btrfs/070, I believe. The problem is that clearing dirty at this point
> > results in a truncate being allowed to clean pages past the end of the
> > new file size without calling btrfs_mark_ordered_io_finished() so we
> > leak that OE unfinished and get stuck.
> 
> Mind to explain exactly where the missing btrfs_mark_ordered_io_finished()
> is?
> 
> The point here is, we always have the folio locked already, the difference
> is just in the timing where the folio dirty flag is cleared.
> 
> I didn't see a problem that the changed timing can cause problem related to
> btrfs_mark_ordered_io_finished().

Sorry, my explanation last night was rushed and clumsy, let me try to be
much more clear and specific!

The key change is your recent one, that went together with the removal
of folio_clear_dirty_for_io():

334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")

and specifically the lines:

  /*
  * If the range is not dirty, the range has been submitted and
  * since we have waited for the writeback, endio has been
  * executed, thus we must skip the range to avoid double
  * accounting for the ordered extent.
  */
  if (!btrfs_folio_test_dirty(fs_info, folio, cur, range_len))
    goto next;


and quoting from the changelog:

  If the OE range is dirty, it means we have allocated an ordered extent but
  have not yet submitted the range. And that's exactly the case where we need
  to truncate the ordered extent.

But with this new fix, which returns folio_clear_dirty_for_io before
submission, the comment, skip, and changelog argument are no longer
true.

Suppose we dirty folios [F,F+K] at the end of a file and start writeback
on them, creating a single OE for [F, F+K]. extent_writepage will keep
looping through the folios submitting them.

Now suppose a truncate races in and shrinks i_size (taking i_rwsem but
no folio locks or blocking on OEs) via:
btrfs_setsize
  truncate_setsize
    i_size_write // shrinks i_size to M <= F
      truncate_pagecache
        folio_wait_writeback (not yet marked writeback)
        folio_invalidate
          btrfs_invalidate_folio (folio clean, skipped, this is the bug)

extent_writepage hits the code

  if (folio->index > end_index ||
    (folio->index == end_index && !pg_offset)) {
      folio_invalidate(folio, 0, folio_size(folio));
      folio_unlock(folio);
      return 0;
  }

which calls btrfs_folio_invalidate() which needs to mark the OE past
isize finished but skips it because the folio is clean (same issue as in
truncate above)

Now a bunch of processes, including a later call in the truncate itself,
will hang waiting on the OE which never finishes because these latter
folios never get submitted, nor does an invalidate truncate and finish
the lost OE.

Here are some trace printks showing the buggy sequence:
# btrfs_alloc_ordered_extent
91.901347  OE-CREATE      ino=306 oe_off=3637248 num_bytes=73728 isize=4587672
# btrfs_setsize the file, changing memory isize to newsize < oe_off
91.901392  TRUNCATE       ino=306 oldsize=4587672 newsize=2573711
# extent_writepage calls folio_invalidate for being past i_size (905 * 4096 = 3706880 > 2473711)
91.901528  WB-INVALIDATE  ino=306 folio_idx=905 isize=2573711 dirty=0 writeback=0
# btrfs_invalidate_folio does goto next and skips calling btrfs_mark_ordered_extent_truncated()
91.901531  SKIP-OE-FINISH ino=306 oe_off=3637248 bytes_left=73728 isize=2573711 writeback=0

I hope this is enough to figure out which one of us is missing
something! Sorry again for being so unclear earlier.

> > This is caused by the other
> > patches which removed the Ordered bitmap tracking to reduce the bitmap
> > size, I believe, as we now use dirty to track the existence of OEs.
> > 
> > Can you confirm whether btrfs/062 and btrfs/070 finish for you every
> > time under this patch? If not, something about my test setup and yours
> > is different, or something is different on the branch I was working on.
> > I will try to re-test with your patches and my patches on for-next as
> > well.
> 
> I have not yet experienced btrfs/062 nor btrfs/070 hang.
> 

I have reproduced it on two systems with your patch (x86 4k pages for both)
and also while working on my own version which was basically your patch with
a hack to clean up the TOWRITE bit only in btrfs_subpage_set_writeback().

While working on the details of my explanation for this email, I was
adding lots of printks and it did go away in one build, so I guess the
race between truncate and writeback is finicky enough that you are
getting unlucky? I played with some delays but haven't yet found a
perfect place to make it 100% reliable.

> [...]
> > >   			if (folio_test_writeback(folio) ||
> > > -			    !folio_test_dirty(folio)) {
> > > +			    !folio_clear_dirty_for_io(folio)) {
> > 
> > 
> > So far, the best solution I have come up with is to call a raw
> > "folio_mkclean" here, as opposed to folio_clear_dirty_for_io().
> 
> I believe the root cause is that we can not rely on folio_*_writeback()
> calls to do the sub-folio level handling correctly.
> 
> I think the long term solution would be the iomap solution by tracking how
> many write bytes are still pending, and only call folio_end_writeback()
> once.

I was not aware of this design, I'll look into it, thanks for the tip.

> 
> But for now, I think a quicker fix is, to de-couple the
> PAGECACHE_TAG_DIRTY/TOWRITE from folio_*_writeback().
> 
> The idea is, we clear PAGECACHE_TAG_DIRTY when the last subblock dirty flag
> is cleared.
> 
> Then do the same for PAGECACHE_TAG_TOWRITE, this time do not rely on
> __folio_end_writeback(), but manually do it inside
> btrfs_subpage_set_writeback().

I have been testing something hacky like this in
btrfs_subpage_set_writeback(), and I believe it is correct for TOWRITE
but still convincing myself about DIRTY. Just putting code out to be
more concrete. I think it's clearly hacky, but it's something we have
thrown around before so it was easy to start with:

 	keep_write = folio_test_dirty(folio);
 	if (!folio_test_writeback(folio))
 		__folio_start_writeback(folio, keep_write);
+	if (!keep_write) {
+		struct address_space *mapping = folio_mapping(folio);
+		unsigned long xas_flags;
+		XA_STATE(xas, &mapping->i_pages, folio->index);
+
+		xas_lock_irqsave(&xas, xas_flags);
+		xas_load(&xas);
+		xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
+		xas_clear_mark(&xas, PAGECACHE_TAG_TOWRITE);
+		xas_unlock_irqrestore(&xas, xas_flags);
+	}
 	spin_unlock_irqrestore(&bfs->lock, flags);

> 
> 
> I think this would be much smaller, and completely free us from the special
> handling of folio_*_writeback().

Under your proposal would we still be calling folio_clear_dirty_for_io()
as in this current incomplete fix? You haven't commented yet either way
on your view of the importance of getting to a folio_mkclean() *somehow*
to write protect the folio, so I just wanted to make sure we were synced
on that, as that is what got me debugging this in the first place, not
the messed up PAGECACHE_TAG_DIRTY, which we obviously need to fix as
well!

Thanks,
Boris

> 
> Thanks,
> Qu
> > 
> > a second place it is needed is in extent_range_clear_dirty_for_io()
> > which also used to call folio_clear_dirty_for_io() and needs stable
> > pages during the compression, for which we need to call folio_mkclean().
> > 
> > Sorry to derail your fix with a whole different problem.. But unless I
> > am missing something I think we will have to fix all of it to fix any of
> > it.
> > Either by fixing forward with folio_mkclean() or by reverting and/or
> > rethinking all of the patches in the series:
> > https://lore.kernel.org/linux-btrfs/cover.1778131118.git.wqu@suse.com/
> > 
> > specifically:
> > 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> > and
> > 7d97bdca4bcb ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> > 
> > Thanks,
> > Boris
> > 
> > >   				folio_unlock(folio);
> > >   				continue;
> > >   			}
> > > -- 
> > > 2.54.0
> > > 
> > 
> 

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

[Qu Wenruo]

在 2026/7/8 04:42, Boris Burkov 写道:
> On Tue, Jul 07, 2026 at 03:10:20PM +0930, Qu Wenruo wrote:
[...]
>>
>>>
>>> Second, and worse, it results in a deadlock which should show on btrfs/062
>>> and btrfs/070, I believe. The problem is that clearing dirty at this point
>>> results in a truncate being allowed to clean pages past the end of the
>>> new file size without calling btrfs_mark_ordered_io_finished() so we
>>> leak that OE unfinished and get stuck.
>>
>> Mind to explain exactly where the missing btrfs_mark_ordered_io_finished()
>> is?
>>
>> The point here is, we always have the folio locked already, the difference
>> is just in the timing where the folio dirty flag is cleared.
>>
>> I didn't see a problem that the changed timing can cause problem related to
>> btrfs_mark_ordered_io_finished().
> 
> Sorry, my explanation last night was rushed and clumsy, let me try to be
> much more clear and specific!
> 
> The key change is your recent one, that went together with the removal
> of folio_clear_dirty_for_io():
> 
> 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> 
> and specifically the lines:
> 
>    /*
>    * If the range is not dirty, the range has been submitted and
>    * since we have waited for the writeback, endio has been
>    * executed, thus we must skip the range to avoid double
>    * accounting for the ordered extent.
>    */
>    if (!btrfs_folio_test_dirty(fs_info, folio, cur, range_len))
>      goto next;
> 
> 
> and quoting from the changelog:
> 
>    If the OE range is dirty, it means we have allocated an ordered extent but
>    have not yet submitted the range. And that's exactly the case where we need
>    to truncate the ordered extent.
> 
> But with this new fix, which returns folio_clear_dirty_for_io before
> submission, the comment, skip, and changelog argument are no longer
> true.
> 
> Suppose we dirty folios [F,F+K] at the end of a file and start writeback
> on them, creating a single OE for [F, F+K]. extent_writepage will keep
> looping through the folios submitting them.
> 
> Now suppose a truncate races in and shrinks i_size (taking i_rwsem but
> no folio locks or blocking on OEs) via:
> btrfs_setsize
>    truncate_setsize
>      i_size_write // shrinks i_size to M <= F
>        truncate_pagecache
>          folio_wait_writeback (not yet marked writeback)
>          folio_invalidate
>            btrfs_invalidate_folio (folio clean, skipped, this is the bug)

Just a small nitpick, the problem is not from the truncate_setsize() 
call site, as truncate_pagecache() is grabbing the folio with FGP_LOCK, 
so it can not grab the folio locked by writeback.

> 
> extent_writepage hits the code
> 
>    if (folio->index > end_index ||
>      (folio->index == end_index && !pg_offset)) {
>        folio_invalidate(folio, 0, folio_size(folio));
>        folio_unlock(folio);
>        return 0;
>    }
> 
> which calls btrfs_folio_invalidate() which needs to mark the OE past
> isize finished but skips it because the folio is clean (same issue as in
> truncate above)

Thanks a lot! Now I understand how things are going wrong now.

But I still have a question, I thought we should have some kind of inode 
lock or something else to prevent i_size_read() from getting stale results.

And digging deeper, for the btrfs_setsize(), the inode is already 
locked. E.g. inside notify_change() there is a WARN_ON_ONCE() if the 
inode is not locked.

Would the root cause of the problem is that we did not lock the inode 
for writeback?

And if we do not have proper inode lock for writeback, then I think 
write back should never bother the isize check, and always do the 
writeback no matter isize.

> 
> Now a bunch of processes, including a later call in the truncate itself,
> will hang waiting on the OE which never finishes because these latter
> folios never get submitted, nor does an invalidate truncate and finish
> the lost OE.
> 
> Here are some trace printks showing the buggy sequence:
> # btrfs_alloc_ordered_extent
> 91.901347  OE-CREATE      ino=306 oe_off=3637248 num_bytes=73728 isize=4587672
> # btrfs_setsize the file, changing memory isize to newsize < oe_off
> 91.901392  TRUNCATE       ino=306 oldsize=4587672 newsize=2573711
> # extent_writepage calls folio_invalidate for being past i_size (905 * 4096 = 3706880 > 2473711)
> 91.901528  WB-INVALIDATE  ino=306 folio_idx=905 isize=2573711 dirty=0 writeback=0
> # btrfs_invalidate_folio does goto next and skips calling btrfs_mark_ordered_extent_truncated()
> 91.901531  SKIP-OE-FINISH ino=306 oe_off=3637248 bytes_left=73728 isize=2573711 writeback=0
> 
> I hope this is enough to figure out which one of us is missing
> something! Sorry again for being so unclear earlier.
> 
>>> This is caused by the other
>>> patches which removed the Ordered bitmap tracking to reduce the bitmap
>>> size, I believe, as we now use dirty to track the existence of OEs.
>>>
>>> Can you confirm whether btrfs/062 and btrfs/070 finish for you every
>>> time under this patch? If not, something about my test setup and yours
>>> is different, or something is different on the branch I was working on.
>>> I will try to re-test with your patches and my patches on for-next as
>>> well.
>>
>> I have not yet experienced btrfs/062 nor btrfs/070 hang.
>>
> 
> I have reproduced it on two systems with your patch (x86 4k pages for both)
> and also while working on my own version which was basically your patch with
> a hack to clean up the TOWRITE bit only in btrfs_subpage_set_writeback().
> 
> While working on the details of my explanation for this email, I was
> adding lots of printks and it did go away in one build, so I guess the
> race between truncate and writeback is finicky enough that you are
> getting unlucky? I played with some delays but haven't yet found a
> perfect place to make it 100% reliable.
> 
>> [...]
>>>>    			if (folio_test_writeback(folio) ||
>>>> -			    !folio_test_dirty(folio)) {
>>>> +			    !folio_clear_dirty_for_io(folio)) {
>>>
>>>
>>> So far, the best solution I have come up with is to call a raw
>>> "folio_mkclean" here, as opposed to folio_clear_dirty_for_io().
>>
>> I believe the root cause is that we can not rely on folio_*_writeback()
>> calls to do the sub-folio level handling correctly.
>>
>> I think the long term solution would be the iomap solution by tracking how
>> many write bytes are still pending, and only call folio_end_writeback()
>> once.
> 
> I was not aware of this design, I'll look into it, thanks for the tip.

That has a lot of extra requirements, and may have also change the 
timing of a lot of beyond-isize handling.

And the biggest requirement is to get rid of the cursed async-submission 
from compression.

So we're still quite away from that.

> 
>>
>> But for now, I think a quicker fix is, to de-couple the
>> PAGECACHE_TAG_DIRTY/TOWRITE from folio_*_writeback().
>>
>> The idea is, we clear PAGECACHE_TAG_DIRTY when the last subblock dirty flag
>> is cleared.
>>
>> Then do the same for PAGECACHE_TAG_TOWRITE, this time do not rely on
>> __folio_end_writeback(), but manually do it inside
>> btrfs_subpage_set_writeback().
> 
> I have been testing something hacky like this in
> btrfs_subpage_set_writeback(), and I believe it is correct for TOWRITE
> but still convincing myself about DIRTY.

Don't be too worry about that. Dirty tag can be cleared multiple times.

> Just putting code out to be
> more concrete. I think it's clearly hacky, but it's something we have
> thrown around before so it was easy to start with:
> 
>   	keep_write = folio_test_dirty(folio);
>   	if (!folio_test_writeback(folio))
>   		__folio_start_writeback(folio, keep_write);
> +	if (!keep_write) {
> +		struct address_space *mapping = folio_mapping(folio);
> +		unsigned long xas_flags;
> +		XA_STATE(xas, &mapping->i_pages, folio->index);
> +
> +		xas_lock_irqsave(&xas, xas_flags);
> +		xas_load(&xas);
> +		xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
> +		xas_clear_mark(&xas, PAGECACHE_TAG_TOWRITE);
> +		xas_unlock_irqrestore(&xas, xas_flags);
> +	}

That's the version I have submitted as v2.

>   	spin_unlock_irqrestore(&bfs->lock, flags);
> 
>>
>>
>> I think this would be much smaller, and completely free us from the special
>> handling of folio_*_writeback().
> 
> Under your proposal would we still be calling folio_clear_dirty_for_io()
> as in this current incomplete fix?

No, no folio_clear_dirty_for_io() call anymore, all changes are inside 
subpage.c now, just the extra PAGECACHE_TAG_* clearing during 
btrfs_subpage_set_writeback().

> You haven't commented yet either way
> on your view of the importance of getting to a folio_mkclean() *somehow*
> to write protect the folio, so I just wanted to make sure we were synced
> on that, as that is what got me debugging this in the first place, not
> the messed up PAGECACHE_TAG_DIRTY, which we obviously need to fix as
> well!

Mind to explain why folio_mkclean() is required? Is it for the same OE 
hang or something else?

Thanks,
Qu

> 
> Thanks,
> Boris
> 
>>
>> Thanks,
>> Qu
>>

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

[Boris Burkov]

On Wed, Jul 08, 2026 at 08:06:26AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/7/8 04:42, Boris Burkov 写道:
> > On Tue, Jul 07, 2026 at 03:10:20PM +0930, Qu Wenruo wrote:
> [...]
> > > 
> > > > 
> > > > Second, and worse, it results in a deadlock which should show on btrfs/062
> > > > and btrfs/070, I believe. The problem is that clearing dirty at this point
> > > > results in a truncate being allowed to clean pages past the end of the
> > > > new file size without calling btrfs_mark_ordered_io_finished() so we
> > > > leak that OE unfinished and get stuck.
> > > 
> > > Mind to explain exactly where the missing btrfs_mark_ordered_io_finished()
> > > is?
> > > 
> > > The point here is, we always have the folio locked already, the difference
> > > is just in the timing where the folio dirty flag is cleared.
> > > 
> > > I didn't see a problem that the changed timing can cause problem related to
> > > btrfs_mark_ordered_io_finished().
> > 
> > Sorry, my explanation last night was rushed and clumsy, let me try to be
> > much more clear and specific!
> > 
> > The key change is your recent one, that went together with the removal
> > of folio_clear_dirty_for_io():
> > 
> > 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> > 
> > and specifically the lines:
> > 
> >    /*
> >    * If the range is not dirty, the range has been submitted and
> >    * since we have waited for the writeback, endio has been
> >    * executed, thus we must skip the range to avoid double
> >    * accounting for the ordered extent.
> >    */
> >    if (!btrfs_folio_test_dirty(fs_info, folio, cur, range_len))
> >      goto next;
> > 
> > 
> > and quoting from the changelog:
> > 
> >    If the OE range is dirty, it means we have allocated an ordered extent but
> >    have not yet submitted the range. And that's exactly the case where we need
> >    to truncate the ordered extent.
> > 
> > But with this new fix, which returns folio_clear_dirty_for_io before
> > submission, the comment, skip, and changelog argument are no longer
> > true.
> > 
> > Suppose we dirty folios [F,F+K] at the end of a file and start writeback
> > on them, creating a single OE for [F, F+K]. extent_writepage will keep
> > looping through the folios submitting them.
> > 
> > Now suppose a truncate races in and shrinks i_size (taking i_rwsem but
> > no folio locks or blocking on OEs) via:
> > btrfs_setsize
> >    truncate_setsize
> >      i_size_write // shrinks i_size to M <= F
> >        truncate_pagecache
> >          folio_wait_writeback (not yet marked writeback)
> >          folio_invalidate
> >            btrfs_invalidate_folio (folio clean, skipped, this is the bug)
> 
> Just a small nitpick, the problem is not from the truncate_setsize() call
> site, as truncate_pagecache() is grabbing the folio with FGP_LOCK, so it can
> not grab the folio locked by writeback.
> 

Good point, but I think the issue is as soon as you do i_size_write(),
so the rest is just my explanation being a little misleading, I think.

> > 
> > extent_writepage hits the code
> > 
> >    if (folio->index > end_index ||
> >      (folio->index == end_index && !pg_offset)) {
> >        folio_invalidate(folio, 0, folio_size(folio));
> >        folio_unlock(folio);
> >        return 0;
> >    }
> > 
> > which calls btrfs_folio_invalidate() which needs to mark the OE past
> > isize finished but skips it because the folio is clean (same issue as in
> > truncate above)
> 
> Thanks a lot! Now I understand how things are going wrong now.
> 
> But I still have a question, I thought we should have some kind of inode
> lock or something else to prevent i_size_read() from getting stale results.
> 
> And digging deeper, for the btrfs_setsize(), the inode is already locked.
> E.g. inside notify_change() there is a WARN_ON_ONCE() if the inode is not
> locked.
> 
> Would the root cause of the problem is that we did not lock the inode for
> writeback?

yes, I think this path to the hang definitely requires the fact that
writeback does not hold i_rwsem while reading i_size.

> 
> And if we do not have proper inode lock for writeback, then I think write
> back should never bother the isize check, and always do the writeback no
> matter isize.
> 

Don't we risk weird truncate vs writeback races where they undo each
other, then? I assume taking the lock in writeback will be "correct" but
it might be slow or introduce some other locking order problem...

> > 
> > Now a bunch of processes, including a later call in the truncate itself,
> > will hang waiting on the OE which never finishes because these latter
> > folios never get submitted, nor does an invalidate truncate and finish
> > the lost OE.
> > 
> > Here are some trace printks showing the buggy sequence:
> > # btrfs_alloc_ordered_extent
> > 91.901347  OE-CREATE      ino=306 oe_off=3637248 num_bytes=73728 isize=4587672
> > # btrfs_setsize the file, changing memory isize to newsize < oe_off
> > 91.901392  TRUNCATE       ino=306 oldsize=4587672 newsize=2573711
> > # extent_writepage calls folio_invalidate for being past i_size (905 * 4096 = 3706880 > 2473711)
> > 91.901528  WB-INVALIDATE  ino=306 folio_idx=905 isize=2573711 dirty=0 writeback=0
> > # btrfs_invalidate_folio does goto next and skips calling btrfs_mark_ordered_extent_truncated()
> > 91.901531  SKIP-OE-FINISH ino=306 oe_off=3637248 bytes_left=73728 isize=2573711 writeback=0
> > 
> > I hope this is enough to figure out which one of us is missing
> > something! Sorry again for being so unclear earlier.
> > 
> > > > This is caused by the other
> > > > patches which removed the Ordered bitmap tracking to reduce the bitmap
> > > > size, I believe, as we now use dirty to track the existence of OEs.
> > > > 
> > > > Can you confirm whether btrfs/062 and btrfs/070 finish for you every
> > > > time under this patch? If not, something about my test setup and yours
> > > > is different, or something is different on the branch I was working on.
> > > > I will try to re-test with your patches and my patches on for-next as
> > > > well.
> > > 
> > > I have not yet experienced btrfs/062 nor btrfs/070 hang.
> > > 
> > 
> > I have reproduced it on two systems with your patch (x86 4k pages for both)
> > and also while working on my own version which was basically your patch with
> > a hack to clean up the TOWRITE bit only in btrfs_subpage_set_writeback().
> > 
> > While working on the details of my explanation for this email, I was
> > adding lots of printks and it did go away in one build, so I guess the
> > race between truncate and writeback is finicky enough that you are
> > getting unlucky? I played with some delays but haven't yet found a
> > perfect place to make it 100% reliable.
> > 
> > > [...]
> > > > >    			if (folio_test_writeback(folio) ||
> > > > > -			    !folio_test_dirty(folio)) {
> > > > > +			    !folio_clear_dirty_for_io(folio)) {
> > > > 
> > > > 
> > > > So far, the best solution I have come up with is to call a raw
> > > > "folio_mkclean" here, as opposed to folio_clear_dirty_for_io().
> > > 
> > > I believe the root cause is that we can not rely on folio_*_writeback()
> > > calls to do the sub-folio level handling correctly.
> > > 
> > > I think the long term solution would be the iomap solution by tracking how
> > > many write bytes are still pending, and only call folio_end_writeback()
> > > once.
> > 
> > I was not aware of this design, I'll look into it, thanks for the tip.
> 
> That has a lot of extra requirements, and may have also change the timing of
> a lot of beyond-isize handling.
> 
> And the biggest requirement is to get rid of the cursed async-submission
> from compression.
> 
> So we're still quite away from that.
> 
> > 
> > > 
> > > But for now, I think a quicker fix is, to de-couple the
> > > PAGECACHE_TAG_DIRTY/TOWRITE from folio_*_writeback().
> > > 
> > > The idea is, we clear PAGECACHE_TAG_DIRTY when the last subblock dirty flag
> > > is cleared.
> > > 
> > > Then do the same for PAGECACHE_TAG_TOWRITE, this time do not rely on
> > > __folio_end_writeback(), but manually do it inside
> > > btrfs_subpage_set_writeback().
> > 
> > I have been testing something hacky like this in
> > btrfs_subpage_set_writeback(), and I believe it is correct for TOWRITE
> > but still convincing myself about DIRTY.
> 
> Don't be too worry about that. Dirty tag can be cleared multiple times.
> 
> > Just putting code out to be
> > more concrete. I think it's clearly hacky, but it's something we have
> > thrown around before so it was easy to start with:
> > 
> >   	keep_write = folio_test_dirty(folio);
> >   	if (!folio_test_writeback(folio))
> >   		__folio_start_writeback(folio, keep_write);
> > +	if (!keep_write) {
> > +		struct address_space *mapping = folio_mapping(folio);
> > +		unsigned long xas_flags;
> > +		XA_STATE(xas, &mapping->i_pages, folio->index);
> > +
> > +		xas_lock_irqsave(&xas, xas_flags);
> > +		xas_load(&xas);
> > +		xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
> > +		xas_clear_mark(&xas, PAGECACHE_TAG_TOWRITE);
> > +		xas_unlock_irqrestore(&xas, xas_flags);
> > +	}
> 
> That's the version I have submitted as v2.
> 

Sounds good.

> >   	spin_unlock_irqrestore(&bfs->lock, flags);
> > 
> > > 
> > > 
> > > I think this would be much smaller, and completely free us from the special
> > > handling of folio_*_writeback().
> > 
> > Under your proposal would we still be calling folio_clear_dirty_for_io()
> > as in this current incomplete fix?
> 
> No, no folio_clear_dirty_for_io() call anymore, all changes are inside
> subpage.c now, just the extra PAGECACHE_TAG_* clearing during
> btrfs_subpage_set_writeback().
> 
> > You haven't commented yet either way
> > on your view of the importance of getting to a folio_mkclean() *somehow*
> > to write protect the folio, so I just wanted to make sure we were synced
> > on that, as that is what got me debugging this in the first place, not
> > the messed up PAGECACHE_TAG_DIRTY, which we obviously need to fix as
> > well!
> 
> Mind to explain why folio_mkclean() is required? Is it for the same OE hang
> or something else?
> 

No it's a csum corruption with mmap being allowed to write to folios
that are under writeback (and thus computing csums)

Since we got rid of folio_clear_dirty_for_io() (and thus
folio_mkclean()), then if we mmap a large folio, then start writeback on
it, we never write protect the folios while they are under writeback so
the mmap user can trigger write faults and mess up the csums. We have
observed this in practice in our early testing of btrfs large folios in
production and we also have a racy reproducer that enables large folios,
mmaps a file and has a bunch of threads racing between touching each
sector with some new random data and fsyncing the file. We do that a bunch
of times, then drop caches and read everything. Do that whole thing in a
loop a few times and eventually you hit it. I can share the exact LLM
reproducer slop code if you would like, but that is the rough shape, to
explain the point.

I can send my holistic fix proposal with your v2 included in a series?

> Thanks,
> Qu
> 
> > 
> > Thanks,
> > Boris
> > 
> > > 
> > > Thanks,
> > > Qu
> > > 

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

[Qu Wenruo]

在 2026/7/8 08:46, Boris Burkov 写道:
> On Wed, Jul 08, 2026 at 08:06:26AM +0930, Qu Wenruo wrote:
>>
>>
>> 在 2026/7/8 04:42, Boris Burkov 写道:
>>> On Tue, Jul 07, 2026 at 03:10:20PM +0930, Qu Wenruo wrote:
>> [...]
>>>>
>>>>>
>>>>> Second, and worse, it results in a deadlock which should show on btrfs/062
>>>>> and btrfs/070, I believe. The problem is that clearing dirty at this point
>>>>> results in a truncate being allowed to clean pages past the end of the
>>>>> new file size without calling btrfs_mark_ordered_io_finished() so we
>>>>> leak that OE unfinished and get stuck.
>>>>
>>>> Mind to explain exactly where the missing btrfs_mark_ordered_io_finished()
>>>> is?
>>>>
>>>> The point here is, we always have the folio locked already, the difference
>>>> is just in the timing where the folio dirty flag is cleared.
>>>>
>>>> I didn't see a problem that the changed timing can cause problem related to
>>>> btrfs_mark_ordered_io_finished().
>>>
>>> Sorry, my explanation last night was rushed and clumsy, let me try to be
>>> much more clear and specific!
>>>
>>> The key change is your recent one, that went together with the removal
>>> of folio_clear_dirty_for_io():
>>>
>>> 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
>>>
>>> and specifically the lines:
>>>
>>>     /*
>>>     * If the range is not dirty, the range has been submitted and
>>>     * since we have waited for the writeback, endio has been
>>>     * executed, thus we must skip the range to avoid double
>>>     * accounting for the ordered extent.
>>>     */
>>>     if (!btrfs_folio_test_dirty(fs_info, folio, cur, range_len))
>>>       goto next;
>>>
>>>
>>> and quoting from the changelog:
>>>
>>>     If the OE range is dirty, it means we have allocated an ordered extent but
>>>     have not yet submitted the range. And that's exactly the case where we need
>>>     to truncate the ordered extent.
>>>
>>> But with this new fix, which returns folio_clear_dirty_for_io before
>>> submission, the comment, skip, and changelog argument are no longer
>>> true.
>>>
>>> Suppose we dirty folios [F,F+K] at the end of a file and start writeback
>>> on them, creating a single OE for [F, F+K]. extent_writepage will keep
>>> looping through the folios submitting them.
>>>
>>> Now suppose a truncate races in and shrinks i_size (taking i_rwsem but
>>> no folio locks or blocking on OEs) via:
>>> btrfs_setsize
>>>     truncate_setsize
>>>       i_size_write // shrinks i_size to M <= F
>>>         truncate_pagecache
>>>           folio_wait_writeback (not yet marked writeback)
>>>           folio_invalidate
>>>             btrfs_invalidate_folio (folio clean, skipped, this is the bug)
>>
>> Just a small nitpick, the problem is not from the truncate_setsize() call
>> site, as truncate_pagecache() is grabbing the folio with FGP_LOCK, so it can
>> not grab the folio locked by writeback.
>>
> 
> Good point, but I think the issue is as soon as you do i_size_write(),
> so the rest is just my explanation being a little misleading, I think.
> 
>>>
>>> extent_writepage hits the code
>>>
>>>     if (folio->index > end_index ||
>>>       (folio->index == end_index && !pg_offset)) {
>>>         folio_invalidate(folio, 0, folio_size(folio));
>>>         folio_unlock(folio);
>>>         return 0;
>>>     }
>>>
>>> which calls btrfs_folio_invalidate() which needs to mark the OE past
>>> isize finished but skips it because the folio is clean (same issue as in
>>> truncate above)
>>
>> Thanks a lot! Now I understand how things are going wrong now.
>>
>> But I still have a question, I thought we should have some kind of inode
>> lock or something else to prevent i_size_read() from getting stale results.
>>
>> And digging deeper, for the btrfs_setsize(), the inode is already locked.
>> E.g. inside notify_change() there is a WARN_ON_ONCE() if the inode is not
>> locked.
>>
>> Would the root cause of the problem is that we did not lock the inode for
>> writeback?
> 
> yes, I think this path to the hang definitely requires the fact that
> writeback does not hold i_rwsem while reading i_size.
> 
>>
>> And if we do not have proper inode lock for writeback, then I think write
>> back should never bother the isize check, and always do the writeback no
>> matter isize.
>>
> 
> Don't we risk weird truncate vs writeback races where they undo each
> other, then?

Writeback still has the folio locked, so truncation should wait for the 
writeback first then invalidate it.

The other direction would also be true, if the invalidation invalidated 
the page first, then writeback should not got a dirty folio anymore.

But I can be totally wrong considering how complex things are.

I can explore if this idea won't break the existing runs first.
>>
>> Mind to explain why folio_mkclean() is required? Is it for the same OE hang
>> or something else?
>>
> 
> No it's a csum corruption with mmap being allowed to write to folios
> that are under writeback (and thus computing csums)
> 
> Since we got rid of folio_clear_dirty_for_io() (and thus
> folio_mkclean()), then if we mmap a large folio, then start writeback on
> it, we never write protect the folios while they are under writeback so
> the mmap user can trigger write faults and mess up the csums. We have
> observed this in practice in our early testing of btrfs large folios in
> production and we also have a racy reproducer that enables large folios,
> mmaps a file and has a bunch of threads racing between touching each
> sector with some new random data and fsyncing the file. We do that a bunch
> of times, then drop caches and read everything. Do that whole thing in a
> loop a few times and eventually you hit it. I can share the exact LLM
> reproducer slop code if you would like, but that is the rough shape, to
> explain the point.
> 
> I can send my holistic fix proposal with your v2 included in a series?

Sounds great. Feel free to include the v2 fix:
https://lore.kernel.org/linux-btrfs/d47af8cae38e32462d61cb62139c2732e4e8fdbe.1783407237.git.wqu@suse.com/T/

Although considering this is already breaking cachestat() and will cause 
extra overhead for writeback, I'd prefer the v2 as a hot fix first.

Thanks,
Qu


> 
>> Thanks,
>> Qu
>>
>>>
>>> Thanks,
>>> Boris
>>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>

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

[Boris Burkov]

On Wed, Jul 08, 2026 at 09:00:39AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/7/8 08:46, Boris Burkov 写道:
> > On Wed, Jul 08, 2026 at 08:06:26AM +0930, Qu Wenruo wrote:
> > > 
> > > 
> > > 在 2026/7/8 04:42, Boris Burkov 写道:
> > > > On Tue, Jul 07, 2026 at 03:10:20PM +0930, Qu Wenruo wrote:
> > > [...]
> > > > > 
> > > > > > 
> > > > > > Second, and worse, it results in a deadlock which should show on btrfs/062
> > > > > > and btrfs/070, I believe. The problem is that clearing dirty at this point
> > > > > > results in a truncate being allowed to clean pages past the end of the
> > > > > > new file size without calling btrfs_mark_ordered_io_finished() so we
> > > > > > leak that OE unfinished and get stuck.
> > > > > 
> > > > > Mind to explain exactly where the missing btrfs_mark_ordered_io_finished()
> > > > > is?
> > > > > 
> > > > > The point here is, we always have the folio locked already, the difference
> > > > > is just in the timing where the folio dirty flag is cleared.
> > > > > 
> > > > > I didn't see a problem that the changed timing can cause problem related to
> > > > > btrfs_mark_ordered_io_finished().
> > > > 
> > > > Sorry, my explanation last night was rushed and clumsy, let me try to be
> > > > much more clear and specific!
> > > > 
> > > > The key change is your recent one, that went together with the removal
> > > > of folio_clear_dirty_for_io():
> > > > 
> > > > 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> > > > 
> > > > and specifically the lines:
> > > > 
> > > >     /*
> > > >     * If the range is not dirty, the range has been submitted and
> > > >     * since we have waited for the writeback, endio has been
> > > >     * executed, thus we must skip the range to avoid double
> > > >     * accounting for the ordered extent.
> > > >     */
> > > >     if (!btrfs_folio_test_dirty(fs_info, folio, cur, range_len))
> > > >       goto next;
> > > > 
> > > > 
> > > > and quoting from the changelog:
> > > > 
> > > >     If the OE range is dirty, it means we have allocated an ordered extent but
> > > >     have not yet submitted the range. And that's exactly the case where we need
> > > >     to truncate the ordered extent.
> > > > 
> > > > But with this new fix, which returns folio_clear_dirty_for_io before
> > > > submission, the comment, skip, and changelog argument are no longer
> > > > true.
> > > > 
> > > > Suppose we dirty folios [F,F+K] at the end of a file and start writeback
> > > > on them, creating a single OE for [F, F+K]. extent_writepage will keep
> > > > looping through the folios submitting them.
> > > > 
> > > > Now suppose a truncate races in and shrinks i_size (taking i_rwsem but
> > > > no folio locks or blocking on OEs) via:
> > > > btrfs_setsize
> > > >     truncate_setsize
> > > >       i_size_write // shrinks i_size to M <= F
> > > >         truncate_pagecache
> > > >           folio_wait_writeback (not yet marked writeback)
> > > >           folio_invalidate
> > > >             btrfs_invalidate_folio (folio clean, skipped, this is the bug)
> > > 
> > > Just a small nitpick, the problem is not from the truncate_setsize() call
> > > site, as truncate_pagecache() is grabbing the folio with FGP_LOCK, so it can
> > > not grab the folio locked by writeback.
> > > 
> > 
> > Good point, but I think the issue is as soon as you do i_size_write(),
> > so the rest is just my explanation being a little misleading, I think.
> > 
> > > > 
> > > > extent_writepage hits the code
> > > > 
> > > >     if (folio->index > end_index ||
> > > >       (folio->index == end_index && !pg_offset)) {
> > > >         folio_invalidate(folio, 0, folio_size(folio));
> > > >         folio_unlock(folio);
> > > >         return 0;
> > > >     }
> > > > 
> > > > which calls btrfs_folio_invalidate() which needs to mark the OE past
> > > > isize finished but skips it because the folio is clean (same issue as in
> > > > truncate above)
> > > 
> > > Thanks a lot! Now I understand how things are going wrong now.
> > > 
> > > But I still have a question, I thought we should have some kind of inode
> > > lock or something else to prevent i_size_read() from getting stale results.
> > > 
> > > And digging deeper, for the btrfs_setsize(), the inode is already locked.
> > > E.g. inside notify_change() there is a WARN_ON_ONCE() if the inode is not
> > > locked.
> > > 
> > > Would the root cause of the problem is that we did not lock the inode for
> > > writeback?
> > 
> > yes, I think this path to the hang definitely requires the fact that
> > writeback does not hold i_rwsem while reading i_size.
> > 
> > > 
> > > And if we do not have proper inode lock for writeback, then I think write
> > > back should never bother the isize check, and always do the writeback no
> > > matter isize.
> > > 
> > 
> > Don't we risk weird truncate vs writeback races where they undo each
> > other, then?
> 
> Writeback still has the folio locked, so truncation should wait for the
> writeback first then invalidate it.
> 
> The other direction would also be true, if the invalidation invalidated the
> page first, then writeback should not got a dirty folio anymore.
> 
> But I can be totally wrong considering how complex things are.
> 
> I can explore if this idea won't break the existing runs first.
> > > 
> > > Mind to explain why folio_mkclean() is required? Is it for the same OE hang
> > > or something else?
> > > 
> > 
> > No it's a csum corruption with mmap being allowed to write to folios
> > that are under writeback (and thus computing csums)
> > 
> > Since we got rid of folio_clear_dirty_for_io() (and thus
> > folio_mkclean()), then if we mmap a large folio, then start writeback on
> > it, we never write protect the folios while they are under writeback so
> > the mmap user can trigger write faults and mess up the csums. We have
> > observed this in practice in our early testing of btrfs large folios in
> > production and we also have a racy reproducer that enables large folios,
> > mmaps a file and has a bunch of threads racing between touching each
> > sector with some new random data and fsyncing the file. We do that a bunch
> > of times, then drop caches and read everything. Do that whole thing in a
> > loop a few times and eventually you hit it. I can share the exact LLM
> > reproducer slop code if you would like, but that is the rough shape, to
> > explain the point.
> > 
> > I can send my holistic fix proposal with your v2 included in a series?
> 
> Sounds great. Feel free to include the v2 fix:
> https://lore.kernel.org/linux-btrfs/d47af8cae38e32462d61cb62139c2732e4e8fdbe.1783407237.git.wqu@suse.com/T/
> 
> Although considering this is already breaking cachestat() and will cause
> extra overhead for writeback, I'd prefer the v2 as a hot fix first.

I gave your v2 my r-b, and will send the patch to fix the corruption
next, separately. It shouldn't affect your patch, as it only conflicted
if we went back to folio_clear_dirty_for_io().

> 
> Thanks,
> Qu
> 
> 
> > 
> > > Thanks,
> > > Qu
> > > 
> > > > 
> > > > Thanks,
> > > > Boris
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Qu
> > > > > 
>