Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for property overrides (WIP) #1867

Closed
wants to merge 1 commit into from

Conversation

behlendorf
Copy link
Contributor

Adds support for property overrides (-o property=value), property
excludes (-x property) and dataset limits (-l <volume|filesystem>)
to zfs receive.

Both -o and -x options mirror the functionality already available
in Oracle's ZFS implementation which is also mentioned in the
upstream feature request #2745:

The -l option allows receive to be limited to specific datasets within
the stream effectively allowing partial restores from a multi dataset
stream.

References:
https://www.illumos.org/issues/2745

Ported-by: Milan-Benes
Issue #1350

NOTES: Man page updates missing and must be added, see patch:
http://blog.multiplay.co.uk/dropzone/freebsd/zfs-recv-properties.patch

Adds support for property overrides (-o property=value), property
excludes (-x property) and dataset limits (-l <volume|filesystem>)
to zfs receive.

Both -o and -x options mirror the functionality already available
in Oracle's ZFS implementation which is also mentioned in the
upstream feature request openzfs#2745:

The -l option allows receive to be limited to specific datasets within
the stream effectively allowing partial restores from a multi dataset
stream.

References:
  https://www.illumos.org/issues/2745

Ported-by: Milan-Benes
Issue openzfs#1350

NOTES: Man page updates missing and must be added, see patch:
http://blog.multiplay.co.uk/dropzone/freebsd/zfs-recv-properties.patch
@ahrens
Copy link
Member

ahrens commented Jun 10, 2014

copy/paste from my review of this code on [email protected]:

I don't quite follow the documentation around the "-o" and "-x" flags:

  • if I specify "-o prop=value", it says it's as though I did "zfs set prop=value". So the property is marked as a "locally set" property, as opposed to a "received" property, right?
  • If I specify "-o prop=value", and it is an inheritable property and I am receiving a "-R" stream, is the behavior:
    • A. set the property on every filesystem (this seems bad)
    • B. set the property on the topmost filesystem that's part of the stream (descendants will inherit it or not depending on what their local settings are)
    • C. set the property on the topmost filesystem, and "force inherit" it onto all descendants (doing the equivalent of a "zfs inherit" on each of the descendants)
    • B or C seem OK, but you should document the behavior. Unfortunately, it looks like A is what was implemented.
  • if I specify "-x prop", it says it's as though the property was not present in the send stream. Given that, I don't understand the following sentences "If a received property needs to be overridden, the effective value will be set or inherited, depending on if the property is inheritable or not. In the case of an incremental update, -x leaves any existing local setting or explicit inheritance unchanged (since the received property is already overridden)." Why would a received property need to be overridden? The latter sentence implies that you're actually setting the property as a received property, and then overriding it with a "local inherit". Which is different from what would happen if the property was not present in the send stream. It sounds like it might be that "-x prop" for inheritable properties actually does the equivalent of "zfs inherit prop <every fs that's received>", unless there is already a local setting for that fs in which case it has no effect. It looks like what's implemented is that -x causes us to ignore that prop if it is in the stream. Please change the documentation to match.

I just read the Oracle documentation. It sounds like they implemented it so that "-o prop=val" implements the option C that I described: set the property on the topmost filesystem, and "force inherit" it onto all descendants (doing the equivalent of a "zfs inherit" on each of the descendants). And I am guessing that they made "-x" do the equivalent of "zfs inherit" after receiving the property. These semantics seems reasonable, and I think we should implement the same behavior. I'm also fine with dropping the "dataset#prop" stuff.

If we are to take the changes as-is, I would like to see a strong argument for why we should implement a feature that on the surface appears to be the same as an Oracle ZFS feature, but is actually subtly different.

Really what I would like to see is a comparison of the behavior of these changes and Oracle ZFS. It should cover things like:

Incremental receive, where the property is set on child filesystems; does -o prop=value set the property on all the filesystems? Does it set the property on the top filesystem that is received and the children inherit that prop?

For example, if you do "zfs send -R test/ds@a | zfs recv -o sharenfs=rw test/recvd", I think that Oracle ZFS would result in the following:

# zfs get -o all -r sharenfs test
NAME                PROPERTY  VALUE     RECEIVED  SOURCE
test                sharenfs  off       -         default
test/ds             sharenfs  on        -         local
test/ds@0           sharenfs  -         -         -
test/ds/child       sharenfs  off       -         local
test/ds/child@0     sharenfs  -         -         -
test/recvd          sharenfs  rw        on        local
test/recvd@0        sharenfs  -         -         -
test/recvd/child    sharenfs  rw        off       inherited from test/recvd
test/recvd/child@0  sharenfs  -         -         -

I.e. the result of doing "zfs set sharenfs=rw test/recvd; zfs inherit sharenfs test/recvd/child". I don't see how the code under review would accomplish this. It seems like it would do "zfs set sharenfs=rw test/recvd; zfs set sharenfs=rw test/recvd/child".

@FransUrbo
Copy link
Contributor

Initial man page for this can be found in FransUrbo@8b7dfd3.

Formatting might be somewhat off though. There's a few things in the patch mentioned I don't quite understand:

+.Ar property Ns @ Ns Op Ar volume Ns | Ns Ar filesystem

@FransUrbo
Copy link
Contributor

In testing this, I noticed that you'd need to use the -p option to zfs send for this to "take affect"...

@behlendorf
Copy link
Contributor Author

Closing as stale.

@behlendorf behlendorf closed this Dec 4, 2015
@FransUrbo
Copy link
Contributor

The patch work quite nicely though! I've been running it for a very long time, so closing it might be premature...

@behlendorf
Copy link
Contributor Author

The idea here is definitely a good one, but these changes here need to be reviewed and rebased by someone. Ideally, we'd want to coordinate with upstream illumos, see https://www.illumos.org/issues/2745.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Feature request or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants