On 2018/08/03 18:16, Lu Fengqi wrote:
> 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?
Ok. Also I'll try to see if it is possible to remove root->objectid.
Misono
--
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
