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

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

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