Fix receiving custom snapshot properties #5189

loli10K · 2016-09-28T19:49:21Z

We never initialize snapprops_nvlist in zfs_receive_one, so custom properties aren't applied to the received snapshot.

Should fix #4338, more info in comments.

behlendorf · 2016-09-30T03:55:51Z

@loli10K thanks for digging in to this and opening a pull request. I won't be able to look at this carefully until tomorrow but in the meanwhile it would be great if you could added a simple test case for sending user properties. To my surprise I didn't see one in tests/zfs-tests/tests/functional/cli_root/zfs_send/ directory. I'd suggest adding a zfs_send_008_pos.ksh to test this, you can model it on one of the existing test cases. You'll also want to make sure you add it to the linux.run file.

@dweeezil can you look at this. Your previous commit dropped this hunk e890dd8.

loli10K · 2016-10-01T09:42:49Z

@behlendorf the commit message is a typo of mine, the issue is actually on the receiving side: should i add a zfs_receive_XXX_pos.ksh instead?

EDIT: first take on the script pushed, can still be improved probably ... i'm not very good with test cases.

behlendorf · 2016-10-02T00:56:28Z

@loli10K adding it as a recv test as you've done is fine. Thanks for adding the test case.

loli10K · 2016-10-02T07:31:15Z

Adding new test cases revealed that the initial fix is not working correctly.

Without the first commit (initialize snapprops_nvlist in zfs_receive_one) ZFS is able to receive a full send stream and set its properties on the dataset, but fails to receive the snapshot properties.

With the "fix" commit ZFS is able to receive the snapshot properties but instead fails to receive the dataset properties with: cannot receive snap1 property on testpool: invalid property value.

Back to the debugger.

EDIT: patch works ok on 0.6.5, the problem is only present on 0.7.0

loli10K · 2016-10-04T15:59:39Z

Update: the problem with my initial patch is the incompatibility with the implementation of zfs_ioc_recv_new (43e52ed): we don't pass the list of properties to be received via a zfs_cmd_t anymore, we send a nvlist_t directly via the new function lzc_receive_one.

Workflow before 43e52ed:

get a new zfs_cmd_t zc
update it with the dataset properties: zcmd_write_src_nvlist(hdl, &zc, props)
lookup the snapshot properties: nvlist_lookup_nvlist(fs, "snapprops", &props). (this is the code i added and tested initially on my systems (0.6.5.X) but breaks 0.7.0)
receive the send stream: zfs_ioctl(hdl, ZFS_IOC_RECV, &zc)
update the snapshot properties only if snapprops_nvlist isn't NULL

Everything seems to work with the previous code. Now with the new zfs_ioc_recv_new implementation:

zfs_cmd_t zc is not needed anymore
we don't update it with: zcmd_write_src_nvlist(hdl, &zc, props)
lookup the snapshot properties: nvlist_lookup_nvlist(fs, "snapprops", &props). (this is the code i added and tested initially on my systems (0.6.5.X) but breaks 0.7.0)
receive the send stream: lzc_receive_one(.., props, ..., &prop_errors)
update the snapshot properties only if snapprops_nvlist isn't NULL

This patch won't let ZFS receive the dataset properties because it updates props directly with the value of the snapshot nvlist before lzc_receive_one(.., props, ..., &prop_errors). Indeed the error reported is cannot receive ${snapshotname} property on testpool: invalid property value. In the old code we could "save" the dataset properties to be received in a zfs_cmd_t before looking up the snapshot properties.

I just applied the same patch on the commit just before 43e52ed and it seems to work.

EDIT: i've squashed all the work in a single commit.

behlendorf

I'm not sure I follow. Your updated code should, and does, work with lzc_receive_one(). It's just that props is copied in to the kernel latter. Now that shouldn't be a problem but if it is we could always use nvlist_dup() to make a copy of it and then free it latter.

Also if you rebase this patch on the latest master version you should be able to eliminate some of the test failures.

behlendorf · 2016-10-04T20:05:45Z

lib/libzfs/libzfs_sendrecv.c

@@ -3122,6 +3122,11 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 			VERIFY(0 == nvlist_add_uint64(props,
 			    zfs_prop_to_name(ZFS_PROP_CANMOUNT), 0));
 		}
