Re: [PATCH v3 20/26] btrfs: make extent_write_locked_range() to be subpage compatible

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Sep 27, 2021 at 03:22:02PM +0800, Qu Wenruo wrote:
> There are two sites are not subpage compatible yet for
> extent_write_locked_range():
> 
> - How @nr_pages are calculated
>   For subpage we can have the following range with 64K page size:
> 
>   0   32K  64K   96K 128K
>   |   |////|/////|   |
> 
>   In that case, although 96K - 32K == 64K, thus it looks like one page
>   is enough, but the range spans across two pages, not one.
> 
>   Fix it by doing proper round_up() and round_down() to calculate
>   @nr_pages.
> 
>   Also add some extra ASSERT()s to ensure the range passed in is already
>   aligned.
> 
> - How the page end is calculated
>   Currently we just use cur + PAGE_SIZE - 1 to calculate the page end.
> 
>   Which can't handle above range layout, and will trigger ASSERT() in
>   btrfs_writepage_endio_finish_ordered(), as the range is no longer
>   covered by the page range.
> 
>   Fix it by take page end into consideration.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 825917f1b623..f05d8896d1ad 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5087,15 +5087,14 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
>  	struct address_space *mapping = inode->i_mapping;
>  	struct page *page;
>  	u64 cur = start;
> -	unsigned long nr_pages = (end - start + PAGE_SIZE) >>
> -		PAGE_SHIFT;
> +	unsigned long nr_pages;
> +	const u32 sectorsize = btrfs_sb(inode->i_sb)->sectorsize;
>  	struct extent_page_data epd = {
>  		.bio_ctrl = { 0 },
>  		.extent_locked = 1,
>  		.sync_io = 1,
>  	};
>  	struct writeback_control wbc_writepages = {
> -		.nr_to_write	= nr_pages * 2,
>  		.sync_mode	= WB_SYNC_ALL,
>  		.range_start	= start,
>  		.range_end	= end + 1,
> @@ -5104,14 +5103,24 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
>  		.no_cgroup_owner = 1,
>  	};
>  
> +	ASSERT(IS_ALIGNED(start, sectorsize) &&
> +	       IS_ALIGNED(end + 1, sectorsize));
> +	nr_pages = (round_up(end, PAGE_SIZE) - round_down(start, PAGE_SIZE)) >>
> +		   PAGE_SHIFT;
> +	wbc_writepages.nr_to_write = nr_pages * 2;
> +
>  	wbc_attach_fdatawrite_inode(&wbc_writepages, inode);
>  	while (cur <= end) {
> +		u64 cur_end = min(round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1,
> +				  end);
> +
>  		page = find_get_page(mapping, cur >> PAGE_SHIFT);
>  		/*
>  		 * All pages in the range are locked since
>  		 * btrfs_run_delalloc_range(), thus there is no way to clear
>  		 * the page dirty flag.
>  		 */
> +		ASSERT(PageLocked(page));

We're tripping this ASSERT() with compression turned on, sorry I didn't notice
this but we've been panicing consistently since this was merged, so I've lost a
weeks worth of xfstests runs.  You can easily reproduce running

./check generic/029

with

MOUNT_OPTIONS="-o compress"

Thanks,

Josef