| Message ID | 20210515023624.8065-1-realwakka@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: subvolume: destroy associated qgroup when delete subvolume | expand |
On 2021/5/15 上午10:36, Sidong Yang wrote: > This patch adds the options --delete-qgroup and --no-delete-qgroup. When > the option is enabled, delete subvolume command destroies associated > qgroup together. This patch make it as default option. Even though quota > is disabled, it enables quota temporary and restore it after. No, this is not a good idea at all. First thing first, if quota is disabled, all qgroup info including the level 0 qgroups will also be deleted, thus no need to enable in the first place. Secondly, there is already a patch in the past to delete level 0 qgroups in kernel space, which should be a much better solution. I didn't remember when, but I'm pretty sure I did send out such patch in the past. Maybe it's time to revive the patch now. Thanks, Qu > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > --- > > I wrote a patch that adds command to delete associated qgroup when > delete subvolume together. It also works when quota disabled. How it > works is enable quota temporary and restore it. I don't know it's good > way. Is there any better way than this? > > cmds/subvolume.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/cmds/subvolume.c b/cmds/subvolume.c > index 9bd17808..18c75083 100644 > --- a/cmds/subvolume.c > +++ b/cmds/subvolume.c > @@ -239,6 +239,8 @@ static const char * const cmd_subvol_delete_usage[] = { > "-c|--commit-after wait for transaction commit at the end of the operation", > "-C|--commit-each wait for transaction commit after deleting each subvolume", > "-i|--subvolid subvolume id of the to be removed subvolume", > + "--delete-qgroup delete associated qgroup together", > + "--no-delete-qgroup don't delete associated qgroup", > "-v|--verbose deprecated, alias for global -v option", > HELPINFO_INSERT_GLOBALS, > HELPINFO_INSERT_VERBOSE, > @@ -266,14 +268,18 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd, > enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 }; > enum btrfs_util_error err; > uint64_t default_subvol_id = 0, target_subvol_id = 0; > + bool delete_qgroup = false; > > optind = 0; > while (1) { > int c; > + enum { GETOPT_VAL_DEL_QGROUP = 256, GETOPT_VAL_NO_DEL_QGROUP }; > static const struct option long_options[] = { > {"commit-after", no_argument, NULL, 'c'}, > {"commit-each", no_argument, NULL, 'C'}, > {"subvolid", required_argument, NULL, 'i'}, > + {"delete-qgroup", no_argument, NULL, GETOPT_VAL_DEL_QGROUP}, > + {"no-delete-qgroup", no_argument, NULL, GETOPT_VAL_NO_DEL_QGROUP}, > {"verbose", no_argument, NULL, 'v'}, > {NULL, 0, NULL, 0} > }; > @@ -295,6 +301,12 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd, > case 'v': > bconf_be_verbose(); > break; > + case GETOPT_VAL_DEL_QGROUP: > + delete_qgroup = true; > + break; > + case GETOPT_VAL_NO_DEL_QGROUP: > + delete_qgroup = false; > + break; > default: > usage_unknown_option(cmd, argv); > } > @@ -388,6 +400,44 @@ again: > goto out; > } > > + if (delete_qgroup) { > + struct btrfs_ioctl_qgroup_create_args args; > + memset(&args, 0, sizeof(args)); > + args.create = 0; > + args.qgroupid = target_subvol_id; > + > + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { > + if (errno == ENOTCONN) { > + struct btrfs_ioctl_quota_ctl_args quota_ctl_args; > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_ENABLE; > + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, "a_ctl_args) < 0) { > + error("unable to enable quota: %s", > + strerror(errno)); > + goto out; > + } > + > + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; > + ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args); > + error("unable to destroy quota group: %s", > + strerror(errno)); > + goto out; > + } > + > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; > + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args) < 0) { > + error("unable to disable quota: %s", > + strerror(errno)); > + goto out; > + } > + } else { > + error("unable to destroy quota group: %s", > + strerror(errno)); > + goto out; > + } > + } > + } > + > pr_verbose(MUST_LOG, "Delete subvolume (%s): ", > commit_mode == COMMIT_EACH || > (commit_mode == COMMIT_AFTER && cnt + 1 == argc) ? > @@ -412,6 +462,7 @@ again: > goto out; > } > > + > if (commit_mode == COMMIT_EACH) { > res = wait_for_commit(fd); > if (res < 0) { >
On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: > > > On 2021/5/15 上午10:36, Sidong Yang wrote: > > This patch adds the options --delete-qgroup and --no-delete-qgroup. When > > the option is enabled, delete subvolume command destroies associated > > qgroup together. This patch make it as default option. Even though quota > > is disabled, it enables quota temporary and restore it after. > Thanks for reply! > No, this is not a good idea at all. > > First thing first, if quota is disabled, all qgroup info including the > level 0 qgroups will also be deleted, thus no need to enable in the > first place. > > Secondly, there is already a patch in the past to delete level 0 qgroups > in kernel space, which should be a much better solution. Good, I think there is no need to make changes for userspace. Thanks, Sidong > > I didn't remember when, but I'm pretty sure I did send out such patch in > the past. > > Maybe it's time to revive the patch now. > > Thanks, > Qu > > > > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > > --- > > > > I wrote a patch that adds command to delete associated qgroup when > > delete subvolume together. It also works when quota disabled. How it > > works is enable quota temporary and restore it. I don't know it's good > > way. Is there any better way than this? > > > > cmds/subvolume.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/cmds/subvolume.c b/cmds/subvolume.c > > index 9bd17808..18c75083 100644 > > --- a/cmds/subvolume.c > > +++ b/cmds/subvolume.c > > @@ -239,6 +239,8 @@ static const char * const cmd_subvol_delete_usage[] = { > > "-c|--commit-after wait for transaction commit at the end of the operation", > > "-C|--commit-each wait for transaction commit after deleting each subvolume", > > "-i|--subvolid subvolume id of the to be removed subvolume", > > + "--delete-qgroup delete associated qgroup together", > > + "--no-delete-qgroup don't delete associated qgroup", > > "-v|--verbose deprecated, alias for global -v option", > > HELPINFO_INSERT_GLOBALS, > > HELPINFO_INSERT_VERBOSE, > > @@ -266,14 +268,18 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd, > > enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 }; > > enum btrfs_util_error err; > > uint64_t default_subvol_id = 0, target_subvol_id = 0; > > + bool delete_qgroup = false; > > > > optind = 0; > > while (1) { > > int c; > > + enum { GETOPT_VAL_DEL_QGROUP = 256, GETOPT_VAL_NO_DEL_QGROUP }; > > static const struct option long_options[] = { > > {"commit-after", no_argument, NULL, 'c'}, > > {"commit-each", no_argument, NULL, 'C'}, > > {"subvolid", required_argument, NULL, 'i'}, > > + {"delete-qgroup", no_argument, NULL, GETOPT_VAL_DEL_QGROUP}, > > + {"no-delete-qgroup", no_argument, NULL, GETOPT_VAL_NO_DEL_QGROUP}, > > {"verbose", no_argument, NULL, 'v'}, > > {NULL, 0, NULL, 0} > > }; > > @@ -295,6 +301,12 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd, > > case 'v': > > bconf_be_verbose(); > > break; > > + case GETOPT_VAL_DEL_QGROUP: > > + delete_qgroup = true; > > + break; > > + case GETOPT_VAL_NO_DEL_QGROUP: > > + delete_qgroup = false; > > + break; > > default: > > usage_unknown_option(cmd, argv); > > } > > @@ -388,6 +400,44 @@ again: > > goto out; > > } > > > > + if (delete_qgroup) { > > + struct btrfs_ioctl_qgroup_create_args args; > > + memset(&args, 0, sizeof(args)); > > + args.create = 0; > > + args.qgroupid = target_subvol_id; > > + > > + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { > > + if (errno == ENOTCONN) { > > + struct btrfs_ioctl_quota_ctl_args quota_ctl_args; > > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_ENABLE; > > + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, "a_ctl_args) < 0) { > > + error("unable to enable quota: %s", > > + strerror(errno)); > > + goto out; > > + } > > + > > + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { > > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; > > + ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args); > > + error("unable to destroy quota group: %s", > > + strerror(errno)); > > + goto out; > > + } > > + > > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; > > + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args) < 0) { > > + error("unable to disable quota: %s", > > + strerror(errno)); > > + goto out; > > + } > > + } else { > > + error("unable to destroy quota group: %s", > > + strerror(errno)); > > + goto out; > > + } > > + } > > + } > > + > > pr_verbose(MUST_LOG, "Delete subvolume (%s): ", > > commit_mode == COMMIT_EACH || > > (commit_mode == COMMIT_AFTER && cnt + 1 == argc) ? > > @@ -412,6 +462,7 @@ again: > > goto out; > > } > > > > + > > if (commit_mode == COMMIT_EACH) { > > res = wait_for_commit(fd); > > if (res < 0) { > >
On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: > On 2021/5/15 上午10:36, Sidong Yang wrote: > > This patch adds the options --delete-qgroup and --no-delete-qgroup. When > > the option is enabled, delete subvolume command destroies associated > > qgroup together. This patch make it as default option. Even though quota > > is disabled, it enables quota temporary and restore it after. > > No, this is not a good idea at all. > > First thing first, if quota is disabled, all qgroup info including the > level 0 qgroups will also be deleted, thus no need to enable in the > first place. > > Secondly, there is already a patch in the past to delete level 0 qgroups > in kernel space, which should be a much better solution. I've filed the issue to do it in the userspace because it gives user more control whether to do the deletion or not and to also cover all kernels that won't get the patch (ie. old stable kernels). Changing the default behaviour is always risky and has a potential to break user scripts. IMO adding the option to progs and changing the default there is safer in this case.
On Sat, May 15, 2021 at 02:36:24AM +0000, Sidong Yang wrote: > This patch adds the options --delete-qgroup and --no-delete-qgroup. When > the option is enabled, delete subvolume command destroies associated > qgroup together. This patch make it as default option. Even though quota > is disabled, it enables quota temporary and restore it after. > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > --- > > I wrote a patch that adds command to delete associated qgroup when > delete subvolume together. It also works when quota disabled. How it > works is enable quota temporary and restore it. I don't know it's good > way. Is there any better way than this? As Qu pointed out, nothing needs to be done in case quotas are not enabled. > + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { > + if (errno == ENOTCONN) { Which turns this into if (errno != ENOTCONN error("..."); > + struct btrfs_ioctl_quota_ctl_args quota_ctl_args; > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_ENABLE; > + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, "a_ctl_args) < 0) { > + error("unable to enable quota: %s", > + strerror(errno)); > + goto out; > + } > + > + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; > + ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args); > + error("unable to destroy quota group: %s", > + strerror(errno)); > + goto out; > + } > + > + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; > + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args) < 0) { > + error("unable to disable quota: %s", > + strerror(errno)); > + goto out; > + } > + } else { > + error("unable to destroy quota group: %s", > + strerror(errno)); strerror(errno) should be replaced by %m in the format string > + goto out; > + } > + } > + } > + > pr_verbose(MUST_LOG, "Delete subvolume (%s): ", > commit_mode == COMMIT_EACH || > (commit_mode == COMMIT_AFTER && cnt + 1 == argc) ? > @@ -412,6 +462,7 @@ again: > goto out; > } > > + Extra newline > if (commit_mode == COMMIT_EACH) { > res = wait_for_commit(fd); > if (res < 0) { > -- > 2.25.1
On 2021/5/15 下午9:35, David Sterba wrote: > On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: >> On 2021/5/15 上午10:36, Sidong Yang wrote: >>> This patch adds the options --delete-qgroup and --no-delete-qgroup. When >>> the option is enabled, delete subvolume command destroies associated >>> qgroup together. This patch make it as default option. Even though quota >>> is disabled, it enables quota temporary and restore it after. >> >> No, this is not a good idea at all. >> >> First thing first, if quota is disabled, all qgroup info including the >> level 0 qgroups will also be deleted, thus no need to enable in the >> first place. >> >> Secondly, there is already a patch in the past to delete level 0 qgroups >> in kernel space, which should be a much better solution. > > I've filed the issue to do it in the userspace because it gives user > more control whether to do the deletion or not and to also cover all > kernels that won't get the patch (ie. old stable kernels). > > Changing the default behaviour is always risky and has a potential to > break user scripts. IMO adding the option to progs and changing the > default there is safer in this case. > Then shouldn't it still go through ioctl options? Doing it completely in user space doesn't seem correct to me. Thanks, Qu
On Sun, May 16, 2021 at 07:55:25AM +0800, Qu Wenruo wrote: > > > On 2021/5/15 下午9:35, David Sterba wrote: > > On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: > > > On 2021/5/15 上午10:36, Sidong Yang wrote: > > > > This patch adds the options --delete-qgroup and --no-delete-qgroup. When > > > > the option is enabled, delete subvolume command destroies associated > > > > qgroup together. This patch make it as default option. Even though quota > > > > is disabled, it enables quota temporary and restore it after. > > > > > > No, this is not a good idea at all. > > > > > > First thing first, if quota is disabled, all qgroup info including the > > > level 0 qgroups will also be deleted, thus no need to enable in the > > > first place. > > > > > > Secondly, there is already a patch in the past to delete level 0 qgroups > > > in kernel space, which should be a much better solution. > > > > I've filed the issue to do it in the userspace because it gives user > > more control whether to do the deletion or not and to also cover all > > kernels that won't get the patch (ie. old stable kernels). > > > > Changing the default behaviour is always risky and has a potential to > > break user scripts. IMO adding the option to progs and changing the > > default there is safer in this case. > > > Then shouldn't it still go through ioctl options? > > Doing it completely in user space doesn't seem correct to me. Yes, It still use ioctl calls for destroying qgroup. I think this code has pros and cons. IMHO, as david said, It also has some benefits when doing it in userspace for old kernel versions. and give a chance to change softly their codes which use btrfs-progs with options. But when kernel supports this operation, maybe this code will always failed. because the qgroup was already destroyed with it's subvolume in kernel. and the code will be meaningless. So in long run, I think it's better way to doing this in kernel. > > Thanks, > Qu
On 2021/5/19 上午12:06, Sidong Yang wrote: > On Sun, May 16, 2021 at 07:55:25AM +0800, Qu Wenruo wrote: >> >> >> On 2021/5/15 下午9:35, David Sterba wrote: >>> On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: >>>> On 2021/5/15 上午10:36, Sidong Yang wrote: >>>>> This patch adds the options --delete-qgroup and --no-delete-qgroup. When >>>>> the option is enabled, delete subvolume command destroies associated >>>>> qgroup together. This patch make it as default option. Even though quota >>>>> is disabled, it enables quota temporary and restore it after. >>>> >>>> No, this is not a good idea at all. >>>> >>>> First thing first, if quota is disabled, all qgroup info including the >>>> level 0 qgroups will also be deleted, thus no need to enable in the >>>> first place. >>>> >>>> Secondly, there is already a patch in the past to delete level 0 qgroups >>>> in kernel space, which should be a much better solution. >>> >>> I've filed the issue to do it in the userspace because it gives user >>> more control whether to do the deletion or not and to also cover all >>> kernels that won't get the patch (ie. old stable kernels). >>> >>> Changing the default behaviour is always risky and has a potential to >>> break user scripts. IMO adding the option to progs and changing the >>> default there is safer in this case. >>> >> Then shouldn't it still go through ioctl options? >> >> Doing it completely in user space doesn't seem correct to me. > > Yes, It still use ioctl calls for destroying qgroup. What I mean is to add new ioctl flags for the existing destory subvolume. By that newer btrfs-progs can try to delete using the new flags, If fails, go back to regular subovlume delete (without deleting qgroup) and gives user a warning. This still allows user to determine whether to delete qgroup along the ioctl, and still keeps compatibility. THanks, Qu > I think this code has pros and cons. > IMHO, as david said, It also has some benefits when doing it in userspace for > old kernel versions. and give a chance to change softly their codes > which use btrfs-progs with options. > > But when kernel supports this operation, maybe this code will always failed. > because the qgroup was already destroyed with it's subvolume in kernel. > and the code will be meaningless. > > So in long run, I think it's better way to doing this in kernel. > >> >> Thanks, >> Qu
On 2021/5/19 上午8:09, Qu Wenruo wrote: > > > On 2021/5/19 上午12:06, Sidong Yang wrote: >> On Sun, May 16, 2021 at 07:55:25AM +0800, Qu Wenruo wrote: >>> >>> >>> On 2021/5/15 下午9:35, David Sterba wrote: >>>> On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: >>>>> On 2021/5/15 上午10:36, Sidong Yang wrote: >>>>>> This patch adds the options --delete-qgroup and >>>>>> --no-delete-qgroup. When >>>>>> the option is enabled, delete subvolume command destroies associated >>>>>> qgroup together. This patch make it as default option. Even though >>>>>> quota >>>>>> is disabled, it enables quota temporary and restore it after. >>>>> >>>>> No, this is not a good idea at all. >>>>> >>>>> First thing first, if quota is disabled, all qgroup info including the >>>>> level 0 qgroups will also be deleted, thus no need to enable in the >>>>> first place. >>>>> >>>>> Secondly, there is already a patch in the past to delete level 0 >>>>> qgroups >>>>> in kernel space, which should be a much better solution. >>>> >>>> I've filed the issue to do it in the userspace because it gives user >>>> more control whether to do the deletion or not and to also cover all >>>> kernels that won't get the patch (ie. old stable kernels). >>>> >>>> Changing the default behaviour is always risky and has a potential to >>>> break user scripts. IMO adding the option to progs and changing the >>>> default there is safer in this case. >>>> >>> Then shouldn't it still go through ioctl options? >>> >>> Doing it completely in user space doesn't seem correct to me. >> >> Yes, It still use ioctl calls for destroying qgroup. > > What I mean is to add new ioctl flags for the existing destory subvolume. > > By that newer btrfs-progs can try to delete using the new flags, > If fails, go back to regular subovlume delete (without deleting qgroup) > and gives user a warning. > > This still allows user to determine whether to delete qgroup along the > ioctl, and still keeps compatibility. One more thing to add is, the qgroup delete should only happen when the qgroup numbers get cleared to 0. This is not completely impossible to be done in user space, but very complex, needs extra ioctl to wait for the subvolume deletion. Or we can hit a case where subvolume is still under deletion, but its qgroup is already removed. This can screw up the qgroup exclusive accounting. Thus I still prefer to do the deletion in kernel side, where we have pretty convenient timing to delete the qgroup, just after the subovlume deletion is done. Thanks, Qu > > THanks, > Qu >> I think this code has pros and cons. >> IMHO, as david said, It also has some benefits when doing it in >> userspace for >> old kernel versions. and give a chance to change softly their codes >> which use btrfs-progs with options. >> >> But when kernel supports this operation, maybe this code will always >> failed. >> because the qgroup was already destroyed with it's subvolume in kernel. >> and the code will be meaningless. >> >> So in long run, I think it's better way to doing this in kernel. >> >>> >>> Thanks, >>> Qu
On Wed, May 19, 2021 at 08:09:45AM +0800, Qu Wenruo wrote: > > > On 2021/5/19 上午12:06, Sidong Yang wrote: > > On Sun, May 16, 2021 at 07:55:25AM +0800, Qu Wenruo wrote: > >> > >> > >> On 2021/5/15 下午9:35, David Sterba wrote: > >>> On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: > >>>> On 2021/5/15 上午10:36, Sidong Yang wrote: > >>>>> This patch adds the options --delete-qgroup and --no-delete-qgroup. When > >>>>> the option is enabled, delete subvolume command destroies associated > >>>>> qgroup together. This patch make it as default option. Even though quota > >>>>> is disabled, it enables quota temporary and restore it after. > >>>> > >>>> No, this is not a good idea at all. > >>>> > >>>> First thing first, if quota is disabled, all qgroup info including the > >>>> level 0 qgroups will also be deleted, thus no need to enable in the > >>>> first place. > >>>> > >>>> Secondly, there is already a patch in the past to delete level 0 qgroups > >>>> in kernel space, which should be a much better solution. > >>> > >>> I've filed the issue to do it in the userspace because it gives user > >>> more control whether to do the deletion or not and to also cover all > >>> kernels that won't get the patch (ie. old stable kernels). > >>> > >>> Changing the default behaviour is always risky and has a potential to > >>> break user scripts. IMO adding the option to progs and changing the > >>> default there is safer in this case. > >>> > >> Then shouldn't it still go through ioctl options? > >> > >> Doing it completely in user space doesn't seem correct to me. > > > > Yes, It still use ioctl calls for destroying qgroup. > > What I mean is to add new ioctl flags for the existing destory subvolume. Which is what I don't want to do. Why: it's doing more than operation so the error code is unclear what it's related to. Either subvolume deletion or qgroup deletion. A similar suggestion was in for subvolume creation to force quota rescan, https://lore.kernel.org/linux-btrfs/20210525162054.GE7604@twin.jikos.cz/333 same reason why not.
On Fri, Jun 11, 2021 at 04:23:29PM +0200, David Sterba wrote: > On Wed, May 19, 2021 at 08:09:45AM +0800, Qu Wenruo wrote: > > > > > > On 2021/5/19 上午12:06, Sidong Yang wrote: > > > On Sun, May 16, 2021 at 07:55:25AM +0800, Qu Wenruo wrote: > > >> > > >> > > >> On 2021/5/15 下午9:35, David Sterba wrote: > > >>> On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote: > > >>>> On 2021/5/15 上午10:36, Sidong Yang wrote: > > >>>>> This patch adds the options --delete-qgroup and --no-delete-qgroup. When > > >>>>> the option is enabled, delete subvolume command destroies associated > > >>>>> qgroup together. This patch make it as default option. Even though quota > > >>>>> is disabled, it enables quota temporary and restore it after. > > >>>> > > >>>> No, this is not a good idea at all. > > >>>> > > >>>> First thing first, if quota is disabled, all qgroup info including the > > >>>> level 0 qgroups will also be deleted, thus no need to enable in the > > >>>> first place. > > >>>> > > >>>> Secondly, there is already a patch in the past to delete level 0 qgroups > > >>>> in kernel space, which should be a much better solution. > > >>> > > >>> I've filed the issue to do it in the userspace because it gives user > > >>> more control whether to do the deletion or not and to also cover all > > >>> kernels that won't get the patch (ie. old stable kernels). > > >>> > > >>> Changing the default behaviour is always risky and has a potential to > > >>> break user scripts. IMO adding the option to progs and changing the > > >>> default there is safer in this case. > > >>> > > >> Then shouldn't it still go through ioctl options? > > >> > > >> Doing it completely in user space doesn't seem correct to me. > > > > > > Yes, It still use ioctl calls for destroying qgroup. > > > > What I mean is to add new ioctl flags for the existing destory subvolume. > > Which is what I don't want to do. Why: it's doing more than operation so > the error code is unclear what it's related to. Either subvolume > deletion or qgroup deletion. IMO, If new ioctl will be added for this problem, maybe it operates that delete subvolume and it's qgroup. I think it makes some problem. If it fails only deleting qgroup but subvolume, it needs to return the error that describes it. It is also duplicated with old ioctl that destroy subvolume only. I think the point is how to support additional operation that usually executed with. If the solution is adding a new ioctl includes old ioctl, there will be too many ioctls and it's hard to manage it. > > A similar suggestion was in for subvolume creation to force quota rescan, > https://lore.kernel.org/linux-btrfs/20210525162054.GE7604@twin.jikos.cz/333 > same reason why not.
diff --git a/cmds/subvolume.c b/cmds/subvolume.c index 9bd17808..18c75083 100644 --- a/cmds/subvolume.c +++ b/cmds/subvolume.c @@ -239,6 +239,8 @@ static const char * const cmd_subvol_delete_usage[] = { "-c|--commit-after wait for transaction commit at the end of the operation", "-C|--commit-each wait for transaction commit after deleting each subvolume", "-i|--subvolid subvolume id of the to be removed subvolume", + "--delete-qgroup delete associated qgroup together", + "--no-delete-qgroup don't delete associated qgroup", "-v|--verbose deprecated, alias for global -v option", HELPINFO_INSERT_GLOBALS, HELPINFO_INSERT_VERBOSE, @@ -266,14 +268,18 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd, enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 }; enum btrfs_util_error err; uint64_t default_subvol_id = 0, target_subvol_id = 0; + bool delete_qgroup = false; optind = 0; while (1) { int c; + enum { GETOPT_VAL_DEL_QGROUP = 256, GETOPT_VAL_NO_DEL_QGROUP }; static const struct option long_options[] = { {"commit-after", no_argument, NULL, 'c'}, {"commit-each", no_argument, NULL, 'C'}, {"subvolid", required_argument, NULL, 'i'}, + {"delete-qgroup", no_argument, NULL, GETOPT_VAL_DEL_QGROUP}, + {"no-delete-qgroup", no_argument, NULL, GETOPT_VAL_NO_DEL_QGROUP}, {"verbose", no_argument, NULL, 'v'}, {NULL, 0, NULL, 0} }; @@ -295,6 +301,12 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd, case 'v': bconf_be_verbose(); break; + case GETOPT_VAL_DEL_QGROUP: + delete_qgroup = true; + break; + case GETOPT_VAL_NO_DEL_QGROUP: + delete_qgroup = false; + break; default: usage_unknown_option(cmd, argv); } @@ -388,6 +400,44 @@ again: goto out; } + if (delete_qgroup) { + struct btrfs_ioctl_qgroup_create_args args; + memset(&args, 0, sizeof(args)); + args.create = 0; + args.qgroupid = target_subvol_id; + + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { + if (errno == ENOTCONN) { + struct btrfs_ioctl_quota_ctl_args quota_ctl_args; + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_ENABLE; + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, "a_ctl_args) < 0) { + error("unable to enable quota: %s", + strerror(errno)); + goto out; + } + + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) { + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; + ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args); + error("unable to destroy quota group: %s", + strerror(errno)); + goto out; + } + + quota_ctl_args.cmd = BTRFS_QUOTA_CTL_DISABLE; + if (ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args) < 0) { + error("unable to disable quota: %s", + strerror(errno)); + goto out; + } + } else { + error("unable to destroy quota group: %s", + strerror(errno)); + goto out; + } + } + } + pr_verbose(MUST_LOG, "Delete subvolume (%s): ", commit_mode == COMMIT_EACH || (commit_mode == COMMIT_AFTER && cnt + 1 == argc) ? @@ -412,6 +462,7 @@ again: goto out; } + if (commit_mode == COMMIT_EACH) { res = wait_for_commit(fd); if (res < 0) {
This patch adds the options --delete-qgroup and --no-delete-qgroup. When the option is enabled, delete subvolume command destroies associated qgroup together. This patch make it as default option. Even though quota is disabled, it enables quota temporary and restore it after. Signed-off-by: Sidong Yang <realwakka@gmail.com> --- I wrote a patch that adds command to delete associated qgroup when delete subvolume together. It also works when quota disabled. How it works is enable quota temporary and restore it. I don't know it's good way. Is there any better way than this? cmds/subvolume.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)