Re: [PATCH 2/4] btrfs: fix race between writes to swap files and scrub

[Anand Jain <anand.jain@oracle.com>]

On 2/4/2021 6:11 PM, Filipe Manana wrote:
> On Thu, Feb 4, 2021 at 8:48 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> On 2/3/2021 7:17 PM, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> When we active a swap file, at btrfs_swap_activate(), we acquire the
>>> exclusive operation lock to prevent the physical location of the swap
>>> file extents to be changed by operations such as balance and device
>>> replace/resize/remove. We also call there can_nocow_extent() which,
>>> among other things, checks if the block group of a swap file extent is
>>> currently RO, and if it is we can not use the extent, since a write
>>> into it would result in COWing the extent.
>>>
>>> However we have no protection against a scrub operation running after we
>>> activate the swap file, which can result in the swap file extents to be
>>> COWed while the scrub is running and operating on the respective block
>>> group, because scrub turns a block group into RO before it processes it
>>> and then back again to RW mode after processing it. That means an attempt
>>> to write into a swap file extent while scrub is processing the respective
>>> block group, will result in COWing the extent, changing its physical
>>> location on disk.
>>>
>>> Fix this by making sure that block groups that have extents that are used
>>> by active swap files can not be turned into RO mode, therefore making it
>>> not possible for a scrub to turn them into RO mode.
>>
>>> When a scrub finds a
>>> block group that can not be turned to RO due to the existence of extents
>>> used by swap files, it proceeds to the next block group and logs a warning
>>> message that mentions the block group was skipped due to active swap
>>> files - this is the same approach we currently use for balance.
>>
>>    It is better if this info is documented in the scrub man-page. IMO.
>>
>>> This ends up removing the need to call btrfs_extent_readonly() from
>>> can_nocow_extent(), as btrfs_swap_activate() now checks if a block group
>>> is RO through the new function btrfs_inc_block_group_swap_extents().
>>>


>>> The only other caller of can_nocow_extent() is the direct IO write path,

There is a third caller. check_can_nocow() also calls 
can_nocow_extent(), which I missed before. Any idea where does it get to 
know that extent is RO in the threads using check_can_nocow()? I have to 
back out the RB for this reason for now.


>>> btrfs_get_blocks_direct_write(), but that already checks if a block group
>>> is RO through the call to btrfs_inc_nocow_writers().

>>> In fact, after this
>>> change we end up optimizing the direct IO write path, since we no longer
>>> iterate the block groups rbtree twice, once with btrfs_extent_readonly(),
>>> through can_nocow_extent(), and once again with btrfs_inc_nocow_writers().
>>> This can save time and reduce contention on the lock that protects the
>>> rbtree (specially because it is a spinlock and not a read/write lock) on
>>> very large filesystems, with several thousands of allocated block groups.
>>>
>>> Fixes: ed46ff3d42378 ("Btrfs: support swap files")
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>
>>    I am not sure about the optimization of direct IO part, but as such
>>    changes looks good.

Clarifying about the optimization part (for both buffered and direct IO) 
- After patch 1, and patch 2, now we check on the RO extents after the 
functions btrfs_cross_ref_exist(), and csum_exist_in_range(), both of 
them have search_slot, whereas, before patch 1, and patch 2, we used to 
fail early (if the extent is RO) and avoided the search_slot, so I am 
not sure if there is optimization.

Thanks, Anand