| Message ID | eb79dcbe0cbfa7459b249f76818a5e5a08a42ea4.1705711967.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: subvol qgroup lifetime invariants | expand |
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);
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>
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?
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.
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.
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!
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?
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);
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(+)