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

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

On 2024/2/13 02:22, Christoph Hellwig wrote:
> On Sat, Feb 10, 2024 at 08:09:47PM +1030, Qu Wenruo wrote:
>>> @@ -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().
>
> Yes.
>
>> 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.
>
> The patch below reinstates the hole handling.  I don't have a system
> that tests the btrfs subpage code right now, so this is untested:
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index cfd2967f04a293..a106036641104c 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1388,7 +1388,6 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   		disk_bytenr = em->block_start + extent_offset;
>
>   		ASSERT(!extent_map_is_compressed(em));
> -		ASSERT(block_start != EXTENT_MAP_HOLE);
>   		ASSERT(block_start != EXTENT_MAP_INLINE);
>
>   		/*
> @@ -1399,6 +1398,15 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   		free_extent_map(em);
>   		em = NULL;
>
> +		if (block_start == EXTENT_MAP_HOLE) {
> +			btrfs_mark_ordered_io_finished(inode, page, cur, iosize,
> +						       true);
> +			btrfs_folio_clear_dirty(fs_info, page_folio(page), cur,
> +						iosize);
> +			cur += iosize;
> +			continue;
> +		}
> +
>   		btrfs_set_range_writeback(inode, cur, cur + iosize - 1);
>   		if (!PageWriteback(page)) {
>   			btrfs_err(inode->root->fs_info,

There are more problem than I initially thought.

In fact, I went another path to make __extent_writepage_io() to only
submit IO for the desired range.

But we would have another problem related to @locked_page handling.

For extent_write_locked_range(), we expect to unlock the @locked_page.
But for subpage case, we can have multiple dirty ranges.

In that case, if we unlock @locked_page for the first iteration, the
next extent_write_locked_range() iteration can still try to get the same
page, and found the page unlocked, triggering an ASSERT().

So the patch itself is not the root cause, it's the lack of subpage
locking causing the problem.

Sorry for the false alerts.

Thanks,
Qu