Skip to content

Commit 340f1aa

Browse files
lorddoskiaskdave
lorddoskias
authored and
kdave
committed
btrfs: qgroups: Move transaction management inside btrfs_quota_enable/disable
Commit 5d23515 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable") not only resulted in an easier to follow code but it also introduced a subtle bug. It changed the timing when the initial transaction rescan was happening: - before the commit: it would happen after transaction commit had occured - after the commit: it might happen before the transaction was committed This results in failure to correctly rescan the quota since there could be data which is still not committed on disk. This patch aims to fix this by moving the transaction creation/commit inside btrfs_quota_enable, which allows to schedule the quota commit after the transaction has been committed. Fixes: 5d23515 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable") Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> Link: https://marc.info/?l=linux-btrfs&m=152999289017582 Signed-off-by: Nikolay Borisov <nborisov@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
1 parent c7b562c commit 340f1aa

File tree

3 files changed

+76
-31
lines changed

3 files changed

+76
-31
lines changed

‎fs/btrfs/ioctl.c

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5105,9 +5105,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
51055105
struct inode *inode = file_inode(file);
51065106
struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
51075107
struct btrfs_ioctl_quota_ctl_args *sa;
5108-
struct btrfs_trans_handle *trans = NULL;
51095108
int ret;
5110-
int err;
51115109

51125110
if (!capable(CAP_SYS_ADMIN))
51135111
return -EPERM;
@@ -5123,28 +5121,19 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
51235121
}
51245122

51255123
down_write(&fs_info->subvol_sem);
5126-
trans = btrfs_start_transaction(fs_info->tree_root, 2);
5127-
if (IS_ERR(trans)) {
5128-
ret = PTR_ERR(trans);
5129-
goto out;
5130-
}
51315124

51325125
switch (sa->cmd) {
51335126
case BTRFS_QUOTA_CTL_ENABLE:
5134-
ret = btrfs_quota_enable(trans, fs_info);
5127+
ret = btrfs_quota_enable(fs_info);
51355128
break;
51365129
case BTRFS_QUOTA_CTL_DISABLE:
5137-
ret = btrfs_quota_disable(trans, fs_info);
5130+
ret = btrfs_quota_disable(fs_info);
51385131
break;
51395132
default:
51405133
ret = -EINVAL;
51415134
break;
51425135
}
51435136

5144-
err = btrfs_commit_transaction(trans);
5145-
if (err && !ret)
5146-
ret = err;
5147-
out:
51485137
kfree(sa);
51495138
up_write(&fs_info->subvol_sem);
51505139
drop_write:

‎fs/btrfs/qgroup.c

Lines changed: 72 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
875875
return ret;
876876
}
877877

878-
int btrfs_quota_enable(struct btrfs_trans_handle *trans,
879-
struct btrfs_fs_info *fs_info)
878+
int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
880879
{
881880
struct btrfs_root *quota_root;
882881
struct btrfs_root *tree_root = fs_info->tree_root;
@@ -886,16 +885,33 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
886885
struct btrfs_key key;
887886
struct btrfs_key found_key;
888887
struct btrfs_qgroup *qgroup = NULL;
888+
struct btrfs_trans_handle *trans = NULL;
889889
int ret = 0;
890890
int slot;
891891

892892
mutex_lock(&fs_info->qgroup_ioctl_lock);
893893
if (fs_info->quota_root)
894894
goto out;
895895

896+
/*
897+
* 1 for quota root item
898+
* 1 for BTRFS_QGROUP_STATUS item
899+
*
900+
* Yet we also need 2*n items for a QGROUP_INFO/QGROUP_LIMIT items
901+
* per subvolume. However those are not currently reserved since it
902+
* would be a lot of overkill.
903+
*/
904+
trans = btrfs_start_transaction(tree_root, 2);
905+
if (IS_ERR(trans)) {
906+
ret = PTR_ERR(trans);
907+
trans = NULL;
908+
goto out;
909+
}
910+
896911
fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
897912
if (!fs_info->qgroup_ulist) {
898913
ret = -ENOMEM;
914+
btrfs_abort_transaction(trans, ret);
899915
goto out;
900916
}
901917

@@ -906,12 +922,14 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
906922
BTRFS_QUOTA_TREE_OBJECTID);
907923
if (IS_ERR(quota_root)) {
908924
ret = PTR_ERR(quota_root);
925+
btrfs_abort_transaction(trans, ret);
909926
goto out;
910927
}
911928

912929
path = btrfs_alloc_path();
913930
if (!path) {
914931
ret = -ENOMEM;
932+
btrfs_abort_transaction(trans, ret);
915933
goto out_free_root;
916934
}
917935

@@ -921,8 +939,10 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
921939

922940
ret = btrfs_insert_empty_item(trans, quota_root, path, &key,
923941
sizeof(*ptr));
924-
if (ret)
942+
if (ret) {
943+
btrfs_abort_transaction(trans, ret);
925944
goto out_free_path;
945+
}
926946

