On Fri, Aug 03, 2018 at 11:39:28AM +0300, Nikolay Borisov wrote:
>
>
>On 3.08.2018 11:37, Misono Tomohiro wrote:
>> On 2018/08/03 16:15, Lu Fengqi wrote:
>>> On Fri, Aug 03, 2018 at 03:21:12PM +0900, Misono Tomohiro wrote:
>>>> When qgroup is on, subvolume deletion does not remove qgroup item
>>>> of the subvolume (qgroup info, limits, relation) from quota tree and
>>>> they needs to get removed manually by "btrfs qgroup destroy".
>>>>
>>>> Since level 0 qgroup cannot be used/inherited by any other subvolume,
>>>> let's remove them automatically when subvolume is deleted
>>>> (to be precise, when the subvolume root is dropped).
>>>>
>>>> Signed-off-by: Misono Tomohiro <misono.tomohiro@xxxxxxxxxxxxxx>
>>>
>>> Looks good to me.
>>>
>>> Reviewed-by: Lu Fengqi <lufq.fnst@xxxxxxxxxxxxxx>
>>
>> Thanks for the review.
>>
>>>
>>> There is an off-topic question below.
>>>
>>>> ---
>>>> Note that btrfs/057 fails, but it is the problem of testcase.
>>>> I will update it too.
>>>>
>>>> v1 -> v2:
>>>> Move call of btrfs_remove_qgroup() from btrfs_delete_subvolume()
>>>> to btrfs_snapshot_destroy() so that it will be called after the
>>>> subvolume root is really dropped
>>>>
>>>> fs/btrfs/extent-tree.c | 16 ++++++++++++----
>>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 9e7b237b9547..b56dea8c8b9f 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -8871,12 +8871,13 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
>>>> struct btrfs_root_item *root_item = &root->root_item;
>>>> struct walk_control *wc;
>>>> struct btrfs_key key;
>>>> + u64 objectid = root->objectid;
>>>> int err = 0;
>>>> int ret;
>>>> int level;
>>>> bool root_dropped = false;
>>>>
>>>> - btrfs_debug(fs_info, "Drop subvolume %llu", root->objectid);
>>>> + btrfs_debug(fs_info, "Drop subvolume %llu", objectid);
>>>>
>>>> path = btrfs_alloc_path();
>>>> if (!path) {
>>>> @@ -9030,7 +9031,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
>>>> goto out_end_trans;
>>>> }
>>>>
>>>> - if (root->root_key.objectid != BTRFS_TREE_RELOC_OBJECTID) {
>>>> + if (objectid != BTRFS_TREE_RELOC_OBJECTID) {
>>>
>>> Here use root->objectid instead of root->root_key.objectid. If I recall
>>> correctly, the root->objectid and root->root_key.objectid are set to the
>>> identical value. I just wonder if there is any difference between the two
>>> "objectid"s after the btrfs_root was created?
>>
>> in __setup_root(root, fs_info, objectid):
>> <snip>
>> root->objectid = objectid;
>> <snip>
>> root->root_key.objectid = objectid;
>> <snip>
>>
>> and I don't see any update of objectid from "grep -r "root_key.objectid ="",
>> I think it the same too (and fstests is ok), but any comment from
>> those who more familiar with code is helpful.
>
>Perhaps root->objectid should be removed altogether, if it's a duplicate
>of root->root_key.objectid
That's great! I hate these useless redundancies because they always make me
confused. So Misono could you update this patch to use
root->root_key.objectid?
--
Thanks,
Lu
>
>>
>> thanks,
>> Misono
>>
>>>
>>> --
>>> Thanks,
>>> Lu
>>>
>>>> ret = btrfs_find_root(tree_root, &root->root_key, path,
>>>> NULL, NULL);
>>>> if (ret < 0) {
>>>> @@ -9043,8 +9044,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
>>>> *
>>>> * The most common failure here is just -ENOENT.
>>>> */
>>>> - btrfs_del_orphan_item(trans, tree_root,
>>>> - root->root_key.objectid);
>>>> + btrfs_del_orphan_item(trans, tree_root, objectid);
>>>> }
>>>> }
>>>>
>>>> @@ -9056,6 +9056,14 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
>>>> btrfs_put_fs_root(root);
>>>> }
>>>> root_dropped = true;
>>>> +
>>>> + /* Remove level-0 qgroup items since no other subvolume can use them */
>>>> + ret = btrfs_remove_qgroup(trans, objectid);
>>>> + if (ret && ret != -EINVAL && ret != -ENOENT) {
>>>> + btrfs_abort_transaction(trans, ret);
>>>> + err = ret;
>>>> + }
>>>> +
>>>> out_end_trans:
>>>> btrfs_end_transaction_throttle(trans);
>>>> out_free:
>>>> --
>>>> 2.14.4
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> 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@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
>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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
