Re: [PATCH 10/16] btrfs: remove non-standard extent handling in __extent_writepage_io

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

On 2023/5/31 15:34, Christoph Hellwig wrote:
> __extent_writepage_io is never called for compressed or inline extents,
> or holes.  Remove the not quite working code for them and replace it with
> asserts that these cases don't happen.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

It's a little too late, but this patch is causing crashing for subpage.

> ---
>   fs/btrfs/extent_io.c | 23 +++++------------------
>   1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 09a9973c27ccfb..a2e1dbd9b92309 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1361,7 +1361,6 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   	struct extent_map *em;
>   	int ret = 0;
>   	int nr = 0;
> -	bool compressed;
>
>   	ret = btrfs_writepage_cow_fixup(page);
>   	if (ret) {
> @@ -1419,10 +1418,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   		ASSERT(cur < end);
>   		ASSERT(IS_ALIGNED(em->start, fs_info->sectorsize));
>   		ASSERT(IS_ALIGNED(em->len, fs_info->sectorsize));
> +
>   		block_start = em->block_start;
> -		compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
>   		disk_bytenr = em->block_start + extent_offset;
>
> +		ASSERT(!test_bit(EXTENT_FLAG_COMPRESSED, &em->flags));
> +		ASSERT(block_start != EXTENT_MAP_HOLE);

For subpage cases, __extent_writepage_io() can be triggered to write
only a subset of the page, from extent_write_locked_range().

In that case, if we have submitted the target range, since our @len is
to the end of the page, we can hit a hole.

In that case, this ASSERT() would be triggered.
And even worse, if CONFIG_BTRFS_ASSERT() is not enabled, we can do wrong
writeback using the wrong disk_bytenr.

So at least we need to skip the hole ranges for subpage.
And thankfully the remaining two cases are impossible for subpage.

Thanks,
Qu

> +		ASSERT(block_start != EXTENT_MAP_INLINE);
> +
>   		/*
>   		 * Note that em_end from extent_map_end() and dirty_range_end from
>   		 * find_next_dirty_byte() are all exclusive
> @@ -1431,22 +1434,6 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   		free_extent_map(em);
>   		em = NULL;
>
> -		/*
> -		 * compressed and inline extents are written through other
> -		 * paths in the FS
> -		 */
> -		if (compressed || block_start == EXTENT_MAP_HOLE ||
> -		    block_start == EXTENT_MAP_INLINE) {
> -			if (compressed)
> -				nr++;
> -			else
> -				btrfs_writepage_endio_finish_ordered(inode,
> -						page, cur, cur + iosize - 1, true);
> -			btrfs_page_clear_dirty(fs_info, page, cur, iosize);
> -			cur += iosize;
> -			continue;
> -		}
> -
>   		btrfs_set_range_writeback(inode, cur, cur + iosize - 1);
>   		if (!PageWriteback(page)) {
>   			btrfs_err(inode->root->fs_info,