[PATCH v4] btrfs: qgroup: Remove qgroup items along with subvolume deletion

[PATCH v4] btrfs: qgroup: Remove qgroup items along with subvolume deletion

[Misono Tomohiro]

When qgroup is on, subvolume deletion does not remove qgroup items
of the subvolume (qgroup info, limit, 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
(to be precise, when the subvolume root is dropped).

Note that qgroup becomes inconsistent in following case:
  1. qgroup relation exists
  2. and subvolume's excl != rref
In this case manual qgroup rescan is needed.

Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
Hi David,
It turned out that this patch may cause qgroup inconsistency in case
described above and need manual rescan. Since current code will keep 
qgroup items but not break qgroup consistency when deleting subvolume,
I cannot clearly say which behavior is better for qgroup usability.
Can I ask your opinion?

v3 -> v4:
  Check return value of btrfs_remove_qgroup() and if it is 1,
  print message in syslog that fs needs qgroup rescan

 fs/btrfs/extent-tree.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 9e7b237b9547..828d9e68047d 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->root_key.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,20 @@ 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 == 1) {
+		/* This means qgroup becomes inconsistent by removing items */
+		btrfs_info(fs_info,
+		    "qgroup inconsistency found, need qgroup rescan");
+	} else if (ret == -EINVAL || ret == -ENOENT) {
+		/* qgroup is not enabled or already removed, just ignore this */
+	} else if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		err = ret;
+	}
+
 out_end_trans:
 	btrfs_end_transaction_throttle(trans);
 out_free:
-- 
2.14.4

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

[Qu Wenruo]

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



On 8/9/18 12:12 PM, Misono Tomohiro wrote:
> When qgroup is on, subvolume deletion does not remove qgroup items
> of the subvolume (qgroup info, limit, 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
> (to be precise, when the subvolume root is dropped).
> 
> Note that qgroup becomes inconsistent in following case:
>   1. qgroup relation exists
>   2. and subvolume's excl != rref

That's a little strange.

If a subvolume is completely dropped, its excl should be the same rfer,
all 0, and removing its relationship should not mark qgroup inconsistent.

So the problem is the timing when btrfs_remove_qgroup() is called.

Since qgroup accounting is only called at transaction commit time, and
we're holding a trans handler, it's almost ensured we can't commit this
transaction, thus the number is not updated yet (still not 0)

So that's why qgroup is inconsistent.

What about commit current transaction and then call btrfs_remove_qgroup()?

(Sorry I didn't catch this problem last time I reviewed this patch)

Thanks,
Qu

> In this case manual qgroup rescan is needed.
> 
> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> ---
> Hi David,
> It turned out that this patch may cause qgroup inconsistency in case
> described above and need manual rescan. Since current code will keep 
> qgroup items but not break qgroup consistency when deleting subvolume,
> I cannot clearly say which behavior is better for qgroup usability.
> Can I ask your opinion?
> 
> v3 -> v4:
>   Check return value of btrfs_remove_qgroup() and if it is 1,
>   print message in syslog that fs needs qgroup rescan
> 
>  fs/btrfs/extent-tree.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 9e7b237b9547..828d9e68047d 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->root_key.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,20 @@ 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 == 1) {
> +		/* This means qgroup becomes inconsistent by removing items */
> +		btrfs_info(fs_info,
> +		    "qgroup inconsistency found, need qgroup rescan");
> +	} else if (ret == -EINVAL || ret == -ENOENT) {
> +		/* qgroup is not enabled or already removed, just ignore this */
> +	} else if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		err = ret;
> +	}
> +
>  out_end_trans:
>  	btrfs_end_transaction_throttle(trans);
>  out_free:
> 


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

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

[Misono Tomohiro]

