[PATCH 0/2] btrfs: subvol qgroup lifetime invariants

[PATCH 0/2] btrfs: subvol qgroup lifetime invariants

[Boris Burkov]

Subvol qgroups (id 0/SUBVOLID) are special. They get created
and reaped automatically for subvols, and can never have any children by
virtue of being at level 0.

Manually managing them doesn't provide much value but does create the
possibility for weird states and races. To that end, ban deleting subvol
qgroups that still have usage and creating subvol qgroups at all.

Testing Note: this patch series breaks btrfs/303 as that test is hunting
a race to do with creating a subvol qgroup which now explicitly fails.

Boris Burkov (2):
  btrfs: forbid creating subvol qgroups
  btrfs: forbid deleting live subvol qgroup

 fs/btrfs/ioctl.c  |  5 +++++
 fs/btrfs/qgroup.c | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

-- 
2.43.0

[PATCH 1/2] btrfs: forbid creating subvol qgroups

[Boris Burkov]

This leads to various races and it isn't helpful, because you can't
specify a subvol id when creating a subvol, so you can't be sure it
will be the right one. Any requirements on the automatic subvol can
be gratified by using a higher level qgroup and the inheritance
parameters of subvol creation.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/ioctl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 58e0c59bc4cd..3d476decde52 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3815,6 +3815,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 		goto out;
 	}
 
+	if (sa->create && is_fstree(sa->qgroupid)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	trans = btrfs_join_transaction(root);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
-- 
2.43.0

[PATCH 2/2] btrfs: forbid deleting live subvol qgroup

[Boris Burkov]

If a subvolume still exists, forbid deleting its qgroup.
This behavior generally leads to incorrect behavior in squotas and
doesn't have a legitimate purpose.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/qgroup.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 63b426cc7798..672896b2b7a0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -29,6 +29,7 @@
 #include "extent-tree.h"
 #include "root-tree.h"
 #include "tree-checker.h"
+#include "super.h"
 
 enum btrfs_qgroup_mode btrfs_qgroup_mode(struct btrfs_fs_info *fs_info)
 {
@@ -1736,6 +1737,15 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 	return ret;
 }
 
+static bool qgroup_has_usage(struct btrfs_qgroup *qgroup)
+{
+	return (qgroup->rfer > 0 || qgroup->rfer_cmpr > 0 ||
+		qgroup->excl > 0 || qgroup->excl_cmpr > 0 ||
+		qgroup->rsv.values[BTRFS_QGROUP_RSV_DATA] > 0 ||
+		qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PREALLOC] > 0 ||
+		qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PERTRANS] > 0);
+}
+
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
@@ -1755,6 +1765,11 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 		goto out;
 	}
 