+		if (0 == nvlist_lookup_nvlist(fs, "snapprops",
+			&snapprops_nvlist)) {


It would make sense to introduce a local variable in this block simply to be used to lookup the snapprops nvlist. As was done with props in the original version of this function. That way snapprops_nvlist will only be non-NULL when snapname exists.

You're right about the non-NULL condition, i just thought that if the first nvlist_lookup_nvlist doesn't exit with errors and incorrectly sets snapprops_nvlist then we either find snapname in the second lookup (overwriting the nvlist with the correct value) or we fail the VERIFY().

behlendorf · 2016-10-04T20:06:54Z

tests/zfs-tests/tests/functional/cli_root/zfs_receive/zfs_receive_013_pos.ksh

+#
+
+#
+# Copyright 2016, [name of copyright owner]. All rights reserved.


You wrote the test case, feel free to add your name here. Or your companies name if that's more appropriate.

behlendorf · 2016-10-06T01:11:40Z

lib/libzfs/libzfs_sendrecv.c

@@ -3090,6 +3090,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 	uint64_t errflags = 0;
 	uint64_t parent_snapguid = 0;
 	prop_changelist_t *clp = NULL;
+	nvlist_t *lookup_nvlist = NULL;


I'd suggest moving this in to the if (stream_avl != NULL) block.

Done, and rebased on current master

behlendorf

Functionally this LGTM and I presume you're not seeing any problems with it when testing it on master?

Please refresh this to address the last two remaining style concerns and add a more descriptive comment. Then add your Signed-off-by to the commit message.

behlendorf · 2016-10-06T17:33:16Z

lib/libzfs/libzfs_sendrecv.c

@@ -3110,6 +3110,8 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 	    ENOENT);

 	if (stream_avl != NULL) {
+		nvlist_t *lookup_nvlist = NULL;
+


[style] No need for an empty line here.

behlendorf · 2016-10-06T17:37:15Z

lib/libzfs/libzfs_sendrecv.c

@@ -3125,6 +3127,11 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 			VERIFY(0 == nvlist_add_uint64(props,
 			    zfs_prop_to_name(ZFS_PROP_CANMOUNT), 0));
 		}
+		if (0 == nvlist_lookup_nvlist(fs, "snapprops",
+			&lookup_nvlist)) {


[style] Four space indent here for continued conditional.

+ if (0 == nvlist_lookup_nvlist(fs, "snapprops", + &lookup_nvlist)) {

Or better yet if you picked a shorter variable name this could all be done on one line. Since the scope of this variable is so small it doesn't need to be super descriptive.

if (0 == nvlist_lookup_nvlist(fs, "snapprops", &lookup)) {

The variable snapprops_nvlist was never initialized, so properties were not applied to the received snapshot. Additionally, add zfs_receive_013_pos.ksh script to ZFS test suite to exercise 'zfs receive' functionality for user properties. Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

loli10K · 2016-10-06T21:48:59Z

Sorry for all the trouble, i will try to keep these things in mind for the next PR.

EDIT: no, no problems, but i'm still running 0.6.X on my systems so this was not tested intensively.

behlendorf · 2016-10-07T17:05:26Z

@loli10K thanks for digging in to this issue.

loli10K changed the title ~~Fix sending custom snapshot properties~~ Fix receiving custom snapshot properties Sep 28, 2016

loli10K force-pushed the master branch from d16b85e to f4d487d Compare October 4, 2016 16:27

behlendorf requested changes Oct 4, 2016

View reviewed changes

loli10K force-pushed the master branch 4 times, most recently from 7105482 to d4e77bb Compare October 5, 2016 10:04

behlendorf requested changes Oct 6, 2016

View reviewed changes

loli10K force-pushed the master branch from d4e77bb to 1739eb9 Compare October 6, 2016 06:49

behlendorf requested changes Oct 6, 2016

View reviewed changes

loli10K force-pushed the master branch from 1739eb9 to 274a284 Compare October 6, 2016 21:46

behlendorf approved these changes Oct 7, 2016

View reviewed changes

behlendorf merged commit 48f783d into openzfs:master Oct 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix receiving custom snapshot properties #5189

Fix receiving custom snapshot properties #5189

loli10K commented Sep 28, 2016

behlendorf commented Sep 30, 2016

loli10K commented Oct 1, 2016 •

edited

Loading

behlendorf commented Oct 2, 2016

loli10K commented Oct 2, 2016 •

edited

Loading

loli10K commented Oct 4, 2016 •

edited

Loading

behlendorf left a comment

behlendorf Oct 4, 2016

loli10K Oct 4, 2016

behlendorf Oct 4, 2016

behlendorf Oct 6, 2016

loli10K Oct 6, 2016

behlendorf left a comment

behlendorf Oct 6, 2016

behlendorf Oct 6, 2016

loli10K commented Oct 6, 2016 •

edited

Loading

behlendorf commented Oct 7, 2016

Fix receiving custom snapshot properties #5189

Fix receiving custom snapshot properties #5189

Conversation

loli10K commented Sep 28, 2016

behlendorf commented Sep 30, 2016

loli10K commented Oct 1, 2016 • edited Loading

behlendorf commented Oct 2, 2016

loli10K commented Oct 2, 2016 • edited Loading

loli10K commented Oct 4, 2016 • edited Loading

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Oct 4, 2016

Choose a reason for hiding this comment

loli10K Oct 4, 2016

Choose a reason for hiding this comment

behlendorf Oct 4, 2016

Choose a reason for hiding this comment

behlendorf Oct 6, 2016

Choose a reason for hiding this comment

loli10K Oct 6, 2016

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Oct 6, 2016

Choose a reason for hiding this comment

behlendorf Oct 6, 2016

Choose a reason for hiding this comment

loli10K commented Oct 6, 2016 • edited Loading

behlendorf commented Oct 7, 2016

loli10K commented Oct 1, 2016 •

edited

Loading

loli10K commented Oct 2, 2016 •

edited

Loading

loli10K commented Oct 4, 2016 •

edited

Loading

loli10K commented Oct 6, 2016 •

edited

Loading