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

From: Nikolay Borisov <nborisov@suse.com>
To: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: make Private2 lifespan more consistent
Date: Fri, 22 Jan 2021 10:04:29 +0200	[thread overview]
Message-ID: <80e61dcf-e44c-44c9-ef8c-7efc81a136ea@suse.com> (raw)
In-Reply-To: <20210122060052.74365-1-wqu@suse.com>

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?

> 
> 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, however it's still not clear how the race occurs.
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.

> 
> 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>