Enabling quota may not correctly rescan on 4.17

Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

Hello Nikolay,

I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
to fail correctly rescanning quota when quota is enabled.

Simple reproducer:

$ mkfs.btrfs -f $DEV
$ mount $DEV /mnt
$ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
$ btrfs quota enbale /mnt
$ umount /mnt
$ btrfs check $DEV
...
checking quota groups
Counts for qgroup id: 0/5 are different
our:            referenced 1019904 referenced compressed 1019904
disk:           referenced 16384 referenced compressed 16384
diff:           referenced 1003520 referenced compressed 1003520
our:            exclusive 1019904 exclusive compressed 1019904
disk:           exclusive 16384 exclusive compressed 16384
diff:           exclusive 1003520 exclusive compressed 1003520
found 1413120 bytes used, error(s) found
...

This can be also observed in btrfs/114. \v(Note that progs < 4.17
returns error code 0 even if quota is not consistency and therefore
test will incorrectly pass.)

My observation is that this commit changed to call initial quota rescan
when quota is enabeld instead of first comit transaction after enabling
quota, and therefore if there is something not commited at that time,
their usage will not be accounted.

Actually this can be simply fixed by calling "btrfs rescan" again or
calling "btrfs fi sync" before "btrfs quota enable".

I think the commit itself makes the code much easier to read, so it may
be better to fix the problem in progs (i.e. calling sync before quota enable).

Do you have any thoughts?

Thanks,
Tomohiro Misono

Re: Enabling quota may not correctly rescan on 4.17

[Nikolay Borisov]

On 26.06.2018 09:00, Misono Tomohiro wrote:
> Hello Nikolay,
> 
> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
> to fail correctly rescanning quota when quota is enabled.

Everytime I hear anything quota-related and I cringe ...

> 
> Simple reproducer:
> 
> $ mkfs.btrfs -f $DEV
> $ mount $DEV /mnt
> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
> $ btrfs quota enbale /mnt
> $ umount /mnt
> $ btrfs check $DEV
> ...
> checking quota groups
> Counts for qgroup id: 0/5 are different
> our:            referenced 1019904 referenced compressed 1019904
> disk:           referenced 16384 referenced compressed 16384
> diff:           referenced 1003520 referenced compressed 1003520
> our:            exclusive 1019904 exclusive compressed 1019904
> disk:           exclusive 16384 exclusive compressed 16384
> diff:           exclusive 1003520 exclusive compressed 1003520
> found 1413120 bytes used, error(s) found
> ...
> 
> This can be also observed in btrfs/114. \v(Note that progs < 4.17
> returns error code 0 even if quota is not consistency and therefore
> test will incorrectly pass.)
> 
> My observation is that this commit changed to call initial quota rescan
> when quota is enabeld instead of first comit transaction after enabling
> quota, and therefore if there is something not commited at that time,
> their usage will not be accounted.
> 
> Actually this can be simply fixed by calling "btrfs rescan" again or
> calling "btrfs fi sync" before "btrfs quota enable".
> 
> I think the commit itself makes the code much easier to read, so it may
> be better to fix the problem in progs (i.e. calling sync before quota enable).
> 
> Do you have any thoughts?

So fixing it in progs will indeed work, however any 3rd party code which
relies on the scan/enable ioctl will be "broken" so that's not good.
Ideally the correct thing should be that quota scanning can also take
into account the in-memory state i.e any delayed refs but I think this
will be way too much work.

Alternatively, what about moving the transaction start from
btrfs_ioctl_quota_ctl to btrfs_quota_enable/btrfs_quota_disable
functions. That way what we can do is perform the transaction commit in
btrfs_quota_enable before queuing the rescan?

> 
> Thanks,
> Tomohiro Misono
> 
> 
> 

[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

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);
> 

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

[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: Enabling quota may not correctly rescan on 4.17

[Nikolay Borisov]

On 26.06.2018 09:00, Misono Tomohiro wrote:
> Hello Nikolay,
> 
> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
> to fail correctly rescanning quota when quota is enabled.
> 
> Simple reproducer:
> 
> $ mkfs.btrfs -f $DEV
> $ mount $DEV /mnt
> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
> $ btrfs quota enbale /mnt
> $ umount /mnt
> $ btrfs check $DEV
> ...
> checking quota groups
> Counts for qgroup id: 0/5 are different
> our:            referenced 1019904 referenced compressed 1019904
> disk:           referenced 16384 referenced compressed 16384
> diff:           referenced 1003520 referenced compressed 1003520
> our:            exclusive 1019904 exclusive compressed 1019904
> disk:           exclusive 16384 exclusive compressed 16384
> diff:           exclusive 1003520 exclusive compressed 1003520
> found 1413120 bytes used, error(s) found
> ...

I ran your script 100 times with progs 4.17 and 4.18-rc1 and didn't
observe this error. I didn't observe btrfs/114 also failing but I ran it
a lot less. Is there anything else i can do to make your small
reproducer more likely to trigger?

> 
> This can be also observed in btrfs/114. \v(Note that progs < 4.17
> returns error code 0 even if quota is not consistency and therefore
> test will incorrectly pass.)
> 
> My observation is that this commit changed to call initial quota rescan
> when quota is enabeld instead of first comit transaction after enabling
> quota, and therefore if there is something not commited at that time,
> their usage will not be accounted.
> 
> Actually this can be simply fixed by calling "btrfs rescan" again or
> calling "btrfs fi sync" before "btrfs quota enable".
> 
> I think the commit itself makes the code much easier to read, so it may
> be better to fix the problem in progs (i.e. calling sync before quota enable).
> 
> Do you have any thoughts?
> 
> Thanks,
> Tomohiro Misono
> 
> 
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

On 2018/06/27 16:40, Nikolay Borisov wrote:
> 
> 
> On 26.06.2018 09:00, Misono Tomohiro wrote:
>> Hello Nikolay,
>>
>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>> to fail correctly rescanning quota when quota is enabled.
>>
>> Simple reproducer:
>>
>> $ mkfs.btrfs -f $DEV
>> $ mount $DEV /mnt
>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>> $ btrfs quota enbale /mnt
>> $ umount /mnt
>> $ btrfs check $DEV
>> ...
>> checking quota groups
>> Counts for qgroup id: 0/5 are different
>> our:            referenced 1019904 referenced compressed 1019904
>> disk:           referenced 16384 referenced compressed 16384
>> diff:           referenced 1003520 referenced compressed 1003520
>> our:            exclusive 1019904 exclusive compressed 1019904
>> disk:           exclusive 16384 exclusive compressed 16384
>> diff:           exclusive 1003520 exclusive compressed 1003520
>> found 1413120 bytes used, error(s) found
>> ...
> 
> I ran your script 100 times with progs 4.17 and 4.18-rc1 and didn't
> observe this error. I didn't observe btrfs/114 also failing but I ran it
> a lot less. Is there anything else i can do to make your small
> reproducer more likely to trigger?

How about btrfs/114? I saw the problem in it first (progs 4.17/kernel 4.18-rc2)
and it seems always happen in my environment. 

> 
>>
>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>> returns error code 0 even if quota is not consistency and therefore
>> test will incorrectly pass.)
>>
>> My observation is that this commit changed to call initial quota rescan
>> when quota is enabeld instead of first comit transaction after enabling
>> quota, and therefore if there is something not commited at that time,
>> their usage will not be accounted.
>>
>> Actually this can be simply fixed by calling "btrfs rescan" again or
>> calling "btrfs fi sync" before "btrfs quota enable".
>>
>> I think the commit itself makes the code much easier to read, so it may
>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>
>> Do you have any thoughts?
>>
>> Thanks,
>> Tomohiro Misono
>>
>>
>> --
>> 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
>>
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Nikolay Borisov]

