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

[Josef Bacik <josef@toxicpanda.com>]

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.

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,

Josef