Add 'zfs send --saved' flag #9007

tcaputi · 2019-07-08T17:34:54Z

This commit adds the --partial (-S) to the 'zfs send' command.
This flag allows a user to send a partially received dataset,
which can be useful when migrating a backup server to new
hardware. This flag is compatible with resumable receives, so
even if the partial transfer is interrupted, it can be resumed.
The flag does not require any user / kernel ABI changes or any
new feature flags in the send stream format.

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)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

tcaputi · 2019-07-08T17:57:10Z

correct. ill have more documentation soon, still fleshing it out.

tcaputi · 2019-07-10T18:40:33Z

This PR should now be ready for review

cmd/zfs/zfs_main.c

module/zfs/zfs_ioctl.c

module/zfs/dmu_send.c

ahrens · 2019-07-17T17:27:15Z

This flag is compatible with resumable receives

It seems like it would require a resumable receive. I.e. the workflow would be:

zfs receive -s pool/fs@snap
<interrupt receive>
zfs send --partial pool/fs

I think we could document this more clearly in the manpage. I.e. the --partial sends the partially resumed state associated with the specified filesystem. We should probably have a separate summary line for this, since I don't think you should be able to do zfs send --partial pool/fs@snap. I'm not sure that all of the other flags make sense either. Or maybe you can explain how this interacts with -R?

ahrens · 2019-07-17T17:30:40Z

I think that zfs send --partial needs to stop iteration at DS_FIELD_RESUME_OBJECT,OFFSET. Otherwise you're sending data past what is valid.

Somewhat similarly, I think we need to take into account DS_FIELD_RESUME_REDACT_BOOKMARK_SNAPS so that we generate a send stream that describes the same redaction operation as was happening originally.

I'm not sure about the other DS_FIELD_RESUME_* but it's possible we need (or want) to take them into account as well (LARGEBLOCK, EMBEDOK, COMPRESSOK, RAWOK), and not allow them to be specified on the command line or ioctl.

alek-p · 2019-07-18T04:12:23Z

@ahrens thanks for the review, I've updated the manpages to be clearer.
Could you elaborate on the sending data past what is valid, I'm not sure I understand that concern yet.

ahrens · 2019-07-18T16:08:05Z

@alek-p Let's say you are doing an incremental receive, and it is interrupted at some object/offset. Then you do zfs send --partial on it. That is going to send the data for all objects and offsets within the filesystem, including those past the object/offset that we interrupted the receive at. So when you receive the --partial stream, it will have WRITE records all the way to the end of the filesystem. The partial stream doesn't have an END record, so the receiving system will set the DS_FIELD_RESUME_OBJECT,OFFSET -- but it will set them to the end of the filesystem. So when you resume this send, it will have no data in it, just an END record. The end result is that you won't actually resume the original send where it left off.

tcaputi · 2019-08-14T20:43:29Z

I updated and rebased the code based on the feed back I got. This should be ready for another review.

behlendorf · 2019-08-21T21:41:06Z

@tcaputi it looks like the newly added test case may not be entirely reliable, see the CentOS 7 failure.

pcd1193182 · 2019-08-21T23:29:52Z

cmd/zfs/zfs_main.c

@@ -4357,10 +4380,40 @@ zfs_do_send(int argc, char **argv)
 			}
 		}

