Re: [PATCH v2 06/17] btrfs-progs: lowmem check: introduce force_cow_in_new_chunk()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8672 bytes --]



On 2017年12月26日 16:17, Su Yue wrote:
> 
> 
> On 12/21/2017 03:12 PM, Qu Wenruo wrote:
>>
>>
>> On 2017年12月21日 15:09, Su Yue wrote:
>>>
>>>
>>> On 12/21/2017 02:51 PM, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2017年12月20日 16:37, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> On 2017年12月20日 16:21, Su Yue wrote:
>>>>>>
>>>>>>
>>>>>> On 12/20/2017 01:41 PM, Qu Wenruo wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2017年12月20日 12:57, Su Yue wrote:
>>>>>>>> Introduce create_chunk_and_block_block_group() to allocate new
>>>>>>>> chunk
>>>>>>>> and corresponding block group.
>>>>>>>>
>>>>>>>> The new function force_cow_in_new_chunk() first allocates new chunk
>>>>>>>> and records its start.
>>>>>>>> Then it modifies all metadata block groups cached and full.
>>>>>>>> Finally it marks the new block group uncached and unfree.
>>>>>>>> In the next CoW, extents states will be updated automatically by
>>>>>>>> cache_block_group().
>>>>>>>>
>>>>>>>> Suggested-by: Qu Wenruo <wqu@suse.com>
>>>>>>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>>>>>>> ---
>>>>>>>>     cmds-check.c | 80
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>     1 file changed, 80 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/cmds-check.c b/cmds-check.c
>>>>>>>> index d98d4bda7357..311c8a9f45e8 100644
>>>>>>>> --- a/cmds-check.c
>>>>>>>> +++ b/cmds-check.c
>>>>>>>> @@ -10911,6 +10911,86 @@ out:
>>>>>>>>         return ret;
>>>>>>>>     }
>>>>>>>>     +static int create_chunk_and_block_group(struct btrfs_fs_info
>>>>>>>> *fs_info,
>>>>>>>> +                    u64 flags, u64 *start, u64 *nbytes)
>>>>>>>> +{
>>>>>>>> +    struct btrfs_trans_handle *trans;
>>>>>>>> +    struct btrfs_root *root = fs_info->extent_root;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    if ((flags & BTRFS_BLOCK_GROUP_TYPE_MASK) == 0)
>>>>>>>> +        return -EINVAL;
>>>>>>>> +
>>>>>>>> +    trans = btrfs_start_transaction(root, 1);
>>>>>>>> +    if (IS_ERR(trans)) {
>>>>>>>> +        ret = PTR_ERR(trans);
>>>>>>>> +        error("error starting transaction %s", strerror(-ret));
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +    ret = btrfs_alloc_chunk(trans, fs_info, start, nbytes, flags);
>>>>>>>> +    if (ret) {
>>>>>>>> +        error("fail to allocate new chunk %s", strerror(-ret));
>>>>>>>> +        goto out;
>>>>>>>> +    }
>>>>>>>> +    ret = btrfs_make_block_group(trans, fs_info, 0, flags,
>>>>>>>> +                 BTRFS_FIRST_CHUNK_TREE_OBJECTID, *start,
>>>>>>>> *nbytes);
>>>>>>>> +    if (ret) {
>>>>>>>> +        error("fail to make block group for chunk %llu %llu %s",
>>>>>>>> +              *start, *nbytes, strerror(-ret));
>>>>>>>> +        goto out;
>>>>>>>> +    }
>>>>>>>> +out:
>>>>>>>> +    btrfs_commit_transaction(trans, root);
>>>>>>>> +    return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int force_cow_in_new_chunk(struct btrfs_fs_info *fs_info)
>>>>>>>> +{
>>>>>>>> +    struct btrfs_block_group_cache *bg;
>>>>>>>> +    u64 start;
>>>>>>>> +    u64 nbytes;
>>>>>>>> +    u64 alloc_profile;
>>>>>>>> +    u64 flags;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    alloc_profile = (fs_info->avail_metadata_alloc_bits &
>>>>>>>> +             fs_info->metadata_alloc_profile);
>>>>>>>> +    flags = BTRFS_BLOCK_GROUP_METADATA | alloc_profile;
>>>>>>>> +    if (btrfs_fs_incompat(fs_info, MIXED_GROUPS))
>>>>>>>> +        flags |= BTRFS_BLOCK_GROUP_DATA;
>>>>>>>> +
>>>>>>>> +    ret = create_chunk_and_block_group(fs_info, flags, &start,
>>>>>>>> &nbytes);
>>>>>>>
>>>>>>> Why bother allocating a new chunk by yourself?
>>>>>>>
>>>>>>> Just mark all block groups full and that's all.
>>>>>>>
>>>>>>> Any later tree block allocation like btrfs_search_slot() with
>>>>>>> @cow = 1
>>>>>>> will trigger chunk allocation automatically.
>>>>>>>
>>>>>>
>>>>>> Tried to let it happen but BUG_ON with -ENOSPC.
>>>>>
>>>>> Then fix it.
>>>>>
>>>>> It's not a normal behavior in this case.
>>>>>
>>>>> Thanks,
>>>>> Qu
>>>>
>>>> And I think the fix to allow btrfs_reserve_extent() to allocate new
>>>> chunk will solve a lot of strange BUG_ON().
>>>>
>>>> it just occurred to me that, a lot of use cases relies on the
>>>> assumption
>>>> that btrfs_reserve_extent() will try to allocate new chunks.
>>>>
>>>> Especially for case like convert, certain btrfs check --repair, some
>>>> rescue tools.
>>>>
>>> Sorry for the previous wrong format mail.
>>>
>>> Yes, it has many dependency so I considered to do chunk allocation
>>> manually in the patchset. If fix is not good enough, many funtions
>>> of btrfs-progs will behave abnormal.
>>> Since you ask it, I will go to fix it.
>>
>> Manually allocation in advance has its advantage, like we can determine
>> if there is enough space for new chunk instead of checking every return
>> value with ENOSPC.
>>
>>
>> However in current case, your metadata usage is limited to the new chunk
>> only.
>> If there extent tree has quite a lot of problem, and the chunk allocated
>> is small (if using single profile and small fs), it can easily hit
>> ENOSPC again, since btrfs doesn't allocate new chunk for later metadata
>> write.
>>
> SAD. After I tried to implement above nice idea, infinite recursive
> brings me back to the reality.
> 
> Here is the reason why btrfs_reserve_extent can not allocate chunk
> by itself if ENOSPC hints:
> 
> btrfs_cow_block
> ...
>   btrfs_reserve_extent
>     btrfs_alloc_chunk
>       btrfs_alloc_dev_extent
>         btrfs_insert_empty_item
>           ...
>            btrfs_cow_block

The order is just wrong.

We should first check for the new chunk space, not doing the insert
right now.

I'll take over the work if you're OK with it.

Thanks,
Qu

> 
> Thanks,
> Su
> 
>> So here, we still need to allow btrfs allocate new meta chunk, even we
>> pre-allocate one meta chunk in advance.
>>
>> Thanks,
>> Qu
>>>
>>> Thanks,
>>> Su
>>>
>>>> So this would be a quite nice start point for such fix.
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>>
>>>>>> Do you mean do_chunk_alloc() in btrfs_reserve_extent() which is
>>>>>> called
>>>>>> while doing CoW?> In progs, allocation of new chunk during CoW
>>>>>> depends @root->ref_cows.
>>>>>> However, @root->ref_cows should be set only if @root is root of a fs
>>>>>> trees.
>>>>>>
>>>>>> Thanks,
>>>>>> Su
>>>>>>
>>>>>>> Thanks,
>>>>>>> Qu
>>>>>>>
>>>>>>>> +    if (!ret)
>>>>>>>> +        printf("Create new chunk %llu %llu\n", start, nbytes);
>>>>>>>> +    else
>>>>>>>> +        goto err;
>>>>>>>> +
>>>>>>>> +    flags = BTRFS_BLOCK_GROUP_METADATA;
>>>>>>>> +    /* Mark all metadata block groups cached and full in free
>>>>>>>> space*/
>>>>>>>> +    ret = modify_block_groups_cache(fs_info, flags, 1);
>>>>>>>> +    if (ret)
>>>>>>>> +        goto clear_bg_cache;
>>>>>>>> +
>>>>>>>> +    bg = btrfs_lookup_block_group(fs_info, start);
>>>>>>>> +    if (!bg) {
>>>>>>>> +        ret = -ENOENT;
>>>>>>>> +        error("fail to look up block group %llu %llu", start,
>>>>>>>> nbytes);
>>>>>>>> +        goto clear_bg_cache;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /* Clear block group cache just allocated */
>>>>>>>> +    ret = modify_block_group_cache(fs_info, bg, 0);
>>>>>>>> +    if (ret)
>>>>>>>> +        goto clear_bg_cache;
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +
>>>>>>>> +clear_bg_cache:
>>>>>>>> +    modify_block_groups_cache(fs_info, flags, 0);
>>>>>>>> +err:
>>>>>>>> +    return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>     static int check_extent_refs(struct btrfs_root *root,
>>>>>>>>                      struct cache_tree *extent_cache)
>>>>>>>>     {
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>>> linux-btrfs" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>>
>>>
>>
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]