[PATCH 0/3] btrfs: qgroup fixes and a cleanup

[PATCH 0/3] btrfs: qgroup fixes and a cleanup

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

A couple fixes and a cleanup, details in the change logs.

Filipe Manana (3):
  btrfs: qgroup: set quota enabled bit if quota disable fails flushing reservations
  btrfs: qgroup: fix qgroup create ioctl returning success after quotas disabled
  btrfs: qgroup: use btrfs_qgroup_enabled() in ioctls

 fs/btrfs/ioctl.c       | 24 ++++--------------------
 fs/btrfs/qgroup.c      | 10 +++++-----
 fs/btrfs/transaction.c |  6 ++++--
 3 files changed, 13 insertions(+), 27 deletions(-)

-- 
2.47.2

[PATCH 1/3] btrfs: qgroup: set quota enabled bit if quota disable fails flushing reservations

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Before waiting for the rescan worker to finish and flushing reservations,
we clear the BTRFS_FS_QUOTA_ENABLED flag from fs_info. If we fail flushing
reservations we leave with the flag not set which is not correct since
quotas are still enabled - we must set back the flag on error paths, such
as when we fail to start a transaction, except for error paths that abort
a transaction. The reservation flushing happens very early before we do
any operation that actually disables quotas and before we start a
transaction, so set back BTRFS_FS_QUOTA_ENABLED if it fails.

Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 42d3cfb84318..eb1bb57dee7d 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1334,11 +1334,14 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
 
 	/*
 	 * We have nothing held here and no trans handle, just return the error
-	 * if there is one.
+	 * if there is one and set back the quota enabled bit since we didn't
+	 * actually disable quotas.
 	 */
 	ret = flush_reservations(fs_info);
