Re: [PATCH] btrfs: make __extent_writepage() not return error if the page is marked error

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

On 2021/7/22 上午3:30, Josef Bacik wrote:
> On 7/20/21 7:45 AM, Qu Wenruo wrote:
>> [BUG]
>> When running btrfs/160 in a loop for subpage with experimental
>> compression support, it has a high chance to crash (~20%):
>>
>>   BTRFS critical (device dm-7): panic in
>> __btrfs_add_ordered_extent:238: inconsistency in ordered tree at
>> offset 0 (errno=-17 Object already exists)
>>   ------------[ cut here ]------------
>>   kernel BUG at fs/btrfs/ordered-data.c:238!
>>   Internal error: Oops - BUG: 0 [#1] SMP
>>   pc : __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
>>   lr : __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
>>   Call trace:
>>    __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
>>    btrfs_add_ordered_extent+0x2c/0x50 [btrfs]
>>    run_delalloc_nocow+0x81c/0x8fc [btrfs]
>>    btrfs_run_delalloc_range+0xa4/0x390 [btrfs]
>>    writepage_delalloc+0xc0/0x1ac [btrfs]
>>    __extent_writepage+0xf4/0x370 [btrfs]
>>    extent_write_cache_pages+0x288/0x4f4 [btrfs]
>>    extent_writepages+0x58/0xe0 [btrfs]
>>    btrfs_writepages+0x1c/0x30 [btrfs]
>>    do_writepages+0x60/0x110
>>    __filemap_fdatawrite_range+0x108/0x170
>>    filemap_fdatawrite_range+0x20/0x30
>>    btrfs_fdatawrite_range+0x34/0x4dc [btrfs]
>>    __btrfs_write_out_cache+0x34c/0x480 [btrfs]
>>    btrfs_write_out_cache+0x144/0x220 [btrfs]
>>    btrfs_start_dirty_block_groups+0x3ac/0x6b0 [btrfs]
>>    btrfs_commit_transaction+0xd0/0xbb4 [btrfs]
>>    btrfs_sync_fs+0x64/0x1cc [btrfs]
>>    sync_fs_one_sb+0x3c/0x50
>>    iterate_supers+0xcc/0x1d4
>>    ksys_sync+0x6c/0xd0
>>    __arm64_sys_sync+0x1c/0x30
>>    invoke_syscall+0x50/0x120
>>    el0_svc_common.constprop.0+0x4c/0xd4
>>    do_el0_svc+0x30/0x9c
>>    el0_svc+0x2c/0x54
>>    el0_sync_handler+0x1a8/0x1b0
>>    el0_sync+0x198/0x1c0
>>   ---[ end trace 336f67369ae6e0af ]---
>>
>> [CAUSE]
>> For subpage case, we can have multiple sectors inside a page, this makes
>> it possible for __extent_writepage() to have part of its page submitted
>> before returning.
>>
>> In btrfs/160, we are using dm-dust to emulate write error, this means
>> for certain pages, we could have everything running fine, but at the end
>> of __extent_writepage(), one of the submitted bios fails due to dm-dust.
>>
>> Then the page is marked Error, and we change @ret from 0 to -EIO.
>>
>> This makes the caller extent_write_cache_pages() to error out, without
>> submitting the remaining pages.
>>
>> Furthermore, since we're erroring out for free space cache, it doesn't
>> really care about the error and will update the inode and retry the
>> writeback.
>>
>> Then we re-run the delalloc range, and will try to insert the same
>> delalloc range while previous delalloc range is still hanging there,
>> triggering the above error.
>
> This seems like the real bug.

That's true.

>  We should do the proper cleanup for the
> range in this case, not ignore errors during writeback.  Thanks,

But if you change the view point, __extent_writepage() is only
submitting the pages to bio.
It shouldn't bother the bio error, but only care the error that affects
the bio submission.

This PageError() check makes the behavior different between subpage and
regular page size, thus this can be considered as a quick fix for subpage.

For a proper fix for both subpage and non-subpage case, I'm trying a
completely different way, and will send out a RFC for comment later.

Thanks,
Qu

>
> Josef