On 27.06.2018 10:55, Misono Tomohiro wrote:
> On 2018/06/27 16:40, Nikolay Borisov wrote:
>>
>>
>> On 26.06.2018 09:00, Misono Tomohiro wrote:
>>> Hello Nikolay,
>>>
>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>> to fail correctly rescanning quota when quota is enabled.
>>>
>>> Simple reproducer:
>>>
>>> $ mkfs.btrfs -f $DEV
>>> $ mount $DEV /mnt
>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>> $ btrfs quota enbale /mnt
>>> $ umount /mnt
>>> $ btrfs check $DEV
>>> ...
>>> checking quota groups
>>> Counts for qgroup id: 0/5 are different
>>> our:            referenced 1019904 referenced compressed 1019904
>>> disk:           referenced 16384 referenced compressed 16384
>>> diff:           referenced 1003520 referenced compressed 1003520
>>> our:            exclusive 1019904 exclusive compressed 1019904
>>> disk:           exclusive 16384 exclusive compressed 16384
>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>> found 1413120 bytes used, error(s) found
>>> ...
>>
>> I ran your script 100 times with progs 4.17 and 4.18-rc1 and didn't
>> observe this error. I didn't observe btrfs/114 also failing but I ran it
>> a lot less. Is there anything else i can do to make your small
>> reproducer more likely to trigger?
> 
> How about btrfs/114? I saw the problem in it first (progs 4.17/kernel 4.18-rc2)
> and it seems always happen in my environment. 

So far nothing, I'm using David's github/misc-next branch, and latest
commit is: 5330a89b3ee3.

My mount options are:

MOUNT_OPTIONS -- -o enospc_debug -o space_cache=v2 /dev/vdc /media/scratch

> 
>>
>>>
>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>> returns error code 0 even if quota is not consistency and therefore
>>> test will incorrectly pass.)
>>>
>>> My observation is that this commit changed to call initial quota rescan
>>> when quota is enabeld instead of first comit transaction after enabling
>>> quota, and therefore if there is something not commited at that time,
>>> their usage will not be accounted.
>>>
>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>
>>> I think the commit itself makes the code much easier to read, so it may
>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>
>>> Do you have any thoughts?
>>>
>>> Thanks,
>>> Tomohiro Misono
>>>
>>>
>>> --
>>> 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
>>>
>> --
>> 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
>>
> 
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

On 2018/06/27 17:04, Nikolay Borisov wrote:
> 
> 
> On 27.06.2018 10:55, Misono Tomohiro wrote:
>> On 2018/06/27 16:40, Nikolay Borisov wrote:
>>>
>>>
>>> On 26.06.2018 09:00, Misono Tomohiro wrote:
>>>> Hello Nikolay,
>>>>
>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>> to fail correctly rescanning quota when quota is enabled.
>>>>
>>>> Simple reproducer:
>>>>
>>>> $ mkfs.btrfs -f $DEV
>>>> $ mount $DEV /mnt
>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>> $ btrfs quota enbale /mnt
>>>> $ umount /mnt
>>>> $ btrfs check $DEV
>>>> ...
>>>> checking quota groups
>>>> Counts for qgroup id: 0/5 are different
>>>> our:            referenced 1019904 referenced compressed 1019904
>>>> disk:           referenced 16384 referenced compressed 16384
>>>> diff:           referenced 1003520 referenced compressed 1003520
>>>> our:            exclusive 1019904 exclusive compressed 1019904
>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>>> found 1413120 bytes used, error(s) found
>>>> ...
>>>
>>> I ran your script 100 times with progs 4.17 and 4.18-rc1 and didn't
>>> observe this error. I didn't observe btrfs/114 also failing but I ran it
>>> a lot less. Is there anything else i can do to make your small
>>> reproducer more likely to trigger?
>>
>> How about btrfs/114? I saw the problem in it first (progs 4.17/kernel 4.18-rc2)
>> and it seems always happen in my environment. 
> 
> So far nothing, I'm using David's github/misc-next branch, and latest
> commit is: 5330a89b3ee3.
> 
> My mount options are:
> 
> MOUNT_OPTIONS -- -o enospc_debug -o space_cache=v2 /dev/vdc /media/scratch

I can see the failure with or without options...
maybe it depends on machine spec?

> 
>>
>>>
>>>>
>>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>>> returns error code 0 even if quota is not consistency and therefore
>>>> test will incorrectly pass.)
>>>>
>>>> My observation is that this commit changed to call initial quota rescan
>>>> when quota is enabeld instead of first comit transaction after enabling
>>>> quota, and therefore if there is something not commited at that time,
>>>> their usage will not be accounted.
>>>>
>>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>>
>>>> I think the commit itself makes the code much easier to read, so it may
>>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>>
>>>> Do you have any thoughts?
>>>>
>>>> Thanks,
>>>> Tomohiro Misono
>>>>
>>>>
>>>> --
>>>> 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
>>>>
>>> --
>>> 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
>>>
>>
>> --
>> 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: Enabling quota may not correctly rescan on 4.17

[Nikolay Borisov]

On 27.06.2018 11:20, Misono Tomohiro wrote:
> I can see the failure with or without options...
> maybe it depends on machine spec?

I'm testing in a virtual machine: 

qemu-system-x86_64 -smp 6 -kernel /home/nborisov/projects/kernel/source/arch/x86_64/boot/bzImage -append root=/dev/vda rw -enable-kvm -m 4096 -drive file=/home/nborisov/projects/qemu/rootfs/ubuntu15.img,if=virtio -virtfs local,id=fsdev1,path=/mnt/vm_share,security_model=passthrough,mount_tag=hostshare -drive file=/home/nborisov/scratch/scratch-images/btrfs-test.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch2.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch3.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch4.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch5.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch6.img,if=virtio -redir tcp:1235::22 -daemonize

Perhaps it's not visible on slow storage. Are you testing on NVME or something like that? 

Re: Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

On 2018/06/27 17:22, Nikolay Borisov wrote:
> 
> 
> On 27.06.2018 11:20, Misono Tomohiro wrote:
>> I can see the failure with or without options...
>> maybe it depends on machine spec?
> 
> I'm testing in a virtual machine: 
> 
> qemu-system-x86_64 -smp 6 -kernel /home/nborisov/projects/kernel/source/arch/x86_64/boot/bzImage -append root=/dev/vda rw -enable-kvm -m 4096 -drive file=/home/nborisov/projects/qemu/rootfs/ubuntu15.img,if=virtio -virtfs local,id=fsdev1,path=/mnt/vm_share,security_model=passthrough,mount_tag=hostshare -drive file=/home/nborisov/scratch/scratch-images/btrfs-test.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch2.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch3.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch4.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch5.img,if=virtio -drive file=/home/nborisov/scratch/scratch-images/scratch6.img,if=virtio -redir tcp:1235::22 -daemonize
> 
> Perhaps it's not visible on slow storage. Are you testing on NVME or something like that? 

No, I use sata ssd and hdd and the problem can be seen on both.

Re: Enabling quota may not correctly rescan on 4.17

[Qu Wenruo]

On 2018年06月26日 14:00, Misono Tomohiro wrote:
> Hello Nikolay,
> 
> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
> to fail correctly rescanning quota when quota is enabled.
> 
> Simple reproducer:
> 
> $ mkfs.btrfs -f $DEV
> $ mount $DEV /mnt
> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
> $ btrfs quota enbale /mnt
> $ umount /mnt
> $ btrfs check $DEV
> ...
> checking quota groups
> Counts for qgroup id: 0/5 are different
> our:            referenced 1019904 referenced compressed 1019904
> disk:           referenced 16384 referenced compressed 16384
> diff:           referenced 1003520 referenced compressed 1003520
> our:            exclusive 1019904 exclusive compressed 1019904
> disk:           exclusive 16384 exclusive compressed 16384
> diff:           exclusive 1003520 exclusive compressed 1003520
> found 1413120 bytes used, error(s) found
> ...
> 
> This can be also observed in btrfs/114. \v(Note that progs < 4.17
> returns error code 0 even if quota is not consistency and therefore
> test will incorrectly pass.)

