Re: [PATCH] btrfs: scrub: Require mandatory block group RO for dev-replace

[Qu WenRuo <wqu@suse.com>]

On 2020/1/23 下午8:06, Filipe Manana wrote:
> On Thu, Jan 23, 2020 at 7:38 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [BUG]
>> For dev-replace test cases with fsstress, like btrfs/06[45] btrfs/071,
>> looped runs can lead to random failure, where scrub finds csum error.
>>
>> The possibility is not high, around 1/20 to 1/100, but it's causing data
>> corruption.
>>
>> The bug is observable after commit b12de52896c0 ("btrfs: scrub: Don't
>> check free space before marking a block group RO")
>>
>> [CAUSE]
>> Dev-replace has two source of writes:
>> - Write duplication
>>   All writes to source device will also be duplicated to target device.
>>
>>   Content:      Latest data/meta
> 
> I find the term "latest" a bit confusing, perhaps "not yet persisted
> data and metadata" is more clear.
> 
>>
>> - Scrub copy
>>   Dev-replace reused scrub code to iterate through existing extents, and
>>   copy the verified data to target device.
>>
>>   Content:      Data/meta in commit root
> 
> And so here "previously persisted data and metadata".
> 
>>
>> The difference in contents makes the following race possible:
>>         Regular Writer          |       Dev-replace
>> -----------------------------------------------------------------
>>   ^                             |
>>   | Preallocate one data extent |
>>   | at bytenr X, len 1M         |
>>   v                             |
>>   ^ Commit transaction          |
>>   | Now extent [X, X+1M) is in  |
>>   v commit root                 |
>>  ================== Dev replace starts =========================
>>                                 | ^
>>                                 | | Scrub extent [X, X+1M)
>>                                 | | Read [X, X+1M)
>>                                 | | (The content are mostly garbage
>>                                 | |  since it's preallocated)
>>   ^                             | v
>>   | Write back happens for      |
>>   | extent [X, X+512K)          |
>>   | New data writes to both     |
>>   | source and target dev.      |
>>   v                             |
>>                                 | ^
>>                                 | | Scrub writes back extent [X, X+1M)
>>                                 | | to target device.
>>                                 | | This will over write the new data in
>>                                 | | [X, X+512K)
>>                                 | v
>>
>> This race can only happen for nocow writes. Thus metadata and data cow
>> writes are safe, as COW will never overwrite extents of previous trans
>> (in commit root).
>>
>> This behavior can be confirmed by disabling all fallocate related calls
>> in fsstress (*), then all related tests can pass a 2000 run loop.
>>
>> *: FSSTRESS_AVOID="-f fallocate=0 -f allocsp=0 -f zero=0 -f insert=0 \
>>                    -f collapse=0 -f punch=0 -f resvsp=0"
>>    I didn't expect resvsp ioctl will fallback to fallocate in VFS...
>>
>> [FIX]
>> Make dev-replace to require mandatory block group RO, and wait for current
>> nocow writes before calling scrub_chunk().
>>
>> This patch will mostly revert commit 76a8efa171bf ("btrfs: Continue replace
>> when set_block_ro failed") for dev-replace path.
>>
>> ENOSPC for dev-replace is still much better than data corruption.
> 
> Technically if we flag the block group RO without being able to
> persist that due to ENOSPC, it's still ok.

Right, I will change the words, since it only slightly increase the
possibility of ENOSPC, not ensured to cause ENOSPC and abort replace.

> We just want to prevent nocow writes racing with scrub copying
> procedure. But that's something for some other time, and this is fine
> to me.
> 
>>
>> Reported-by: Filipe Manana <fdmanana@suse.com>
>> Fixes: 76a8efa171bf ("btrfs: Continue replace when set_block_ro failed")
>> Fixes: b12de52896c0 ("btrfs: scrub: Don't check free space before marking a block group RO")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> RFC->v1:
>> - Remove the RFC tag
>>   Since the cause is pinned and verified, no need for RFC.
>>
>> - Only wait for nocow writes for dev-replace
>>   CoW writes are safe as they will never overwrite extents in commit
>>   root.
>>
[...]
>> +               /*
>> +                * Now the target block is marked RO, wait for nocow writes to
>> +                * finish before dev-replace.
>> +                * COW is fine, as COW never overwrites extents in commit tree.
>> +                */
>> +               if (sctx->is_dev_replace)
>> +                       btrfs_wait_nocow_writers(cache);
> 
> So this only waits for nocow ordered extents to be created - it
> doesn't wait for them to complete.
> After that you still need to call:
> 
> btrfs_wait_ordered_roots(fs_info, U64_MAX, cache->start, cache->length);

Forgot that, although 1000 runs doesn't expose any problem you are
completely right.

I'll update the patch to address all mentioned comments.

Thanks,
Qu

> 
> Other than that, looks good to me.
> 
> Thanks.
> 
>> +
>> +               scrub_pause_off(fs_info);
>>                 down_write(&dev_replace->rwsem);
>>                 dev_replace->cursor_right = found_key.offset + length;
>>                 dev_replace->cursor_left = found_key.offset;
>> --
>> 2.25.0
>>
> 
>