On 2020/7/1 上午12:57, David Sterba wrote:
> On Sun, Jun 28, 2020 at 01:07:15PM +0800, Qu Wenruo wrote:
>> @@ -1030,6 +1040,11 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>> btrfs_abort_transaction(trans, ret);
>> goto out_free_path;
>> }
>> + ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
>> + if (ret < 0) {
>> + btrfs_abort_transaction(trans, ret);
>> + goto out_free_path;
>> + }
>> }
>> ret = btrfs_next_item(tree_root, path);
>> if (ret < 0) {
>> @@ -1054,6 +1069,11 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>> btrfs_abort_transaction(trans, ret);
>> goto out_free_path;
>> }
>> + ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
>> + if (ret < 0) {
>> + btrfs_abort_transaction(trans, ret);
>> + goto out_free_path;
>> + }
>
> This adds 2 new transaction abort sites altough I don't think it's
> justified, the filesystem is fine. If system is that low on memory it's
> gonna be very bad elsewhere too so we don't need to make things worse
> jsut because of some missing sysfs entries.
The problem here is, we don't have good enough way to revert back to
previous status.
This is common among a lot of qgroup code, and I prefer to fix them
later, as it will be a big qgroup error patch cleanup.
>
> A warning would be better, though in that case the validity of the
> kobjects should be double checked where it's accessed.
>
It would be even worse if the qgroup relationship is also exported
through sysfs.
In that case, warning is not good enough.
So I still prefer error path cleanup as the ultimate fix.
The objective is, if we hit any error during qgroup enabling or other
qgroup operations, we revert to previous status if possible.
For qgroup enable, if we hit any non-critical error, we don't abort
trans at all, but remove all qgroups along with its qgroup items, remove
the qgroup tree, then reverts back to qgroup disabled case.
This includes -ENOMEM case.
While for critical error like tree operations errors, we still abort
transaction.
In fact, I'm already working on a similar project, but for extent_changeset.
So I guess it won't take too long for qgroup.
Thanks,
Qu