BTW, would you please try to dump the quota tree for such mismatch case?

It could be a btrfs-progs bug which it should skip quota checking if it
found the quota status item has RESCAN flag.

Thanks,
Qu

> 
> My observation is that this commit changed to call initial quota rescan
> when quota is enabeld instead of first comit transaction after enabling
> quota, and therefore if there is something not commited at that time,
> their usage will not be accounted.
> 
> Actually this can be simply fixed by calling "btrfs rescan" again or
> calling "btrfs fi sync" before "btrfs quota enable".
> 
> I think the commit itself makes the code much easier to read, so it may
> be better to fix the problem in progs (i.e. calling sync before quota enable).
> 
> Do you have any thoughts?
> 
> Thanks,
> Tomohiro Misono
> 
> 
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

On 2018/06/27 17:10, Qu Wenruo wrote:
> 
> 
> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>> Hello Nikolay,
>>
>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>> to fail correctly rescanning quota when quota is enabled.
>>
>> Simple reproducer:
>>
>> $ mkfs.btrfs -f $DEV
>> $ mount $DEV /mnt
>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>> $ btrfs quota enbale /mnt
>> $ umount /mnt
>> $ btrfs check $DEV
>> ...
>> checking quota groups
>> Counts for qgroup id: 0/5 are different
>> our:            referenced 1019904 referenced compressed 1019904
>> disk:           referenced 16384 referenced compressed 16384
>> diff:           referenced 1003520 referenced compressed 1003520
>> our:            exclusive 1019904 exclusive compressed 1019904
>> disk:           exclusive 16384 exclusive compressed 16384
>> diff:           exclusive 1003520 exclusive compressed 1003520
>> found 1413120 bytes used, error(s) found
>> ...
>>
>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>> returns error code 0 even if quota is not consistency and therefore
>> test will incorrectly pass.)
> 
> BTW, would you please try to dump the quota tree for such mismatch case?
> 
> It could be a btrfs-progs bug which it should skip quota checking if it
> found the quota status item has RESCAN flag.

Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):

$ sudo btrfs check -Q /dev/sdh1
Checking filesystem on /dev/sdh1
UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
Print quota groups for /dev/sdh1
UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
Counts for qgroup id: 0/5 are different
our:            referenced 170999808 referenced compressed 170999808
disk:           referenced 16384 referenced compressed 16384
diff:           referenced 170983424 referenced compressed 170983424
our:            exclusive 170999808 exclusive compressed 170999808
disk:           exclusive 16384 exclusive compressed 16384
diff:           exclusive 170983424 exclusive compressed 170983424


$ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
btrfs-progs v4.17
quota tree key (QUOTA_TREE ROOT_ITEM 0)
leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
leaf 213958656 flags 0x1(WRITTEN) backref revision 1
fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
        item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
                version 1 generation 9 flags ON scan 30572545
        item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
                generation 7
                referenced 16384 referenced_compressed 16384
                exclusive 16384 exclusive_compressed 16384
        item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
                flags 0
                max_referenced 0 max_exclusive 0
                rsv_referenced 0 rsv_exclusive 0
total bytes 26843545600
bytes used 171769856
uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb


And if I mount+rescan again:

$ sudo mount /dev/sdh1 /mnt
$ sudo btrfs quota rescan -w /mnt
$ sudo umount /mnt

$ sudo btrfs check -Q /dev/sdh1
Checking filesystem on /dev/sdh1
UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
Print quota groups for /dev/sdh1
UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
Counts for qgroup id: 0/5
our:            referenced 170999808 referenced compressed 170999808
disk:           referenced 170999808 referenced compressed 170999808
our:            exclusive 170999808 exclusive compressed 170999808
disk:           exclusive 170999808 exclusive compressed 170999808

$ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
btrfs-progs v4.17
quota tree key (QUOTA_TREE ROOT_ITEM 0)
leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
leaf 31309824 flags 0x1(WRITTEN) backref revision 1
fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
        item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
                version 1 generation 13 flags ON scan 213827585
        item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
                generation 11
                referenced 170999808 referenced_compressed 170999808
                exclusive 170999808 exclusive_compressed 170999808
        item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
                flags 0
                max_referenced 0 max_exclusive 0
                rsv_referenced 0 rsv_exclusive 0
total bytes 26843545600
bytes used 171769856
uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb

> 
> Thanks,
> Qu> 
>>
>> My observation is that this commit changed to call initial quota rescan
>> when quota is enabeld instead of first comit transaction after enabling
>> quota, and therefore if there is something not commited at that time,
>> their usage will not be accounted.
>>
>> Actually this can be simply fixed by calling "btrfs rescan" again or
>> calling "btrfs fi sync" before "btrfs quota enable".
>>
>> I think the commit itself makes the code much easier to read, so it may
>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>
>> Do you have any thoughts?
>>
>> Thanks,
>> Tomohiro Misono
>>
>>
>> --
>> 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: Enabling quota may not correctly rescan on 4.17

[Qu Wenruo]

On 2018年06月27日 16:25, Misono Tomohiro wrote:
> On 2018/06/27 17:10, Qu Wenruo wrote:
>>
>>
>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>> Hello Nikolay,
>>>
>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>> to fail correctly rescanning quota when quota is enabled.
>>>
>>> Simple reproducer:
>>>
>>> $ mkfs.btrfs -f $DEV
>>> $ mount $DEV /mnt
>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>> $ btrfs quota enbale /mnt
>>> $ umount /mnt
>>> $ btrfs check $DEV
>>> ...
>>> checking quota groups
>>> Counts for qgroup id: 0/5 are different
>>> our:            referenced 1019904 referenced compressed 1019904
>>> disk:           referenced 16384 referenced compressed 16384
>>> diff:           referenced 1003520 referenced compressed 1003520
>>> our:            exclusive 1019904 exclusive compressed 1019904
>>> disk:           exclusive 16384 exclusive compressed 16384
>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>> found 1413120 bytes used, error(s) found
>>> ...
>>>
>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>> returns error code 0 even if quota is not consistency and therefore
>>> test will incorrectly pass.)
>>
>> BTW, would you please try to dump the quota tree for such mismatch case?
>>
>> It could be a btrfs-progs bug which it should skip quota checking if it
>> found the quota status item has RESCAN flag.
> 
> Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):
> 
> $ sudo btrfs check -Q /dev/sdh1
> Checking filesystem on /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Print quota groups for /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Counts for qgroup id: 0/5 are different
> our:            referenced 170999808 referenced compressed 170999808
> disk:           referenced 16384 referenced compressed 16384
> diff:           referenced 170983424 referenced compressed 170983424
> our:            exclusive 170999808 exclusive compressed 170999808
> disk:           exclusive 16384 exclusive compressed 16384
> diff:           exclusive 170983424 exclusive compressed 170983424
> 
> 
> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
> btrfs-progs v4.17
> quota tree key (QUOTA_TREE ROOT_ITEM 0)
> leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
> leaf 213958656 flags 0x1(WRITTEN) backref revision 1
> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>                 version 1 generation 9 flags ON scan 30572545

Scan is not -1 and flags is only ON, without RESCAN.

