Allow zfs send to exclude datasets #13158

kithrup · 2022-02-27T00:29:57Z

Add support for a -exclude/-X option to zfs send to allow dataset hierarchies to be excluded.

Motivation and Context

Snapshots can be excluded using a channel program; however,
this can result in failures with 'zfs send -R'; this option allows them
to be excluded. In my case, I specifically want to exclude Time
Machine backup datasets, as they have more churn than they are
worth in most cases.

Description

Fortunately, this required a change only to cmd/zfs/zfs_main.c,
using the already-existing callback argument to zfs_send that
is currently unused.

How Has This Been Tested?

Did zfs send -R with and without exclusions, and used zfs recv -nv to see the list of datasets being transmitted. Verified that excluded datasets were not included.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain [Signed-off-by](
https://github.com/openzfs/zfs/blob/master/.github/CONTRIBUTING.md#signed-off-by

).

man/man8/zfs-send.8

behlendorf · 2022-02-28T19:48:51Z

cc: @pcd1193182 @problame @PaulZ-98 for review feedback.

cmd/zfs/zfs_main.c

behlendorf · 2022-02-28T23:14:43Z

cmd/zfs/zfs_main.c

+struct zfs_send_exclude_arg {
+	size_t count;
+	char **list;
+};


Let's define and use a typedef as is done elsewhere.

typedef struct zfs_send_exclude_arg { } zfs_send_exclude_arg_t;

cmd/zfs/zfs_main.c

behlendorf · 2022-02-28T23:21:57Z

cmd/zfs/zfs_main.c

@@ -4533,6 +4594,8 @@ zfs_do_send(int argc, char **argv)
 	if (flags.parsable && flags.verbosity == 0)
 		flags.verbosity = 1;

+	/* Should we warn about no -R but an exclusion list? */


It does seem like this should be a fatal error. Offhand I can't think of a case where it would make sense to specify -x but not -R.

Really? Ok. Do I need to do i18n myself, or is the call to gettext() enough? (The commit is coming in a couple of minutes.)

I could really go either way on this, but unless somebody has a use case where this would actually make sense I'd lean towards generating an error so the user at least knows they're using it wrong. Using gettext() is plenty.

cmd/zfs/zfs_main.c

behlendorf · 2022-02-28T23:26:10Z

To make sure we don't accidentally break this functionality in the future I think we're going to need a basic functional test added to tests/zfs-tests/tests/functional/rsend/.

kithrup · 2022-02-28T23:40:44Z

To make sure we don't accidentally break this functionality in the future I think we're going to need a basic functional test added to tests/zfs-tests/tests/functional/rsend/.

Ok. I'll go poke at right now.

problame

Took a quick look.

cmd/zfs/zfs_main.c

problame · 2022-03-01T11:52:16Z

tests/zfs-tests/tests/functional/rsend/rsend_025_pos.ksh

+
+log_must_busy zfs destroy -rf $POOL2@presend
+
+log_must eval "zfs send -R -X ds1/second > $BACKDIR/presend"


We need more (separate!) tests that assert multiple -X works.

Well, I wrote the one because it was requested, but because I couldn't get it to work locally I didn't write more. Since it does seem to have worked (right?), that gives me a template for adding a couple more...

Ok. The tests all fail for me. However, it looks like they fail for everyone (based on a couple of open pull requests I just checked), so I don't think it's me. That said: I don't know if the test I created actually works! So I'm not sure what to do here.

Well, there is a continuous wrestling match between tests being flaky and whack a mole, but every test bot failing is generally still a sign of a problem.

If you look at e.g. the Centos 7 bot, the only test that failed is the one you added, and if you look at the detailed output:

Test: /usr/share/zfs/zfs-tests/tests/functional/rsend/rsend_025_pos (run as root) [00:03] [FAIL] 22:06:58.12 ASSERTION: zfs send -R -X will skip excluded dataset(s) 22:06:58.12 SUCCESS: rm -rf /mnt/backdir-rsend/* 22:06:58.14 SUCCESS: zfs destroy -Rf testpool2 22:06:58.15 SUCCESS: rm -rf /mnt/backdir-rsend/* 22:06:58.48 SUCCESS: zfs destroy -Rf testpool 22:06:58.56 SUCCESS: zfs create -p testpool/testfs/fs1/fs2 22:06:58.58 SUCCESS: zfs snapshot testpool@psnap 22:06:58.60 SUCCESS: zfs clone testpool@psnap testpool/pclone 22:06:58.64 SUCCESS: zfs create -V 16M testpool/vol 22:06:58.66 SUCCESS: zfs create -V 16M testpool/testfs/vol 22:06:58.85 SUCCESS: zfs snapshot testpool/testfs/vol@vsnap 22:06:58.88 SUCCESS: zfs clone testpool/testfs/vol@vsnap testpool/testfs/vclone 22:06:59.08 SUCCESS: snapshot_tree testpool/testfs/fs1/fs2@fsnap 22:06:59.10 SUCCESS: zfs clone testpool/testfs/fs1/fs2@fsnap testpool/testfs/fs1/fclone 22:06:59.13 SUCCESS: zfs snapshot -r testpool@init 22:06:59.17 SUCCESS: snapshot_tree testpool@snapA 22:06:59.20 SUCCESS: snapshot_tree testpool@snapC 22:06:59.31 SUCCESS: snapshot_tree testpool/pclone@snapB 22:06:59.33 SUCCESS: snapshot_tree testpool/testfs@snapB 22:06:59.41 SUCCESS: snapshot_tree testpool/testfs@snapC 22:06:59.44 SUCCESS: snapshot_tree testpool/testfs/fs1@snapA 22:06:59.51 SUCCESS: snapshot_tree testpool/testfs/fs1@snapB 22:06:59.55 SUCCESS: snapshot_tree testpool/testfs/fs1@snapC 22:06:59.58 SUCCESS: snapshot_tree testpool/testfs/fs1/fclone@snapA 22:06:59.60 SUCCESS: zfs snapshot testpool/vol@snapA 22:06:59.61 SUCCESS: zfs snapshot testpool/testfs/vol@snapB 22:06:59.63 SUCCESS: zfs snapshot testpool/testfs/vol@snapC 22:06:59.64 SUCCESS: zfs snapshot testpool/testfs/vclone@snapC 22:06:59.67 SUCCESS: zfs snapshot -r testpool@final 22:06:59.68 SUCCESS: setup_test_model testpool 22:06:59.70 SUCCESS: zfs create testpool/ds1 22:06:59.73 SUCCESS: zfs create testpool/ds1/second 22:06:59.76 SUCCESS: zfs create testpool/ds1/second/third 22:06:59.78 SUCCESS: zfs create testpool/ds2 22:06:59.81 SUCCESS: zfs create testpool/ds2/second 22:06:59.81 invalid option 'r' usage: create [-Pnpuv] [-o property=value] ... <filesystem> create [-Pnpsv] [-b blocksize] [-o property=value] ... -V <size> <volume> For the property list, run: zfs set|get For the delegated permission list, run: zfs allow|unallow 22:06:59.81 ERROR: zfs create snapshot -r testpool@presend exited 2

Thanks! I shall monitor the centos 7 one! As soon as my server comes back to life.

nabijaczleweli · 2022-03-01T15:59:26Z

cmd/zfs/zfs_main.c

+	for (tempString = strsep(&exclude, ",");
+	    tempString != NULL;
+	    tempString = strsep(&exclude, ",")) {


isn't this, uh, while((ts = strsep(&e, ",") != NULL)?

It started out as something else. I've got it changed but I don't want to do anything since I'm trying to run the tests.

nabijaczleweli · 2022-03-01T16:00:52Z

man/man8/zfs-send.8

@@ -40,6 +40,7 @@
 .Nm zfs
 .Cm send
 .Op Fl DLPRbcehnpsvw
+.Op Fl X Ar dataset Ns Oo , Ns Ar dataset Oc Ns ...


again,

.Op Fl X Ar dataset Ns Oo , Ns Ar dataset Oc Ns …

likewise below

If you are talking about the ellipses, I can't type those.

So paste them in? I hardly see what the issue is.

The issue is that that is a non-ASCII character.

And you're purporting this to be an issue because? Your computer hasn't exploded from the other ellipses.

Well, we have to have the initial commit at less than 80 characters, and there's a bunch of other restrictions requiring ASCII conformance.

Also: I don't use a GUI for programming, and my character set is ASCII. There are multiple instances of '...': zfs-allow.8, zed.8, zpool-scrub.8, zfs-program.8, cstyle.1, and zfsprops.7. And now zfs-send.8.

nabijaczleweli · 2022-03-03T21:02:22Z

cmd/zfs/zfs_main.c

+	const char *name;
+
+	name = fullname + strlen(poolname);
+	if (name && *name == '/')


redundant: name always holds

Grr that was supposed to have been changed. Thanks. grumble. (That's why I converted this to a draft.)

kithrup · 2022-03-04T19:44:03Z

Sorry it took so many tries to get the tests working. But back to ready for review please!

behlendorf · 2022-03-04T19:59:32Z

@kithrup if you rebase this branch on the latest master source and force update the PR that will resolve the unrelated FreeBSD CI failures.

kithrup · 2022-03-04T20:07:48Z

Done!

cmd/zfs/zfs_main.c

pcd1193182 · 2022-03-08T21:14:00Z

cmd/zfs/zfs_main.c

-	err = zfs_send(zhp, fromname, toname, &flags, STDOUT_FILENO, NULL, 0,
-	    flags.verbosity >= 3 ? &dbgnv : NULL);
+	err = zfs_send(zhp, fromname, toname, &flags, STDOUT_FILENO,
+	    exclude_context.count > 0 ? zfs_do_send_exclude : NULL,


It is interesting that this parameter exists even though we don't use it anywhere in the codebase as far as I can tell. I wonder why?

I know! I thought that too! I assume there was some use for it at some point in history?

kithrup · 2022-03-08T21:35:43Z

But here is a question: should I clarify the man page to note that the exclusion works on "/datasetname" but not "pool/datasetname" and not "datasetname"? Or should I change the code to assume the / is always there, and allow it to be "datasetname"? I hadn't done that because I thought anchoring it was the better way to go, but then I realized that I also ignore the pool name, so perhaps that is all confusing?

nabijaczleweli · 2022-03-08T21:43:52Z

cmd/zfs/zfs_main.c

+	size_t indx;
+	const char *name = strchrnul(fullname, '/');
+
+	if (exclude == NULL || *name == '\0')


This still swallows root dataset exclusions – you probably want to forbid those explicitly.

Error out if it doesn't have a "/" in it?

Error out in add_dataset_excludes() if !zfs_name_valid(tok, ZFS_TYPE_DATASET) || strchr(tok, '/') == NULL, yeah (this would require taking full dataset paths, of course, but that's more consistent and farms out the validation to libzfs, which is a net plus).

nabijaczleweli · 2022-03-08T21:46:55Z

I was def. confused by the lack of preceding pool name when testing – no other user-facing facility does this; I'd expect zfs send -RX 'tst/sub' tst@tosend to work, but only zfs send -RX '/sub' tst@tosend does (and not just sub) – this is a very weird interface, and to make it (a) least confusing and (b) most like everything else I'd just take full paths.

nabijaczleweli · 2022-03-08T21:48:55Z

cmd/zfs/zfs_main.c

 		{0, 0, 0, 0}
 	};

 	/* check options */
-	while ((c = getopt_long(argc, argv, ":i:I:RsDpvnPLeht:cwbd:S",
+	while ((c = getopt_long(argc, argv, ":i:I:RsDpvnPLeht:cwbd:SX:",


GitHub doesn't let you comment on untouched lines, but: you also need to add [-X dataset[,dataset]...] to (a) the comment on top of do_zfs_send() and (b) the usage strings.

kithrup · 2022-03-08T21:51:30Z

Ok. So use the pool name, complain about only the pool name, update the man page to clarify all of this, and also change the usage description in zfs_main.c. Right? (Going to do so now.)

dataset hierarchies to be excluded. Fix some style issues. One of them is ugly: (sizeof (char **)) * (tempSize + 1)); has extra parentheses because the style checker decided it was both a cast and a sizeof operator and they have conflicting requirements. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Per feedback from nabijaczleweli, change the man pages slightly. (For which I am very grateful, I'm at least 5 years out of date on all the macros now.) Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Change to the suggested mdoc usage, for consistency. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Apply Brian's feedback. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Change -x back to -X. Attempt to write a test script. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Oops, remove debugging set -x Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Incorporate feedback Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Add my new test script to the tests Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Change a for to a while, per feedback Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Fix my broken test script. Hopefully, anyway. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Fix another typo Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Next attempt to get my new test working Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Left-over zfs create Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Fix bugs found by the test script! Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Add 2 more rsend tests Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Per feedback, checking for NULL is redundant Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Forgot to add my new tests to the roster Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Incorporate review feedback Signed-off-by: Sean Eric Fagan <kithrup@mac.com> grr missed a style check Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Replace the parsing code with the version nabijaczleweli wants. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Per feedback: 1. Update usage messages. 2. Change the exclusion to require the pool name. 3. Error if the exclusion name isn't a proper pool/dataset name. 4. Update the man page accordingly. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> More feedback. This mostly involved usage and man page changes. Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Fix my man page brokening, and add a couple more tests. Also fix the test to match the actual usage of the zfs send command. Signed-off-by: Sean Eric Fagan <kithrup@mac.com>

Signed-off-by: Sean Eric Fagan <kithrup@mac.com>

kithrup · 2022-03-10T05:33:05Z

I did a rebase on master, thus the force push.

behlendorf · 2022-03-15T22:25:46Z

@kithrup thanks for refreshing this, it looks like this PR is in good shape. It'd be great if folks could take one last look, and if there are no concerns let's get it merged.

Add support for a -exclude/-X option to `zfs send` to allow dataset hierarchies to be excluded. Snapshots can be excluded using a channel program; however, this can result in failures with 'zfs send -R'; this option allows them to be excluded. Fortunately, this required a change only to cmd/zfs/zfs_main.c, using the already-existing callback argument to zfs_send() that is currently unused. Reviewed-by: Paul Dagnelie <pcd@delphix.com> Reviewed-by: Christian Schwarz <christian.schwarz@nutanix.com> Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Sean Eric Fagan <kithrup@mac.com> Signed-off-by: Sean Eric Fagan <kithrup@mac.com> Closes openzfs#13158

kithrup force-pushed the sef/send-exclude branch from 87c22b4 to 1f4eccd Compare February 27, 2022 01:21

nabijaczleweli reviewed Feb 28, 2022

View reviewed changes

man/man8/zfs-send.8 Outdated Show resolved Hide resolved

nabijaczleweli reviewed Feb 28, 2022

View reviewed changes

man/man8/zfs-send.8 Outdated Show resolved Hide resolved

behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 28, 2022

kithrup force-pushed the sef/send-exclude branch from 192bd0e to 85c908e Compare February 28, 2022 23:17

behlendorf reviewed Feb 28, 2022

View reviewed changes

problame suggested changes Mar 1, 2022

View reviewed changes

nabijaczleweli reviewed Mar 1, 2022

View reviewed changes

kithrup marked this pull request as draft March 3, 2022 20:30

nabijaczleweli reviewed Mar 3, 2022

View reviewed changes

kithrup marked this pull request as ready for review March 4, 2022 19:43

kithrup force-pushed the sef/send-exclude branch from 6771370 to 918f7ee Compare March 4, 2022 20:07

kithrup requested a review from behlendorf March 7, 2022 16:08

ahrens requested a review from pcd1193182 March 8, 2022 16:43

nabijaczleweli reviewed Mar 8, 2022

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

nabijaczleweli reviewed Mar 8, 2022

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

nabijaczleweli reviewed Mar 8, 2022

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

pcd1193182 approved these changes Mar 8, 2022

View reviewed changes

nabijaczleweli reviewed Mar 8, 2022

View reviewed changes

Sean Eric Fagan and others added 2 commits March 9, 2022 21:06

Added file got lost

f62c0ed

Signed-off-by: Sean Eric Fagan <kithrup@mac.com>

kithrup force-pushed the sef/send-exclude branch from 34a04f3 to f62c0ed Compare March 10, 2022 05:07

behlendorf approved these changes Mar 11, 2022

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 11, 2022

kithrup requested review from pcd1193182 and behlendorf March 12, 2022 07:07

behlendorf approved these changes Mar 15, 2022

View reviewed changes

ahrens assigned behlendorf Mar 17, 2022

behlendorf merged commit 565089f into openzfs:master Mar 19, 2022


		log_must_busy zfs destroy -rf $POOL2@presend

		log_must eval "zfs send -R -X ds1/second > $BACKDIR/presend"

Allow zfs send to exclude datasets #13158

Allow zfs send to exclude datasets #13158

Conversation

kithrup commented Feb 27, 2022

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf commented Feb 28, 2022

Choose a reason for hiding this comment

behlendorf Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf commented Feb 28, 2022

kithrup commented Feb 28, 2022

problame left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kithrup commented Mar 4, 2022

behlendorf commented Mar 4, 2022

kithrup commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kithrup commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabijaczleweli commented Mar 8, 2022

nabijaczleweli Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

kithrup commented Mar 8, 2022

kithrup commented Mar 10, 2022

behlendorf commented Mar 15, 2022

behlendorf Feb 28, 2022 •

edited

Loading

nabijaczleweli Mar 8, 2022 •

edited

Loading