[PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Nikolay Borisov]

Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
inside btrfs_quota_enable, which allows to schedule the quota commit
after the transaction has been committed.

Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
Link: https://marc.info/?l=linux-btrfs&m=152999289017582
Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c  | 15 ++-------------
 fs/btrfs/qgroup.c | 38 +++++++++++++++++++++++++++++++-------
 fs/btrfs/qgroup.h |  6 ++----
 3 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a399750b9e41..316fb1af15e2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5135,9 +5135,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_ioctl_quota_ctl_args *sa;
-	struct btrfs_trans_handle *trans = NULL;
 	int ret;
-	int err;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -5153,28 +5151,19 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
 	}
 
 	down_write(&fs_info->subvol_sem);
-	trans = btrfs_start_transaction(fs_info->tree_root, 2);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		goto out;
-	}
 
 	switch (sa->cmd) {
 	case BTRFS_QUOTA_CTL_ENABLE:
-		ret = btrfs_quota_enable(trans, fs_info);
+		ret = btrfs_quota_enable(fs_info);
 		break;
 	case BTRFS_QUOTA_CTL_DISABLE:
-		ret = btrfs_quota_disable(trans, fs_info);
+		ret = btrfs_quota_disable(fs_info);
 		break;
 	default:
 		ret = -EINVAL;
 		break;
 	}
 
-	err = btrfs_commit_transaction(trans);
-	if (err && !ret)
-		ret = err;
-out:
 	kfree(sa);
 	up_write(&fs_info->subvol_sem);
 drop_write:
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c25dc47210a3..1012c7138633 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_quota_enable(struct btrfs_trans_handle *trans,
-		       struct btrfs_fs_info *fs_info)
+int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_root *tree_root = fs_info->tree_root;
@@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	struct btrfs_key key;
 	struct btrfs_key found_key;
 	struct btrfs_qgroup *qgroup = NULL;
+	struct btrfs_trans_handle *trans = NULL;
 	int ret = 0;
 	int slot;
 
@@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	if (fs_info->quota_root)
 		goto out;
 
+	trans = btrfs_start_transaction(tree_root, 2);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
+
 	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
 	if (!fs_info->qgroup_ulist) {
 		ret = -ENOMEM;
@@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	fs_info->quota_root = quota_root;
 	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
 	spin_unlock(&fs_info->qgroup_lock);
+
+	ret = btrfs_commit_transaction(trans);
+	if (ret)
+		goto out_free_path;
+
 	ret = qgroup_rescan_init(fs_info, 0, 1);
 	if (!ret) {
 	        qgroup_rescan_zero_tracking(fs_info);
@@ -1011,15 +1022,22 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_quota_disable(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info)
+int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *quota_root;
+	struct btrfs_trans_handle *trans = NULL;
 	int ret = 0;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root)
 		goto out;
+
+	trans = btrfs_start_transaction(fs_info->tree_root, 2);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
+
 	clear_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
 	btrfs_qgroup_wait_for_completion(fs_info, false);
 	spin_lock(&fs_info->qgroup_lock);
@@ -1031,12 +1049,16 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 	btrfs_free_qgroup_config(fs_info);
 
 	ret = btrfs_clean_quota_tree(trans, quota_root);
-	if (ret)
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
 		goto out;
+	}
 
 	ret = btrfs_del_root(trans, fs_info, &quota_root->root_key);
-	if (ret)
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
 		goto out;
+	}
 
 	list_del(&quota_root->dirty_list);
 
@@ -1048,6 +1070,8 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 	free_extent_buffer(quota_root->node);
 	free_extent_buffer(quota_root->commit_root);
 	kfree(quota_root);
+
+	ret = btrfs_commit_transaction(trans);
 out:
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
@@ -3070,7 +3094,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free && reserved)
 		return qgroup_free_reserved_data(inode, reserved, start, len);
 	extent_changeset_init(&changeset);
