[PATCH] btrfs-progs: add option to only list parent subvolumes

[PATCH] btrfs-progs: add option to only list parent subvolumes

[Holger Hoffstätte]

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(-)

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,
-- 
2.14.2

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[Graham Cobb]

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

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[Holger Hoffstätte]

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

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[Holger Hoffstätte]

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

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[Graham Cobb]

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

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[Misono, Tomohiro]

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
> 

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[Nicholas D Steeves]

[-- Attachment #1: Type: text/plain, Size: 3122 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] btrfs-progs: add option to only list parent subvolumes

[David Sterba]

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.