>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>                 generation 7
>                 referenced 16384 referenced_compressed 16384
>                 exclusive 16384 exclusive_compressed 16384
>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>                 flags 0
>                 max_referenced 0 max_exclusive 0
>                 rsv_referenced 0 rsv_exclusive 0
> total bytes 26843545600
> bytes used 171769856
> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> 
> 
> And if I mount+rescan again:
> 
> $ sudo mount /dev/sdh1 /mnt
> $ sudo btrfs quota rescan -w /mnt
> $ sudo umount /mnt
> 
> $ sudo btrfs check -Q /dev/sdh1
> Checking filesystem on /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Print quota groups for /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Counts for qgroup id: 0/5
> our:            referenced 170999808 referenced compressed 170999808
> disk:           referenced 170999808 referenced compressed 170999808
> our:            exclusive 170999808 exclusive compressed 170999808
> disk:           exclusive 170999808 exclusive compressed 170999808
> 
> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
> btrfs-progs v4.17
> quota tree key (QUOTA_TREE ROOT_ITEM 0)
> leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
> leaf 31309824 flags 0x1(WRITTEN) backref revision 1
> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>                 version 1 generation 13 flags ON scan 213827585

Still doesn't look good.

In v4.17.2 (sorry, just checking the behavior on my host), after correct
rescan + sync, if we don't have RESCAN flag, we should have scan set to -1.

While in in v4.18-rc1, it doesn't reset the scan progress to -1 after
finished.
And just as explained in previous reply, if later dirty extents are
after scan progress, it won't be accounted.
So this explains everything.

We just need to find why scan progress is not set correctly after rescan
is finished.

Thanks,
Qu

>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>                 generation 11
>                 referenced 170999808 referenced_compressed 170999808
>                 exclusive 170999808 exclusive_compressed 170999808
>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>                 flags 0
>                 max_referenced 0 max_exclusive 0
>                 rsv_referenced 0 rsv_exclusive 0
> total bytes 26843545600
> bytes used 171769856
> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> 
>>
>> Thanks,
>> Qu> 
>>>
>>> My observation is that this commit changed to call initial quota rescan
>>> when quota is enabeld instead of first comit transaction after enabling
>>> quota, and therefore if there is something not commited at that time,
>>> their usage will not be accounted.
>>>
>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>
>>> I think the commit itself makes the code much easier to read, so it may
>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>
>>> Do you have any thoughts?
>>>
>>> Thanks,
>>> Tomohiro Misono
>>>
>>>
>>> --
>>> 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: Enabling quota may not correctly rescan on 4.17

[Qu Wenruo]

On 2018年06月27日 16:34, Qu Wenruo wrote:
> 
> 
> On 2018年06月27日 16:25, Misono Tomohiro wrote:
>> On 2018/06/27 17:10, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>>> Hello Nikolay,
>>>>
>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>> to fail correctly rescanning quota when quota is enabled.
>>>>
>>>> Simple reproducer:
>>>>
>>>> $ mkfs.btrfs -f $DEV
>>>> $ mount $DEV /mnt
>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>> $ btrfs quota enbale /mnt
>>>> $ umount /mnt
>>>> $ btrfs check $DEV
>>>> ...
>>>> checking quota groups
>>>> Counts for qgroup id: 0/5 are different
>>>> our:            referenced 1019904 referenced compressed 1019904
>>>> disk:           referenced 16384 referenced compressed 16384
>>>> diff:           referenced 1003520 referenced compressed 1003520
>>>> our:            exclusive 1019904 exclusive compressed 1019904
>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>>> found 1413120 bytes used, error(s) found
>>>> ...
>>>>
>>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>>> returns error code 0 even if quota is not consistency and therefore
>>>> test will incorrectly pass.)
>>>
>>> BTW, would you please try to dump the quota tree for such mismatch case?
>>>
>>> It could be a btrfs-progs bug which it should skip quota checking if it
>>> found the quota status item has RESCAN flag.
>>
>> Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):
>>
>> $ sudo btrfs check -Q /dev/sdh1
>> Checking filesystem on /dev/sdh1
>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>> Print quota groups for /dev/sdh1
>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>> Counts for qgroup id: 0/5 are different
>> our:            referenced 170999808 referenced compressed 170999808
>> disk:           referenced 16384 referenced compressed 16384
>> diff:           referenced 170983424 referenced compressed 170983424
>> our:            exclusive 170999808 exclusive compressed 170999808
>> disk:           exclusive 16384 exclusive compressed 16384
>> diff:           exclusive 170983424 exclusive compressed 170983424
>>
>>
>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>> btrfs-progs v4.17
>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>> leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
>> leaf 213958656 flags 0x1(WRITTEN) backref revision 1
>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>                 version 1 generation 9 flags ON scan 30572545
> 
> Scan is not -1 and flags is only ON, without RESCAN.
> 
>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>                 generation 7
>>                 referenced 16384 referenced_compressed 16384
>>                 exclusive 16384 exclusive_compressed 16384
>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>                 flags 0
>>                 max_referenced 0 max_exclusive 0
>>                 rsv_referenced 0 rsv_exclusive 0
>> total bytes 26843545600
>> bytes used 171769856
>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>
>>
>> And if I mount+rescan again:
>>
>> $ sudo mount /dev/sdh1 /mnt
>> $ sudo btrfs quota rescan -w /mnt
>> $ sudo umount /mnt
>>
>> $ sudo btrfs check -Q /dev/sdh1
>> Checking filesystem on /dev/sdh1
>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>> Print quota groups for /dev/sdh1
>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>> Counts for qgroup id: 0/5
>> our:            referenced 170999808 referenced compressed 170999808
>> disk:           referenced 170999808 referenced compressed 170999808
>> our:            exclusive 170999808 exclusive compressed 170999808
>> disk:           exclusive 170999808 exclusive compressed 170999808
>>
>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>> btrfs-progs v4.17
>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>> leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
>> leaf 31309824 flags 0x1(WRITTEN) backref revision 1
>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>                 version 1 generation 13 flags ON scan 213827585
> 
> Still doesn't look good.
> 
> In v4.17.2 (sorry, just checking the behavior on my host), after correct
> rescan + sync, if we don't have RESCAN flag, we should have scan set to -1.
> 
> While in in v4.18-rc1, it doesn't reset the scan progress to -1 after
> finished.
> And just as explained in previous reply, if later dirty extents are
> after scan progress, it won't be accounted.
> So this explains everything.
> 
> We just need to find why scan progress is not set correctly after rescan
> is finished.

OK, in fact this is my fault, not Nikolay's.
My bad. Sorry, Nikolay.

It's caused by my commit, ff3d27a048d9 ("btrfs: qgroup: Finish rescan
when hit the last leaf of extent tree").

Where I added another exit path for qgroup_rescan_leaf(), and in that
case it doesn't set the progress.
I'll send out the fix soon.

Thanks,
Qu

> 
> Thanks,
> Qu
> 
>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>                 generation 11
>>                 referenced 170999808 referenced_compressed 170999808
>>                 exclusive 170999808 exclusive_compressed 170999808
>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>                 flags 0
>>                 max_referenced 0 max_exclusive 0
>>                 rsv_referenced 0 rsv_exclusive 0
>> total bytes 26843545600
>> bytes used 171769856
>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>
>>>
>>> Thanks,
>>> Qu> 
>>>>
>>>> My observation is that this commit changed to call initial quota rescan
>>>> when quota is enabeld instead of first comit transaction after enabling
>>>> quota, and therefore if there is something not commited at that time,
>>>> their usage will not be accounted.
>>>>
>>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>>
>>>> I think the commit itself makes the code much easier to read, so it may
>>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>>
>>>> Do you have any thoughts?
>>>>
>>>> Thanks,
>>>> Tomohiro Misono
>>>>
>>>>
>>>> --
>>>> 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
>>>>
>>>
>>
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Nikolay Borisov]

On 27.06.2018 11:38, Qu Wenruo wrote:
> 
> 
> On 2018年06月27日 16:34, Qu Wenruo wrote:
>>
>>
>> On 2018年06月27日 16:25, Misono Tomohiro wrote:
>>> On 2018/06/27 17:10, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>>>> Hello Nikolay,
>>>>>
>>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>>> to fail correctly rescanning quota when quota is enabled.
>>>>>
>>>>> Simple reproducer:
>>>>>
>>>>> $ mkfs.btrfs -f $DEV
>>>>> $ mount $DEV /mnt
>>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>>> $ btrfs quota enbale /mnt
>>>>> $ umount /mnt
>>>>> $ btrfs check $DEV
>>>>> ...
>>>>> checking quota groups
>>>>> Counts for qgroup id: 0/5 are different
>>>>> our:            referenced 1019904 referenced compressed 1019904
>>>>> disk:           referenced 16384 referenced compressed 16384
>>>>> diff:           referenced 1003520 referenced compressed 1003520
>>>>> our:            exclusive 1019904 exclusive compressed 1019904
>>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>>>> found 1413120 bytes used, error(s) found
>>>>> ...
>>>>>
>>>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>>>> returns error code 0 even if quota is not consistency and therefore
>>>>> test will incorrectly pass.)
>>>>
>>>> BTW, would you please try to dump the quota tree for such mismatch case?
>>>>
>>>> It could be a btrfs-progs bug which it should skip quota checking if it
>>>> found the quota status item has RESCAN flag.
>>>
>>> Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):
>>>
>>> $ sudo btrfs check -Q /dev/sdh1
>>> Checking filesystem on /dev/sdh1
>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>> Print quota groups for /dev/sdh1
>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>> Counts for qgroup id: 0/5 are different
>>> our:            referenced 170999808 referenced compressed 170999808
>>> disk:           referenced 16384 referenced compressed 16384
>>> diff:           referenced 170983424 referenced compressed 170983424
>>> our:            exclusive 170999808 exclusive compressed 170999808
>>> disk:           exclusive 16384 exclusive compressed 16384
>>> diff:           exclusive 170983424 exclusive compressed 170983424
>>>
>>>
>>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>>> btrfs-progs v4.17
>>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>>> leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
>>> leaf 213958656 flags 0x1(WRITTEN) backref revision 1
>>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>>                 version 1 generation 9 flags ON scan 30572545
>>
>> Scan is not -1 and flags is only ON, without RESCAN.
>>
>>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>>                 generation 7
>>>                 referenced 16384 referenced_compressed 16384
>>>                 exclusive 16384 exclusive_compressed 16384
>>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>>                 flags 0
>>>                 max_referenced 0 max_exclusive 0
>>>                 rsv_referenced 0 rsv_exclusive 0
>>> total bytes 26843545600
>>> bytes used 171769856
>>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>
>>>
>>> And if I mount+rescan again:
>>>
>>> $ sudo mount /dev/sdh1 /mnt
>>> $ sudo btrfs quota rescan -w /mnt
>>> $ sudo umount /mnt
>>>
>>> $ sudo btrfs check -Q /dev/sdh1
>>> Checking filesystem on /dev/sdh1
>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>> Print quota groups for /dev/sdh1
>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>> Counts for qgroup id: 0/5
>>> our:            referenced 170999808 referenced compressed 170999808
>>> disk:           referenced 170999808 referenced compressed 170999808
>>> our:            exclusive 170999808 exclusive compressed 170999808
>>> disk:           exclusive 170999808 exclusive compressed 170999808
>>>
>>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>>> btrfs-progs v4.17
>>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>>> leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
>>> leaf 31309824 flags 0x1(WRITTEN) backref revision 1
>>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>>                 version 1 generation 13 flags ON scan 213827585
>>
>> Still doesn't look good.
>>
>> In v4.17.2 (sorry, just checking the behavior on my host), after correct
>> rescan + sync, if we don't have RESCAN flag, we should have scan set to -1.
>>
>> While in in v4.18-rc1, it doesn't reset the scan progress to -1 after
>> finished.
>> And just as explained in previous reply, if later dirty extents are
>> after scan progress, it won't be accounted.
>> So this explains everything.
>>
>> We just need to find why scan progress is not set correctly after rescan
>> is finished.
> 
> OK, in fact this is my fault, not Nikolay's.
> My bad. Sorry, Nikolay.
> 
> It's caused by my commit, ff3d27a048d9 ("btrfs: qgroup: Finish rescan
> when hit the last leaf of extent tree").
> 
> Where I added another exit path for qgroup_rescan_leaf(), and in that
> case it doesn't set the progress.
> I'll send out the fix soon.


I'm confused why I'm not hitting this on David's misc-next branch.
Also your commit landed on 4.18, meaning Misono shouldn't have observed
the issue on 4.17. Misono, did you reproduce the issue on 4.17 if not
you might want to revise your bisection process since if my patch was to
blame it should have failed on 4.17 whereas Qu's patch landed in 4.18 so
it should have been fairly obvious if this was introduced in 4.17 or 4.18.

> 
> Thanks,
> Qu
> 
>>
>> Thanks,
>> Qu
>>
>>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>>                 generation 11
>>>                 referenced 170999808 referenced_compressed 170999808
>>>                 exclusive 170999808 exclusive_compressed 170999808
>>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>>                 flags 0
>>>                 max_referenced 0 max_exclusive 0
>>>                 rsv_referenced 0 rsv_exclusive 0
>>> total bytes 26843545600
>>> bytes used 171769856
>>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>
>>>>
>>>> Thanks,
>>>> Qu> 
>>>>>
>>>>> My observation is that this commit changed to call initial quota rescan
>>>>> when quota is enabeld instead of first comit transaction after enabling
>>>>> quota, and therefore if there is something not commited at that time,
>>>>> their usage will not be accounted.
>>>>>
>>>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>>>
>>>>> I think the commit itself makes the code much easier to read, so it may
>>>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>>>
>>>>> Do you have any thoughts?
>>>>>
>>>>> Thanks,
>>>>> Tomohiro Misono
>>>>>
>>>>>
>>>>> --
>>>>> 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
>>>>>
>>>>
>>>
>> --
>> 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
>>
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Qu Wenruo]

