Re: [PATCH] btrfs: make Private2 lifespan more consistent

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

On 2021/1/22 下午4:24, Qu Wenruo wrote:
>
>
> On 2021/1/22 下午4:04, Nikolay Borisov wrote:
>>
>>
>> On 22.01.21 г. 8:00 ч., Qu Wenruo wrote:
>>> Currently btrfs uses page Private2 bit to incidate if we have ordered
>>> extent for the page range.
>>>
>>> But the lifespan of it is not consistent, during regular writeback path,
>>> there are two locations to clear the same PagePrivate2:
>>>
>>>      T ----- Page marked Dirty
>>>      |
>>>      + ----- Page marked Private2, through btrfs_run_dealloc_range()
>>>      |
>>>      + ----- Page cleared Private2, through btrfs_writepage_cow_fixup()
>>>      |       in __extent_writepage_io()
>>>      |       ^^^ Private2 cleared for the first time
>>>      |
>>>      + ----- Page marked Writeback, through btrfs_set_range_writeback()
>>>      |       in __extent_writepage_io().
>>>      |
>>>      + ----- Page cleared Private2, through
>>>      |       btrfs_writepage_endio_finish_ordered()
>>>      |       ^^^ Private2 cleared for the second time.>     |
>>>      + ----- Page cleared Writeback, through
>>>              btrfs_writepage_endio_finish_ordered()
>>
>> Where exactly is page writeback cleared in btrfs_writepage_endio_finish
>> or  finish_ordered_fn?
>
> My bad. It's in finish_ordered_io().

Oh no, it's in end_bio_extent_writepage().

This means my original subpage plan to fix is not corrrect at all.
The Private2 is cleared before clearing page Writeback anyway...

The fix should be reworked now...
>
>>
>>>
>>> Currently PagePrivate2 is mostly to prevent ordered extent accounting
>>> being executed for both endio and invalidatepage.
>>> Thus only the one who cleared page Private2 is responsible for ordered
>>> extent accounting.
>>
>> SO this patch likely fixes the race and double accounting you've seen on
>> the subpage branch,
>
> Nope, it's unrelated at all.
>
> The subpage problem is in the patch where I convert
> btrfs_writepage_endio_finish_ordered_io() to support subpage.
>
> In that patch I wrongly moved the timing of ClearPagePrivate2() after we
> queued the ordered extents.
>
> Thus there will no be specific patch for that fix, just update that
> patch to solve the problem.
>
>> however it's still not clear how the race occurs.
>
> There is no race in current code base.
>
> The invalidatepage() will wait writeback, thus it means there are the
> following possible combinations:
>
> - Page Writeback | Private2
>    Then invalidatepage() will wait for Writeback, and during endio,
>    Private2 will be cleared.
>
>    Accounting is done in endio.
>
> - Page Writeback but NO Private2
>    The same as previous cases
>
> - Page Private2 but NO Writeback
>    Invalidatepage() will just clear Private2 and do the ordered extent
>    accounting.
>
>    Accounting is done in invalidagepage()
>
> - Page without Private2 nor Writeback
>    Do nothing.
>
>
>
>> IIUC PagePrivate must ensure that invalidatepage and endio don't run
>> concurrently. To that effect invalidatepage indeed checks to see if it's
>> the one which cleared pageprivate and if so it will run
>> btrfs_dec_test_ordered_pending and btrfs_finish_ordered_io. However, in
>> __extent_writepage_io btrfs_writepage_cow_fixup clears it
>> unconditionally and calls btrfs_writepage_endio_finish_ordered for hole
>> extents, right?
>>
>> But in this case invalidate invalidatepage can never have
>> cleared_private2 set to true. IMO the actual problem this could lead
>> warrants more explanation.
> Your understanding is correct and it matches the correct code and my
> understanding too.
>
> Thus this patch is really just to make the life span easier to read, no
> functional change at all.
>
> Thanks,
> Qu
>
>>
>>>
>>> But the fact is, in btrfs_writepage_endio_finish_ordered(), page
>>> Private2 is cleared and ordered extent accounting is executed
>>> unconditionally.
>>>
>>> The race prevention only happens through btrfs_invalidatepage(), where
>>> we wait the page writeback first, before checking the Private2 bit.
>>>
>>> This means, Private2 is also protected by Writeback bit, and there is no
>>> need for btrfs_writepage_cow_fixup() to clear Priavte2.
>>>
>>> This patch will change btrfs_writepage_cow_fixup() to just
>>> check PagePrivate2, not to clear it.
>>> The clear will happen either in btrfs_invalidatepage() or
>>> btrfs_writepage_endio_finish_ordered().
>>>
>>
>> <snip>
>>