* Re: [PATCH] btrfs-progs: add recursive subvol delete
2024-06-27 15:21 [PATCH] btrfs-progs: add recursive subvol delete Mark Harmstone
@ 2024-08-01 16:17 ` Mark Harmstone
2024-08-01 22:06 ` Qu Wenruo
2024-08-02 10:04 ` Neal Gompa
2 siblings, 0 replies; 5+ messages in thread
From: Mark Harmstone @ 2024-08-01 16:17 UTC (permalink / raw)
To: linux-btrfs@vger.kernel.org
Would someone mind having a look at this please? I think it's been missed.
Thanks
On 27/6/24 16:21, Mark Harmstone wrote:
> 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>
> ---
> 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) {
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: add recursive subvol delete
2024-06-27 15:21 [PATCH] btrfs-progs: add recursive subvol delete Mark Harmstone
2024-08-01 16:17 ` Mark Harmstone
@ 2024-08-01 22:06 ` Qu Wenruo
2024-08-11 2:55 ` Qu Wenruo
2024-08-02 10:04 ` Neal Gompa
2 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2024-08-01 22:06 UTC (permalink / raw)
To: Mark Harmstone, linux-btrfs; +Cc: Omar Sandoval, Mark Harmstone
在 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>
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) {
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: add recursive subvol delete
2024-08-01 22:06 ` Qu Wenruo
@ 2024-08-11 2:55 ` Qu Wenruo
0 siblings, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2024-08-11 2:55 UTC (permalink / raw)
To: Mark Harmstone, linux-btrfs; +Cc: Omar Sandoval, Mark Harmstone
在 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) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs-progs: add recursive subvol delete
2024-06-27 15:21 [PATCH] btrfs-progs: add recursive subvol delete Mark Harmstone
2024-08-01 16:17 ` Mark Harmstone
2024-08-01 22:06 ` Qu Wenruo
@ 2024-08-02 10:04 ` Neal Gompa
2 siblings, 0 replies; 5+ messages in thread
From: Neal Gompa @ 2024-08-02 10:04 UTC (permalink / raw)
To: Mark Harmstone; +Cc: linux-btrfs, Omar Sandoval, Mark Harmstone
On Thu, Jun 27, 2024 at 11:22 AM Mark Harmstone <maharmstone@fb.com> wrote:
>
> 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>
> ---
> 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) {
> --
> 2.44.2
>
>
Looks good to me.
Reviewed-by: Neal Gompa <neal@gompa.dev>
--
真実はいつも一つ！/ Always, there's only one truth!
^ permalink raw reply [flat|nested] 5+ messages in thread