On 2.01.19 г. 12:08 ч., Qu Wenruo wrote:
>
>
> On 2019/1/2 下午4:57, Nikolay Borisov wrote:
>>
>>
>> On 25.12.18 г. 7:28 ч., Qu Wenruo wrote:
>>> The code is mostly ported from kernel with minimal change.
>>>
>>> Since btrfs-progs doesn't support replaying log, there is some code
>>> unnecessary for btrfs-progs, but to keep the code the same, that
>>> unnecessary code is kept as it.
>>>
>>> Now "btrfs check --repair" will update backup roots correctly.
>>>
>>> Signed-off-by: Qu Wenruo <w...@suse.com>
>>> ---
>>> disk-io.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 78 insertions(+)
>>>
>>> diff --git a/disk-io.c b/disk-io.c
>>> index 133835a4d063..f28a6b526391 100644
>>> --- a/disk-io.c
>>> +++ b/disk-io.c
>>> @@ -1621,6 +1621,83 @@ write_err:
>>> return ret;
>>> }
>>>
>>> +/*
>>> + * copy all the root pointers into the super backup array.
>>> + * this will bump the backup pointer by one when it is
>>> + * done
>>> + */
>>> +static void backup_super_roots(struct btrfs_fs_info *info)
>>> +{
>>> + struct btrfs_root_backup *root_backup;
>>> + int next_backup;
>>> + int last_backup;
>>> +
>>> + last_backup = find_best_backup_root(info->super_copy);
>>
>> Slightly offtopic: but find_best_backup_root only partially resembles
>> it's kernel counterpart - find_newest_super_backup. And it's missing the
>> code for handling wraparound. So in kernel space if the newest root is
>> at index 3 and index 0 backup also has the same gen then we switch the
>> index to 0, why is this needed?
>
> I'm not sure either.
>
> IIRC kernel shouldn't create two backup roots with the same gen.
>
>> In any case I think
>> 'find_best_backup_root' in progs should be renamed to
>> 'find_newest_super_backup' and copied from kernel space.
>>
>>> + next_backup = (last_backup + 1) % BTRFS_NUM_BACKUP_ROOTS;
>>
>> This logic is different than the kernel one which does:
>> last_backup = (next_backup + BTRFS_NUM_BACKUP_ROOTS - 1) %
>> BTRFS_NUM_BACKUP_ROOTS;
>>
>> So in the kernel when info->backup_root_index == 0 then the newest
>> backup root is going to be written at position 3, the next one in pos 1,
>> the next in pos 2 and so on. OTOH in userspace we might potentially
>> start writing at 1 (which doesn't seem problematic per se). So why the
>> difference in logic?
>
> The logic is the same, for kernel it's 'last_backup' which should, and
> is, going backward one slot.
>
> And the next one is still written to next slot.
On a second look at the code you are right and I like more progs'
version - it's more explicit. I think the performance cost is going to
be negligible so it might be worth it copying the implementation to
kernel (once the overflow handling is figured out, I've asked Chris for
clarification).
>
> Just as the dump super shows:
> $ btrfs ins dump-super -f /dev/data/btrfs | grep -A1 "backup "
> backup 0:
> backup_tree_root: 30818304 gen: 10 level: 0
> --
> backup 1:
> backup_tree_root: 30621696 gen: 7 level: 0
> --
> backup 2:
> backup_tree_root: 30703616 gen: 8 level: 0
> --
> backup 3:
> backup_tree_root: 30785536 gen: 9 level: 0
>
> So the logic should be the same.
>
> Just simpler implementation in btrfs-progs, without
> fs_info->backup_root_index.
<snip>
