Re: [PATCH AUTOSEL 5.11 55/67] btrfs: only let one thread pre-flush delayed refs in commit

[David Sterba]

On Wed, Feb 24, 2021 at 07:50:13AM -0500, Sasha Levin wrote:
> From: Josef Bacik <josef@xxxxxxxxxxxxxx>
> 
> [ Upstream commit e19eb11f4f3d3b0463cd897016064a79cb6d8c6d ]
> 
> I've been running a stress test that runs 20 workers in their own
> subvolume, which are running an fsstress instance with 4 threads per
> worker, which is 80 total fsstress threads.  In addition to this I'm
> running balance in the background as well as creating and deleting
> snapshots.  This test takes around 12 hours to run normally, going
> slower and slower as the test goes on.
> 
> The reason for this is because fsstress is running fsync sometimes, and
> because we're messing with block groups we often fall through to
> btrfs_commit_transaction, so will often have 20-30 threads all calling
> btrfs_commit_transaction at the same time.
> 
> These all get stuck contending on the extent tree while they try to run
> delayed refs during the initial part of the commit.
> 
> This is suboptimal, really because the extent tree is a single point of
> failure we only want one thread acting on that tree at once to reduce
> lock contention.
> 
> Fix this by making the flushing mechanism a bit operation, to make it
> easy to use test_and_set_bit() in order to make sure only one task does
> this initial flush.
> 
> Once we're into the transaction commit we only have one thread doing
> delayed ref running, it's just this initial pre-flush that is
> problematic.  With this patch my stress test takes around 90 minutes to
> run, instead of 12 hours.
> 
> The memory barrier is not necessary for the flushing bit as it's
> ordered, unlike plain int. The transaction state accessed in
> btrfs_should_end_transaction could be affected by that too as it's not
> always used under transaction lock. Upon Nikolay's analysis in [1]
> it's not necessary:
> 
>   In should_end_transaction it's read without holding any locks. (U)
> 
>   It's modified in btrfs_cleanup_transaction without holding the
>   fs_info->trans_lock (U), but the STATE_ERROR flag is going to be set.
> 
>   set in cleanup_transaction under fs_info->trans_lock (L)
>   set in btrfs_commit_trans to COMMIT_START under fs_info->trans_lock.(L)
>   set in btrfs_commit_trans to COMMIT_DOING under fs_info->trans_lock.(L)
>   set in btrfs_commit_trans to COMMIT_UNBLOCK under
>   fs_info->trans_lock.(L)
> 
>   set in btrfs_commit_trans to COMMIT_COMPLETED without locks but at this
>   point the transaction is finished and fs_info->running_trans is NULL (U
>   but irrelevant).
> 
>   So by the looks of it we can have a concurrent READ race with a WRITE,
>   due to reads not taking a lock. In this case what we want to ensure is
>   we either see new or old state. I consulted with Will Deacon and he said
>   that in such a case we'd want to annotate the accesses to ->state with
>   (READ|WRITE)_ONCE so as to avoid a theoretical tear, in this case I
>   don't think this could happen but I imagine at some point KCSAN would
>   flag such an access as racy (which it is).
> 
> [1] https://lore.kernel.org/linux-btrfs/e1fd5cc1-0f28-f670-69f4-e9958b4964e6@xxxxxxxx
> 
> Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> [ add comments regarding memory barrier ]
> Signed-off-by: David Sterba <dsterba@xxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

Please drop this patch from autosel queue, it's part of a larger series
that reworks flushing and is not a standalone fix.