| Message ID | 6c01643a-b6cd-21ae-45ec-e074995a5de2@applied-asynchrony.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/09/17 14:08, Holger Hoffstätte wrote: > A "root" subvolume is identified by a null parent UUID, so adding a new > subvolume filter and flag -P ("Parent") does the trick. I don't like the naming. The flag you are proposing is really nothing to do with whether a subvolume is a parent or not: it is about whether it is a snapshot or not (many subvolumes are both snapshots and also parents of other snapshots, and many non-snapshots are not the parent of any subvolumes). I have two suggestions: 1) Use -S (meaning "not a snapshot", the inverse of -s). Along with this change. I would change the usage text to say something like: -s list subvolumes originally created as snapshots -S list subvolumes originally created not as snapshots Presumably specifying both -s and -S should be an error. 2) Add a -P (parent) option but make it take an argument: the UUID of the parent to match. This would display only subvolumes originally created as snapshots of the specified subvolume (which may or may not still exist, of course). A null value ('' -- or a special text like 'NULL' or 'NONE' if you prefer) would create the search you were looking for: subvolumes with a null Parent UUID. The second option is more code, of course, but I see being able to list all the snapshots of a particular subvolume as quite useful. If you do choose the second option you need to decide what to do if the -P is specified more than once. Probably treat it as an error (unless you want to allow a list of UUIDs any of which can match). You might also want to reject an attempt to specify both -s and -P. Graham -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/30/17 18:37, Graham Cobb wrote: > On 30/09/17 14:08, Holger Hoffstätte wrote: >> A "root" subvolume is identified by a null parent UUID, so adding a new >> subvolume filter and flag -P ("Parent") does the trick. > > I don't like the naming. The flag you are proposing is really nothing to > do with whether a subvolume is a parent or not: it is about whether it > is a snapshot or not (many subvolumes are both snapshots and also > parents of other snapshots, and many non-snapshots are not the parent of > any subvolumes). You're completely right. I wrote that patch about a year ago because I needed it for my own homegrown backup program and spent approx. 5 seconds finding a free option letter; ultimately I ended up using the mentioned shell hackery as alternative. Anyway, I was sure that at the time the other letters sounded even worse/were taken, but that may just have been in my head. ;-) I just rechecked and -S is still available, so that's good. > I have two suggestions: > > 1) Use -S (meaning "not a snapshot", the inverse of -s). Along with this > change. I would change the usage text to say something like: > > -s list subvolumes originally created as snapshots > -S list subvolumes originally created not as snapshots > > Presumably specifying both -s and -S should be an error. That sounds much better indeed and is a quick fix. While I agree that the "-P <uuid>/none" filter would be useful too, it's also a different feature and more work than I want to invest in this right now. Maybe later "-S" can simply delegate to "-P none". cheers Holger -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/30/17 19:56, Holger Hoffstätte wrote: > shell hackery as alternative. Anyway, I was sure that at the time the > other letters sounded even worse/were taken, but that may just have been > in my head. ;-) > > I just rechecked and -S is still available, so that's good. Except that it isn't really, since there is already an 'S' case in cmds-subvolume.c as shortcut to --sort: -- case 'S': ret = btrfs_list_parse_sort_string(optarg, &comparer_set); -- ..which is why I picked P at the time. Holger -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 30, 2017 at 07:56:25PM +0200, Holger Hoffstätte wrote: > On 09/30/17 18:37, Graham Cobb wrote: > > On 30/09/17 14:08, Holger Hoffstätte wrote: > >> A "root" subvolume is identified by a null parent UUID, so adding a new > >> subvolume filter and flag -P ("Parent") does the trick. > > > > I don't like the naming. The flag you are proposing is really nothing to > > do with whether a subvolume is a parent or not: it is about whether it > > is a snapshot or not (many subvolumes are both snapshots and also > > parents of other snapshots, and many non-snapshots are not the parent of > > any subvolumes). > > You're completely right. I wrote that patch about a year ago because I > needed it for my own homegrown backup program and spent approx. 5 seconds > finding a free option letter; ultimately I ended up using the mentioned > shell hackery as alternative. Anyway, I was sure that at the time the > other letters sounded even worse/were taken, but that may just have been > in my head. ;-) > > I just rechecked and -S is still available, so that's good. Hi Holger, On the topic of shell hackery, someone on #btrfs and I discussed this two days ago. The method I decided to use was blacklisting snapshots (subvols with parent UUID) that also had to ro property set. The rational was that a rw snapshot (with parent UUID but not ro property) could contain live data. eg: subvol containerA, containerB, and containerC are all rw children of subvol containerX. My backup script would include containerA, containerB, and containerC in the automatically generated list of live data subvols that need to be snapshotted and backed up. Because containerX has no parents it is part of this list, regardless of whether or not it's ro. Would you please consider addressing rw child subvols (eg: containers cloned from a base subvol) in your patch? > > I have two suggestions: > > > > 1) Use -S (meaning "not a snapshot", the inverse of -s). Along with this > > change. I would change the usage text to say something like: > > > > -s list subvolumes originally created as snapshots > > -S list subvolumes originally created not as snapshots > > > > Presumably specifying both -s and -S should be an error. > > That sounds much better indeed and is a quick fix. While I agree > that the "-P <uuid>/none" filter would be useful too, it's also > a different feature and more work than I want to invest in this > right now. Maybe later "-S" can simply delegate to "-P none". Proposed -s will exclude both containerX and container{A,B,C} Proposed -S will add containerX (base subvolume) to list, and will exclude container{A,B,C} I believe the quickest way to solve this would be to add a check for ro property to: -s list read-only subvolumes originally created as snapshots Then in the backup script: 1) get the full subvolume list 2) get the ro snapshot list 3) Use #2 to remove ro snapshots from #1 4) Backup subvols from #1 list Thank you for working on this! :-) Sincerely, Nicholas P.S. Please CC me for any replies
On 30/09/17 19:17, Holger Hoffstätte wrote: > On 09/30/17 19:56, Holger Hoffstätte wrote: >> shell hackery as alternative. Anyway, I was sure that at the time the >> other letters sounded even worse/were taken, but that may just have been >> in my head. ;-) >> >> I just rechecked and -S is still available, so that's good. > > Except that it isn't really, since there is already an 'S' > case in cmds-subvolume.c as shortcut to --sort: That's a shame (and it is also a shame to waste a single letter option without documenting it!). I still would encourage you to avoid -P. I think there is user confusion by "parent" having more than one meaning even within btrfs. And I feel it also tends to perpetuate the mistaken belief that snapshots are somehow "special", and different from other subvolumes (rather than just a piece of information about how two subvolumes are related). It also allows -P to be used one day for the "search by parent UUID" feature. Given the constraints, I would suggest -n. It is mostly arbitrary but it is the second letter of snapshot and also the first of "not a snapshot". Thanks for considering. Graham -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017/10/01 3:17, Holger Hoffstätte wrote: > On 09/30/17 19:56, Holger Hoffstätte wrote: >> shell hackery as alternative. Anyway, I was sure that at the time the >> other letters sounded even worse/were taken, but that may just have been >> in my head. ;-) >> >> I just rechecked and -S is still available, so that's good. > > Except that it isn't really, since there is already an 'S' > case in cmds-subvolume.c as shortcut to --sort: > > -- > case 'S': > ret = btrfs_list_parse_sort_string(optarg, > &comparer_set); > -- > > ..which is why I picked P at the time. Hello, It is confusing, but the third args of getopt_long doesn't take 'S': 445 static const struct option long_options[] = { 446 {"sort", required_argument, NULL, 'S'}, 447 {NULL, 0, NULL, 0} 448 }; 449 450 c = getopt_long(argc, argv, 451 "acdgopqsurRG:C:t", long_options, NULL); So, you can use -S option if you change 'S' in L.446 to something. (I'm not sure this is intended or someone forgot to add 'S'.) > > Holger > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 30, 2017 at 03:08:13PM +0200, Holger Hoffstätte wrote: > Hi, > > When listing subvolumes it can be useful to filter out any snapshots; > surprisingly enough I couldn't find an option to do this easily, only > the opposite (list only snapshots). > > A "root" subvolume is identified by a null parent UUID, so adding a new > subvolume filter and flag -P ("Parent") does the trick. > > The same can of course be accomplished with shell hackery, e.g.: > > btrfs subvol list -q -o <path> | grep -i "parent_uuid -" | cut -d " " -f 11 > > but a built-in way seems less fragile. > > I originally cooked this up for myself, but David Sterba encouraged me to > send this to the list, so here it is. I'm not too proud of the -P but > couldn't find a better option letter; suggestions welcome. :) Thanks for sending it. Filtering by non-snapshots is indeed missing. I've applied the patch as is, but I suspect the option name will change before it ends up in a release. We have terminology problems (again), as 'subvolume' is commonly used for both plain subvolumes and also snapshots, but for the filtering we need to make the clear distinction. For the snapshot it's easy, the starting point of a snapshot is another subvolume or snapshot, ie. there's the parent UUID. Plain subvolume gets created by 'btrfs subvolume create'. While the 'parent' notion reflects that a snapshot originates from another subvolume, there's also directory structure that uses the term. And in combination with subvolumes it's not always true that a parent subvolume is also parent directory. So my idea is to use '-S' as for "subvolumes", and '-s' for "snapshots" by the strict defintion. The output of subvolume listing will get reworked, so I'd like to keep the final decision about the option naming open, until I get the whole picture and some working prototype with the libsmartcols. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/btrfs-list.c b/btrfs-list.c index 4cc2ed49..6aa7a290 100644 --- a/btrfs-list.c +++ b/btrfs-list.c @@ -1175,6 +1175,11 @@ static int filter_deleted(struct root_info *ri, u64 data) return ri->deleted; } +static int filter_parent_subvol_only(struct root_info *ri, u64 data) +{ + return uuid_is_null(ri->puuid); +} + static btrfs_list_filter_func all_filter_funcs[] = { [BTRFS_LIST_FILTER_ROOTID] = filter_by_rootid, [BTRFS_LIST_FILTER_SNAPSHOT_ONLY] = filter_snapshot, @@ -1189,6 +1194,7 @@ static btrfs_list_filter_func all_filter_funcs[] = { [BTRFS_LIST_FILTER_FULL_PATH] = filter_full_path, [BTRFS_LIST_FILTER_BY_PARENT] = filter_by_parent, [BTRFS_LIST_FILTER_DELETED] = filter_deleted, + [BTRFS_LIST_FILTER_PARENT_SUBVOL_ONLY] = filter_parent_subvol_only, }; struct btrfs_list_filter_set *btrfs_list_alloc_filter_set(void) diff --git a/btrfs-list.h b/btrfs-list.h index 13f44c3a..54aab123 100644 --- a/btrfs-list.h +++ b/btrfs-list.h @@ -142,6 +142,7 @@ enum btrfs_list_filter_enum { BTRFS_LIST_FILTER_FULL_PATH, BTRFS_LIST_FILTER_BY_PARENT, BTRFS_LIST_FILTER_DELETED, + BTRFS_LIST_FILTER_PARENT_SUBVOL_ONLY, BTRFS_LIST_FILTER_MAX, }; diff --git a/cmds-subvolume.c b/cmds-subvolume.c index e7ef67d3..2371338e 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c @@ -404,6 +404,7 @@ static const char * const cmd_subvol_list_usage[] = { "-q print the parent uuid of the snapshots", "-R print the uuid of the received snapshots", "-t print the result as a table", + "-P list parent subvolumes only", "-s list snapshots only in the filesystem", "-r list readonly subvolumes (including snapshots)", "-d list deleted subvolumes that are not yet cleaned", @@ -445,7 +446,7 @@ static int cmd_subvol_list(int argc, char **argv) }; c = getopt_long(argc, argv, - "acdgopqsurRG:C:t", long_options, NULL); + "acdgopPqsurRG:C:t", long_options, NULL); if (c < 0) break; @@ -473,6 +474,11 @@ static int cmd_subvol_list(int argc, char **argv) case 't': is_tab_result = 1; break; + case 'P': + btrfs_list_setup_filter(&filter_set, + BTRFS_LIST_FILTER_PARENT_SUBVOL_ONLY, + 0); + break; case 's': btrfs_list_setup_filter(&filter_set, BTRFS_LIST_FILTER_SNAPSHOT_ONLY,
Hi, When listing subvolumes it can be useful to filter out any snapshots; surprisingly enough I couldn't find an option to do this easily, only the opposite (list only snapshots). A "root" subvolume is identified by a null parent UUID, so adding a new subvolume filter and flag -P ("Parent") does the trick. The same can of course be accomplished with shell hackery, e.g.: btrfs subvol list -q -o <path> | grep -i "parent_uuid -" | cut -d " " -f 11 but a built-in way seems less fragile. I originally cooked this up for myself, but David Sterba encouraged me to send this to the list, so here it is. I'm not too proud of the -P but couldn't find a better option letter; suggestions welcome. :) cheers, Holger Signed-off-by: Holger Hoffstätte <holger@applied-asynchrony.com> --- btrfs-list.c | 6 ++++++ btrfs-list.h | 1 + cmds-subvolume.c | 8 +++++++- 3 files changed, 14 insertions(+), 1 deletion(-)