Re: [PATCH] btrfs-progs: add recursive subvol delete

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/8/2 07:36, Qu Wenruo 写道:
>
>
> 在 2024/6/28 00:51, Mark Harmstone 写道:
>> From: Omar Sandoval <osandov@fb.com>
>>
>> Adds a --recursive option to btrfs subvol delete, causing it to pass the
>> BTRFS_UTIL_DELETE_SUBVOLUME_RECURSIVE flag through to libbtrfsutil.
>>
>> This is a resubmission of part of a patch that Omar Sandoval sent in
>> 2018, which appears to have been overlooked:
>> https://lore.kernel.org/linux-btrfs/e42cdc5d5287269faf4d09e8c9786d0b3adeb658.1516991902.git.osandov@fb.com/
>>
>> Signed-off-by: Mark Harmstone <maharmstone@meta.com>
>> Co-authored-by: Mark Harmstone <maharmstone@meta.com>
>
> Looks very reasonable to me, especially the flag is already there.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Sorry to bother you, although this patch already has two reviewed-by
tags, I thought it would be an easy push thus created the PR for you:

   https://github.com/kdave/btrfs-progs/pull/861

But I got some resistance from David, focusing on certain technical
details on the help string (which I strongly disagree).

I think you may want to contribute your opinions to that.
(The thread on cmds/subvolumes.c)

And if possible, you may want to send an updated version, and create
your PR to the repo (and I will close my PR).

Furthermore, you may want to submit your PR to btrfs-progs repo, to get
some more direct feedback.

Thanks,
Qu

>
> Thanks,
> Qu
>> ---
>>   Documentation/btrfs-subvolume.rst |  4 ++++
>>   cmds/subvolume.c                  | 15 +++++++++++++--
>>   2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/btrfs-subvolume.rst
>> b/Documentation/btrfs-subvolume.rst
>> index d1e89f15..b1f22344 100644
>> --- a/Documentation/btrfs-subvolume.rst
>> +++ b/Documentation/btrfs-subvolume.rst
>> @@ -112,6 +112,10 @@ delete [options] [<subvolume> [<subvolume>...]],
>> delete -i|--subvolid <subvolid>
>>           -i|--subvolid <subvolid>
>>                   subvolume id to be removed instead of the <path>
>> that should point to the
>>                   filesystem with the subvolume
>> +
>> +        -R|--recursive
>> +                delete subvolumes beneath each subvolume recursively
>> +
>>           -v|--verbose
>>                   (deprecated) alias for global *-v* option
>>
>> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
>> index 52bc8850..b4151af2 100644
>> --- a/cmds/subvolume.c
>> +++ b/cmds/subvolume.c
>> @@ -347,6 +347,7 @@ static const char * const
>> cmd_subvolume_delete_usage[] = {
>>       OPTLINE("-c|--commit-after", "wait for transaction commit at the
>> end of the operation"),
>>       OPTLINE("-C|--commit-each", "wait for transaction commit after
>> deleting each subvolume"),
>>       OPTLINE("-i|--subvolid", "subvolume id of the to be removed
>> subvolume"),
>> +    OPTLINE("-R|--recursive", "delete subvolumes beneath each
>> subvolume recursively"),
>>       OPTLINE("-v|--verbose", "deprecated, alias for global -v option"),
>>       HELPINFO_INSERT_GLOBALS,
>>       HELPINFO_INSERT_VERBOSE,
>> @@ -367,6 +368,7 @@ static int cmd_subvolume_delete(const struct
>> cmd_struct *cmd, int argc, char **a
>>       char    *path = NULL;
>>       int commit_mode = 0;
>>       bool subvol_path_not_found = false;
>> +    int flags = 0;
>>       u8 fsid[BTRFS_FSID_SIZE];
>>       u64 subvolid = 0;
>>       char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>> @@ -383,11 +385,12 @@ static int cmd_subvolume_delete(const struct
>> cmd_struct *cmd, int argc, char **a
>>               {"commit-after", no_argument, NULL, 'c'},
>>               {"commit-each", no_argument, NULL, 'C'},
>>               {"subvolid", required_argument, NULL, 'i'},
>> +            {"recursive", no_argument, NULL, 'R'},
>>               {"verbose", no_argument, NULL, 'v'},
>>               {NULL, 0, NULL, 0}
>>           };
>>
>> -        c = getopt_long(argc, argv, "cCi:v", long_options, NULL);
>> +        c = getopt_long(argc, argv, "cCi:Rv", long_options, NULL);
>>           if (c < 0)
>>               break;
>>
>> @@ -401,6 +404,9 @@ static int cmd_subvolume_delete(const struct
>> cmd_struct *cmd, int argc, char **a
>>           case 'i':
>>               subvolid = arg_strtou64(optarg);
>>               break;
>> +        case 'R':
>> +            flags |= BTRFS_UTIL_DELETE_SUBVOLUME_RECURSIVE;
>> +            break;
>>           case 'v':
>>               bconf_be_verbose();
>>               break;
>> @@ -416,6 +422,11 @@ static int cmd_subvolume_delete(const struct
>> cmd_struct *cmd, int argc, char **a
>>       if (subvolid > 0 && check_argc_exact(argc - optind, 1))
>>           return 1;
>>
>> +    if (subvolid > 0 && flags & BTRFS_UTIL_DELETE_SUBVOLUME_RECURSIVE) {
>> +        error("option --recursive not supported with --subvolid");
>> +        return 1;
>> +    }
>> +
>>       pr_verbose(LOG_INFO, "Transaction commit: %s\n",
>>              !commit_mode ? "none (default)" :
>>              commit_mode == COMMIT_AFTER ? "at the end" : "after each");
>> @@ -528,7 +539,7 @@ again:
>>
>>       /* Start deleting. */
>>       if (subvolid == 0)
>> -        err = btrfs_util_delete_subvolume_fd(fd, vname, 0);
>> +        err = btrfs_util_delete_subvolume_fd(fd, vname, flags);
>>       else
>>           err = btrfs_util_delete_subvolume_by_id_fd(fd, subvolid);
>>       if (err) {
>