+	if (is_fstree(qgroupid) && qgroup_has_usage(qgroup)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	/* Check if there are no children of this qgroup */
 	if (!list_empty(&qgroup->members)) {
 		ret = -EBUSY;
-- 
2.43.0

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[Qu Wenruo]

On 2024/1/20 11:25, Boris Burkov wrote:
> This leads to various races and it isn't helpful, because you can't
> specify a subvol id when creating a subvol, so you can't be sure it
> will be the right one. Any requirements on the automatic subvol can
> be gratified by using a higher level qgroup and the inheritance
> parameters of subvol creation.
>
> Signed-off-by: Boris Burkov <boris@bur.io>

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

Thanks,
Qu
> ---
>   fs/btrfs/ioctl.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 58e0c59bc4cd..3d476decde52 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3815,6 +3815,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>   		goto out;
>   	}
>
> +	if (sa->create && is_fstree(sa->qgroupid)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>   	trans = btrfs_join_transaction(root);
>   	if (IS_ERR(trans)) {
>   		ret = PTR_ERR(trans);

Re: [PATCH 2/2] btrfs: forbid deleting live subvol qgroup

[Qu Wenruo]

On 2024/1/20 11:25, Boris Burkov wrote:
> If a subvolume still exists, forbid deleting its qgroup.
> This behavior generally leads to incorrect behavior in squotas and
> doesn't have a legitimate purpose.
>
> Signed-off-by: Boris Burkov <boris@bur.io>

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

Thanks,
Qu

> ---
>   fs/btrfs/qgroup.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 63b426cc7798..672896b2b7a0 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -29,6 +29,7 @@
>   #include "extent-tree.h"
>   #include "root-tree.h"
>   #include "tree-checker.h"
> +#include "super.h"
>
>   enum btrfs_qgroup_mode btrfs_qgroup_mode(struct btrfs_fs_info *fs_info)
>   {
> @@ -1736,6 +1737,15 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>   	return ret;
>   }
>
> +static bool qgroup_has_usage(struct btrfs_qgroup *qgroup)
> +{
> +	return (qgroup->rfer > 0 || qgroup->rfer_cmpr > 0 ||
> +		qgroup->excl > 0 || qgroup->excl_cmpr > 0 ||
> +		qgroup->rsv.values[BTRFS_QGROUP_RSV_DATA] > 0 ||
> +		qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PREALLOC] > 0 ||
> +		qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PERTRANS] > 0);
> +}
> +
>   int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>   {
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
> @@ -1755,6 +1765,11 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>   		goto out;
>   	}
>
> +	if (is_fstree(qgroupid) && qgroup_has_usage(qgroup)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
>   	/* Check if there are no children of this qgroup */
>   	if (!list_empty(&qgroup->members)) {
>   		ret = -EBUSY;

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[David Sterba]

On Fri, Jan 19, 2024 at 04:55:58PM -0800, Boris Burkov wrote:
> This leads to various races and it isn't helpful, because you can't
> specify a subvol id when creating a subvol, so you can't be sure it
> will be the right one. Any requirements on the automatic subvol can
> be gratified by using a higher level qgroup and the inheritance
> parameters of subvol creation.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

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

Re: [PATCH 2/2] btrfs: forbid deleting live subvol qgroup

[David Sterba]

On Fri, Jan 19, 2024 at 04:55:59PM -0800, Boris Burkov wrote:
> If a subvolume still exists, forbid deleting its qgroup.
> This behavior generally leads to incorrect behavior in squotas and
> doesn't have a legitimate purpose.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

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

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[Neal Gompa]

On Fri, Jan 19, 2024 at 7:55 PM Boris Burkov <boris@bur.io> wrote:
>
> This leads to various races and it isn't helpful, because you can't
> specify a subvol id when creating a subvol, so you can't be sure it
> will be the right one. Any requirements on the automatic subvol can
> be gratified by using a higher level qgroup and the inheritance
> parameters of subvol creation.
>

Hold up, does this mean that qgroups can't be used *at all* on Fedora,
where we use subvolumes for both the root and user home directory
hierarchies?


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[David Sterba]

On Wed, Jan 24, 2024 at 07:52:28AM -0500, Neal Gompa wrote:
> On Fri, Jan 19, 2024 at 7:55 PM Boris Burkov <boris@bur.io> wrote:
> >
> > This leads to various races and it isn't helpful, because you can't
> > specify a subvol id when creating a subvol, so you can't be sure it
> > will be the right one. Any requirements on the automatic subvol can
> > be gratified by using a higher level qgroup and the inheritance
> > parameters of subvol creation.
> 
> Hold up, does this mean that qgroups can't be used *at all* on Fedora,
> where we use subvolumes for both the root and user home directory
> hierarchies?

How do you imply that from the patch? This is about preventing creating
the subvolume qgroups, i.e. with the level 0 and referred to as 0/1234
where 1234 is a subvolume id. Such qgroups are supposed to be created
only at the time the subvolume is created.

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[Neal Gompa]

On Wed, Jan 24, 2024 at 11:37 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Jan 24, 2024 at 07:52:28AM -0500, Neal Gompa wrote:
> > On Fri, Jan 19, 2024 at 7:55 PM Boris Burkov <boris@bur.io> wrote:
> > >
> > > This leads to various races and it isn't helpful, because you can't
> > > specify a subvol id when creating a subvol, so you can't be sure it
> > > will be the right one. Any requirements on the automatic subvol can
> > > be gratified by using a higher level qgroup and the inheritance
> > > parameters of subvol creation.
> >
> > Hold up, does this mean that qgroups can't be used *at all* on Fedora,
> > where we use subvolumes for both the root and user home directory
> > hierarchies?
>
> How do you imply that from the patch? This is about preventing creating
> the subvolume qgroups, i.e. with the level 0 and referred to as 0/1234
> where 1234 is a subvolume id. Such qgroups are supposed to be created
> only at the time the subvolume is created.
>

Because I don't really understand what the description of this patch
implies. It is not clear to me what is actually changing here, that's
why I'm asking.


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[Boris Burkov]

On Wed, Jan 24, 2024 at 10:32:54PM -0500, Neal Gompa wrote:
> On Wed, Jan 24, 2024 at 11:37 AM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Wed, Jan 24, 2024 at 07:52:28AM -0500, Neal Gompa wrote:
> > > On Fri, Jan 19, 2024 at 7:55 PM Boris Burkov <boris@bur.io> wrote:
> > > >
> > > > This leads to various races and it isn't helpful, because you can't
> > > > specify a subvol id when creating a subvol, so you can't be sure it
> > > > will be the right one. Any requirements on the automatic subvol can
> > > > be gratified by using a higher level qgroup and the inheritance
> > > > parameters of subvol creation.
> > >
> > > Hold up, does this mean that qgroups can't be used *at all* on Fedora,
> > > where we use subvolumes for both the root and user home directory
> > > hierarchies?
> >
> > How do you imply that from the patch? This is about preventing creating
> > the subvolume qgroups, i.e. with the level 0 and referred to as 0/1234
> > where 1234 is a subvolume id. Such qgroups are supposed to be created
> > only at the time the subvolume is created.
> >
> 
> Because I don't really understand what the description of this patch
> implies. It is not clear to me what is actually changing here, that's
> why I'm asking.

Sorry for the unclear patch description!

If Fedora is creating/snapshotting subvolumes but not explicitly
creating subvolume qgroups (qg id of the form 0/X), that is fully
supported and there should be no issue.

More details, just to be extra thorough to make up for the bad initial
description:

In the qgroup hierarchy, level 0 is special, these are the so called
"subvolume qgroups". The qgroup with id 0/X is the qgroup for the
subvolume with id X. When that subvolume accumulates usage, it will
always be reported into that qgroup (in a hard-coded fashion in
fs/btrfs/qgroup.c).

This patch prevents users from explicitly creating subvolume qgroups
with BTRFS_IOC_QGROUP_CREATE.

This does not affect the existing behavior that creating a subvolume
also creates the corresponding subvolume qgroup.

examples using the btrfs cli to illustrate:
# create a subvol qgroup explicitly; EINVAL
btrfs qgroup create 0/1234 <mnt>
# create a higher level qgroup explicitly; OK
btrfs qgroup create 1/1234 <mnt>
# create sv with id X and qgroup with id 0/X
btrfs subvol create <mnt>/<sv>

The basic motivation for both of the patches in this series is that
qgroup lifetime is more complicated in simple quotas and these are
relatively unobtrusive changes that make it (a lot) easier to manage
without affecting any known useful classic qgroups use case.

Thanks,
Boris

> 
> 
> -- 
> 真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH 1/2] btrfs: forbid creating subvol qgroups

[Neal Gompa]

On Thu, Jan 25, 2024 at 6:11 PM Boris Burkov <boris@bur.io> wrote:
>
> On Wed, Jan 24, 2024 at 10:32:54PM -0500, Neal Gompa wrote:
> > On Wed, Jan 24, 2024 at 11:37 AM David Sterba <dsterba@suse.cz> wrote:
> > >
> > > On Wed, Jan 24, 2024 at 07:52:28AM -0500, Neal Gompa wrote:
> > > > On Fri, Jan 19, 2024 at 7:55 PM Boris Burkov <boris@bur.io> wrote:
> > > > >
> > > > > This leads to various races and it isn't helpful, because you can't
> > > > > specify a subvol id when creating a subvol, so you can't be sure it
> > > > > will be the right one. Any requirements on the automatic subvol can
> > > > > be gratified by using a higher level qgroup and the inheritance
> > > > > parameters of subvol creation.
> > > >
> > > > Hold up, does this mean that qgroups can't be used *at all* on Fedora,
> > > > where we use subvolumes for both the root and user home directory
> > > > hierarchies?
> > >
> > > How do you imply that from the patch? This is about preventing creating
> > > the subvolume qgroups, i.e. with the level 0 and referred to as 0/1234
> > > where 1234 is a subvolume id. Such qgroups are supposed to be created
> > > only at the time the subvolume is created.
> > >
> >
> > Because I don't really understand what the description of this patch
> > implies. It is not clear to me what is actually changing here, that's
> > why I'm asking.
>
> Sorry for the unclear patch description!
>
> If Fedora is creating/snapshotting subvolumes but not explicitly
> creating subvolume qgroups (qg id of the form 0/X), that is fully
> supported and there should be no issue.
>
> More details, just to be extra thorough to make up for the bad initial
> description:
>
> In the qgroup hierarchy, level 0 is special, these are the so called
> "subvolume qgroups". The qgroup with id 0/X is the qgroup for the
> subvolume with id X. When that subvolume accumulates usage, it will
> always be reported into that qgroup (in a hard-coded fashion in
> fs/btrfs/qgroup.c).
>
> This patch prevents users from explicitly creating subvolume qgroups
> with BTRFS_IOC_QGROUP_CREATE.
>
> This does not affect the existing behavior that creating a subvolume
> also creates the corresponding subvolume qgroup.
>
> examples using the btrfs cli to illustrate:
> # create a subvol qgroup explicitly; EINVAL
> btrfs qgroup create 0/1234 <mnt>
> # create a higher level qgroup explicitly; OK
> btrfs qgroup create 1/1234 <mnt>
> # create sv with id X and qgroup with id 0/X
> btrfs subvol create <mnt>/<sv>
>
> The basic motivation for both of the patches in this series is that
> qgroup lifetime is more complicated in simple quotas and these are
> relatively unobtrusive changes that make it (a lot) easier to manage
> without affecting any known useful classic qgroups use case.
>

Okay, that makes sense! Can you put some of this into the initial
patch description then, assuming David hasn't already checked it in?



-- 
真実はいつも一つ！/ Always, there's only one truth!