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

[Qu Wenruo <wqu@suse.com>]

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