On 2018/08/09 14:47, Qu Wenruo wrote:
> 
> 
> On 8/9/18 12:12 PM, Misono Tomohiro wrote:
>> When qgroup is on, subvolume deletion does not remove qgroup items
>> of the subvolume (qgroup info, limit, 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
>> (to be precise, when the subvolume root is dropped).
>>
>> Note that qgroup becomes inconsistent in following case:
>>   1. qgroup relation exists
>>   2. and subvolume's excl != rref
> 
> That's a little strange.
> 
> If a subvolume is completely dropped, its excl should be the same rfer,
> all 0, and removing its relationship should not mark qgroup inconsistent.
> 
> So the problem is the timing when btrfs_remove_qgroup() is called.
> 
> Since qgroup accounting is only called at transaction commit time, and
> we're holding a trans handler, it's almost ensured we can't commit this
> transaction, thus the number is not updated yet (still not 0)
> 
> So that's why qgroup is inconsistent.
> 
> What about commit current transaction and then call btrfs_remove_qgroup()?
> 
> (Sorry I didn't catch this problem last time I reviewed this patch)

well, I'm little confusing about flow of transaction commit.
btrfs_drop_snapshot() is called from cleaner_kthread and
is it ok to commit transaction in it?

> 
> Thanks,
> Qu
> 
>> In this case manual qgroup rescan is needed.
>>
>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> ---
>> Hi David,
>> It turned out that this patch may cause qgroup inconsistency in case
>> described above and need manual rescan. Since current code will keep 
>> qgroup items but not break qgroup consistency when deleting subvolume,
>> I cannot clearly say which behavior is better for qgroup usability.
>> Can I ask your opinion?
>>
>> v3 -> v4:
>>   Check return value of btrfs_remove_qgroup() and if it is 1,
>>   print message in syslog that fs needs qgroup rescan
>>
>>  fs/btrfs/extent-tree.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 9e7b237b9547..828d9e68047d 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->root_key.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,20 @@ 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 == 1) {
>> +		/* This means qgroup becomes inconsistent by removing items */
>> +		btrfs_info(fs_info,
>> +		    "qgroup inconsistency found, need qgroup rescan");
>> +	} else if (ret == -EINVAL || ret == -ENOENT) {
>> +		/* qgroup is not enabled or already removed, just ignore this */
>> +	} else if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		err = ret;
>> +	}
>> +
>>  out_end_trans:
>>  	btrfs_end_transaction_throttle(trans);
>>  out_free:
>>
> 

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

[Qu Wenruo]

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



On 8/9/18 2:05 PM, Misono Tomohiro wrote:
> On 2018/08/09 14:47, Qu Wenruo wrote:
>>
>>
>> On 8/9/18 12:12 PM, Misono Tomohiro wrote:
>>> When qgroup is on, subvolume deletion does not remove qgroup items
>>> of the subvolume (qgroup info, limit, 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
>>> (to be precise, when the subvolume root is dropped).
>>>
>>> Note that qgroup becomes inconsistent in following case:
>>>   1. qgroup relation exists
>>>   2. and subvolume's excl != rref
>>
>> That's a little strange.
>>
>> If a subvolume is completely dropped, its excl should be the same rfer,
>> all 0, and removing its relationship should not mark qgroup inconsistent.
>>
>> So the problem is the timing when btrfs_remove_qgroup() is called.
>>
>> Since qgroup accounting is only called at transaction commit time, and
>> we're holding a trans handler, it's almost ensured we can't commit this
>> transaction, thus the number is not updated yet (still not 0)
>>
>> So that's why qgroup is inconsistent.
>>
>> What about commit current transaction and then call btrfs_remove_qgroup()?
>>
>> (Sorry I didn't catch this problem last time I reviewed this patch)
> 
> well, I'm little confusing about flow of transaction commit.
> btrfs_drop_snapshot() is called from cleaner_kthread and
> is it ok to commit transaction in it?

Not completely clear of the cleaner_kthread(), but from what I see in
btrfs_drop_snapshot(), btrfs_end_transaction_throttle() itself could
commit current transaction.

So in theory we should be OK to finish all the original work of
btrfs_drop_snapshot(), and then commit current transaction, and finally
do the qgroup cleanup work.

But I could totally be wrong, and feel free to point what I'm missing.

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>> In this case manual qgroup rescan is needed.
>>>
>>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>>> ---
>>> Hi David,
>>> It turned out that this patch may cause qgroup inconsistency in case
>>> described above and need manual rescan. Since current code will keep 
>>> qgroup items but not break qgroup consistency when deleting subvolume,
>>> I cannot clearly say which behavior is better for qgroup usability.
>>> Can I ask your opinion?
>>>
>>> v3 -> v4:
>>>   Check return value of btrfs_remove_qgroup() and if it is 1,
>>>   print message in syslog that fs needs qgroup rescan
>>>
>>>  fs/btrfs/extent-tree.c | 22 ++++++++++++++++++----
>>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 9e7b237b9547..828d9e68047d 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->root_key.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,20 @@ 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 == 1) {
>>> +		/* This means qgroup becomes inconsistent by removing items */
>>> +		btrfs_info(fs_info,
>>> +		    "qgroup inconsistency found, need qgroup rescan");
>>> +	} else if (ret == -EINVAL || ret == -ENOENT) {
>>> +		/* qgroup is not enabled or already removed, just ignore this */
>>> +	} else if (ret) {
>>> +		btrfs_abort_transaction(trans, ret);
>>> +		err = ret;
>>> +	}
>>> +
>>>  out_end_trans:
>>>  	btrfs_end_transaction_throttle(trans);
>>>  out_free:
>>>
>>
> 
> --
> 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
> 


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

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

[Misono Tomohiro]

On 2018/08/09 15:14, Qu Wenruo wrote:
> 
> 
> On 8/9/18 2:05 PM, Misono Tomohiro wrote:
>> On 2018/08/09 14:47, Qu Wenruo wrote:
>>>
>>>
>>> On 8/9/18 12:12 PM, Misono Tomohiro wrote:
>>>> When qgroup is on, subvolume deletion does not remove qgroup items
>>>> of the subvolume (qgroup info, limit, 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
>>>> (to be precise, when the subvolume root is dropped).
>>>>
>>>> Note that qgroup becomes inconsistent in following case:
>>>>   1. qgroup relation exists
>>>>   2. and subvolume's excl != rref
>>>
>>> That's a little strange.
>>>
>>> If a subvolume is completely dropped, its excl should be the same rfer,
>>> all 0, and removing its relationship should not mark qgroup inconsistent.
>>>
>>> So the problem is the timing when btrfs_remove_qgroup() is called.
>>>
>>> Since qgroup accounting is only called at transaction commit time, and
>>> we're holding a trans handler, it's almost ensured we can't commit this
>>> transaction, thus the number is not updated yet (still not 0)
>>>
>>> So that's why qgroup is inconsistent.
>>>
>>> What about commit current transaction and then call btrfs_remove_qgroup()?
>>>
>>> (Sorry I didn't catch this problem last time I reviewed this patch)
>>
>> well, I'm little confusing about flow of transaction commit.
>> btrfs_drop_snapshot() is called from cleaner_kthread and
>> is it ok to commit transaction in it?
> 
> Not completely clear of the cleaner_kthread(), but from what I see in
> btrfs_drop_snapshot(), btrfs_end_transaction_throttle() itself could
> commit current transaction.
> 
> So in theory we should be OK to finish all the original work of
> btrfs_drop_snapshot(), and then commit current transaction, and finally
> do the qgroup cleanup work.
> 
> But I could totally be wrong, and feel free to point what I'm missing.

Thank you very much for explanation.
I changed code to commit transaction and it works,
so I hope next version will solve all the problem.

Thanks,
Misono

> 
> Thanks,
> Qu
> 
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>> In this case manual qgroup rescan is needed.
>>>>
>>>> Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>>>> ---
>>>> Hi David,
>>>> It turned out that this patch may cause qgroup inconsistency in case
>>>> described above and need manual rescan. Since current code will keep 
>>>> qgroup items but not break qgroup consistency when deleting subvolume,
>>>> I cannot clearly say which behavior is better for qgroup usability.
>>>> Can I ask your opinion?
>>>>
>>>> v3 -> v4:
>>>>   Check return value of btrfs_remove_qgroup() and if it is 1,
>>>>   print message in syslog that fs needs qgroup rescan
>>>>
>>>>  fs/btrfs/extent-tree.c | 22 ++++++++++++++++++----
>>>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 9e7b237b9547..828d9e68047d 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->root_key.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,20 @@ 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 == 1) {
>>>> +		/* This means qgroup becomes inconsistent by removing items */
>>>> +		btrfs_info(fs_info,
>>>> +		    "qgroup inconsistency found, need qgroup rescan");
>>>> +	} else if (ret == -EINVAL || ret == -ENOENT) {
>>>> +		/* qgroup is not enabled or already removed, just ignore this */
>>>> +	} else if (ret) {
>>>> +		btrfs_abort_transaction(trans, ret);
>>>> +		err = ret;
>>>> +	}
>>>> +
>>>>  out_end_trans:
>>>>  	btrfs_end_transaction_throttle(trans);
>>>>  out_free:
>>>>
>>>
>>
>> --
>> 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 v5] btrfs: qgroup: Remove qgroup items along with subvolume deletion

[Misono Tomohiro]

When qgroup is on, subvolume deletion does not remove qgroup items
of the subvolume (qgroup info, limit, 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
(to be precise, when the subvolume root is dropped).

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
v4 -> v5:
  Commit current transaction before calling btrfs_remove_qgroup() to
  keep qgroup consistency in all case. This resolves the concern in
  v4 path and there should be no demerit in this patch.

 fs/btrfs/extent-tree.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 9e7b237b9547..ed052105e741 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->root_key.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,43 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
 		btrfs_put_fs_root(root);
 	}
 	root_dropped = true;
+
+	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
+		/*
+		 * Remove level-0 qgroup items since no other subvolume can
+		 * use them.
+		 *
+		 * First, commit current transaction in order to make sure
+		 * this subvolume's excl == rfer == 0. Otherwise removing
+		 * qgroup relation causes qgroup inconsistency if excl != rfer.
+		 */
+		ret = btrfs_commit_transaction(trans);
+		if (ret)
+			goto out_free;
+
+		/* Start new transaction and remove qgroup items */
+		trans = btrfs_start_transaction(tree_root, 0);
+		if (IS_ERR(trans)) {
+			err = PTR_ERR(trans);
+			goto out_free;
+		}
+
+		ret = btrfs_remove_qgroup(trans, objectid);
+		if (ret == 1) {
+			/*
+			 * This means qgroup becomes inconsistent
+			 * (should not happen since we did transaction commit)
+			 */
+			btrfs_warn(fs_info,
+			"qgroup inconsistency found, need qgroup rescan");
+		} else if (ret == -EINVAL || ret == -ENOENT) {
+			/* qgroup is already removed, just ignore this */
+		} else if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			err = ret;
+		}
+	}
+
 out_end_trans:
 	btrfs_end_transaction_throttle(trans);
 out_free:
-- 
2.14.4

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

[David Sterba]

On Thu, Aug 09, 2018 at 04:05:36PM +0900, Misono Tomohiro wrote:
> When qgroup is on, subvolume deletion does not remove qgroup items
> of the subvolume (qgroup info, limit, 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
> (to be precise, when the subvolume root is dropped).

Please note that dropping the 0-level qgroup has user-visible impact
that needs to be evaluated. I don't see anything like that in the
changelog. If there's a potential or actual breakage after this patch,
it needs to be addressed in some way.

This is not the first time somebody proposes to do the auto deletion.
While I'm not against it, it still has to be done the right way.
Anything that touches user interfaces must get extra care, and review
bandwidth for that is unfortunatelly extra low. I can't give you an ETA
or merge target for this patch.

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

[Qu Wenruo]

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



On 2018/8/15 下午9:06, David Sterba wrote:
> On Thu, Aug 09, 2018 at 04:05:36PM +0900, Misono Tomohiro wrote:
>> When qgroup is on, subvolume deletion does not remove qgroup items
>> of the subvolume (qgroup info, limit, 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
>> (to be precise, when the subvolume root is dropped).
> 
> Please note that dropping the 0-level qgroup has user-visible impact
> that needs to be evaluated.

I wonder if this is the case.

Normal btrfs subvolume creation using the highest objectid available in
root tree, thus later subvolume won't take the id of the to-be-deleted
subvolume.

Further more, this auto-removal only happens when the to-be-deleted
subvolume get completely removed, thus there should be no way to access
the subvolume already before we hit the branch in this patch.

So yes, the level 0 qgroup auto-removal is bringing a user visible
change, but user can't do anything anyway, and the result should just
save some "btrfs qgroup destroy/remove" calls.

Or did I miss something?

Thanks,
Qu


> I don't see anything like that in the
> changelog.
> If there's a potential or actual breakage after this patch,
> it needs to be addressed in some way.
> 
> This is not the first time somebody proposes to do the auto deletion.
> While I'm not against it, it still has to be done the right way.
> Anything that touches user interfaces must get extra care, and review
> bandwidth for that is unfortunatelly extra low. I can't give you an ETA
> or merge target for this patch.
> 


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

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

[Qu Wenruo]

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



On 2018/8/9 下午3:05, Misono Tomohiro wrote:
> When qgroup is on, subvolume deletion does not remove qgroup items
> of the subvolume (qgroup info, limit, 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
> (to be precise, when the subvolume root is dropped).
> 
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> ---
> v4 -> v5:
>   Commit current transaction before calling btrfs_remove_qgroup() to
>   keep qgroup consistency in all case. This resolves the concern in
>   v4 path and there should be no demerit in this patch.
> 
>  fs/btrfs/extent-tree.c | 45 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 9e7b237b9547..ed052105e741 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->root_key.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,43 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
>  		btrfs_put_fs_root(root);
>  	}
>  	root_dropped = true;
> +
> +	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {

I would prefer to put the code even further after the out tag just in case.
Current implement changed @trans and I'm not 100% sure if it's a good idea.

> +		/*
> +		 * Remove level-0 qgroup items since no other subvolume can
> +		 * use them.
> +		 *
> +		 * First, commit current transaction in order to make sure
> +		 * this subvolume's excl == rfer == 0. Otherwise removing
> +		 * qgroup relation causes qgroup inconsistency if excl != rfer.
> +		 */
> +		ret = btrfs_commit_transaction(trans);
> +		if (ret)
> +			goto out_free;
> +
> +		/* Start new transaction and remove qgroup items */
> +		trans = btrfs_start_transaction(tree_root, 0);
> +		if (IS_ERR(trans)) {
> +			err = PTR_ERR(trans);
> +			goto out_free;
> +		}
> +

It would be better to add some sanity check for current qgroup's rfer/excl.

If it's not 0/0, something must went wrong.

> +		ret = btrfs_remove_qgroup(trans, objectid);
> +		if (ret == 1) {

And if we do extra sanity check, return value from btrfs_remove_qgroup()
should never be larger than 0.

Thanks,
Qu
> +			/*
> +			 * This means qgroup becomes inconsistent
> +			 * (should not happen since we did transaction commit)
> +			 */
> +			btrfs_warn(fs_info,
> +			"qgroup inconsistency found, need qgroup rescan");
> +		} else if (ret == -EINVAL || ret == -ENOENT) {
> +			/* qgroup is already removed, just ignore this */
> +		} else if (ret) {
> +			btrfs_abort_transaction(trans, ret);
> +			err = ret;
> +		}
> +	}
> +
>  out_end_trans:
>  	btrfs_end_transaction_throttle(trans);
>  out_free:
> 


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