-		zhp = zfs_open(g_zfs, argv[0], ZFS_TYPE_DATASET);
+		/*


What if we implemented this logic and the logic to find the incremental source of the partial send in libzfs? That way other programs could take advantage of it, and we wouldn't have to do this logic here, in the cmd code. We could add a libzfs_partial_send function that takes argv[0] and figures out everything else, and then invokes the send api directly.

Hmm. I see your point here, but I'm a bit conflicted because I also want the interface to libzfs to be as consistent as possible. zfs_send_one() takes a zhp so having a separate interface that takes argv[0] seems strange. I think in an ideal world, zfs_main.c would not be opening zhp's at all, but I'm not sure this is the place to make that change. Thoughts @pcd1193182 ?

I see your point, but I think the value of making this partial send logic accessible to programs not using the command line is valuable enough to be worth being a little different here. That's just my opinion, though, but I understand if others feel strongly the other way.

module/zfs/dmu_send.c

tcaputi · 2019-09-24T21:11:37Z

@behlendorf @ahrens I have updated the implementation to use the existing resumable receive code. It should be ready for review again.

codecov · 2019-09-25T15:45:51Z

Codecov Report

Merging #9007 into master will increase coverage by <1%.
The diff coverage is 67%.

@@           Coverage Diff            @@
##           master    #9007    +/-   ##
========================================
+ Coverage      79%      80%   +<1%     
========================================
  Files         385      385            
  Lines      121492   121610   +118     
========================================
+ Hits        96511    96898   +387     
+ Misses      24981    24712   -269

Flag	Coverage Δ
#kernel	`80% <65%> (ø)`	⬆️
#user	`67% <41%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c24fa4b...97243ac. Read the comment docs.

tcaputi · 2019-11-19T21:13:54Z

@behlendorf this patch has been sitting here for a little while. I just rebased it. Assuming the tests come back ok, is there anything we can do to get it over the finish line?

behlendorf

Thanks for updating this. From my perspective it sounds like there's general agreement that this would be a useful feature.

One thing we haven't hashed out if the option should be renamed --saved instead of --partial as @ahrens suggested. For the reasons mentioned in that comment, I'd personally prefer we go with --saved, but I could be convinced otherwise. We'd want to update the code as well to match the new name which does make this a bit more work.

Additionally, there are still a handful of minor review comments to address. The only major one being resolving the documentation conflict. Once we wrap up that remaining work I think we're in good shape. The code itself functionally looks correct to me.

behlendorf · 2019-11-20T21:10:37Z

cmd/zfs/zfs_main.c

@@ -4190,11 +4191,12 @@ zfs_do_send(int argc, char **argv)
 		{"raw",		no_argument,		NULL, 'w'},
 		{"backup",	no_argument,		NULL, 'b'},
 		{"holds",	no_argument,		NULL, 'h'},
+		{"partial",	no_argument,		NULL, 'S'},


Initially I was similarly confused, I think switching the long opt to --saved would be clearer.

behlendorf · 2019-11-20T21:12:02Z

cmd/zfs/zfs_main.c

+		    flags.compress || flags.raw || redactbook != NULL) {
+			(void) fprintf(stderr,
+			    gettext("incompatible flags combined with partial "
+			    "send flag\n"));


nit: I'd suggest moving the gettext(... up to fit this on two lines.

behlendorf · 2019-11-20T21:12:12Z

cmd/zfs/zfs_main.c

+		if (strchr(argv[0], '@') != NULL) {
+			(void) fprintf(stderr,
+			    gettext("partial send cannot send full "
+			    "snapshots\n"));


behlendorf · 2019-11-20T21:26:59Z

lib/libzfs/libzfs_sendrecv.c

+				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
+				    "'%s' used in the initial send no "
+				    "longer exists"),
+				    toname);


nit: can be moved to the previous line

behlendorf · 2019-11-20T21:34:40Z

lib/libzfs/libzfs_sendrecv.c

+		goto out;
+
+	partial_nvl = zfs_send_resume_token_to_nvlist(hdl, token_buf);
+	if (partial_nvl == NULL) {


Similar to zfs_send_resume() I think this comment is worth adding.

/* * zfs_error_aux has already been set by * zfs_send_resume_token_to_nvlist */

behlendorf · 2019-11-20T21:35:06Z

lib/libzfs/libzfs_sendrecv.c