-	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
+	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
 			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
 	if (ret < 0)
 		goto out;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index d60dd06445ce..bec7c9b17a8e 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -141,10 +141,8 @@ struct btrfs_qgroup {
 #define QGROUP_RELEASE		(1<<1)
 #define QGROUP_FREE		(1<<2)
 
-int btrfs_quota_enable(struct btrfs_trans_handle *trans,
-		       struct btrfs_fs_info *fs_info);
-int btrfs_quota_disable(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info);
+int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
+int btrfs_quota_disable(struct btrfs_fs_info *fs_info);
 int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
 void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
 int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,
-- 
2.7.4

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Nikolay Borisov]

On  2.07.2018 14:00, Nikolay Borisov wrote:
> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
> inside btrfs_quota_enable, which allows to schedule the quota commit
> after the transaction has been committed.
> 
> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
> Link: https://marc.info/?l=linux-btrfs&m=152999289017582
> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

David,

This is a fix for an issue which Misono is seeing. So far neither I nor
Qu were able to reproduce it. Imho it should be material for this RC.

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[David Sterba]

On Mon, Jul 02, 2018 at 02:00:34PM +0300, Nikolay Borisov wrote:
> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
> inside btrfs_quota_enable, which allows to schedule the quota commit
> after the transaction has been committed.
> 
> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
> Link: https://marc.info/?l=linux-btrfs&m=152999289017582

Please use https://lkml.kernel.org/r/<message-id>

> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ioctl.c  | 15 ++-------------
>  fs/btrfs/qgroup.c | 38 +++++++++++++++++++++++++++++++-------
>  fs/btrfs/qgroup.h |  6 ++----
>  3 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a399750b9e41..316fb1af15e2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5135,9 +5135,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>  	struct inode *inode = file_inode(file);
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_ioctl_quota_ctl_args *sa;
> -	struct btrfs_trans_handle *trans = NULL;
>  	int ret;
> -	int err;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -5153,28 +5151,19 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>  	}
>  
>  	down_write(&fs_info->subvol_sem);
> -	trans = btrfs_start_transaction(fs_info->tree_root, 2);
> -	if (IS_ERR(trans)) {
> -		ret = PTR_ERR(trans);
> -		goto out;
> -	}
>  
>  	switch (sa->cmd) {
>  	case BTRFS_QUOTA_CTL_ENABLE:
> -		ret = btrfs_quota_enable(trans, fs_info);
> +		ret = btrfs_quota_enable(fs_info);
>  		break;
>  	case BTRFS_QUOTA_CTL_DISABLE:
> -		ret = btrfs_quota_disable(trans, fs_info);
> +		ret = btrfs_quota_disable(fs_info);
>  		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
>  	}
>  
> -	err = btrfs_commit_transaction(trans);
> -	if (err && !ret)
> -		ret = err;
> -out:
>  	kfree(sa);
>  	up_write(&fs_info->subvol_sem);
>  drop_write:
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c25dc47210a3..1012c7138633 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> -		       struct btrfs_fs_info *fs_info)
> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_root *quota_root;
>  	struct btrfs_root *tree_root = fs_info->tree_root;
> @@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	struct btrfs_key key;
>  	struct btrfs_key found_key;
>  	struct btrfs_qgroup *qgroup = NULL;
> +	struct btrfs_trans_handle *trans = NULL;
>  	int ret = 0;
>  	int slot;
>  
> @@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	if (fs_info->quota_root)
>  		goto out;
>  
> +	trans = btrfs_start_transaction(tree_root, 2);

Please document what transaction items are requested.

> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
>  	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
>  	if (!fs_info->qgroup_ulist) {
>  		ret = -ENOMEM;
> @@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	fs_info->quota_root = quota_root;
>  	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>  	spin_unlock(&fs_info->qgroup_lock);
> +
> +	ret = btrfs_commit_transaction(trans);
> +	if (ret)
> +		goto out_free_path;
> +
>  	ret = qgroup_rescan_init(fs_info, 0, 1);
>  	if (!ret) {
>  	        qgroup_rescan_zero_tracking(fs_info);
> @@ -1011,15 +1022,22 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -int btrfs_quota_disable(struct btrfs_trans_handle *trans,
> -			struct btrfs_fs_info *fs_info)
> +int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_root *quota_root;
> +	struct btrfs_trans_handle *trans = NULL;
>  	int ret = 0;
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root)
>  		goto out;
> +
> +	trans = btrfs_start_transaction(fs_info->tree_root, 2);

Same here.

> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
>  	clear_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>  	btrfs_qgroup_wait_for_completion(fs_info, false);
>  	spin_lock(&fs_info->qgroup_lock);
> @@ -1031,12 +1049,16 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>  	btrfs_free_qgroup_config(fs_info);
>  
>  	ret = btrfs_clean_quota_tree(trans, quota_root);
> -	if (ret)
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
>  		goto out;
> +	}
>  
>  	ret = btrfs_del_root(trans, fs_info, &quota_root->root_key);
> -	if (ret)
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
>  		goto out;
> +	}
>  
>  	list_del(&quota_root->dirty_list);
>  
> @@ -1048,6 +1070,8 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>  	free_extent_buffer(quota_root->node);
>  	free_extent_buffer(quota_root->commit_root);
>  	kfree(quota_root);
> +
> +	ret = btrfs_commit_transaction(trans);
>  out:
>  	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  	return ret;
> @@ -3070,7 +3094,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>  	if (free && reserved)
>  		return qgroup_free_reserved_data(inode, reserved, start, len);
>  	extent_changeset_init(&changeset);
> -	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
> +	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,

I see an unrelated whitespace change

>  			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
>  	if (ret < 0)
>  		goto out;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index d60dd06445ce..bec7c9b17a8e 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -141,10 +141,8 @@ struct btrfs_qgroup {
>  #define QGROUP_RELEASE		(1<<1)
>  #define QGROUP_FREE		(1<<2)
>  
> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> -		       struct btrfs_fs_info *fs_info);
> -int btrfs_quota_disable(struct btrfs_trans_handle *trans,
> -			struct btrfs_fs_info *fs_info);
> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
> +int btrfs_quota_disable(struct btrfs_fs_info *fs_info);
>  int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
>  void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
>  int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Misono Tomohiro]

On 2018/07/02 20:00, Nikolay Borisov wrote:
> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
> inside btrfs_quota_enable, which allows to schedule the quota commit
> after the transaction has been committed.
> 
> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
> Link: https://marc.info/?l=linux-btrfs&m=152999289017582
> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ioctl.c  | 15 ++-------------
>  fs/btrfs/qgroup.c | 38 +++++++++++++++++++++++++++++++-------
>  fs/btrfs/qgroup.h |  6 ++----
>  3 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a399750b9e41..316fb1af15e2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5135,9 +5135,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>  	struct inode *inode = file_inode(file);
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_ioctl_quota_ctl_args *sa;
> -	struct btrfs_trans_handle *trans = NULL;
>  	int ret;
> -	int err;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -5153,28 +5151,19 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>  	}
>  
>  	down_write(&fs_info->subvol_sem);
> -	trans = btrfs_start_transaction(fs_info->tree_root, 2);
> -	if (IS_ERR(trans)) {
> -		ret = PTR_ERR(trans);
> -		goto out;
> -	}
>  
>  	switch (sa->cmd) {
>  	case BTRFS_QUOTA_CTL_ENABLE:
> -		ret = btrfs_quota_enable(trans, fs_info);
> +		ret = btrfs_quota_enable(fs_info);
>  		break;
>  	case BTRFS_QUOTA_CTL_DISABLE:
> -		ret = btrfs_quota_disable(trans, fs_info);
> +		ret = btrfs_quota_disable(fs_info);
>  		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
>  	}
>  
> -	err = btrfs_commit_transaction(trans);
> -	if (err && !ret)
> -		ret = err;
> -out:
>  	kfree(sa);
>  	up_write(&fs_info->subvol_sem);
>  drop_write:
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c25dc47210a3..1012c7138633 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> -		       struct btrfs_fs_info *fs_info)
> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_root *quota_root;
>  	struct btrfs_root *tree_root = fs_info->tree_root;
> @@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	struct btrfs_key key;
>  	struct btrfs_key found_key;
>  	struct btrfs_qgroup *qgroup = NULL;
> +	struct btrfs_trans_handle *trans = NULL;
>  	int ret = 0;
>  	int slot;
>  
> @@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	if (fs_info->quota_root)
>  		goto out;
>  
> +	trans = btrfs_start_transaction(tree_root, 2);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
>  	if (!fs_info->qgroup_ulist) {
>  		ret = -ENOMEM;
> @@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	fs_info->quota_root = quota_root;
>  	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>  	spin_unlock(&fs_info->qgroup_lock);
> +
> +	ret = btrfs_commit_transaction(trans);
> +	if (ret)
> +		goto out_free_path;
> +

I realized that some error paths also need to finish transaction (continue to below). 

>  	ret = qgroup_rescan_init(fs_info, 0, 1);
>  	if (!ret) {
>  	        qgroup_rescan_zero_tracking(fs_info);
> @@ -1011,15 +1022,22 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -int btrfs_quota_disable(struct btrfs_trans_handle *trans,
> -			struct btrfs_fs_info *fs_info)
> +int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_root *quota_root;
> +	struct btrfs_trans_handle *trans = NULL;
>  	int ret = 0;
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root)
>  		goto out;
> +
> +	trans = btrfs_start_transaction(fs_info->tree_root, 2);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
>  	clear_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>  	btrfs_qgroup_wait_for_completion(fs_info, false);
>  	spin_lock(&fs_info->qgroup_lock);
> @@ -1031,12 +1049,16 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>  	btrfs_free_qgroup_config(fs_info);
>  
>  	ret = btrfs_clean_quota_tree(trans, quota_root);
> -	if (ret)
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);

