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.f...@cn.fujitsu.com> >> Reviewed-by: Qu Wenruo <w...@suse.com> >> Signed-off-by: Misono Tomohiro <misono.tomoh...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html