On 2018年06月27日 16:47, Nikolay Borisov wrote:
> 
> 
> On 27.06.2018 11:38, Qu Wenruo wrote:
>>
>>
>> On 2018年06月27日 16:34, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年06月27日 16:25, Misono Tomohiro wrote:
>>>> On 2018/06/27 17:10, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>>>>> Hello Nikolay,
>>>>>>
>>>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>>>> to fail correctly rescanning quota when quota is enabled.
>>>>>>
>>>>>> Simple reproducer:
>>>>>>
>>>>>> $ mkfs.btrfs -f $DEV
>>>>>> $ mount $DEV /mnt
>>>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>>>> $ btrfs quota enbale /mnt
>>>>>> $ umount /mnt
>>>>>> $ btrfs check $DEV
>>>>>> ...
>>>>>> checking quota groups
>>>>>> Counts for qgroup id: 0/5 are different
>>>>>> our:            referenced 1019904 referenced compressed 1019904
>>>>>> disk:           referenced 16384 referenced compressed 16384
>>>>>> diff:           referenced 1003520 referenced compressed 1003520
>>>>>> our:            exclusive 1019904 exclusive compressed 1019904
>>>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>>>>> found 1413120 bytes used, error(s) found
>>>>>> ...
>>>>>>
>>>>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>>>>> returns error code 0 even if quota is not consistency and therefore
>>>>>> test will incorrectly pass.)
>>>>>
>>>>> BTW, would you please try to dump the quota tree for such mismatch case?
>>>>>
>>>>> It could be a btrfs-progs bug which it should skip quota checking if it
>>>>> found the quota status item has RESCAN flag.
>>>>
>>>> Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):
>>>>
>>>> $ sudo btrfs check -Q /dev/sdh1
>>>> Checking filesystem on /dev/sdh1
>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>> Print quota groups for /dev/sdh1
>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>> Counts for qgroup id: 0/5 are different
>>>> our:            referenced 170999808 referenced compressed 170999808
>>>> disk:           referenced 16384 referenced compressed 16384
>>>> diff:           referenced 170983424 referenced compressed 170983424
>>>> our:            exclusive 170999808 exclusive compressed 170999808
>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>> diff:           exclusive 170983424 exclusive compressed 170983424
>>>>
>>>>
>>>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>>>> btrfs-progs v4.17
>>>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>>>> leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
>>>> leaf 213958656 flags 0x1(WRITTEN) backref revision 1
>>>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>>>                 version 1 generation 9 flags ON scan 30572545
>>>
>>> Scan is not -1 and flags is only ON, without RESCAN.
>>>
>>>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>>>                 generation 7
>>>>                 referenced 16384 referenced_compressed 16384
>>>>                 exclusive 16384 exclusive_compressed 16384
>>>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>>>                 flags 0
>>>>                 max_referenced 0 max_exclusive 0
>>>>                 rsv_referenced 0 rsv_exclusive 0
>>>> total bytes 26843545600
>>>> bytes used 171769856
>>>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>
>>>>
>>>> And if I mount+rescan again:
>>>>
>>>> $ sudo mount /dev/sdh1 /mnt
>>>> $ sudo btrfs quota rescan -w /mnt
>>>> $ sudo umount /mnt
>>>>
>>>> $ sudo btrfs check -Q /dev/sdh1
>>>> Checking filesystem on /dev/sdh1
>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>> Print quota groups for /dev/sdh1
>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>> Counts for qgroup id: 0/5
>>>> our:            referenced 170999808 referenced compressed 170999808
>>>> disk:           referenced 170999808 referenced compressed 170999808
>>>> our:            exclusive 170999808 exclusive compressed 170999808
>>>> disk:           exclusive 170999808 exclusive compressed 170999808
>>>>
>>>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>>>> btrfs-progs v4.17
>>>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>>>> leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
>>>> leaf 31309824 flags 0x1(WRITTEN) backref revision 1
>>>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>>>                 version 1 generation 13 flags ON scan 213827585
>>>
>>> Still doesn't look good.
>>>
>>> In v4.17.2 (sorry, just checking the behavior on my host), after correct
>>> rescan + sync, if we don't have RESCAN flag, we should have scan set to -1.
>>>
>>> While in in v4.18-rc1, it doesn't reset the scan progress to -1 after
>>> finished.
>>> And just as explained in previous reply, if later dirty extents are
>>> after scan progress, it won't be accounted.
>>> So this explains everything.
>>>
>>> We just need to find why scan progress is not set correctly after rescan
>>> is finished.
>>
>> OK, in fact this is my fault, not Nikolay's.
>> My bad. Sorry, Nikolay.
>>
>> It's caused by my commit, ff3d27a048d9 ("btrfs: qgroup: Finish rescan
>> when hit the last leaf of extent tree").
>>
>> Where I added another exit path for qgroup_rescan_leaf(), and in that
>> case it doesn't set the progress.
>> I'll send out the fix soon.
> 
> 
> I'm confused why I'm not hitting this on David's misc-next branch.

I think btrfs/114 may not be the best case to hit it.
The best way to hit the bug should be some modified version of
btrfs/017, extra write after rescan should trigger it.
(My new test case will go that way)

> Also your commit landed on 4.18, meaning Misono shouldn't have observed
> the issue on 4.17.

Yep, but the difference behavior of quota status item looks pretty
convincing (along with the code), and no possibility is involved, so I
prefer to believe my observation.

Thanks,
Qu

> Misono, did you reproduce the issue on 4.17 if not
> you might want to revise your bisection process since if my patch was to
> blame it should have failed on 4.17 whereas Qu's patch landed in 4.18 so
> it should have been fairly obvious if this was introduced in 4.17 or 4.18.
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>>>                 generation 11
>>>>                 referenced 170999808 referenced_compressed 170999808
>>>>                 exclusive 170999808 exclusive_compressed 170999808
>>>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>>>                 flags 0
>>>>                 max_referenced 0 max_exclusive 0
>>>>                 rsv_referenced 0 rsv_exclusive 0
>>>> total bytes 26843545600
>>>> bytes used 171769856
>>>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>
>>>>>
>>>>> Thanks,
>>>>> Qu> 
>>>>>>
>>>>>> My observation is that this commit changed to call initial quota rescan
>>>>>> when quota is enabeld instead of first comit transaction after enabling
>>>>>> quota, and therefore if there is something not commited at that time,
>>>>>> their usage will not be accounted.
>>>>>>
>>>>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>>>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>>>>
>>>>>> I think the commit itself makes the code much easier to read, so it may
>>>>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>>>>
>>>>>> Do you have any thoughts?
>>>>>>
>>>>>> Thanks,
>>>>>> Tomohiro Misono
>>>>>>
>>>>>>
>>>>>> --
>>>>>> 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
>>>>>>
>>>>>
>>>>
>>> --
>>> 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
>>>
>> --
>> 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
>>
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Qu Wenruo]

