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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 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
>>
>