Add support for property overrides (-o|-x) in zfs receive #5349
Conversation
|
@loli10K, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pcd1193182, @ahrens and @danmcd to be potential reviewers. |
There was a problem hiding this comment.
Thank you for picking up where the previous implementation left off. I suspect that we're going to need to have a few more discussion and do some coordination between the proposed behavior here and the behavior that the rest of the OpenZFS community feels would be best. I've left some inline comments in addition to responses to some of your high level questions:
- The man page wording is alright, except in the way that I noted below.
- Linux doesn't have zones, so I don't think the zone property is important for you right now, though that question would probably be best answered by a more experienced linux developer.
- That should be checked in libzfs. The kernel should also behave sanely in the event that such properties are passed in (probably by returning failure); I don't know how it would react today.
- Personal preferences aside, consistency across platforms is important. The decision as to whether it inherits should probably be based on a discussion with individuals from all the OpenZFS platforms. I know @ahrens had some feelings on that, based on the previous ZoL patch commentary.
Overall, I think that the patch as it stands now is going to be very confusing to end users; if they try to pass -o to a zfs receive of a send stream that isn't compound, it will silently ignore all their -os and -xs, except for -o origin, and the man page says nothing about that. The only way for them to figure out what is happening and why would be to read the source. That's in addition to my notes about the behavior of inheritance above.
lib/libzfs/libzfs_sendrecv.c
Outdated
| @@ -2725,10 +2725,137 @@ recv_incremental_replication(libzfs_handle_t *hdl, const char *tofs, | |||
| return (needagain); | |||
| } | |||
|
|
|||
| /* | |||
| * Update stream_avl based on override properties: | |||
| * 1. remove properties from "props" e "snapprops" nvlists if they are present | |||
lib/libzfs/libzfs_sendrecv.c
Outdated
| if (avl_is_empty(stream_avl) || nvlist_empty(props)) | ||
| return (0); /* No properties to override */ | ||
|
|
||
| VERIFY(nvlist_alloc(&noprops, NV_UNIQUE_NAME, 0) == 0); |
There was a problem hiding this comment.
You should use fnvlist_alloc() here.
lib/libzfs/libzfs_sendrecv.c
Outdated
| props_override(libzfs_handle_t *hdl, avl_tree_t *stream_avl, nvlist_t *props, | ||
| recvflags_t *flags, const char *errbuf) | ||
| { | ||
| nvlist_t *noprops; |
There was a problem hiding this comment.
Why is this called noprops? It appears to be storing a list of properties to use to override. override_props would seem like a better name?
There was a problem hiding this comment.
New Override Props ... override_props would definitely be better but i started using abbreviated variable names because keeping all these nested loop (to deal with nested nvlist) in 80 cols was becoming a real challange ...
There was a problem hiding this comment.
The nesting can be reduced by doing if (nvlist not present) continue; <handle nvlist> rather than if (nvlist present) {<handle nvlist>}. In any case, this particular variable doesn't appear deep in the nesting, so it can afford to have a longer name. I understand the desire to avoid overly-verbose names, though; this is the only one whose name I really have issue with.
lib/libzfs/libzfs_sendrecv.c
Outdated
| * added (-o) properties to be later processed by zfs_valid_proplist(). | ||
| */ | ||
| nvp = nvlist_next_nvpair(props, NULL); | ||
| while (nvp != NULL) { |
There was a problem hiding this comment.
I personally prefer a for loop for iterating over nvlists, but that's not a big deal.
lib/libzfs/libzfs_sendrecv.c
Outdated
| (fsn = avl_walk(stream_avl, fsn, AVL_AFTER))) { | ||
| nvlist_t *nvl = NULL; | ||
|
|
||
| if (0 == nvlist_lookup_nvlist(fsn->fn_nvfs, |
There was a problem hiding this comment.
I general, use fnvlist functions whenever possible.
| @@ -3755,7 +3887,8 @@ zfs_receive_impl(libzfs_handle_t *hdl, const char *tosnap, | |||
| assert(DMU_GET_STREAM_HDRTYPE(drrb->drr_versioninfo) == | |||
| DMU_COMPOUNDSTREAM); | |||
| return (zfs_receive_package(hdl, infd, tosnap, flags, &drr, | |||
There was a problem hiding this comment.
So it looks like we don't use these properties in the case of a normal zfs send streams, we only use them for zfs send streams created with -R, -I, and -p? That seems like an unfortunate restriction, to me. At the very least, the man page should explicitly state that these arguments will be ignored in this case (except for -o origin, which will continue to work), and the receive should fail with a message that setting properties for non-compound send streams is invalid (ideally in more user-friendly and understandable words).
There was a problem hiding this comment.
This was not intentional and i will try to fix it: it's my first time reading zfs_receive_*() functions and it was kind of confusing trying to understand the code path of a zfs recv ... sorry about that.
There was a problem hiding this comment.
A lot of the libzfs_sendrecv code is hard to trace. Most of it is because of the way that -R and -I were implemented, and the "compound stream" idea. If you modify zfs_receive_one() to take a props nvlist, and combine that with the property nvlist handling code already present, that should work.
|
@pcd1193182 i will try to get my hands on (somewhat new) a Solaris box to study its |
|
I managed to get ahold of a Solaris 11.3 iso just to discover it behaves in a very different way: the idea was to implement this feature changing the contents of the source stream as we read it and override (-o) or remove (-x) properties just before we actually receive them. What the Solaris implementation seems to do is to always (even with -o|-x) receive the original properties from them source stream and then apply custom properties locally: this way we get to keep both original (source=received) and override/excluded (source=local/default) properties. This is probably going to require much more logic/code (maybe also in |
|
@loli10K Sounds like the Solaris way is similar to:
I think the semantics of that are nicer than the current implementation. |
I think that too, i'm going to try to fix this properly but i'll take some time: i started this while on vacation when i had a lot of spare time but now i'm back to a full time job and can only work in the evening/weekends. Also i'm struggling to understand some of the code i've never used (basically most of it) so i'm doing some studying before rewriting this implementation. For instance, what is |
Great, thanks for your continued work on this!
This is used to pass the pool config (which specifies vdev configuration) between userland and kernel. This is typically used for
The new ioctl method uses only a small subset of the fields in the
If you are modifying an existing ioctl (e.g. |
I tend to agree.
Thanks! It's greatly appreciated.
For backwards compatibility reasons a new style |
|
I'm sorry to bother you all (again) but i could really use some help: the way i see it this has to be done in That said, i have some questions:
|
|
On Mon, Nov 21, 2016 at 5:54 AM, LOLi notifications@github.com wrote:
--matt |
When we are processing properties in
Yes
I was going to expand the existing
Sorry, i don't want to cause problems for the project ... |
Right. I think that libzfs knows that. I think that the kernel should not have a concept of receiving top vs child datasets. Instead, userland should express to the kernel what properties should be set/inherited locally/received. I would suggest that the user->kernel interface be: Receive this send stream, and set these properties locally, and set these other properties as received. ("set" here also encompasses the functionality of "inherit", which IIRC is implemented as "set to empty value"). So when libzfs is processing the top-level dataset, it would make the ioctl passing the CLI args as "set these properties locally". And when libzfs is processing children datasets, it would make the ioctl passing the CLI args as "inherit (set to empty value) these properties locally". |
Thanks @ahrens , i was probably on the wrong track trying to do the heavy lifting from the kernel, i will try to implement it this way. |
lib/libzfs_core/libzfs_core.c
Outdated
| @@ -613,6 +613,9 @@ recv_impl(const char *snapname, nvlist_t *props, const char *origin, | |||
| if (props != NULL) | |||
| fnvlist_add_nvlist(innvl, "props", props); | |||
|
|
|||
| if (cmdprops != NULL) | |||
| fnvlist_add_nvlist(innvl, "cmdprops", cmdprops); | |||
There was a problem hiding this comment.
A better name for this might be localprops, i.e. properties to set locally.
lib/libzfs_core/libzfs_core.c
Outdated
| @@ -613,6 +613,9 @@ recv_impl(const char *snapname, nvlist_t *props, const char *origin, | |||
| if (props != NULL) | |||
| fnvlist_add_nvlist(innvl, "props", props); | |||
There was a problem hiding this comment.
You might rename the argument to recvdprops to distinguish it from localprops. (Probably not worth trying to change the nvlist key though, for backwards-compatibility with older kernels.)
lib/libzfs_core/libzfs_core.c
Outdated
| packed = fnvlist_pack(props, &size); | ||
| allprops = fnvlist_alloc(); | ||
| if (props != NULL) | ||
| fnvlist_add_nvlist(allprops, "props", props); |
There was a problem hiding this comment.
Well, I guess this is going to confuse older kernels anyway... @behlendorf can weigh in on the right strategy here.
man/man8/zfs.8
Outdated
| .ad | ||
| .sp .6 | ||
| .RS 4n | ||
| Creates a snapshot whose contents are as specified in the stream provided on standard input. If a full stream is received, then a new file system is created as well. Streams are created using the \fBzfs send\fR subcommand, which by default creates a full stream. \fBzfs recv\fR can be used as an alias for \fBzfs receive\fR. | ||
| .sp | ||
| If an incremental stream is received, then the destination file system must already exist, and its most recent snapshot must match the incremental stream's source. For \fBzvols\fR, the destination device link is destroyed and recreated, which means the \fBzvol\fR cannot be accessed during the \fBreceive\fR operation. | ||
| .sp | ||
| When a snapshot replication package stream that is generated by using the \fBzfs send\fR \fB-R\fR command is received, any snapshots that do not exist on the sending location are destroyed by using the \fBzfs destroy\fR \fB-d\fR command. | ||
| When a snapshot replication package stream that is generated by using the \fBzfs send\fR \fB-R\fR command is received, any snapshots that do not exist on the sending location are destroyed by using the \fBzfs destroy\fR \fB-d\fR command. If \fB-o\fR \fIproperty\fR=\fIvalue\fR or \fB-x\fR \fIproperty\fR is specified, it applies to the effective value of the property throughout the entire subtree of replicated datasets. Effective property values may be set or inherited, depending on the property and whether the dataset is the topmost in the replicated subtree. Received properties are retained in spite of being overridden and may be restored with \fBzfs inherit\fR \fB-rS\fR. |
There was a problem hiding this comment.
I would say just -S rather than -rS, since the -r is optional and may or may not apply depending on what they are trying to do.
There was a problem hiding this comment.
It probably makes sense for the new text to be in its own paragraph.
There was a problem hiding this comment.
I think it's implied (to experts) by "applies to the effective value throughout", but it might be good to explicitly state what will happen with -R streams: The property will be set (by -x) or inherited (by -o) on the topmost filesystem that is received. In descendant datasets, if the property is set by the send stream, we will override that by forcing the property to be inherited (from the topmost filesystem).
man/man8/zfs.8
Outdated
| .ad | ||
| .sp .6 | ||
| .RS 4n | ||
| Sets the specified property as if the command \fBzfs set\fR \fIproperty\fR=\fIvalue\fR was invoked at the same time the received dataset was created from the non-incremental send stream or updated from the incremental send stream. |
There was a problem hiding this comment.
You might add: When receiving a stream from zfs send -R, causes the property to be inherited by all descendant datasets, as through zfs inherit property was run on any descendant datasets that have this property set on the sending system.
man/man8/zfs.8
Outdated
| .sp | ||
| Any editable ZFS property can also be set at receive time. Set-once properties bound to the received data, such as \fBnormalization\fR and \fBcasesensitivity\fR, cannot be set at receive time even when the datasets are newly created by zfs receive. | ||
| .sp | ||
| Multiple \fB-o\fR options can be specified. An error results if the same property is specified in multiple \fB-o\fR or \fB-x\fR options. |
There was a problem hiding this comment.
How about: "The -o option may be specified multiple times, for different properties."
module/zfs/zfs_ioctl.c
Outdated
| zfs_ioc_recv_impl(char *tofs, char *tosnap, char *origin, | ||
| nvlist_t *props, boolean_t force, boolean_t resumable, int input_fd, | ||
| zfs_ioc_recv_impl(char *tofs, char *tosnap, char *origin, nvlist_t *recvprops, | ||
| nvlist_t *cmdprops, boolean_t force, boolean_t resumable, int input_fd, |
module/zfs/zfs_ioctl.c
Outdated
| // fnvlist_add_string(xprops, name, ""); | ||
| // fnvlist_add_boolean(xprops, name); | ||
| dsl_prop_inherit(tofs, name, | ||
| ZPROP_SRC_INHERITED); |
There was a problem hiding this comment.
Kind of unfortunate performance characteristics here, if there are a bunch of -x options, since this does a txg_wait_synced(0). But I suppose it's OK for now.
There was a problem hiding this comment.
Sorry, i know, i tried to use zfs_set_prop_nvlist(tofs, ZPROP_SRC_INHERITED, xprops, *errors) (commented out after the loop) but without much success so i opted for this "temporary" solution. The code, right now, just tries to mimic the correct inheritance behaviour. Additionally, not using zfs_set_prop_nvlist, we are also bypassing perms checks, so this will have to be reworked anyway.
| @@ -4285,8 +4306,8 @@ zfs_ioc_recv_impl(char *tofs, char *tosnap, char *origin, | |||
| /* | |||
| * On error, restore the original props. | |||
There was a problem hiding this comment.
Should we also be trying to restore the original localprops? Not sure...
There was a problem hiding this comment.
I think we should, i'm still kind of struggling to come up with a way to stash away all existing properties (recvd, local and inherited) in a sane way that would not involve too many different nvlists and loops on restore.
|
So while i was trying to figure out a way to restore properties on error i came across this strange ... behaviour: when i force an error in Script for the lazy (like myself) here: https://gist.github.com/loli10K/3828245287eb6405bfc661c17a458166. Unfortunately i cannot test this on Illumos because i don't know how to force an error in EDIT: the problem is in |
|
@zettabot go |
|
EDIT: too much content in a single post, moved outputs to external gists. More testing later, another inconsistency: on Solaris, when we receive a stream with no properties set, we wipe existing received properties: https://gist.github.com/loli10K/4feb4ffe1a33d9ebd13914d241e7240a Which is the "right" behaviour? I feel like we should keep existing properties if we don't clear them explicitly. This brings to an implementation issue with the current user->kernel interface used to pass "local" override properties: right now i'm using a
The problem is, when we want to keep locally set properties how do we convey that to the kernel? We can't force
|
FWIW, I thought that was the original design and behavior of This issue is separate from the new -o / -x code, right? |
Ok, i'm going to try and fix that too then. (EDIT2: fixed in master)
Yes, i reproduced it on ZoL zfs-0.7.0-rc2 (which was tagged before i even started working on this) and SmartOS 20160527T033529Z (which i don't even know how to build yet, so definitely not related to the new code). EDIT: Maybe broken in illumos/illumos-gate@3b2aab1? I can still reproduce the problem in smartos until 20130222T000747Z which is the newer version that seems to work as intended. Regarding the other issue, could you give me some guidance too? I don't want to implement this in a way that would not be accepted upstream because that ultimately won't benefit some users. To recap, we need to pass to the kernel both "inherited" and "excluded" properties at the same time (same nvlist) if we want to keep locally set properties on receive, so we can't use DATA_TYPE_BOOLEAN for both of them. We could:
|
Why do we need to pass excluded properties to the kernel? Per my previous recommendation:
More specifically, the nvlist passed to the kernel could contain: |
|
@ahrens consider the following example: How do we keep that AIUI the idea is to set:
Those nvlists are to tell the kernel to set received Also we can't set because that would wipe the locally set With this method we need to know beforehand, from usermode, that compression is locally set so it doesn't need a BOOLEAN entry in the nvlist to force its effective value. |
|
@loli10K I see, I thought that I think you're proposing that the ending on-disk state records:
I don't think that is how it should work. I think that the manpage changes describe the behavior we want:
In your example, there is a local setting ( If there was no local setting (it was implicitly inherited), then Essentially, |
Ok, so we need to decide whether to add a explicit local inherit (or not) based on existing local properties:
This force us to gather locally set properties before any receive to calculate if we need and explicit inherit. EDIT: which is basically what i was trying to say in my last sentence:
Setting a property source to 709 case ZPROP_SRC_INHERITED:
710 /*
711 * explicitly inherit
712 * - remove propname
713 * - set propname$inherit
714 */
715 err = zap_remove(mos, zapobj, propname, tx);
716 ASSERT(err == 0 || err == ENOENT);
717 if (version >= SPA_VERSION_RECVD_PROPS &&
718 dsl_prop_get_int_ds(ds, ZPROP_HAS_RECVD, &dummy) == 0) {
719 dummy = 0;
720 VERIFY0(zap_update(mos, zapobj, inheritstr,
721 8, 1, &dummy, tx));
722 }
723 break; |
|
@loli10K That's right. If we want to avoid gathering that info in userland to figure out whether we should do "set received prop and also set local inherit" vs "set received prop", we could move that logic into the kernel. Essentially add a mode where the kernel will "locally inherit this property if it is not already set locally". That mode could apply to all "locally inherit" properties that are part of the receive (since |
|
@ahrens the kernel is already going to gather existing props before the receive anyway since we need to restore them if anything goes wrong, it would probably be optimal to handle this logic into the kernel: we just need to convey that idea via the two EDIT: ended up implementing it as suggested, in userland. EDIT2: this consistently breaks the following test cases: Also we still need to restore locally set properties on a failed receive (i just checked on sol11.3 and they do). This is going to get messy. |
|
@ahrens could you please review this (again)? I know the github thingy still says "Changes requested" but i already implemented the last part (restore properties on error) along with a test case. I've also recently rebased on master, resolved all conflicts with the latest OpenZFS patches and renamed the test script to conform to the new (non-numeric) "standard" as suggested. I also ported the test case (along with some of the utility functions from the ZTS) in a self-contained shell script and run it on a solaris box. Everything passes except a couple of tests: it does seem that |
lib/libzfs/libzfs_sendrecv.c
Outdated
| if (prop == ZPROP_INVAL) { | ||
| if (zfs_prop_user(name) != B_TRUE) { | ||
| zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, | ||
| "cannot override invalid property '%s'"), |
There was a problem hiding this comment.
looks like this function will be called for both -x and -o meaning you should probably update this message to something like "cannot override or exclude invalid property '%s'"
lib/libzfs/libzfs_sendrecv.c
Outdated
| continue; | ||
|
|
||
| /* cannot override readonly and set-once properties */ | ||
| if (zfs_prop_readonly(prop) == B_TRUE) { |
There was a problem hiding this comment.
you could combine this if statement with the one bellow it and update the message here as well
|
If anyone still cares about this: i've rebased this PR on master to fix the build issues with mainline Linux, all tests seem to be passing now. Also manual inspection of the buildbot console shows that |
cmd/zfs/zfs_main.c
Outdated
| * Modifies the argument inplace. | ||
| */ | ||
| static int | ||
| parsepropname(nvlist_t *props, char *propname) |
There was a problem hiding this comment.
seems like this could return B_TRUE or B_FALSE, aka a boolean_t as it's a failed or not type of return code.
This will also allow you to do "if (parsepropname(p, p2))" - without checking the return value. Looks like you were trying to be consistent with parseprop() which arguably should have been doing the same thing
lib/libzfs/libzfs_sendrecv.c
Outdated
| int ret = 0; | ||
|
|
||
| if (nvlist_empty(cmdprops)) | ||
| return (0); /* No properties to override */ |
There was a problem hiding this comment.
comment should probably be "No properties to override or exclude"?
lib/libzfs/libzfs_sendrecv.c
Outdated
| if (nvlist_empty(cmdprops)) | ||
| return (0); /* No properties to override */ | ||
|
|
||
| *oxprops = fnvlist_alloc(); |
There was a problem hiding this comment.
where is this freed, especially when we return an error?
lib/libzfs/libzfs_sendrecv.c
Outdated
| toplevel = chopprefix[0] != '/'; | ||
| if (drrb->drr_type == DMU_OST_ZVOL) | ||
| type = ZFS_TYPE_VOLUME; | ||
| else |
There was a problem hiding this comment.
can we be a bit more defensive here?
I'm thinking something like
else if (drrb->drr_type == DMU_OST_ZFS) type = ZFS_TYPE_FILESYSTEM; else goto out;
or something simular?
lib/libzfs/libzfs_sendrecv.c
Outdated
| nvlist_free(props); | ||
| nvlist_free(rcvprops); | ||
|
|
||
| if (origprops != NULL) |
There was a problem hiding this comment.
this check for NULL is unnecessary as nvlist_free() does it internally
lib/libzfs/libzfs_sendrecv.c
Outdated
| { | ||
| nvpair_t *nvp; | ||
| int err = 0; | ||
| const char *name; |
There was a problem hiding this comment.
for consistency, you should either define prop and name variables outside the while or inside
lib/libzfs/libzfs_sendrecv.c
Outdated
| /* | ||
| * Check properties we were asked to override (both -o|-x) | ||
| */ | ||
| static int |
There was a problem hiding this comment.
seems like this can return a boolean_t
lib/libzfs/libzfs_sendrecv.c
Outdated
| if (zfs_prop_user(name) != B_TRUE) { | ||
| zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, | ||
| "invalid property '%s'"), name); | ||
| err = 1; |
There was a problem hiding this comment.
seems like you can simplify this function by doing return (1) here, or preferably a boolean, and avoiding err variable altogether.
man/man8/zfs.8
Outdated
| .sp | ||
| If a received property needs to be overridden, the effective value can be set or inherited, depending on the property. | ||
| .sp | ||
| In the case of an incremental update, \fB-x\fR leaves any existing local setting or explicit inheritance unchanged (since the received property is already overridden). |
There was a problem hiding this comment.
I would remove the part in parens, to me, it just adds confusion.
man/man8/zfs.8
Outdated
| .sp | ||
| In the case of an incremental update, \fB-x\fR leaves any existing local setting or explicit inheritance unchanged (since the received property is already overridden). | ||
| .sp | ||
| All \fB-o\fR restrictions apply equally to \fB-x\fR. |
There was a problem hiding this comment.
this is pretty ambiguous, I would add more info here
|
@alek-p thanks for taking the time to review this! I'll try to get those things fixed as requested except the man pages: i'm not good at explaining things, my contribution on that would only add more confusion. Sorry about that. Also, before you read the code in "zfs_ioctl.c", i can only say that i'm sorry for all the trouble and wish you good luck! |
|
@loli10K Thanks for all your work on this. I will do another review and hope to have that done next week. |
cmd/zfs/zfs_main.c
Outdated
| "<filesystem|volume|snapshot>\n" | ||
| "\treceive [-vnsFu] [[-o property=<value>] | " | ||
| "[-x property]] ... \n" | ||
| "\t [-d | -e] [-o origin=<snapshot>] <filesystem>\n" |
There was a problem hiding this comment.
I think we can remove [-o origin=<snapshot>] since that's covered by -o property=value (at least in the usage message -- we'll want to make a special note of being able to set origin in the manpage).
For -o property=<value>, I think it should be -o <property>=<value> since property is not a literal value.
With [[-o prop=val] | [-x prop]] ..., I think you're trying to indicate that for a given property, you can either use -o or -x, but not both. But I'm not sure this is really clear, and it may just be adding to the confusion. Consider changing to the simpler [-o prop=val] ... [-x prop] ...
lib/libzfs/libzfs_sendrecv.c
Outdated
| * cmdprops: input properties from command line | ||
| * origprops: original properties if the destination exists, NULL otherwise | ||
| * oxprops: output override (-o) and excluded (-x) properties | ||
| */ |
There was a problem hiding this comment.
How does the oxprops differ from cmdprops? They both have the properties that are excluded and overridden, right?
lib/libzfs/libzfs_sendrecv.c
Outdated
| * Prepare a new nvlist of properties that are to override (-o) or be excluded | ||
| * (-x) from the received dataset | ||
| * cmdprops: input properties from command line | ||
| * origprops: original properties if the destination exists, NULL otherwise |
There was a problem hiding this comment.
how about properties currently set on the target filesystem, if it exists (as opposed to the original properties in the send stream). Does this include both locally-set and received properties?
lib/libzfs/libzfs_sendrecv.c
Outdated
| /* second pass: process "-o" properties */ | ||
| nvp = NULL; | ||
| while ((nvp = nvlist_next_nvpair(voprops, nvp)) != NULL) { | ||
| fnvlist_add_nvpair(*oxprops, nvp); |
lib/libzfs/libzfs_sendrecv.c
Outdated
| switch (nvpair_type(nvp)) { | ||
| case DATA_TYPE_BOOLEAN: /* -x property */ | ||
| if (!nvlist_exists(origprops, name)) | ||
| fnvlist_add_nvpair(*oxprops, nvp); |
There was a problem hiding this comment.
I think origprops includes both received and locally-set properties. So if you do -x, and the property is a already set as a received property, then we ignore the -x, and allow the send stream to change the received property to the new value? The -x is supposed to ensure that the property setting from the stream does not take effect, right? Do we do that by doing a local explicit inherit, to override the received value?
There was a problem hiding this comment.
Sent properties should be still received and updated to the new value, even if -x is specified, but you're right we need to make sure they're also explicitly inherited. I don't think the current test case cover this, i'm updating that too.
module/zfs/zfs_ioctl.c
Outdated
| @@ -4356,6 +4472,7 @@ zfs_ioc_recv_impl(char *tofs, char *tosnap, char *origin, | |||
| * inputs: | |||
| * zc_name name of containing filesystem (unused) | |||
| * zc_nvlist_src{_size} nvlist of properties to apply | |||
| * zc_nvlist_conf{_size}nvlist of properties to override or exclude | |||
There was a problem hiding this comment.
can you describe this more completely? I think we interpret boolean entries differently than number/string.
lib/libzfs_core/libzfs_core.c
Outdated
| @@ -810,13 +819,13 @@ lzc_receive_with_header(const char *snapname, nvlist_t *props, | |||
| * The 'errors' nvlist contains an entry for each unapplied received | |||
| * property. Callers are responsible for freeing this nvlist. | |||
| */ | |||
| int lzc_receive_one(const char *snapname, nvlist_t *props, | |||
| int lzc_receive_one(const char *snapname, nvlist_t *props, nvlist_t *cmdprops, | |||
There was a problem hiding this comment.
We should document the cmdprops somewhere.
For backwards compatibility, consider leaving this function signature unchanged, and adding a new function e.g. lzc_receive_with_cmdprops. This isn't a strict requirement at this point, but it seems like the nice thing to do in cases where it isn't difficult.
| if (!zfs_prop_inheritable(prop)) | ||
| continue; | ||
| } else if (!zfs_prop_user(name)) | ||
| continue; |
There was a problem hiding this comment.
for cstyle, use braces on all if/else branches
So, we silently ignore -x of non-native, non-user properties, and also we ignore native, non-inheritable properties? What's the rationale for ignoring these vs. returning an error (and relying on userland to filter them out)?
How does -x <non-inheritable property> work? (e.g. -x quota)
There was a problem hiding this comment.
use braces on all if/else branches
Even on single line conditionals?
How does -x
<non-inheritable property>work?
I think we should deal with this in userland, if the user ask us to -x <non-inheritable property> we could pass this information to the kernel as -o <default-value-of-non-inheritable property> on both the root dataset and descendants: would this be acceptable?
There was a problem hiding this comment.
Even on single line conditionals?
No, just when one of the if/else branches has braces - basically either all or none of the if/else bodies should have braces.
A use case for -x quota might be that they have already set the quota locally, and they want to ignore whatever the sending system says about quotas. -o quota=X isn't always a good solution, because they could have a send -R stream, and want different quotas for different child filesystems. Even if there's only one filesystem, it would be annoying to have to specify the quota every time they receive.
I think a better semantic would be for -x quota (and other non-inheritable properties) to remove the property from the send stream. Thus for -x non-inheritable-property, we leave whatever value there already is.
There was a problem hiding this comment.
remove the property from the send stream
Isn't this the behaviour that came with the original patch from #1867 and that we tried to avoid to be as close as possible to the other implementation(s)?
Though i agree this seems a better, albeit different, behaviour and i'm ok with implementing it this way.
There was a problem hiding this comment.
The principle is that -x should cause the property value from the send stream to have no effect - which was not achieved for non-inheritable properties (and treating it as -o prop=default_value doesn't achieve it either).
Inheritable properties have the concept of a received vs locally-set version, which we want to honor (but #1867 did not). Non-inheritable properties (e.g. quota) have a single value, so there's really no choice about it.
| (void) zfs_set_prop_nvlist(tofs, ZPROP_SRC_LOCAL, | ||
| oprops, *errors); | ||
| (void) zfs_set_prop_nvlist(tofs, ZPROP_SRC_INHERITED, | ||
| xprops, *errors); |
There was a problem hiding this comment.
If we're just going to sort them into -x and -o in the kernel, we could instead have userland sort them, passing down 2 nvlists (perhaps as child nvlists). But feel free to take this as just a suggestion.
module/zfs/zfs_ioctl.c
Outdated
| @@ -4429,7 +4554,8 @@ zfs_ioc_recv(zfs_cmd_t *zc) | |||
| /* | |||
| * innvl: { | |||
| * "snapname" -> full name of the snapshot to create | |||
| * (optional) "props" -> properties to set (nvlist) | |||
| * (optional) "recvprops" -> received properties to set (nvlist) | |||
There was a problem hiding this comment.
this change seems inaccurate, it's still "props"
module/zfs/zfs_ioctl.c
Outdated
| @@ -4472,7 +4472,8 @@ zfs_ioc_recv_impl(char *tofs, char *tosnap, char *origin, nvlist_t *recvprops, | |||
| * inputs: | |||
| * zc_name name of containing filesystem (unused) | |||
| * zc_nvlist_src{_size} nvlist of properties to apply | |||
| * zc_nvlist_conf{_size}nvlist of properties to override or exclude | |||
| * zc_nvlist_conf{_size}nvlist of properties to exclude (DATA_TYPE_BOOLEAN) and | |||
There was a problem hiding this comment.
there should be some whitespace after {_size}
| typeset streamfile_repl1=/var/tmp/streamfile_repl1.$$ | ||
| typeset streamfile_repl2=/var/tmp/streamfile_repl2.$$ | ||
| typeset streamfile_repl3=/var/tmp/streamfile_repl3.$$ | ||
|
|
||
| function cleanup | ||
| { | ||
| log_must $RM $streamfile_full |
There was a problem hiding this comment.
When you merge with master you'll need to change the test script $RM->rm, etc.
| * the send stream contains, for instance, a child ZVOL and | ||
| * we're trying to receive it with "-o atime=on" | ||
| */ | ||
| if (!zfs_prop_valid_for_type(prop, type, B_FALSE) && |
There was a problem hiding this comment.
Shouldn't we continue; here if the property isn't valid for the type but it is recursive?
There was a problem hiding this comment.
Right, this seems to work but then we can see that, for instance, properties like "atime" can be received on ZVOLs: https://gist.github.com/loli10K/f129d5b7f6ef4b45954857e361a5a618.
I'm working to fix this and update the test case.
|
sorry I've been lagging on this. I'll make sure to finish up reviewing by Monday. |
This allows users to specify "-o property=value" to override and "-x property" to exclude properties when receiving a zfs send stream. Both native and user properties can be specified. This is useful when using zfs send/receive for periodic backup/replication because it lets users change properties such as canmount, mountpoint, or compression without modifying the source. References: https://www.illumos.org/issues/2745 https://www.illumos.org/issues/3753 Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
|
@loli10K thanks for pushing this one forward. This LGTM as well, are there any additional changes you still want to make? If not we can get this merged, the test failures were all unrelated and I've resubmitted those runs. |
|
@behlendorf the code should be good to go, i just wanted to port this to Illumos, post the patch on their issue tracker and try to get more testing and reviews but i've not yet had the chance to set up a fully working openindiana development box. I think we can merge this now and come back to it if/when we need to. |
|
@loli10K sounds good. Merged. |
|
Sorry. I know I'm a bit late to the party here. I was looking at the code a bit to see how it would interact with encryption and I noticed that |
|
@tcaputi my reasoning was that we don't need to delay those properties because if they were to prevent the receive operation from succeeding we could always apply them later with For instance, if the following fails (EDQUOT) we could To be honest i haven't read the new encryption code, can you clarify why |
|
In the encryption patch, The other relevant bit of background info is how raw (encrypted) receives work. The purpose of a raw receive is to allow the user to send their data encrypted exactly as it is on disk. This requires the dataset's master keys to be sent in the |
It's not entirely clear to me why this would be a problem, but again the real reason override properties are not delayed is because i did not see the need to: now that there's a real use case we can address it. |
|
@ptx0 I am aware of this. It's next on my list. |
Description
This allow users to specify "-o property=value" to override and "-x property" to exclude properties when receiving a stream, which is useful when using zfs send/receive for periodic backup/replication because it allows to change properties such as canmount, mountpoint, or compression without modifying the source dataset.
Motivation and Context
Fix #1350
Also
https://www.illumos.org/issues/2745
https://www.illumos.org/issues/3753
How Has This Been Tested?
Test case added to the ZTS
Types of changes
Checklist:
Signed-off-by.