On 2018年06月27日 16:57, Qu Wenruo wrote:
> 
> 
> On 2018年06月27日 16:47, Nikolay Borisov wrote:
>>
>>
>> On 27.06.2018 11:38, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年06月27日 16:34, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2018年06月27日 16:25, Misono Tomohiro wrote:
>>>>> On 2018/06/27 17:10, Qu Wenruo wrote:
>>>>>>
>>>>>>
>>>>>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>>>>>> Hello Nikolay,
>>>>>>>
>>>>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>>>>> to fail correctly rescanning quota when quota is enabled.
>>>>>>>
>>>>>>> Simple reproducer:
>>>>>>>
>>>>>>> $ mkfs.btrfs -f $DEV
>>>>>>> $ mount $DEV /mnt
>>>>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>>>>> $ btrfs quota enbale /mnt
>>>>>>> $ umount /mnt
>>>>>>> $ btrfs check $DEV
>>>>>>> ...
>>>>>>> checking quota groups
>>>>>>> Counts for qgroup id: 0/5 are different
>>>>>>> our:            referenced 1019904 referenced compressed 1019904
>>>>>>> disk:           referenced 16384 referenced compressed 16384
>>>>>>> diff:           referenced 1003520 referenced compressed 1003520
>>>>>>> our:            exclusive 1019904 exclusive compressed 1019904
>>>>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>>>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>>>>>> found 1413120 bytes used, error(s) found
>>>>>>> ...
>>>>>>>
>>>>>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>>>>>> returns error code 0 even if quota is not consistency and therefore
>>>>>>> test will incorrectly pass.)
>>>>>>
>>>>>> BTW, would you please try to dump the quota tree for such mismatch case?
>>>>>>
>>>>>> It could be a btrfs-progs bug which it should skip quota checking if it
>>>>>> found the quota status item has RESCAN flag.
>>>>>
>>>>> Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):
>>>>>
>>>>> $ sudo btrfs check -Q /dev/sdh1
>>>>> Checking filesystem on /dev/sdh1
>>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>> Print quota groups for /dev/sdh1
>>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>> Counts for qgroup id: 0/5 are different
>>>>> our:            referenced 170999808 referenced compressed 170999808
>>>>> disk:           referenced 16384 referenced compressed 16384
>>>>> diff:           referenced 170983424 referenced compressed 170983424
>>>>> our:            exclusive 170999808 exclusive compressed 170999808
>>>>> disk:           exclusive 16384 exclusive compressed 16384
>>>>> diff:           exclusive 170983424 exclusive compressed 170983424
>>>>>
>>>>>
>>>>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>>>>> btrfs-progs v4.17
>>>>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>>>>> leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
>>>>> leaf 213958656 flags 0x1(WRITTEN) backref revision 1
>>>>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>>>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>>>>                 version 1 generation 9 flags ON scan 30572545
>>>>
>>>> Scan is not -1 and flags is only ON, without RESCAN.
>>>>
>>>>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>>>>                 generation 7
>>>>>                 referenced 16384 referenced_compressed 16384
>>>>>                 exclusive 16384 exclusive_compressed 16384
>>>>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>>>>                 flags 0
>>>>>                 max_referenced 0 max_exclusive 0
>>>>>                 rsv_referenced 0 rsv_exclusive 0
>>>>> total bytes 26843545600
>>>>> bytes used 171769856
>>>>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>>
>>>>>
>>>>> And if I mount+rescan again:
>>>>>
>>>>> $ sudo mount /dev/sdh1 /mnt
>>>>> $ sudo btrfs quota rescan -w /mnt
>>>>> $ sudo umount /mnt
>>>>>
>>>>> $ sudo btrfs check -Q /dev/sdh1
>>>>> Checking filesystem on /dev/sdh1
>>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>> Print quota groups for /dev/sdh1
>>>>> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>> Counts for qgroup id: 0/5
>>>>> our:            referenced 170999808 referenced compressed 170999808
>>>>> disk:           referenced 170999808 referenced compressed 170999808
>>>>> our:            exclusive 170999808 exclusive compressed 170999808
>>>>> disk:           exclusive 170999808 exclusive compressed 170999808
>>>>>
>>>>> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
>>>>> btrfs-progs v4.17
>>>>> quota tree key (QUOTA_TREE ROOT_ITEM 0)
>>>>> leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
>>>>> leaf 31309824 flags 0x1(WRITTEN) backref revision 1
>>>>> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>>>>>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>>>>>                 version 1 generation 13 flags ON scan 213827585
>>>>
>>>> Still doesn't look good.
>>>>
>>>> In v4.17.2 (sorry, just checking the behavior on my host), after correct
>>>> rescan + sync, if we don't have RESCAN flag, we should have scan set to -1.
>>>>
>>>> While in in v4.18-rc1, it doesn't reset the scan progress to -1 after
>>>> finished.
>>>> And just as explained in previous reply, if later dirty extents are
>>>> after scan progress, it won't be accounted.
>>>> So this explains everything.
>>>>
>>>> We just need to find why scan progress is not set correctly after rescan
>>>> is finished.
>>>
>>> OK, in fact this is my fault, not Nikolay's.
>>> My bad. Sorry, Nikolay.
>>>
>>> It's caused by my commit, ff3d27a048d9 ("btrfs: qgroup: Finish rescan
>>> when hit the last leaf of extent tree").
>>>
>>> Where I added another exit path for qgroup_rescan_leaf(), and in that
>>> case it doesn't set the progress.
>>> I'll send out the fix soon.
>>
>>
>> I'm confused why I'm not hitting this on David's misc-next branch.
> 
> I think btrfs/114 may not be the best case to hit it.
> The best way to hit the bug should be some modified version of
> btrfs/017, extra write after rescan should trigger it.
> (My new test case will go that way)

It looks like that I'm not in correct condition yet.
The skip of quota accounting only happens if we are still under rescan.

So although the behavior changes, it shouldn't cause the bug directly.
It may enlarge the race window, but still not the direct cause.

Anyway, the fix still makes sense, at least it makes the debug tree
result nicer.

> 
>> Also your commit landed on 4.18, meaning Misono shouldn't have observed
>> the issue on 4.17.
> 
> Yep, but the difference behavior of quota status item looks pretty
> convincing (along with the code), and no possibility is involved, so I
> prefer to believe my observation.

(Well, my previous words are just bullsh*t, as it only works when rescan
is still happening, when rescan flag is gone, it won't cause any real
damage)

Thanks,
Qu

> 
> Thanks,
> Qu
> 
>> Misono, did you reproduce the issue on 4.17 if not
>> you might want to revise your bisection process since if my patch was to
>> blame it should have failed on 4.17 whereas Qu's patch landed in 4.18 so
>> it should have been fairly obvious if this was introduced in 4.17 or 4.18.
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>>>>>                 generation 11
>>>>>                 referenced 170999808 referenced_compressed 170999808
>>>>>                 exclusive 170999808 exclusive_compressed 170999808
>>>>>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>>>>>                 flags 0
>>>>>                 max_referenced 0 max_exclusive 0
>>>>>                 rsv_referenced 0 rsv_exclusive 0
>>>>> total bytes 26843545600
>>>>> bytes used 171769856
>>>>> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Qu> 
>>>>>>>
>>>>>>> My observation is that this commit changed to call initial quota rescan
>>>>>>> when quota is enabeld instead of first comit transaction after enabling
>>>>>>> quota, and therefore if there is something not commited at that time,
>>>>>>> their usage will not be accounted.
>>>>>>>
>>>>>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>>>>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>>>>>
>>>>>>> I think the commit itself makes the code much easier to read, so it may
>>>>>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>>>>>
>>>>>>> Do you have any thoughts?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Tomohiro Misono
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> 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
>>>>>>>
>>>>>>
>>>>>
>>>> --
>>>> 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
>>>>
>>> --
>>> 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
>>>
>> --
>> 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: Enabling quota may not correctly rescan on 4.17

[Qu Wenruo]

On 2018年06月27日 16:25, Misono Tomohiro wrote:
> On 2018/06/27 17:10, Qu Wenruo wrote:
>>
>>
>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>> Hello Nikolay,
>>>
>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>> to fail correctly rescanning quota when quota is enabled.
>>>
>>> Simple reproducer:
>>>
>>> $ mkfs.btrfs -f $DEV
>>> $ mount $DEV /mnt
>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>> $ btrfs quota enbale /mnt
>>> $ umount /mnt
>>> $ btrfs check $DEV
>>> ...
>>> checking quota groups
>>> Counts for qgroup id: 0/5 are different
>>> our:            referenced 1019904 referenced compressed 1019904
>>> disk:           referenced 16384 referenced compressed 16384
>>> diff:           referenced 1003520 referenced compressed 1003520
>>> our:            exclusive 1019904 exclusive compressed 1019904
>>> disk:           exclusive 16384 exclusive compressed 16384
>>> diff:           exclusive 1003520 exclusive compressed 1003520
>>> found 1413120 bytes used, error(s) found
>>> ...
>>>
>>> This can be also observed in btrfs/114. \v(Note that progs < 4.17
>>> returns error code 0 even if quota is not consistency and therefore
>>> test will incorrectly pass.)
>>
>> BTW, would you please try to dump the quota tree for such mismatch case?
>>
>> It could be a btrfs-progs bug which it should skip quota checking if it
>> found the quota status item has RESCAN flag.
> 
> Yes, this is what I see after running btrfs/114 (/dev/sdh1 is scratch dev):
> 
> $ sudo btrfs check -Q /dev/sdh1
> Checking filesystem on /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Print quota groups for /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Counts for qgroup id: 0/5 are different
> our:            referenced 170999808 referenced compressed 170999808
> disk:           referenced 16384 referenced compressed 16384
> diff:           referenced 170983424 referenced compressed 170983424
> our:            exclusive 170999808 exclusive compressed 170999808
> disk:           exclusive 16384 exclusive compressed 16384
> diff:           exclusive 170983424 exclusive compressed 170983424
> 

Unfortunately in my environment, btrfs/114 failed to reproduce it with
1024 runs overnight, with v4.18-rc1 kernel.

Would you please provide the whole btrfs-image dump of the corrupted fs?