And btrfs_end_transaction() is needed here and below.

Thanks,
Misono

>  		goto out;
> +	}
>  
>  	ret = btrfs_del_root(trans, fs_info, &quota_root->root_key);
> -	if (ret)
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
>  		goto out;
> +	}
>  
>  	list_del(&quota_root->dirty_list);
>  
> @@ -1048,6 +1070,8 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>  	free_extent_buffer(quota_root->node);
>  	free_extent_buffer(quota_root->commit_root);
>  	kfree(quota_root);
> +
> +	ret = btrfs_commit_transaction(trans);
>  out:
>  	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  	return ret;
> @@ -3070,7 +3094,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>  	if (free && reserved)
>  		return qgroup_free_reserved_data(inode, reserved, start, len);
>  	extent_changeset_init(&changeset);
> -	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
> +	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
>  			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
>  	if (ret < 0)
>  		goto out;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index d60dd06445ce..bec7c9b17a8e 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -141,10 +141,8 @@ struct btrfs_qgroup {
>  #define QGROUP_RELEASE		(1<<1)
>  #define QGROUP_FREE		(1<<2)
>  
> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> -		       struct btrfs_fs_info *fs_info);
> -int btrfs_quota_disable(struct btrfs_trans_handle *trans,
> -			struct btrfs_fs_info *fs_info);
> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
> +int btrfs_quota_disable(struct btrfs_fs_info *fs_info);
>  int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
>  void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
>  int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,
> 

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Nikolay Borisov]

On  2.07.2018 18:40, David Sterba wrote:
> On Mon, Jul 02, 2018 at 02:00:34PM +0300, Nikolay Borisov wrote:
>> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
>> inside btrfs_quota_enable, which allows to schedule the quota commit
>> after the transaction has been committed.
>>
>> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
>> Link: https://marc.info/?l=linux-btrfs&m=152999289017582
> 
> Please use https://lkml.kernel.org/r/<message-id>

That won't work since lkml.kernel is , well, only for lkml and this was
posted to the btrfs mailing list.

> 
>> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[David Sterba]

On Tue, Jul 03, 2018 at 11:54:11AM +0300, Nikolay Borisov wrote:
> 
> 
> On  2.07.2018 18:40, David Sterba wrote:
> > On Mon, Jul 02, 2018 at 02:00:34PM +0300, Nikolay Borisov wrote:
> >> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
> >> inside btrfs_quota_enable, which allows to schedule the quota commit
> >> after the transaction has been committed.
> >>
> >> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
> >> Link: https://marc.info/?l=linux-btrfs&m=152999289017582
> > 
> > Please use https://lkml.kernel.org/r/<message-id>
> 
> That won't work since lkml.kernel is , well, only for lkml and this was
> posted to the btrfs mailing list.

