| Message ID | 90a4ae6ae4be63ef4df3d020707fb7b1ae004634.1715064550.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: qgroup: stale qgroups related impromvents | expand |
On Tue, May 07, 2024 at 04:28:11PM +0930, Qu Wenruo wrote: > Currently if we fully removed a subvolume (not only unlinked, but fully > dropped its root item), its qgroup would not be removed. > > Thus we have "btrfs qgroup clear-stale" to handle such 0 level qgroups. > > This patch changes the behavior by automatically removing the qgroup of > a fully dropped subvolume when possible: > > - Full qgroup but still consistent > We can and should remove the qgroup. > The qgroup numbers should be 0, without any rsv. > > - Full qgroup but inconsistent > Can happen with drop_subtree_threshold feature (skip accounting > and mark qgroup inconsistent). > > We can and should remove the qgroup. > Higher level qgroup numbers will be incorrect, but since qgroup > is already inconsistent, it should not be a problem. > > - Squota mode > This is the special case, we can only drop the qgroup if its numbers > are all 0. > > This would be handled by can_delete_qgroup(), so we only need to check > the return value and ignore the -EBUSY error. > > Link: https://bugzilla.suse.com/show_bug.cgi?id=1222847 > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Boris Burkov <boris@bur.io> > --- > fs/btrfs/extent-tree.c | 8 ++++++++ > fs/btrfs/qgroup.c | 38 ++++++++++++++++++++++++++++++++++++++ > fs/btrfs/qgroup.h | 2 ++ > 3 files changed, 48 insertions(+) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 47d48233b592..21e07b698625 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -5833,6 +5833,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc) > struct btrfs_root_item *root_item = &root->root_item; > struct walk_control *wc; > struct btrfs_key key; > + const u64 rootid = btrfs_root_id(root); > int err = 0; > int ret; > int level; > @@ -6063,6 +6064,13 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc) > kfree(wc); > btrfs_free_path(path); > out: > + if (!err && root_dropped) { > + ret = btrfs_qgroup_cleanup_dropped_subvolume(fs_info, rootid); > + if (ret < 0) > + btrfs_warn_rl(fs_info, > + "failed to cleanup qgroup 0/%llu: %d", > + rootid, ret); > + } > /* > * We were an unfinished drop root, check to see if there are any > * pending, and if not clear and wake up any waiters. > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index d89f16366a1c..d894a7e2bf30 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1864,6 +1864,44 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) > return ret; > } > > +int btrfs_qgroup_cleanup_dropped_subvolume(struct btrfs_fs_info *fs_info, > + u64 subvolid) > +{ > + struct btrfs_trans_handle *trans; > + int ret; > + > + if (!is_fstree(subvolid) || !btrfs_qgroup_enabled(fs_info) || > + !fs_info->quota_root) > + return 0; > + > + /* > + * Commit current transaction to make sure all the rfer/excl numbers > + * get updated. > + */ > + trans = btrfs_start_transaction(fs_info->quota_root, 0); > + if (IS_ERR(trans)) > + return PTR_ERR(trans); > + > + ret = btrfs_commit_transaction(trans); > + if (ret < 0) > + return ret; > + > + /* Start new trans to delete the qgroup info and limit items. */ > + trans = btrfs_start_transaction(fs_info->quota_root, 2); > + if (IS_ERR(trans)) > + return PTR_ERR(trans); > + ret = btrfs_remove_qgroup(trans, subvolid); > + btrfs_end_transaction(trans); > + /* > + * It's squota and the subvolume still has numbers needed > + * for future accounting, in this case we can not delete. > + * Just skip it. > + */ Maybe throw in an ASSERT or WARN or whatever you think is best checking for squota mode, if we are sure this shouldn't happen for normal qgroup? > + if (ret == -EBUSY) > + ret = 0; > + return ret; > +} > + > int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, > struct btrfs_qgroup_limit *limit) > { > diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h > index 706640be0ec2..3f93856a02e1 100644 > --- a/fs/btrfs/qgroup.h > +++ b/fs/btrfs/qgroup.h > @@ -327,6 +327,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, > u64 dst); > int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid); > int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid); > +int btrfs_qgroup_cleanup_dropped_subvolume(struct btrfs_fs_info *fs_info, > + u64 subvolid); > int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, > struct btrfs_qgroup_limit *limit); > int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info); > -- > 2.45.0 >
在 2024/5/21 08:20, Boris Burkov 写道: [...] >> + /* >> + * It's squota and the subvolume still has numbers needed >> + * for future accounting, in this case we can not delete. >> + * Just skip it. >> + */ > > Maybe throw in an ASSERT or WARN or whatever you think is best checking > for squota mode, if we are sure this shouldn't happen for normal qgroup? Sounds good. Would add an ASSERT() for making sure it's squota mode. Thanks, Qu > >> + if (ret == -EBUSY) >> + ret = 0; >> + return ret; >> +} >> + >> int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, >> struct btrfs_qgroup_limit *limit) >> { >> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h >> index 706640be0ec2..3f93856a02e1 100644 >> --- a/fs/btrfs/qgroup.h >> +++ b/fs/btrfs/qgroup.h >> @@ -327,6 +327,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, >> u64 dst); >> int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid); >> int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid); >> +int btrfs_qgroup_cleanup_dropped_subvolume(struct btrfs_fs_info *fs_info, >> + u64 subvolid); >> int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, >> struct btrfs_qgroup_limit *limit); >> int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info); >> -- >> 2.45.0 >>
在 2024/5/21 08:33, Qu Wenruo 写道: > > > 在 2024/5/21 08:20, Boris Burkov 写道: > [...] >>> + /* >>> + * It's squota and the subvolume still has numbers needed >>> + * for future accounting, in this case we can not delete. >>> + * Just skip it. >>> + */ >> >> Maybe throw in an ASSERT or WARN or whatever you think is best checking >> for squota mode, if we are sure this shouldn't happen for normal qgroup? > > Sounds good. > > Would add an ASSERT() for making sure it's squota mode. After more thought, I believe ASSERT() can lead to false alerts. The problem here is, we do not have any extra race prevention here, really rely on one time call on btrfs_remove_qgroup() to do the proper locking. But after btrfs_remove_qgroup() returned -EBUSY, we can race with qgroup disabling, thus doing an ASSERT() without the proper lock context can lead to false alert, e.g. the qgroup is disabled after btrfs_remove_qgroup() call. So I'm afraid we can not do extra checks here. Thanks, Qu > > Thanks, > Qu > >> >>> + if (ret == -EBUSY) >>> + ret = 0; >>> + return ret; >>> +} >>> + >>> int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, >>> struct btrfs_qgroup_limit *limit) >>> { >>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h >>> index 706640be0ec2..3f93856a02e1 100644 >>> --- a/fs/btrfs/qgroup.h >>> +++ b/fs/btrfs/qgroup.h >>> @@ -327,6 +327,8 @@ int btrfs_del_qgroup_relation(struct >>> btrfs_trans_handle *trans, u64 src, >>> u64 dst); >>> int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 >>> qgroupid); >>> int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 >>> qgroupid); >>> +int btrfs_qgroup_cleanup_dropped_subvolume(struct btrfs_fs_info >>> *fs_info, >>> + u64 subvolid); >>> int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, >>> struct btrfs_qgroup_limit *limit); >>> int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info); >>> -- >>> 2.45.0 >>> >
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 47d48233b592..21e07b698625 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -5833,6 +5833,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc) struct btrfs_root_item *root_item = &root->root_item; struct walk_control *wc; struct btrfs_key key; + const u64 rootid = btrfs_root_id(root); int err = 0; int ret; int level; @@ -6063,6 +6064,13 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc) kfree(wc); btrfs_free_path(path); out: + if (!err && root_dropped) { + ret = btrfs_qgroup_cleanup_dropped_subvolume(fs_info, rootid); + if (ret < 0) + btrfs_warn_rl(fs_info, + "failed to cleanup qgroup 0/%llu: %d", + rootid, ret); + } /* * We were an unfinished drop root, check to see if there are any * pending, and if not clear and wake up any waiters. diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index d89f16366a1c..d894a7e2bf30 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1864,6 +1864,44 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) return ret; } +int btrfs_qgroup_cleanup_dropped_subvolume(struct btrfs_fs_info *fs_info, + u64 subvolid) +{ + struct btrfs_trans_handle *trans; + int ret; + + if (!is_fstree(subvolid) || !btrfs_qgroup_enabled(fs_info) || + !fs_info->quota_root) + return 0; + + /* + * Commit current transaction to make sure all the rfer/excl numbers + * get updated. + */ + trans = btrfs_start_transaction(fs_info->quota_root, 0); + if (IS_ERR(trans)) + return PTR_ERR(trans); + + ret = btrfs_commit_transaction(trans); + if (ret < 0) + return ret; + + /* Start new trans to delete the qgroup info and limit items. */ + trans = btrfs_start_transaction(fs_info->quota_root, 2); + if (IS_ERR(trans)) + return PTR_ERR(trans); + ret = btrfs_remove_qgroup(trans, subvolid); + btrfs_end_transaction(trans); + /* + * It's squota and the subvolume still has numbers needed + * for future accounting, in this case we can not delete. + * Just skip it. + */ + if (ret == -EBUSY) + ret = 0; + return ret; +} + int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, struct btrfs_qgroup_limit *limit) { diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h index 706640be0ec2..3f93856a02e1 100644 --- a/fs/btrfs/qgroup.h +++ b/fs/btrfs/qgroup.h @@ -327,6 +327,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst); int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid); int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid); +int btrfs_qgroup_cleanup_dropped_subvolume(struct btrfs_fs_info *fs_info, + u64 subvolid); int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid, struct btrfs_qgroup_limit *limit); int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info);
Currently if we fully removed a subvolume (not only unlinked, but fully dropped its root item), its qgroup would not be removed. Thus we have "btrfs qgroup clear-stale" to handle such 0 level qgroups. This patch changes the behavior by automatically removing the qgroup of a fully dropped subvolume when possible: - Full qgroup but still consistent We can and should remove the qgroup. The qgroup numbers should be 0, without any rsv. - Full qgroup but inconsistent Can happen with drop_subtree_threshold feature (skip accounting and mark qgroup inconsistent). We can and should remove the qgroup. Higher level qgroup numbers will be incorrect, but since qgroup is already inconsistent, it should not be a problem. - Squota mode This is the special case, we can only drop the qgroup if its numbers are all 0. This would be handled by can_delete_qgroup(), so we only need to check the return value and ignore the -EBUSY error. Link: https://bugzilla.suse.com/show_bug.cgi?id=1222847 Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent-tree.c | 8 ++++++++ fs/btrfs/qgroup.c | 38 ++++++++++++++++++++++++++++++++++++++ fs/btrfs/qgroup.h | 2 ++ 3 files changed, 48 insertions(+)