Re: [PATCH v6 23/28] btrfs: support dev-replace in HMZONED mode

Josef Bacik
2019-12-18T14:58:12Z

On 12/18/19 1:00 AM, Naohiro Aota wrote:
> On Tue, Dec 17, 2019 at 04:05:25PM -0500, Josef Bacik wrote:
>> On 12/12/19 11:09 PM, Naohiro Aota wrote:
>>> We have two type of I/Os during the device-replace process. One is a I/O to
>>> "copy" (by the scrub functions) all the device extents on the source device
>>> to the destination device.  The other one is a I/O to "clone" (by
>>> handle_ops_on_dev_replace()) new incoming write I/Os from users to the
>>> source device into the target device.
>>>
>>> Cloning incoming I/Os can break the sequential write rule in the target
>>> device. When write is mapped in the middle of a block group, that I/O is
>>> directed in the middle of a zone of target device, which breaks the
>>> sequential write rule.
>>>
>>> However, the cloning function cannot be simply disabled since incoming I/Os
>>> targeting already copied device extents must be cloned so that the I/O is
>>> executed on the target device.
>>>
>>> We cannot use dev_replace->cursor_{left,right} to determine whether bio is
>>> going to not yet copied region.  Since we have time gap between finishing
>>> btrfs_scrub_dev() and rewriting the mapping tree in
>>> btrfs_dev_replace_finishing(), we can have newly allocated device extent
>>> which is never cloned nor copied.
>>>
>>> So the point is to copy only already existing device extents. This patch
>>> introduces mark_block_group_to_copy() to mark existing block group as a
>>> target of copying. Then, handle_ops_on_dev_replace() and dev-replace can
>>> check the flag to do their job.
>>>
>>> Device-replace process in HMZONED mode must copy or clone all the extents
>>> in the source device exctly once.  So, we need to use to ensure allocations
>>> started just before the dev-replace process to have their corresponding
>>> extent information in the B-trees. finish_extent_writes_for_hmzoned()
>>> implements that functionality, which basically is the removed code in the
>>> commit 042528f8d840 ("Btrfs: fix block group remaining RO forever after
>>> error during device replace").
>>>
>>> This patch also handles empty region between used extents. Since
>>> dev-replace is smart to copy only used extents on source device, we have to
>>> fill the gap to honor the sequential write rule in the target device.
>>>
>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>
>> Can you split up the copying part and the cloning part into different patches,
>> this is a bear to review.  Also I don't quite understand the zeroout behavior.
>> It _looks_ like for cloning you are doing a zeroout for the gap between the
>> last wp position and the current cloned bio, which makes sense, but doesn't
>> this gap exist because copying is ongoing?  Can you copy into a zero'ed out
>> position?  Or am I missing something here?  Thanks,
>>
>> Josef
>
> OK, I will split this in the next version. (but, it's mostly "copying"
> part)
>
> Let me clarify first that I am using "copying" for copying existing
> extents to the new device and "cloning" for cloning a new incoming BIO
> to the new device.
>
> For zeroout, it is for "copying" which is done with the scrub code to
> copy existing extents on the source devie to the destination
> device. Since copying or scrub only scans for living extents, there
> can be a gap between two living extents. So, we need to fill a gap
> with zeroout to make the writing stream sequential.
>
> And "cloning" is only done for new block groups or already fully
> copied block groups. So there is no gaps for them because the
> allocator and the IO locks ensures the sequential allocation and
> submit.
>

Got it, thanks. It looked like the cloning part was using the zeroout behavior
which was what was confusing me.

Josef