| Message ID | 20240627152156.1349692-1-maharmstone@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: add recursive subvol delete | expand |
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) {
在 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) {
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>
在 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) { >
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) {