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
>
> 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
