[v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion
diff mbox series

Message ID 9e46637d-f717-7210-06e2-a077a58d9638@jp.fujitsu.com
State New
Headers show
Series
  • [v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion
Related show

Commit Message

Misono Tomohiro Aug. 3, 2018, 6:21 a.m. UTC
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@jp.fujitsu.com>
---
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(-)

Comments

Lu Fengqi Aug. 3, 2018, 7:15 a.m. UTC | #1
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@jp.fujitsu.com>

Looks good to me.

Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

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?

--
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@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
Misono Tomohiro Aug. 3, 2018, 8:37 a.m. UTC | #2
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@jp.fujitsu.com>
> 
> Looks good to me.
> 
> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

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.

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

--
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
Nikolay Borisov Aug. 3, 2018, 8:39 a.m. UTC | #3
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@jp.fujitsu.com>
>>
>> Looks good to me.
>>
>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
> 
> 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@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
>>
> 
> --
> 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
Qu Wenruo Aug. 3, 2018, 8:46 a.m. UTC | #4
On 2018年08月03日 16:39, 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@jp.fujitsu.com>
>>>
>>> Looks good to me.
>>>
>>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

[snip]
>>>> -	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

Yep, cleaning up root->objectid looks pretty good to me.

In fact, root->objectid is not enough to distinguish tree reloc trees.
So using root->root_key should be a safer way to go.

Thanks,
Qu

> 
>>
>> 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@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
>>>
>>
>> --
>> 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
> 
--
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
Lu Fengqi Aug. 3, 2018, 9:16 a.m. UTC | #5
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@jp.fujitsu.com>
>>>
>>> Looks good to me.
>>>
>>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>> 
>> 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?
Misono Tomohiro Aug. 6, 2018, 12:56 a.m. UTC | #6
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@jp.fujitsu.com>
>>>>
>>>> Looks good to me.
>>>>
>>>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>>>
>>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox series

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) {
 		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: