From: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>, <linux-btrfs@vger.kernel.org>
Cc: David Sterba <dsterba@suse.cz>
Subject: Re: [PATCH v4] btrfs: qgroup: Remove qgroup items along with subvolume deletion
Date: Thu, 9 Aug 2018 16:02:42 +0900 [thread overview]
Message-ID: <5cc212ee-5925-c836-43e8-1b93a2965c5f@jp.fujitsu.com> (raw)
In-Reply-To: <da63c4e5-5cb0-d608-aca1-62d95f26f110@gmx.com>
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
>>
>
next prev parent reply other threads:[~2018-08-09 9:26 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1533783766.git.misono.tomohiro@jp.fujitsu.com>
2018-08-09 4:12 ` [PATCH v4] btrfs: qgroup: Remove qgroup items along with subvolume deletion Misono Tomohiro
2018-08-09 5:47 ` Qu Wenruo
2018-08-09 6:05 ` Misono Tomohiro
2018-08-09 6:14 ` Qu Wenruo
2018-08-09 7:02 ` Misono Tomohiro [this message]
2018-08-09 7:05 ` [PATCH v5] " Misono Tomohiro
2018-08-15 13:06 ` David Sterba
2018-08-15 14:22 ` Qu Wenruo
2018-08-15 14:33 ` Qu Wenruo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5cc212ee-5925-c836-43e8-1b93a2965c5f@jp.fujitsu.com \
--to=misono.tomohiro@jp.fujitsu.com \
--cc=dsterba@suse.cz \
--cc=linux-btrfs@vger.kernel.org \
--cc=quwenruo.btrfs@gmx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).