Re: [PATCH RFC 4/4] btrfs: inode: make btrfs_invalidatepage() to be subpage compatible

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

On 2020/12/17 下午10:51, Josef Bacik wrote:
> On 12/16/20 11:57 PM, Qu Wenruo wrote:
>> [BUG]
>> With current subpage RW patchset, the following script can lead to
>> filesystem hang:
>>    # mkfs.btrfs -f -s 4k $dev
>>    # mount $dev -o nospace_cache $mnt
>>    # fsstress -w -n 100 -p 1 -s 1608140256 -v -d $mnt
>>
>> The file system will hang at wait_event() of
>> btrfs_start_ordered_extent().
>>
>> [CAUSE]
>> The root cause is, btrfs_invalidatepage() is freeing page::private which
>> still has subpage dirty bit set.
>>
>> The offending situation happens like this:
>> btrfs_fllocate()
>> |- btrfs_zero_range()
>>     |- btrfs_punch_hole_lock_range()
>>        |- truncate_pagecache_range()
>>           |- btrfs_invalidatepage()
>>
>> The involved range looks like:
>>
>> 0    32K    64K    96K    128K
>>     |///////||//////|
>>     | Range to drop |
>>
>> For the [32K, 64K) range, since the offset is 32K, the page won't be
>> invalidated.
>>
>> But for the [64K, 96K) range, the offset is 0, current
>> btrfs_invalidatepage() will call clear_page_extent_mapped() which will
>> detach page::private, making the subpage dirty bitmap being cleared.
>>
>> This prevents later __extent_writepage_io() to locate any range to
>> write, thus no way to wake up the ordered extents.
>>
>> [FIX]
>> To fix the problem this patch will:
>> - Only clear page status and detach page private when the full page
>>    is invalidated
>>
>> - Change how we handle unfinished ordered extent
>>    If there is any ordered extent unfinished in the page range, we can't
>>    call clear_extent_bit() with delete == true.
>>
>> [REASON FOR RFC]
>> There is still uncertainty around the btrfs_releasepage() call.
>>
>> 1. Why we need btrfs_releasepage() call for non-full-page condition?
>>     Other fs (aka. xfs) just exit without doing special handling if
>>     invalidatepage() is called with part of the page.
>>
>>     Thus I didn't completely understand why btrfs_releasepage() here is
>>     needed for non-full page call.
>>
>> 2. Why "if (offset)" is not causing problem for current code?
>>     This existing if (offset) call can be skipped for cases like
>>     offset == 0 length == 2K.
>>     As MM layer can call invalidatepage() with unaligned offset/length,
>>     for cases like truncate_inode_pages_range().
>>     This will make btrfs_invalidatepage() to truncate the whole page when
>>     we only need to zero part of the page.
>>
>
> Are we ever calling with a different length when pagesize ==
> sectorsize?  That's probably why it works fine now.

The range passed in can be unaligned at all.

MM layer functions like truncate_inode_pages_range() relies on that.

That's why I'm wondering why the current code is working.

As for start == 0 and length != PAGE_SIZE case it may clear the
Private2/Checked bit unintentionally.

Or is that CoW fixup saving the problem?
>
> But I think we should follow what all the other file systems do, if len
> != PAGE_SIZE || offset != 0 then just skip it, that would probably be
> easier and work for you as well?  Thanks,

Definitely it would work for me.

Thanks,
Qu

>
> Josef