927947
leaf = path->nodes[0];
928948
ptr = btrfs_item_ptr(leaf, path->slots[0],
@@ -944,9 +964,10 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
944964
ret = btrfs_search_slot_for_read(tree_root, &key, path, 1, 0);
945965
if (ret > 0)
946966
goto out_add_root;
947-
if (ret < 0)
967+
if (ret < 0) {
968+
btrfs_abort_transaction(trans, ret);
948969
goto out_free_path;
949-
970+
}
950971

951972
while (1) {
952973
slot = path->slots[0];
@@ -956,37 +977,52 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
956977
if (found_key.type == BTRFS_ROOT_REF_KEY) {
957978
ret = add_qgroup_item(trans, quota_root,
958979
found_key.offset);
959-
if (ret)
980+
if (ret) {
981+
btrfs_abort_transaction(trans, ret);
960982
goto out_free_path;
983+
}
961984

962985
qgroup = add_qgroup_rb(fs_info, found_key.offset);
963986
if (IS_ERR(qgroup)) {
964987
ret = PTR_ERR(qgroup);
988+
btrfs_abort_transaction(trans, ret);
965989
goto out_free_path;
966990
}
967991
}
968992
ret = btrfs_next_item(tree_root, path);
969-
if (ret < 0)
993+
if (ret < 0) {
994+
btrfs_abort_transaction(trans, ret);
970995
goto out_free_path;
996+
}
971997
if (ret)
972998
break;
973999
}
9741000

9751001
out_add_root:
9761002
btrfs_release_path(path);
9771003
ret = add_qgroup_item(trans, quota_root, BTRFS_FS_TREE_OBJECTID);
978-
if (ret)
1004+
if (ret) {
1005+
btrfs_abort_transaction(trans, ret);
9791006
goto out_free_path;
1007+
}
9801008

9811009
qgroup = add_qgroup_rb(fs_info, BTRFS_FS_TREE_OBJECTID);
9821010
if (IS_ERR(qgroup)) {
9831011
ret = PTR_ERR(qgroup);
1012+
btrfs_abort_transaction(trans, ret);
9841013
goto out_free_path;
9851014
}
9861015
spin_lock(&fs_info->qgroup_lock);
9871016
fs_info->quota_root = quota_root;
9881017
set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
9891018
spin_unlock(&fs_info->qgroup_lock);
1019+
1020+
ret = btrfs_commit_transaction(trans);
1021+
if (ret) {
1022+
trans = NULL;
1023+
goto out_free_path;
1024+
}
1025+
9901026
ret = qgroup_rescan_init(fs_info, 0, 1);
9911027
if (!ret) {
9921028
qgroup_rescan_zero_tracking(fs_info);
@@ -1006,20 +1042,35 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
10061042
if (ret) {
10071043
ulist_free(fs_info->qgroup_ulist);
10081044
fs_info->qgroup_ulist = NULL;
1045+
if (trans)
1046+
btrfs_end_transaction(trans);
10091047
}
10101048
mutex_unlock(&fs_info->qgroup_ioctl_lock);
10111049
return ret;
10121050
}
10131051

1014-
int btrfs_quota_disable(struct btrfs_trans_handle *trans,
1015-
struct btrfs_fs_info *fs_info)
1052+
int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
10161053
{
10171054
struct btrfs_root *quota_root;
1055+
struct btrfs_trans_handle *trans = NULL;
10181056
int ret = 0;
10191057

10201058
mutex_lock(&fs_info->qgroup_ioctl_lock);
10211059
if (!fs_info->quota_root)
10221060
goto out;
1061+
1062+
/*
1063+
* 1 For the root item
1064+
*
1065+
* We should also reserve enough items for the quota tree deletion in
1066+
* btrfs_clean_quota_tree but this is not done.
1067+
*/
1068+
trans = btrfs_start_transaction(fs_info->tree_root, 1);
1069+
if (IS_ERR(trans)) {
1070+
ret = PTR_ERR(trans);
1071+
goto out;
1072+
}
1073+
10231074
clear_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
10241075
btrfs_qgroup_wait_for_completion(fs_info, false);
10251076
spin_lock(&fs_info->qgroup_lock);
@@ -1031,12 +1082,16 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
10311082
btrfs_free_qgroup_config(fs_info);
10321083

10331084
ret = btrfs_clean_quota_tree(trans, quota_root);
1034-
if (ret)
1035-
goto out;
1085+
if (ret) {
1086+
btrfs_abort_transaction(trans, ret);
1087+
goto end_trans;
1088+
}
10361089

10371090
ret = btrfs_del_root(trans, fs_info, &quota_root->root_key);
1038-
if (ret)
1039-
goto out;
1091+
if (ret) {
1092+
btrfs_abort_transaction(trans, ret);
1093+
goto end_trans;
1094+
}
10401095

10411096
list_del(&quota_root->dirty_list);
10421097

@@ -1048,6 +1103,9 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
10481103
free_extent_buffer(quota_root->node);
10491104
free_extent_buffer(quota_root->commit_root);
10501105
kfree(quota_root);
1106+
1107+
end_trans:
1108+
ret = btrfs_end_transaction(trans);
10511109
out:
10521110
mutex_unlock(&fs_info->qgroup_ioctl_lock);
10531111
return ret;

‎fs/btrfs/qgroup.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,8 @@ struct btrfs_qgroup {
141141
#define QGROUP_RELEASE (1<<1)
142142
#define QGROUP_FREE (1<<2)
143143

144-
int btrfs_quota_enable(struct btrfs_trans_handle *trans,
145-
struct btrfs_fs_info *fs_info);
146-
int btrfs_quota_disable(struct btrfs_trans_handle *trans,
147-
struct btrfs_fs_info *fs_info);
144+
int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
145+
int btrfs_quota_disable(struct btrfs_fs_info *fs_info);
148146
int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
149147
void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
150148
int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,

0 commit comments

Comments
 (0)