Right. Would be better to have the message-id at least but, oh well.

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Qu Wenruo]

On 2018年06月26日 16:46, Misono Tomohiro wrote:
> On 2018/06/26 16:09, Nikolay Borisov wrote:
>> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
>> inside btrfs_quota_enable, which allows to schedule the quota commit
>> after the transaction has been committed.
>>
>> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
>> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> Hi Misono, 
>>
>> Care to test the following patch ? If you say it's ok I will do a similar one 
>> for the btrfs_quota_disable function. This will also allow me to get rid of 
>> the extra err variable in btrfs_ioctl_quota_ctl. Additionally I think the 
>> number of blocks (2) passed to the transaction for reservation might be 
>> wrong. 
> 
> Hi,
> 
> The patch does not removes start_transaction() from btrfs_ioctl_quota_ctl(),
> so this does not work but I understand your approach (continue to  below).
> 
>>
>>  fs/btrfs/ioctl.c  |  2 +-
>>  fs/btrfs/qgroup.c | 17 ++++++++++++++---
>>  fs/btrfs/qgroup.h |  3 +--
>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index a399750b9e41..bf99d7aae3ae 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -5161,7 +5161,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>>  
>>  	switch (sa->cmd) {
>>  	case BTRFS_QUOTA_CTL_ENABLE:
>> -		ret = btrfs_quota_enable(trans, fs_info);
>> +		ret = btrfs_quota_enable(fs_info);
>>  		break;
>>  	case BTRFS_QUOTA_CTL_DISABLE:
>>  		ret = btrfs_quota_disable(trans, fs_info);
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 1874a6d2e6f5..91bb7e97c0d0 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
>>  	return ret;
>>  }
>>  
>> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>> -		       struct btrfs_fs_info *fs_info)
>> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>>  {
>>  	struct btrfs_root *quota_root;
>>  	struct btrfs_root *tree_root = fs_info->tree_root;
>> @@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>>  	struct btrfs_key key;
>>  	struct btrfs_key found_key;
>>  	struct btrfs_qgroup *qgroup = NULL;
>> +	struct btrfs_trans_handle *trans = NULL;
>>  	int ret = 0;
>>  	int slot;
>>  
>> @@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>>  	if (fs_info->quota_root)
>>  		goto out;
>>  
>> +	trans = btrfs_start_transaction(tree_root, 2);
> 
> (Should we use fs_root for quota?)
> 
>> +	if (IS_ERR(trans)) {
>> +		ret = PTR_ERR(trans);
>> +		goto out;
>> +	}
>> +
>>  	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
>>  	if (!fs_info->qgroup_ulist) {
>>  		ret = -ENOMEM;
>> @@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>>  	fs_info->quota_root = quota_root;
>>  	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>  	spin_unlock(&fs_info->qgroup_lock);
>> +
>> +	ret = btrfs_commit_transaction(trans);
>> +	if (ret)
>> +		goto out_free_path;
>> +
>>  	ret = qgroup_rescan_init(fs_info, 0, 1);
> 
> However, I'm not sure if this approach completely works well when some files are
> concurrently written while quota is being enabled.
> Since before the commit 5d23515be669, quota_rescan_init() is called during transaction
> commit, but now quota_rescan_init() is called outside of transacation.
> So, is there still a slight possibility that the same problem occurs here?

This is the tricky part of btrfs quota rescan.
For rescan, it only scans commit root (even before the large quota
rework), and records where the current scanning location.

Then, qgroup code does a trick, if new dirty extents is after current
scanning location, which means later rescan would scan that extent
later, so it skips the accounting and let rescan to handle it.

Currently since qgroup only accounts extent at transaction commit time,
the only possible cause of problems should be the timing of
@qgroup_rescan_progress initialization.

I think we should hold a trans handle when setting
@qgroup_rescan_progress, but I may need extra investigation into the race.

Thanks,
Qu

> 
> (I don't completely understands how quota works yet , so correct me if I'm wrong.)
> 
>>  	if (!ret) {
>>  	        qgroup_rescan_zero_tracking(fs_info);
>> @@ -3061,7 +3072,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>  	if (free && reserved)
>>  		return qgroup_free_reserved_data(inode, reserved, start, len);
>>  	extent_changeset_init(&changeset);
>> -	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
>> +	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
>>  			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
>>  	if (ret < 0)
>>  		goto out;
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index d60dd06445ce..3900efab0e70 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -141,8 +141,7 @@ struct btrfs_qgroup {
>>  #define QGROUP_RELEASE		(1<<1)
>>  #define QGROUP_FREE		(1<<2)
>>  
>> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>> -		       struct btrfs_fs_info *fs_info);
>> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
>>  int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>>  			struct btrfs_fs_info *fs_info);
>>  int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Nikolay Borisov]

On 26.06.2018 11:46, Misono Tomohiro wrote:
> On 2018/06/26 16:09, Nikolay Borisov wrote:
>> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
>> inside btrfs_quota_enable, which allows to schedule the quota commit
>> after the transaction has been committed.
>>
>> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
>> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> Hi Misono, 
>>
>> Care to test the following patch ? If you say it's ok I will do a similar one 
>> for the btrfs_quota_disable function. This will also allow me to get rid of 
>> the extra err variable in btrfs_ioctl_quota_ctl. Additionally I think the 
>> number of blocks (2) passed to the transaction for reservation might be 
>> wrong. 
> 
> Hi,
> 
> The patch does not removes start_transaction() from btrfs_ioctl_quota_ctl(),
> so this does not work but I understand your approach (continue to  below).

Ah you are right, the reason I didn't do it is because in a 2nd patch I
wanted to also remove the trans argument to btrfs_quota_disable. So
yeah, I will have to do that in one go.
> 
>>
>>  fs/btrfs/ioctl.c  |  2 +-
>>  fs/btrfs/qgroup.c | 17 ++++++++++++++---
>>  fs/btrfs/qgroup.h |  3 +--
>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index a399750b9e41..bf99d7aae3ae 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -5161,7 +5161,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>>  
>>  	switch (sa->cmd) {
>>  	case BTRFS_QUOTA_CTL_ENABLE:
>> -		ret = btrfs_quota_enable(trans, fs_info);
>> +		ret = btrfs_quota_enable(fs_info);
>>  		break;
>>  	case BTRFS_QUOTA_CTL_DISABLE:
>>  		ret = btrfs_quota_disable(trans, fs_info);
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 1874a6d2e6f5..91bb7e97c0d0 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
>>  	return ret;
>>  }
>>  
>> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>> -		       struct btrfs_fs_info *fs_info)
>> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>>  {
>>  	struct btrfs_root *quota_root;
>>  	struct btrfs_root *tree_root = fs_info->tree_root;
>> @@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>>  	struct btrfs_key key;
>>  	struct btrfs_key found_key;
>>  	struct btrfs_qgroup *qgroup = NULL;
>> +	struct btrfs_trans_handle *trans = NULL;
>>  	int ret = 0;
>>  	int slot;
>>  
>> @@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>>  	if (fs_info->quota_root)
>>  		goto out;
>>  
>> +	trans = btrfs_start_transaction(tree_root, 2);
> 
> (Should we use fs_root for quota?)

Good question, I just copied the code. The thing is however, when
enabling the quota we might also have to insert the quota tree root item
in the tree_root, I guess that's why it was originally set to tree_root.
> 
>> +	if (IS_ERR(trans)) {
>> +		ret = PTR_ERR(trans);
>> +		goto out;
>> +	}
>> +
>>  	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
>>  	if (!fs_info->qgroup_ulist) {
>>  		ret = -ENOMEM;
>> @@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>>  	fs_info->quota_root = quota_root;
>>  	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>  	spin_unlock(&fs_info->qgroup_lock);
>> +
>> +	ret = btrfs_commit_transaction(trans);
>> +	if (ret)
>> +		goto out_free_path;
>> +
>>  	ret = qgroup_rescan_init(fs_info, 0, 1);
> 
> However, I'm not sure if this approach completely works well when some files are
> concurrently written while quota is being enabled.
> Since before the commit 5d23515be669, quota_rescan_init() is called during transaction
> commit, but now quota_rescan_init() is called outside of transacation.
> So, is there still a slight possibility that the same problem occurs here?

Good point, I will have to defer to Qu since he seems to be a quota
expert. My thinking would be that quota will be consistent w.r.t the
last committed transaction. Commit 5d23515be669 just moved the initial
quota rescan and not the running of dirty qgroups. We still have the
btrfs_run_qgroups call in transaction commit.

> 
> (I don't completely understands how quota works yet , so correct me if I'm wrong.)
> 
>>  	if (!ret) {
>>  	        qgroup_rescan_zero_tracking(fs_info);
>> @@ -3061,7 +3072,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>  	if (free && reserved)
>>  		return qgroup_free_reserved_data(inode, reserved, start, len);
>>  	extent_changeset_init(&changeset);
>> -	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
>> +	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
>>  			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
>>  	if (ret < 0)
>>  		goto out;
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index d60dd06445ce..3900efab0e70 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -141,8 +141,7 @@ struct btrfs_qgroup {
>>  #define QGROUP_RELEASE		(1<<1)
>>  #define QGROUP_FREE		(1<<2)
>>  
>> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>> -		       struct btrfs_fs_info *fs_info);
>> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
>>  int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>>  			struct btrfs_fs_info *fs_info);
>>  int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Misono Tomohiro]

On 2018/06/26 16:09, Nikolay Borisov wrote:
> Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
> inside btrfs_quota_enable, which allows to schedule the quota commit
> after the transaction has been committed.
> 
> Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> Hi Misono, 
> 
> Care to test the following patch ? If you say it's ok I will do a similar one 
> for the btrfs_quota_disable function. This will also allow me to get rid of 
> the extra err variable in btrfs_ioctl_quota_ctl. Additionally I think the 
> number of blocks (2) passed to the transaction for reservation might be 
> wrong. 

Hi,

The patch does not removes start_transaction() from btrfs_ioctl_quota_ctl(),
so this does not work but I understand your approach (continue to  below).

> 
>  fs/btrfs/ioctl.c  |  2 +-
>  fs/btrfs/qgroup.c | 17 ++++++++++++++---
>  fs/btrfs/qgroup.h |  3 +--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a399750b9e41..bf99d7aae3ae 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5161,7 +5161,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>  
>  	switch (sa->cmd) {
>  	case BTRFS_QUOTA_CTL_ENABLE:
> -		ret = btrfs_quota_enable(trans, fs_info);
> +		ret = btrfs_quota_enable(fs_info);
>  		break;
>  	case BTRFS_QUOTA_CTL_DISABLE:
>  		ret = btrfs_quota_disable(trans, fs_info);
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1874a6d2e6f5..91bb7e97c0d0 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> -		       struct btrfs_fs_info *fs_info)
> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_root *quota_root;
>  	struct btrfs_root *tree_root = fs_info->tree_root;
> @@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	struct btrfs_key key;
>  	struct btrfs_key found_key;
>  	struct btrfs_qgroup *qgroup = NULL;
> +	struct btrfs_trans_handle *trans = NULL;
>  	int ret = 0;
>  	int slot;
>  
> @@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	if (fs_info->quota_root)
>  		goto out;
>  
> +	trans = btrfs_start_transaction(tree_root, 2);

(Should we use fs_root for quota?)

> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
>  	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
>  	if (!fs_info->qgroup_ulist) {
>  		ret = -ENOMEM;
> @@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
>  	fs_info->quota_root = quota_root;
>  	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>  	spin_unlock(&fs_info->qgroup_lock);
> +
> +	ret = btrfs_commit_transaction(trans);
> +	if (ret)
> +		goto out_free_path;
> +
>  	ret = qgroup_rescan_init(fs_info, 0, 1);

However, I'm not sure if this approach completely works well when some files are
concurrently written while quota is being enabled.
Since before the commit 5d23515be669, quota_rescan_init() is called during transaction
commit, but now quota_rescan_init() is called outside of transacation.
So, is there still a slight possibility that the same problem occurs here?

(I don't completely understands how quota works yet , so correct me if I'm wrong.)

>  	if (!ret) {
>  	        qgroup_rescan_zero_tracking(fs_info);
> @@ -3061,7 +3072,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>  	if (free && reserved)
>  		return qgroup_free_reserved_data(inode, reserved, start, len);
>  	extent_changeset_init(&changeset);
> -	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
> +	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
>  			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
>  	if (ret < 0)
>  		goto out;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index d60dd06445ce..3900efab0e70 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -141,8 +141,7 @@ struct btrfs_qgroup {
>  #define QGROUP_RELEASE		(1<<1)
>  #define QGROUP_FREE		(1<<2)
>  
> -int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> -		       struct btrfs_fs_info *fs_info);
> +int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
>  int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>  			struct btrfs_fs_info *fs_info);
>  int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
> 

[PATCH] btrfs: qgroups: Move transaction managed inside btrfs_quota_enable

[Nikolay Borisov]

Commit 5d23515be669 ("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 but 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 movign the transaction creation/commit
inside btrfs_quota_enable, which allows to schedule the quota commit
after the transaction has been committed.

Fixes: 5d23515be669 ("btrfs: Move qgroup rescan on quota enable to btrfs_quota_enable")
Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
Hi Misono, 

Care to test the following patch ? If you say it's ok I will do a similar one 
for the btrfs_quota_disable function. This will also allow me to get rid of 
the extra err variable in btrfs_ioctl_quota_ctl. Additionally I think the 
number of blocks (2) passed to the transaction for reservation might be 
wrong. 

 fs/btrfs/ioctl.c  |  2 +-
 fs/btrfs/qgroup.c | 17 ++++++++++++++---
 fs/btrfs/qgroup.h |  3 +--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a399750b9e41..bf99d7aae3ae 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5161,7 +5161,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
 
 	switch (sa->cmd) {
 	case BTRFS_QUOTA_CTL_ENABLE:
-		ret = btrfs_quota_enable(trans, fs_info);
+		ret = btrfs_quota_enable(fs_info);
 		break;
 	case BTRFS_QUOTA_CTL_DISABLE:
 		ret = btrfs_quota_disable(trans, fs_info);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 1874a6d2e6f5..91bb7e97c0d0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -875,8 +875,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_quota_enable(struct btrfs_trans_handle *trans,
-		       struct btrfs_fs_info *fs_info)
+int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_root *tree_root = fs_info->tree_root;
@@ -886,6 +885,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	struct btrfs_key key;
 	struct btrfs_key found_key;
 	struct btrfs_qgroup *qgroup = NULL;
+	struct btrfs_trans_handle *trans = NULL;
 	int ret = 0;
 	int slot;
 
@@ -893,6 +893,12 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	if (fs_info->quota_root)
 		goto out;
 
+	trans = btrfs_start_transaction(tree_root, 2);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
+
 	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
 	if (!fs_info->qgroup_ulist) {
 		ret = -ENOMEM;
@@ -987,6 +993,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	fs_info->quota_root = quota_root;
 	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
 	spin_unlock(&fs_info->qgroup_lock);
+
+	ret = btrfs_commit_transaction(trans);
+	if (ret)
+		goto out_free_path;
+
 	ret = qgroup_rescan_init(fs_info, 0, 1);
 	if (!ret) {
 	        qgroup_rescan_zero_tracking(fs_info);
@@ -3061,7 +3072,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free && reserved)
 		return qgroup_free_reserved_data(inode, reserved, start, len);
 	extent_changeset_init(&changeset);
-	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
+	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
 			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
 	if (ret < 0)
 		goto out;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index d60dd06445ce..3900efab0e70 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -141,8 +141,7 @@ struct btrfs_qgroup {
 #define QGROUP_RELEASE		(1<<1)
 #define QGROUP_FREE		(1<<2)
 
-int btrfs_quota_enable(struct btrfs_trans_handle *trans,
-		       struct btrfs_fs_info *fs_info);
+int btrfs_quota_enable(struct btrfs_fs_info *fs_info);
 int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 			struct btrfs_fs_info *fs_info);
 int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
-- 
2.7.4