Re: [PATCH v4 01/18] btrfs: update locked page dirty/writeback/error bits in __process_pages_contig()

[Qu Wenruo <wqu@suse.com>]

On 2021/1/24 上午3:13, David Sterba wrote:
> On Thu, Jan 21, 2021 at 02:51:46PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/1/21 下午2:32, Qu Wenruo wrote:
>>>
>>>
>>> On 2021/1/20 上午5:41, Josef Bacik wrote:
>>>> On 1/16/21 2:15 AM, Qu Wenruo wrote:
>>>>> When __process_pages_contig() get called for
>>>>> extent_clear_unlock_delalloc(), if we hit the locked page, only Private2
>>>>> bit is updated, but dirty/writeback/error bits are all skipped.
>>>>>
>>>>> There are several call sites call extent_clear_unlock_delalloc() with
>>>>> @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK
>>>>>
>>>>> - cow_file_range()
>>>>> - run_delalloc_nocow()
>>>>> - cow_file_range_async()
>>>>>     All for their error handling branches.
>>>>>
>>>>> For those call sites, since we skip the locked page for
>>>>> dirty/error/writeback bit update, the locked page will still have its
>>>>> dirty bit remaining.
>>>>>
>>>>> Thankfully, since all those call sites can only be hit with various
>>>>> serious errors, it's pretty hard to hit and shouldn't affect regular
>>>>> btrfs operations.
>>>>>
>>>>> But still, we shouldn't leave the locked_page with its
>>>>> dirty/error/writeback bits untouched.
>>>>>
>>>>> Fix this by only skipping lock/unlock page operations for locked_page.
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>
>>>> Except this is handled by the callers.  We clear_page_dirty_for_io() the
>>>> page before calling btrfs_run_delalloc_range(), so we don't need the
>>>> PAGE_CLEAR_DIRTY, it's already cleared.  The SetPageError() is handled
>>>> in the error path for locked_page, as is the
>>>> set_writeback/end_writeback.  Now I don't think this patch causes
>>>> problems specifically, but the changelog is at least wrong, and I'd
>>>> rather we'd skip the handling of the locked_page here and leave it in
>>>> the proper error handling.  If you need to do this for some other reason
>>>> that I haven't gotten to yet then you need to make that clear in the
>>>> changelog, because as of right now I don't see why this is needed.
>>>> Thanks,
>>>
>>> This is mostly to co-operate with a later patch on
>>> __process_pages_contig(), where we need to make sure page locked by
>>> __process_pages_contig() is only unlocked by __process_pages_contig() too.
>>>
>>> The exception is after cow_file_inline(), we call
>>> __process_pages_contig() on the locked page, making it to clear page
>>> writeback and unlock it.
>>
>> To be more clear, we call extent_clear_unlock_delalloc() with
>> locked_page == NULL, to allow __process_pages_contig() to unlock the
>> locked page (while the locked page isn't locked by
>> __process_pages_contig()).
>>
>> For subpage data, we need writers accounting for subpage, but that
>> accounting only happens in __process_pages_contig(), thus we don't want
>> pages without the accounting to be unlocked by __process_pages_contig().
>>
>> I can do extra page unlock/clear_dirty/end_writeback just for that
>> exception, but it would definitely needs more comments.
> 
> This is patch 1 and other depend on the changed behaviour so if it's
> just updated changelog and comments, and Josef is ok with the result, I
> can take it but otherwise this could delay the series once the rest is
> reworked.
> 
In fact there aren't many changes depending on it, until we hit RW support.

Thus I can move this patch to RW series, so that we can fully focus on 
RO support.

The patchset will be delayed for a while (ETA in week 04), mostly due to 
the change in how we handle metadata ref count (other than just 
under_alloc).

Would this be OK for you?

Thanks,
Qu