[PATCH] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[PATCH] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Misono Tomohiro]

When qgroup is on, subvolume deletion does not remove qgroup items
of the subvolume (qgroup info, limits, relation) from quota tree and
they need 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.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---

I don't see any reason to keep these items after subvolume deletion,
but is there something I'm missing?

 fs/btrfs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3f51ddc18f98..4ec60a1b53a3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
 		}
 	}
 
+	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);
+	if (ret == -EINVAL || ret == -ENOENT)
+		ret = 0;
+
 out_end_trans:
 	trans->block_rsv = NULL;
 	trans->bytes_reserved = 0;
-- 
2.14.4

Re: [PATCH] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Qu Wenruo]

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



On 2018年08月03日 12:08, Misono Tomohiro wrote:
> When qgroup is on, subvolume deletion does not remove qgroup items
> of the subvolume (qgroup info, limits, relation) from quota tree and
> they need 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.
> 
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> ---
> 
> I don't see any reason to keep these items after subvolume deletion,
> but is there something I'm missing?
> 
>  fs/btrfs/inode.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3f51ddc18f98..4ec60a1b53a3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>  		}
>  	}
>  
> +	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);

According to the caller, it only unlinks the subvolume without really
delete the whole subvolume.

I'm wondering if we should call btrfs_remove_qgroup() only after we have
deleted the whole subvolume, e.g inside btrfs_drop_snapshot().

Thanks,
Qu

> +	if (ret == -EINVAL || ret == -ENOENT)
> +		ret = 0;
> +
>  out_end_trans:
>  	trans->block_rsv = NULL;
>  	trans->bytes_reserved = 0;
> 


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

Re: [PATCH] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Lu Fengqi]

On Fri, Aug 03, 2018 at 12:17:26PM +0800, Qu Wenruo wrote:
>
>
>On 2018年08月03日 12:08, Misono Tomohiro wrote:
>> When qgroup is on, subvolume deletion does not remove qgroup items
>> of the subvolume (qgroup info, limits, relation) from quota tree and
>> they need 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.
>> 
>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> ---
>> 
>> I don't see any reason to keep these items after subvolume deletion,
>> but is there something I'm missing?
>> 
>>  fs/btrfs/inode.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 3f51ddc18f98..4ec60a1b53a3 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>>  		}
>>  	}
>>  
>> +	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);
>
>According to the caller, it only unlinks the subvolume without really
>delete the whole subvolume.
>
>I'm wondering if we should call btrfs_remove_qgroup() only after we have
>deleted the whole subvolume, e.g inside btrfs_drop_snapshot().

I agree with Qu's point, because the ongoing online undelete subvolume will
be able to recover the subvolume which is intact ondisk, including the qgroup.

-- 
Thanks,
Lu

>
>Thanks,
>Qu
>
>> +	if (ret == -EINVAL || ret == -ENOENT)
>> +		ret = 0;
>> +
>>  out_end_trans:
>>  	trans->block_rsv = NULL;
>>  	trans->bytes_reserved = 0;
>> 
>

Re: [PATCH] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Misono Tomohiro]

On 2018/08/03 13:23, Lu Fengqi wrote:
> On Fri, Aug 03, 2018 at 12:17:26PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年08月03日 12:08, Misono Tomohiro wrote:
>>> When qgroup is on, subvolume deletion does not remove qgroup items
>>> of the subvolume (qgroup info, limits, relation) from quota tree and
>>> they need 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.
>>>
>>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>>> ---
>>>
>>> I don't see any reason to keep these items after subvolume deletion,
>>> but is there something I'm missing?
>>>
>>>  fs/btrfs/inode.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 3f51ddc18f98..4ec60a1b53a3 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>>>  		}
>>>  	}
>>>  
>>> +	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);
>>
>> According to the caller, it only unlinks the subvolume without really
>> delete the whole subvolume.
>>
>> I'm wondering if we should call btrfs_remove_qgroup() only after we have
>> deleted the whole subvolume, e.g inside btrfs_drop_snapshot().
> 
> I agree with Qu's point, because the ongoing online undelete subvolume will
> be able to recover the subvolume which is intact ondisk, including the qgroup.
> 

Thanks to both of you, I'll update the patch.
Misono

[PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Misono Tomohiro]

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

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

Re: [PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Lu Fengqi]

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

Re: [PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Misono Tomohiro]

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
> 

Re: [PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Nikolay Borisov]

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
> 

Re: [PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Qu Wenruo]

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
> 

Re: [PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Lu Fengqi]

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?

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

Re: [PATCH v2] btrfs: qgroup: Remove qgroup item along with subvolume deletion

[Misono Tomohiro]

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