There are several different assumptions on how the bug happens, with
your btrfs-image dump, it would help a lot to rule out some assumption.

Thanks,
Qu

> 
> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
> btrfs-progs v4.17
> quota tree key (QUOTA_TREE ROOT_ITEM 0)
> leaf 213958656 items 3 free space 16096 generation 9 owner QUOTA_TREE
> leaf 213958656 flags 0x1(WRITTEN) backref revision 1
> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>                 version 1 generation 9 flags ON scan 30572545
>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>                 generation 7
>                 referenced 16384 referenced_compressed 16384
>                 exclusive 16384 exclusive_compressed 16384
>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>                 flags 0
>                 max_referenced 0 max_exclusive 0
>                 rsv_referenced 0 rsv_exclusive 0
> total bytes 26843545600
> bytes used 171769856
> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> 
> 
> And if I mount+rescan again:
> 
> $ sudo mount /dev/sdh1 /mnt
> $ sudo btrfs quota rescan -w /mnt
> $ sudo umount /mnt
> 
> $ sudo btrfs check -Q /dev/sdh1
> Checking filesystem on /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Print quota groups for /dev/sdh1
> UUID: d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> Counts for qgroup id: 0/5
> our:            referenced 170999808 referenced compressed 170999808
> disk:           referenced 170999808 referenced compressed 170999808
> our:            exclusive 170999808 exclusive compressed 170999808
> disk:           exclusive 170999808 exclusive compressed 170999808
> 
> $ sudo btrfs inspect-internal dump-tree -t quota /dev/sdh1
> btrfs-progs v4.17
> quota tree key (QUOTA_TREE ROOT_ITEM 0)
> leaf 31309824 items 3 free space 16096 generation 13 owner QUOTA_TREE
> leaf 31309824 flags 0x1(WRITTEN) backref revision 1
> fs uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> chunk uuid 78d753d0-eeb7-4c3e-b825-b6c2c5de5c7a
>         item 0 key (0 QGROUP_STATUS 0) itemoff 16251 itemsize 32
>                 version 1 generation 13 flags ON scan 213827585
>         item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
>                 generation 11
>                 referenced 170999808 referenced_compressed 170999808
>                 exclusive 170999808 exclusive_compressed 170999808
>         item 2 key (0 QGROUP_LIMIT 0/5) itemoff 16171 itemsize 40
>                 flags 0
>                 max_referenced 0 max_exclusive 0
>                 rsv_referenced 0 rsv_exclusive 0
> total bytes 26843545600
> bytes used 171769856
> uuid d07f6028-0ae7-40d4-ac45-01a4505ddcfb
> 
>>
>> Thanks,
>> Qu> 
>>>
>>> My observation is that this commit changed to call initial quota rescan
>>> when quota is enabeld instead of first comit transaction after enabling
>>> quota, and therefore if there is something not commited at that time,
>>> their usage will not be accounted.
>>>
>>> Actually this can be simply fixed by calling "btrfs rescan" again or
>>> calling "btrfs fi sync" before "btrfs quota enable".
>>>
>>> I think the commit itself makes the code much easier to read, so it may
>>> be better to fix the problem in progs (i.e. calling sync before quota enable).
>>>
>>> Do you have any thoughts?
>>>
>>> Thanks,
>>> Tomohiro Misono
>>>
>>>
>>> --
>>> 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
>>>
>>
> 
> --
> 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: Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On 2018/06/28 16:12, Qu Wenruo wrote:
> 
> 
> On 2018年06月27日 16:25, Misono Tomohiro wrote:
>> On 2018/06/27 17:10, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>>> Hello Nikolay,
>>>>
>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>> to fail correctly rescanning quota when quota is enabled.
>>>>
>>>> Simple reproducer:
>>>>
>>>> $ mkfs.btrfs -f $DEV
>>>> $ mount $DEV /mnt
>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>> $ btrfs quota enbale /mnt
>>>> $ umount /mnt
>>>> $ btrfs check $DEV
>>>> ...
> Unfortunately in my environment, btrfs/114 failed to reprocduce it with
> 1024 runs overnight, with v4.18-rc1 kernel.
> 
> Would you please provide the whole btrfs-image dump of the corrupted fs?

Yes.
The attached file is an image-dump of above reproducer (kernel 4.17.0, progs 4.17)
as the dump of btrfs/114 is a bit large for mail.

Though this does not always happen, I see the failure both on 4.17.0 or 4.18-rc2.

Thanks,
Tomohiro Misono

> 
> There are several different assumptions on how the bug happens, with
> your btrfs-image dump, it would help a lot to rule out some assumption.
> 
> Thanks,
> Qu

[-- Attachment #2: btrfs-image --]
[-- Type: application/octet-stream, Size: 5120 bytes --]

Re: Enabling quota may not correctly rescan on 4.17

[Nikolay Borisov]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]



On 28.06.2018 11:10, Misono Tomohiro wrote:
> On 2018/06/28 16:12, Qu Wenruo wrote:
>>
>>
>> On 2018年06月27日 16:25, Misono Tomohiro wrote:
>>> On 2018/06/27 17:10, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2018年06月26日 14:00, Misono Tomohiro wrote:
>>>>> Hello Nikolay,
>>>>>
>>>>> I noticed that commit 5d23515be669 ("btrfs: Move qgroup rescan
>>>>> on quota enable to btrfs_quota_enable") in 4.17 sometimes causes
>>>>> to fail correctly rescanning quota when quota is enabled.
>>>>>
>>>>> Simple reproducer:
>>>>>
>>>>> $ mkfs.btrfs -f $DEV
>>>>> $ mount $DEV /mnt
>>>>> $ dd if=/dev/urandom of=/mnt/file bs=1000 count=1000
>>>>> $ btrfs quota enbale /mnt
>>>>> $ umount /mnt
>>>>> $ btrfs check $DEV
>>>>> ...
>> Unfortunately in my environment, btrfs/114 failed to reprocduce it with
>> 1024 runs overnight, with v4.18-rc1 kernel.
>>
>> Would you please provide the whole btrfs-image dump of the corrupted fs?
> 
> Yes.
> The attached file is an image-dump of above reproducer (kernel 4.17.0, progs 4.17)
> as the dump of btrfs/114 is a bit large for mail.
> 
> Though this does not always happen, I see the failure both on 4.17.0 or 4.18-rc2.
> 
> Thanks,
> Tomohiro Misono

Misono,

Can you please try the attached patch?

> 
>>
>> There are several different assumptions on how the bug happens, with
>> your btrfs-image dump, it would help a lot to rule out some assumption.
>>
>> Thanks,
>> Qu

[-- Attachment #2: 0001-btrfs-qgroups-Move-transaction-managed-inside-btrfs_.patch --]
[-- Type: text/x-patch, Size: 6759 bytes --]

>From 10345e21bc2b4e61644da6b76ee4528710b2be25 Mon Sep 17 00:00:00 2001
From: Nikolay Borisov <nborisov@suse.com>
Date: Tue, 26 Jun 2018 10:03:58 +0300
Subject: [PATCH] btrfs: qgroups: Move transaction managed inside
 btrfs_quota_enable/disable

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. For the sake of symmetry this patch 
also moves the transaction logic inside btrfs_quota_disable

Fixes: 5d23515be669 ("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>
---
 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 1874a6d2e6f5..1d84af0d053f 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;
@@ -3061,7 +3085,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: Enabling quota may not correctly rescan on 4.17

[Misono Tomohiro]

> Misono,
> 
> Can you please try the attached patch?
> 

I tried and it works (on 4.18.0-rc3).

Committing transaction before starting rescan worker is
what btrfs_qgroup_resan() does, so it looks fine. 

(though I'm not sure why you don't see the problem in your machine.)

Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
Thanks.

[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

[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

[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,
>