+
+	/*
+	 * If a resume token is provided we use the object and offset
+	 * from that instead of the default, which starts frm the


behlendorf · 2019-11-20T21:38:29Z

man/man8/zfs.8

@@ -40,6 +40,313 @@
 .Nm
 .Fl ?V
 .Nm
+<<<<<<< 8221bcf1e4efc40d92d1354de710048c70eb709b
+=======


Recently the man pages were split by subcommand, you'll want to update man/man8/zfs-send.8

tcaputi · 2019-11-22T18:49:43Z

@ahrens @behlendorf I have addressed your review comments. The feature is now called saved send in the code and documentation.

behlendorf

Thanks. The code looks good to me, and I verified it works as intended in my local testing.

tcaputi · 2019-12-09T13:23:07Z

@behlendorf Can we get this pushed through at some point?

ahrens · 2019-12-09T17:55:51Z

@tcaputi I have this on my to-do list to take another look at the code this week. Thanks for your patience!

problame

I took a look at the dmu_send code.
The handling of if (err == ENOENT) { when trying to hold the .../%recv filesystem deserves attention or an explanatory comment IMHO.

problame · 2019-12-14T14:13:45Z

module/zfs/dmu_send.c

+			char name[ZFS_MAX_DATASET_NAME_LEN + 6];
+
+			(void) snprintf(name, sizeof (name),
+			    "%s/%%recv", tosnap);


Should probably use recv_clone_name constant instead of %%recv (and compute the size of name dependent on recv_clone_name's length)

https://github.com/zfsonlinux/zfs/blob/caf9dd209fdcfccabc2f32b3f23c5386ccfb896c/module/zfs/dmu_recv.c#L66

I think we allocate ZFS_MAX_DATASET_NAME_LEN on the stack in several other places, but it looks like you could easily use kmem_asprintf() to allocate it on the heap here.

problame · 2019-12-14T14:35:29Z

module/zfs/dmu_send.c

+			err = dsl_dataset_own_force(dspp.dp, name, dsflags,
+			    FTAG, &dspp.to_ds);
+			if (err == ENOENT) {
+				err = dsl_dataset_own_force(dspp.dp, tosnap,


as far as I can tell, we reach the body of this if stmt iff savedok == true but there is no saved state (i.e. no .../%recv dataset). Is that correct?

If above is correct, the code that should execute after dsl_dataset_own_force(dspp.dp, tosnap,... should be identical to what happens if !savedok in line 2732 (outer else branch below).

problame · 2019-12-14T14:36:09Z

module/zfs/dmu_send.c

+				    dsflags, FTAG, &dspp.to_ds);
+			}
+
+			if (err == 0) {


if err = ENOENT above, we still take this branch. Is that correct? Because we don't execute the code in this branch after line 2732

Is DS_FIELD_RESUME_TOGUID even defined for the tosnap's (as opposed to the name dataset)?

@tcaputi can probably speak to this better, but my understanding is that when you do zfs send --saved pool/fs, there are 3 cases:

there is no saved state, give an error

there was an incremental receive on an existing fs in progress, therefore we have saved state on pool/fs/%recv

there was receive that created a new fs in progress (typically a full receive, but could be an incremental which creates a clone), therefore we have saved state on pool/fs (which has no snapshots or children)

I think this code is handling these correctly, but would appreciate your verification.

problame · 2019-12-14T14:38:04Z

module/zfs/dmu_send.c

+			}
+
+			if (err == 0) {
+				err = zap_lookup(dspp.dp->dp_meta_objset,


ahrens · 2019-12-16T04:18:39Z

cmd/zfs/zfs_main.c

@@ -306,7 +306,8 @@ get_usage(zfs_help_t idx)
 		    "<filesystem|volume|snapshot>\n"
 		    "\tsend [-DnPpvLec] [-i bookmark|snapshot] "
 		    "--redact <bookmark> <snapshot>\n"
-		    "\tsend [-nvPe] -t <receive_resume_token>\n"));
+		    "\tsend [-nvPe] -t <receive_resume_token>\n"
+		    "\tsend [-Pnv] -S filesystem\n"));


It might be more "discoverable" to document it as --saved here, rather than -S.

ahrens · 2019-12-16T04:28:51Z

cmd/zfs/zfs_main.c

+			(void) fprintf(stderr, gettext("saved send cannot "
+			    "send full snapshots\n"));


This error message could be a little confusing. How about something like, saved send: must specify the filesystem with partially-received state

ahrens · 2019-12-16T04:38:36Z

module/zfs/dmu_send.c

+			char name[ZFS_MAX_DATASET_NAME_LEN + 6];
+
+			(void) snprintf(name, sizeof (name),
+			    "%s/%%recv", tosnap);


I think we allocate ZFS_MAX_DATASET_NAME_LEN on the stack in several other places, but it looks like you could easily use kmem_asprintf() to allocate it on the heap here.

ahrens · 2019-12-16T04:40:01Z

module/zfs/dmu_send.c

+	 */
+	if (!ds->ds_is_snapshot) {
+		dsl_dataset_name(origds, dsname);
+		(void) strcat(dsname, "/%recv");


same constant "%recv" here. Also, I think dsname can be declared inside the if.

ahrens · 2019-12-16T04:47:33Z

module/zfs/dmu_send.c

+	/*
+	 * If this is a saved send we may actually be sending
+	 * from the %recv clone used for resuming.
+	 */
+	if (!ds->ds_is_snapshot) {


Should we be getting a flag to indicate that this is a saved send? Otherwise, it seems like we could be taking on %recv when they are actually estimating a send to the filesystem itself (which I think is allowed as long as it isn't mounted writeable).

This hasn't been allowed in the past. The old code just did:

if (!ds->ds_is_snapshot) return (SET_ERROR(EINVAL));

I spoke to Brian about this offline and we don't believe that this change should be needed. We came to the conclusion that the send estimate code shouldn't really have to care about whether this is a saved send or not. Let me know if you have any other thoughts.

tcaputi · 2020-01-08T19:32:29Z

@ahrens and @problame
I have addressed your comments. Let me know if the block comment added in dmu_send.c doesn't make everything clear.

module/zfs/dmu_send.c

This commit adds the --saved (-S) to the 'zfs send' command. This flag allows a user to send a partially received dataset, which can be useful when migrating a backup server to new hardware. This flag is compatible with resumable receives, so even if the saved send is interrupted, it can be resumed. The flag does not require any user / kernel ABI changes or any new feature flags in the send stream format. Signed-off-by: Tom Caputi <[email protected]>

tcaputi added the Status: Work in Progress Not yet ready for general review label Jul 8, 2019

tcaputi force-pushed the partial_send branch from 5f08963 to 4fc42f9 Compare July 10, 2019 18:37

tcaputi changed the title ~~Partial send / recv Proof of Concept~~ Add 'zfs send --partial' flag Jul 10, 2019

tcaputi removed the Status: Work in Progress Not yet ready for general review label Jul 10, 2019

tcaputi force-pushed the partial_send branch 2 times, most recently from f5845a0 to 8fcdf61 Compare July 10, 2019 20:17

behlendorf requested review from pcd1193182, behlendorf and ahrens July 10, 2019 22:51

tcaputi force-pushed the partial_send branch from 8fcdf61 to f55cea9 Compare July 11, 2019 18:49

tcaputi mentioned this pull request Jul 11, 2019

Ensure dsl_destroy_head() decrypts objsets #9021

Merged

12 tasks

PaulZ-98 reviewed Jul 17, 2019

View reviewed changes

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

ahrens added Type: Feature Feature request or new feature Status: Design Review Needed Architecture or design is under discussion labels Jul 17, 2019

ahrens reviewed Jul 17, 2019

View reviewed changes

module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved

module/zfs/dmu_send.c Outdated Show resolved Hide resolved

module/zfs/dmu_send.c Outdated Show resolved Hide resolved

alek-p force-pushed the partial_send branch from f55cea9 to cd8b0c4 Compare July 18, 2019 04:06

alek-p force-pushed the partial_send branch 4 times, most recently from ab09006 to 4c3d0f3 Compare July 19, 2019 04:40

tcaputi force-pushed the partial_send branch from 4c3d0f3 to 2a5885e Compare August 14, 2019 20:42

pcd1193182 reviewed Aug 21, 2019

View reviewed changes

tcaputi force-pushed the partial_send branch from 538875f to 3de7547 Compare September 24, 2019 21:10

tcaputi force-pushed the partial_send branch from 3de7547 to 5313887 Compare September 25, 2019 15:45

tcaputi force-pushed the partial_send branch from 5313887 to 52cf371 Compare November 19, 2019 21:12

behlendorf requested changes Nov 20, 2019

View reviewed changes

tcaputi force-pushed the partial_send branch from 52cf371 to 6b9642c Compare November 22, 2019 18:48

tcaputi changed the title ~~Add 'zfs send --partial' flag~~ Add 'zfs send --saved' flag Nov 22, 2019

tcaputi force-pushed the partial_send branch from 6b9642c to 1b3d0e1 Compare November 22, 2019 19:25

behlendorf approved these changes Nov 22, 2019

View reviewed changes

behlendorf requested review from pcd1193182 and ahrens November 22, 2019 22:50

problame reviewed Dec 14, 2019

View reviewed changes

ahrens reviewed Dec 16, 2019

View reviewed changes

tcaputi force-pushed the partial_send branch 2 times, most recently from c38e1fb to 2543042 Compare January 8, 2020 19:30

tcaputi force-pushed the partial_send branch from 2543042 to 76ac939 Compare January 8, 2020 20:48

ahrens reviewed Jan 8, 2020

View reviewed changes

module/zfs/dmu_send.c Show resolved Hide resolved

tcaputi force-pushed the partial_send branch from 76ac939 to 97243ac Compare January 8, 2020 22:05

ahrens approved these changes Jan 9, 2020

View reviewed changes

behlendorf approved these changes Jan 9, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 9, 2020

behlendorf merged commit ba0ba69 into openzfs:master Jan 10, 2020

		(void) fprintf(stderr, gettext("saved send cannot "
		"send full snapshots\n"));

Add 'zfs send --saved' flag #9007

Add 'zfs send --saved' flag #9007

Uh oh!

Conversation

tcaputi commented Jul 8, 2019 • edited by behlendorf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Checklist:

Uh oh!

tcaputi commented Jul 8, 2019

Uh oh!

tcaputi commented Jul 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahrens commented Jul 17, 2019

Uh oh!

ahrens commented Jul 17, 2019

Uh oh!

alek-p commented Jul 18, 2019

Uh oh!

ahrens commented Jul 18, 2019

Uh oh!

tcaputi commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

behlendorf commented Aug 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcaputi commented Sep 24, 2019

Uh oh!

codecov bot commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tcaputi commented Nov 19, 2019

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcaputi commented Nov 22, 2019

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

tcaputi commented Dec 9, 2019

Uh oh!

ahrens commented Dec 9, 2019

Uh oh!

problame left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcaputi commented Jul 8, 2019 •

edited by behlendorf

Loading

tcaputi commented Aug 14, 2019 •

edited

Loading

codecov bot commented Sep 25, 2019 •

edited

Loading