| Message ID | 1b5d9d72-5e6b-a88b-0192-8676a19b9732@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] btrfs: qgroup: Remove qgroup items along with subvolume deletion | expand |
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.
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. >
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: >
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:
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(-)