-	if (ret)
+	if (ret) {
+		set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
 		return ret;
+	}
 
 	/*
 	 * 1 For the root item
-- 
2.47.2

Re: [PATCH 1/3] btrfs: qgroup: set quota enabled bit if quota disable fails flushing reservations

[Qu Wenruo]

在 2025/7/2 01:12, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Before waiting for the rescan worker to finish and flushing reservations,
> we clear the BTRFS_FS_QUOTA_ENABLED flag from fs_info. If we fail flushing
> reservations we leave with the flag not set which is not correct since
> quotas are still enabled - we must set back the flag on error paths, such
> as when we fail to start a transaction, except for error paths that abort
> a transaction. The reservation flushing happens very early before we do
> any operation that actually disables quotas and before we start a
> transaction, so set back BTRFS_FS_QUOTA_ENABLED if it fails.
> 
> Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

I initially thought it will be a little racy when setting/clearing the 
ENABLED flag without qgroup_ioctl_mutex, but since we're already holding 
subvol_sem() for both enable and disable, it should be fine.

Thanks,
Qu

> ---
>   fs/btrfs/qgroup.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 42d3cfb84318..eb1bb57dee7d 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1334,11 +1334,14 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>   
>   	/*
>   	 * We have nothing held here and no trans handle, just return the error
> -	 * if there is one.
> +	 * if there is one and set back the quota enabled bit since we didn't
> +	 * actually disable quotas.
>   	 */
>   	ret = flush_reservations(fs_info);
> -	if (ret)
> +	if (ret) {
> +		set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>   		return ret;
> +	}
>   
>   	/*
>   	 * 1 For the root item

[PATCH 2/3] btrfs: qgroup: fix qgroup create ioctl returning success after quotas disabled

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When quotas are disabled qgroup ioctls are supposed to return -ENOTCONN,
but the qgroup create ioctl stopped doing that when it races with a quota
disable operation, returning 0 instead. This change of behaviour happened
in commit 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot
creation").

The issue happens as follows:

1) Task A enters btrfs_ioctl_qgroup_create(), qgroups are enabled and so
   qgroup_enabled() returns true since fs_info->quota_root is not NULL;

2) Task B enters btrfs_ioctl_quota_ctl() -> btrfs_quota_disable() and
   disables qgroups, so now fs_info->quota_root is NULL;

3) Task A enters btrfs_create_qgroup() and calls btrfs_qgroup_mode(),
   which returns BTRFS_QGROUP_MODE_DISABLED since quotas are disabled,
   and then btrfs_create_qgroup() returns 0 to the caller, which makes
   the ioctl return 0 instead of -ENOTCONN.

   The check for fs_info->quota_root and returning -ENOTCONN if it's NULL
   is made only after the call btrfs_qgroup_mode().

Fix this by moving the check for disabled quotas with btrfs_qgroup_mode()
into transaction.c:create_pending_snapshot(), so that we don't abort the
transaction if btrfs_create_qgroup() returns -ENOTCONN and quotas are
disabled.

Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c      | 3 ---
 fs/btrfs/transaction.c | 6 ++++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index eb1bb57dee7d..ae9bc7c71a34 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1662,9 +1662,6 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 	struct btrfs_qgroup *prealloc = NULL;
 	int ret = 0;
 
-	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED)
-		return 0;
-
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
 		ret = -ENOTCONN;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 2e07c90be5cd..c5c0d9cf1a80 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1735,8 +1735,10 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_create_qgroup(trans, objectid);
 	if (ret && ret != -EEXIST) {
-		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		if (ret != -ENOTCONN || btrfs_qgroup_enabled(fs_info)) {
+			btrfs_abort_transaction(trans, ret);
+			goto fail;
+		}
 	}
 
 	/*
-- 
2.47.2

Re: [PATCH 2/3] btrfs: qgroup: fix qgroup create ioctl returning success after quotas disabled

[Qu Wenruo]

在 2025/7/2 01:12, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When quotas are disabled qgroup ioctls are supposed to return -ENOTCONN,
> but the qgroup create ioctl stopped doing that when it races with a quota
> disable operation, returning 0 instead. This change of behaviour happened
> in commit 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot
> creation").
> 
> The issue happens as follows:
> 
> 1) Task A enters btrfs_ioctl_qgroup_create(), qgroups are enabled and so
>     qgroup_enabled() returns true since fs_info->quota_root is not NULL;
> 
> 2) Task B enters btrfs_ioctl_quota_ctl() -> btrfs_quota_disable() and
>     disables qgroups, so now fs_info->quota_root is NULL;
> 
> 3) Task A enters btrfs_create_qgroup() and calls btrfs_qgroup_mode(),
>     which returns BTRFS_QGROUP_MODE_DISABLED since quotas are disabled,
>     and then btrfs_create_qgroup() returns 0 to the caller, which makes
>     the ioctl return 0 instead of -ENOTCONN.
> 
>     The check for fs_info->quota_root and returning -ENOTCONN if it's NULL
>     is made only after the call btrfs_qgroup_mode().
> 
> Fix this by moving the check for disabled quotas with btrfs_qgroup_mode()
> into transaction.c:create_pending_snapshot(), so that we don't abort the
> transaction if btrfs_create_qgroup() returns -ENOTCONN and quotas are
> disabled.
> 
> Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Since create_pending_snapshot() is called during 
btrfs_commit_transaction(), we're safe from a btrfs_quota_enable() call 
as it requires a trans handle.

So there will not be a race between create_pending_snapshot() and 
btrfs_quota_enable().

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu


> ---
>   fs/btrfs/qgroup.c      | 3 ---
>   fs/btrfs/transaction.c | 6 ++++--
>   2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index eb1bb57dee7d..ae9bc7c71a34 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1662,9 +1662,6 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>   	struct btrfs_qgroup *prealloc = NULL;
>   	int ret = 0;
>   
> -	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED)
> -		return 0;
> -
>   	mutex_lock(&fs_info->qgroup_ioctl_lock);
>   	if (!fs_info->quota_root) {
>   		ret = -ENOTCONN;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 2e07c90be5cd..c5c0d9cf1a80 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1735,8 +1735,10 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>   
>   	ret = btrfs_create_qgroup(trans, objectid);
>   	if (ret && ret != -EEXIST) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto fail;
> +		if (ret != -ENOTCONN || btrfs_qgroup_enabled(fs_info)) {
> +			btrfs_abort_transaction(trans, ret);
> +			goto fail;
> +		}
>   	}
>   
>   	/*

[PATCH 3/3] btrfs: qgroup: use btrfs_qgroup_enabled() in ioctls

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have a publicly exported btrfs_qgroup_enabled() and an ioctl.c private
qgroup_enabled() helper. Both of these test if qgroups are enabled, the
first check if the flag BTRFS_FS_QUOTA_ENABLED is set in fs_info->flags
while the second checks if fs_info->quota_root is not NULL while holding
the mutex fs_info->qgroup_ioctl_lock.

We can get away with the private ioctl.c:qgroup_enabled(), as all entry
points into the qgroup code check if fs_info->quota_root is NULL or not
while holding the mutex fs_info->qgroup_ioctl_lock, and returning the
error -ENOTCONN in case it's NULL.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 3621ed2eacd1..3c4619375bc9 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3714,22 +3714,6 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
 	return ret;
 }
 
-/*
- * Quick check for ioctl handlers if quotas are enabled. Proper locking must be
- * done before any operations.
- */
-static bool qgroup_enabled(struct btrfs_fs_info *fs_info)
-{
-	bool ret = true;
-
-	mutex_lock(&fs_info->qgroup_ioctl_lock);
-	if (!fs_info->quota_root)
-		ret = false;
-	mutex_unlock(&fs_info->qgroup_ioctl_lock);
-
-	return ret;
-}
-
 static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 {
 	struct inode *inode = file_inode(file);
@@ -3744,7 +3728,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!qgroup_enabled(root->fs_info))
+	if (!btrfs_qgroup_enabled(fs_info))
 		return -ENOTCONN;
 
 	ret = mnt_want_write_file(file);
@@ -3814,7 +3798,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!qgroup_enabled(root->fs_info))
+	if (!btrfs_qgroup_enabled(root->fs_info))
 		return -ENOTCONN;
 
 	ret = mnt_want_write_file(file);
@@ -3873,7 +3857,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!qgroup_enabled(root->fs_info))
+	if (!btrfs_qgroup_enabled(root->fs_info))
 		return -ENOTCONN;
 
 	ret = mnt_want_write_file(file);
@@ -3921,7 +3905,7 @@ static long btrfs_ioctl_quota_rescan(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!qgroup_enabled(fs_info))
+	if (!btrfs_qgroup_enabled(fs_info))
 		return -ENOTCONN;
 
 	ret = mnt_want_write_file(file);
-- 
2.47.2

Re: [PATCH 3/3] btrfs: qgroup: use btrfs_qgroup_enabled() in ioctls

[Qu Wenruo]

在 2025/7/2 01:12, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We have a publicly exported btrfs_qgroup_enabled() and an ioctl.c private
> qgroup_enabled() helper. Both of these test if qgroups are enabled, the
> first check if the flag BTRFS_FS_QUOTA_ENABLED is set in fs_info->flags
> while the second checks if fs_info->quota_root is not NULL while holding
> the mutex fs_info->qgroup_ioctl_lock.
> 
> We can get away with the private ioctl.c:qgroup_enabled(), as all entry
> points into the qgroup code check if fs_info->quota_root is NULL or not
> while holding the mutex fs_info->qgroup_ioctl_lock, and returning the
> error -ENOTCONN in case it's NULL.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/ioctl.c | 24 ++++--------------------
>   1 file changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3621ed2eacd1..3c4619375bc9 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3714,22 +3714,6 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>   	return ret;
>   }
>   
> -/*
> - * Quick check for ioctl handlers if quotas are enabled. Proper locking must be
> - * done before any operations.
> - */
> -static bool qgroup_enabled(struct btrfs_fs_info *fs_info)
> -{
> -	bool ret = true;
> -
> -	mutex_lock(&fs_info->qgroup_ioctl_lock);
> -	if (!fs_info->quota_root)
> -		ret = false;
> -	mutex_unlock(&fs_info->qgroup_ioctl_lock);
> -
> -	return ret;
> -}
> -
>   static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   {
>   	struct inode *inode = file_inode(file);
> @@ -3744,7 +3728,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	if (!qgroup_enabled(root->fs_info))
> +	if (!btrfs_qgroup_enabled(fs_info))
>   		return -ENOTCONN;
>   
>   	ret = mnt_want_write_file(file);
> @@ -3814,7 +3798,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	if (!qgroup_enabled(root->fs_info))
> +	if (!btrfs_qgroup_enabled(root->fs_info))
>   		return -ENOTCONN;
>   
>   	ret = mnt_want_write_file(file);
> @@ -3873,7 +3857,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	if (!qgroup_enabled(root->fs_info))
> +	if (!btrfs_qgroup_enabled(root->fs_info))
>   		return -ENOTCONN;
>   
>   	ret = mnt_want_write_file(file);
> @@ -3921,7 +3905,7 @@ static long btrfs_ioctl_quota_rescan(struct file *file, void __user *arg)
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	if (!qgroup_enabled(fs_info))
> +	if (!btrfs_qgroup_enabled(fs_info))
>   		return -ENOTCONN;
>   
>   	ret = mnt_want_write_file(file);

Re: [PATCH 0/3] btrfs: qgroup fixes and a cleanup

[David Sterba]

On Tue, Jul 01, 2025 at 04:42:19PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> A couple fixes and a cleanup, details in the change logs.
> 
> Filipe Manana (3):
>   btrfs: qgroup: set quota enabled bit if quota disable fails flushing reservations
>   btrfs: qgroup: fix qgroup create ioctl returning success after quotas disabled
>   btrfs: qgroup: use btrfs_qgroup_enabled() in ioctls

Reviewed-by: David Sterba <dsterba@suse.com>