On Sat, Apr 5, 2014 at 4:22 PM, Matthew Ahrens <mahr...@delphix.com> wrote:
> 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*. > > Do you have a response to the above comment? 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. On Thu, Apr 17, 2014 at 11:33 AM, Alek Pinchuk <develo...@lists.illumos.org>wrote: > Hello, > > I've updated the webrev a bit based on some review comments. > > Please see the updated webrev: > http://<http://alek_p.bitbucket.org/webrevs/2745/> > alek_p.bitbucket.org > <http://alek_p.bitbucket.org/webrevs/2745/>/<http://alek_p.bitbucket.org/webrevs/2745/> > webrevs > <http://alek_p.bitbucket.org/webrevs/2745/>/2745/<http://alek_p.bitbucket.org/webrevs/2745/> > > HTML version of the modified manpage: > http://<http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> > alek_p.bitbucket.org<http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> > / > <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>webrevs<http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> > /2745/ > <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>zfs<http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> > .1m.html <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> > > > The biggest change from the earlier webrevs is that now the > add_unique_option() call doesn't return anything, and will ignore duplicate > props instead of failing. > Since the behavior is pretty obvious (the identical options are useless but harmless, just like if I specify any other option multiple times, e.g. -ee), how about not printing anything in this case? > Again, this is Steven's FreeBSD patch with some slight modifications. > > > -Alek > >> On Apr 8, 2014 2:03 PM, "Alek Pinchuk" <pinchuk.a...@gmail.com> wrote: >> >>> Hello Matt, >>> >>> Thanks for taking a look. See response inline: >>> >>> >>> On Sat, Apr 5, 2014 at 4:22 PM, Matthew Ahrens <mahr...@delphix.com>wrote: >>> >>>> 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. >>>> >>>> >>> In the interest of time I've dropped the "dataset#prop" stuff from the >>> patch. If someone needs this features in illumos please post a webrev that >>> adds it. >>> >>> >>> >>>> --matt >>>> >>>> >>>> On Sat, Apr 5, 2014 at 4:14 PM, Matthew Ahrens <mahr...@delphix.com>wrote: >>>> >>>>> Alek, thanks a lot for picking up these changes. I have a few >>>>> comments / questions: >>>>> >>>>> ndmpd_zfs.c and be_create.c have Nexenta copyrights added to them. I >>>>> just wanted to confirm that these changes were contributed by Nexenta. >>>>> It's fine if so, it just stood out compared to the other files which have >>>>> no copyright added (which is also fine). >>>>> >>>> >>> Yes the changes in those two files are Nexenta sponsored work. I've made >>> them while "on the clock". >>> >>> >>>> >>>>> zfs.1m: >>>>> It looks like you've copied the documentation from the Oracle >>>>> manpages. Are you sure that we are allowed to do that? I.e. what license >>>>> grants us the right to reuse Oracle's documentation? >>>>> >>>>> >>> I just ported what was in the FreeBSD patch. The manpage part was >>> changing the FreeBSD markup to use the illumos manpage markup. Did not know >>> that the manpage was lifted from Oracle. >>> In light of this I "rewrote" the manpage for this patch. >>> >>> It is now probably too minimalistic but is not copied from Oracle. It >>> would be great to get some feedback on what people would like to see in >>> there. >>> >>> >>>> 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. >>>>> >>>>> libzfs_sendrecv.c: >>>>> 2043: should be "skipping receive of %s" >>>>> >>>> >>> fixed >>> >>> 2935: can you explain the handling of has_exprops? It seems like (a) >>>>> you could just inline nvlist_empty(exprops) where necessary, and (b) if >>>>> (stream_wantsnewfs), has_exprops is uninitialized (dsname is also >>>>> uninitialized) >>>>> >>>> >>> fixed >>> >>> >>>> 2567: I think that "props" is the properties from the send stream (not >>>>> "current properties"), and "exprops" is the properties set on the command >>>>> line (via -x and -o). I don't know what "external properties" means. >>>>> >>>> >>> changed the comments, but the variable name is still exprops. I can >>> change that (to cli_props?) if needed. >>> >>> Please see the updated webrev: http://alek_p.bitbucket.org/webrevs/2745/ >>> HTML version of the modified manpage: >>> http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html >>> >>> -Alek >>> >>> >>>>> >>>>> >>>>> >>>>> On Fri, Apr 4, 2014 at 9:11 PM, Alek Pinchuk >>>>> <pinchuk.a...@gmail.com>wrote: >>>>> >>>>>> Hello, >>>>>> >>>>>> I know members of the OpenZFS community (including my employer) would >>>>>> really like to see this cross the goalline and be integrated. >>>>>> >>>>>> And since it looks like this effort has stalled (and I couldn't get a >>>>>> hold of Steven) I've decided to port his FreeBSD patch to illumos and the >>>>>> webrev is available at http://alek_p.bitbucket.org/webrevs/2745 >>>>>> >>>>>> I had to touch two files (ndmpd_zfs.c & be_create.c) that were not >>>>>> part of the FreeBSD patch. >>>>>> >>>>>> The rest of the code is basically unchanged from Steven's. I think >>>>>> I've addressed all of Matt's review comments from the previous email. >>>>>> Please take a look at the webrev and let me know if there is anything >>>>>> else >>>>>> that needs to be changed. >>>>>> >>>>>> This is what the illumos ZFS manpage looks like after the patch: >>>>>> http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html >>>>>> >>>>>> I'll make sure Steven Hartland is the author of the patch submitted >>>>>> for RTI. >>>>>> >>>>>> -Alek >>>>>> >>>>>> P.S. >>>>>> git pbchk will complain since I didn't add any copyright to the files >>>>>> changed by the FreeBSD patch since it didn't have any copyright additions >>>>>> in it. >>>>>> Copyrights: >>>>>> usr/src/lib/libzfs/common/libzfs_impl.h: no copyright claim for >>>>>> current year found >>>>>> usr/src/lib/libzfs/common/libzfs_dataset.c: no copyright claim for >>>>>> current year found >>>>>> usr/src/cmd/zfs/zfs_main.c: no copyright claim for current year found >>>>>> usr/src/lib/libzfs/common/libzfs.h: no copyright claim for current >>>>>> year found >>>>>> usr/src/lib/libzfs/common/libzfs_sendrecv.c: no copyright claim for >>>>>> current year found >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Feb 25, 2013 at 1:02 PM, Matthew Ahrens >>>>>> <mahr...@delphix.com>wrote: >>>>>> >>>>>>> Steven, thanks for following up on this. >>>>>>> >>>>>>> It would be really great if you could integrate this into illumos. >>>>>>> I think all you would need to do is convert the manpage changes >>>>>>> (illumos >>>>>>> uses a different markup language for manpages), apply your diffs, build >>>>>>> and >>>>>>> test on illumos, and post a webrev for review on >>>>>>> z...@lists.illumos.org. >>>>>>> >>>>>>> It would also be great if you could work with John Kennedy to >>>>>>> integrate test for the new features into the zfs test suite. >>>>>>> >>>>>>> I took another look at your diffs and noticed just a few small >>>>>>> things that could be improved: >>>>>>> >>>>>>> zfs.8 line ~2383 misspells "receive" >>>>>>> line ~2420: >>>>>>> +If a received property needs to be overridden, the effective value >>>>>>> can be >>>>>>> +set or inherited, depending on the property. >>>>>>> I think you mean "will be set or inherited" (the user doesn't >>>>>>> choose). You could also mention what it depends on ("depending on if >>>>>>> the >>>>>>> property is inheritable or not"). >>>>>>> >>>>>>> We shoud clarify that the filesystem/volume names must be specified >>>>>>> as they existed on the sending system (as opposed to the names that they >>>>>>> will be received into). >>>>>>> >>>>>>> zfs_main.c: >>>>>>> since parsepropname() is used not just for properties (e.g. for -l), >>>>>>> you might call it something like "addonce" and not have its error >>>>>>> message >>>>>>> use the term "property". >>>>>>> >>>>>>> props_override(): >>>>>>> >>>>>>> Can your calls to nvlist_add_*() ever fail? (e.g. due to the name >>>>>>> already existing)? I don't think so, in which case you should >>>>>>> VERIFY0(nvlist_add_*()), or better yet use the fnvlist_* routines. >>>>>>> >>>>>>> + char *sepp = strchr(propname, sepp); >>>>>>> I don't think this will compile; you probably want "strchr(propname, >>>>>>> sep)". Be sure to test this out. >>>>>>> >>>>>>> zfs_receive_one(): "skip" should be a boolean_t. >>>>>>> >>>>>>> Again, thanks for doing this Steven. Looking forward to having it >>>>>>> in illumos and FreeBSD! >>>>>>> >>>>>>> --matt >>>>>>> >>>>>>> On Sat, Feb 23, 2013 at 6:02 PM, Steven Hartland < >>>>>>> kill...@multiplay.co.uk> wrote: >>>>>>> >>>>>>>> Sorry its taken so long to get round to this, just had a really busy >>>>>>>> number of months. Anyway all the changes have been made, see below >>>>>>>> for individual details, patches are attached. >>>>>>>> >>>>>>>> ----- Original Message ----- From: "Steven Hartland" >>>>>>>> >>>>>>>>> ----- Original Message ----- From: "Matthew Ahrens" < >>>>>>>>> mahr...@delphix.com> >>>>>>>>> >>>>>>>>>> Here are some code-specific notes: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> parsepropname(): I see that you're following the bad example set >>>>>>>>>> by >>>>>>>>>> parseprop() and parse_depth() of operating on optarg. It would be >>>>>>>>>> better to explicitly pass the name of the property to parse, >>>>>>>>>> rather >>>>>>>>>> than operating on the global optarg. To isolate the uglyness of >>>>>>>>>> this >>>>>>>>>> global variable, optarg should only be used within the function >>>>>>>>>> that >>>>>>>>>> calls getopt(). It would be great if you wanted to fix all of >>>>>>>>>> these, >>>>>>>>>> but if not that's OK. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I'll look into this thanks, as you said following the example ;-) >>>>>>>>> >>>>>>>> >>>>>>>> Fixed this, and created a seperate patch for fixing the original >>>>>>>> code too >>>>>>>> see attached >>>>>>>> >>>>>>>> >>>>>>>> zfs_receive() can't deal with an empty list of properties or >>>>>>>>>> datasets? >>>>>>>>>> >>>>>>>>> >>>>>>>>> Was just forward loading that logic at the time, will review and >>>>>>>>> update >>>>>>>>> if its doesn't make sense any more. >>>>>>>>> >>>>>>>> >>>>>>>> Minor changes needed to faciliated this which have been done. >>>>>>>> >>>>>>>> >>>>>>>> props_overrride(): >>>>>>>>>> >>>>>>>>>> this function could use some comments explaining what is doing. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Will do. >>>>>>>>> >>>>>>>> >>>>>>>> Comment expanded to give more insite into what its doing >>>>>>>> >>>>>>>> >>>>>>>> however I think that the following idiom for iterating over a nvlist >>>>>>>>>> is more easily understood, because it keeps all the iteration >>>>>>>>>> logic in >>>>>>>>>> one place, and handles "continue" statements: >>>>>>>>>> >>>>>>>>>> for (nvpair_t *pair = nvlist_next_nvpair(nvl, NULL); >>>>>>>>>> pair != NULL; >>>>>>>>>> pair = nvlist_next_nvpair(nvl, pair)) { >>>>>>>>>> ... >>>>>>>>>> } >>>>>>>>>> >>>>>>>>> >>>>>>>>> Not a problem will change. >>>>>>>>> >>>>>>>> >>>>>>>> Changed >>>>>>>> >>>>>>>> >>>>>>>> the variable you call "tods" would usually be named "atp" (the >>>>>>>>>> pointer >>>>>>>>>> to the "at" character). This is where you will have a bug when >>>>>>>>>> someone does "-o userquota@username@pool/fsname=1g". >>>>>>>>>> >>>>>>>>> >>>>>>>>> I wasn't aware that style of permissions existed, not sure they do >>>>>>>>> in the version of FreeBSD I based this work on, but definitely >>>>>>>>> something to get fixed. >>>>>>>>> >>>>>>>>> Any suggestions on an alternative syntax? >>>>>>>>> >>>>>>>> >>>>>>>> Separator changed to # and code changes to sepp (the pointer to the >>>>>>>> separator character) >>>>>>>> >>>>>>>> >>>>>>>> It seems like the only way you could get a non-string, non-boolean >>>>>>>>>> nvpair would be a programming error (i.e. bad user input could not >>>>>>>>>> cause it). Therefore you should assert this is the case, rather >>>>>>>>>> than >>>>>>>>>> print an error message. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Yes I agree, will do. >>>>>>>>> >>>>>>>> >>>>>>>> These are now asserts >>>>>>>> >>>>>>>> >>>>>>>> It seems like you could omit the "if (!nvlist_exists([gd]xprops)" >>>>>>>>>> check, because the -x props will be removed after this anyway. >>>>>>>>>> >>>>>>>>> >>>>>>>>> It would but didn't want it going through all the effort of adding >>>>>>>>> it >>>>>>>>> only to remove it later for performance was my thinking. Do you >>>>>>>>> believe >>>>>>>>> this wouldn't result in any performance loss if removed? >>>>>>>>> >>>>>>>>> Likewise the "!nvlist_exists(doprops" check, because the dx props >>>>>>>>>> will >>>>>>>>>> be added after this anyway. So you can simplify the logic to: >>>>>>>>>> >>>>>>>>> >>>>>>>> Given there will be little performance difference either way I've >>>>>>>> decided >>>>>>>> to keep it as it was so that the output in verbose case is what the >>>>>>>> user >>>>>>>> would expect. >>>>>>>> >>>>>>>> >>>>>>>> Regards >>>>>>>> Steve >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> ================================================ >>>>>>>> This e.mail is private and confidential between Multiplay (UK) Ltd. >>>>>>>> and the person or entity to whom it is addressed. In the event of >>>>>>>> misdirection, the recipient is prohibited from using, copying, >>>>>>>> printing or >>>>>>>> otherwise disseminating it or any information contained in it. >>>>>>>> In the event of misdirection, illegible or incomplete transmission >>>>>>>> please telephone +44 845 868 1337 >>>>>>>> or return the E.mail to postmas...@multiplay.co.uk. >>>>>>>> >>>>>>>> >>>>>>>> ------------------------------------------- >>>>>>>> illumos-developer >>>>>>>> Archives: https://www.listbox.com/member/archive/182179/=now >>>>>>>> RSS Feed: https://www.listbox.com/member/archive/rss/182179/ >>>>>>>> 21175174-cd73734d >>>>>>>> Modify Your Subscription: https://www.listbox.com/member/?&id_ >>>>>>>> secret=21175174-792643f6 <https://www.listbox.com/member/?&;> >>>>>>>> >>>>>>>> Powered by Listbox: http://www.listbox.com >>>>>>>> >>>>>>> >>>>>>> *illumos-zfs* | >>>>>>> Archives<https://www.listbox.com/member/archive/182191/=now> >>>>>>> <https://www.listbox.com/member/archive/rss/182191/23976944-8a53fa63>| >>>>>>> Modify <https://www.listbox.com/member/?&;> Your Subscription >>>>>>> <http://www.listbox.com> >>>>>>> >>>>>> >>>>>> *illumos-zfs* | >>>>>> Archives<https://www.listbox.com/member/archive/182191/=now> >>>>>> <https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>| >>>>>> Modify <https://www.listbox.com/member/?&;> Your Subscription >>>>>> <http://www.listbox.com> >>>>>> >>>>> >>>>> >>>> *illumos-zfs* | >>>> Archives<https://www.listbox.com/member/archive/182191/=now> >>>> <https://www.listbox.com/member/archive/rss/182191/23976944-8a53fa63>| >>>> Modify <https://www.listbox.com/member/?&;> Your Subscription >>>> <http://www.listbox.com> >>>> >>> >>> > *illumos-developer* | > Archives<https://www.listbox.com/member/archive/182179/=now> > <https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> | > Modify<https://www.listbox.com/member/?member_id=21175174&id_secret=21175174-792643f6>Your > Subscription > <http://www.listbox.com> >
_______________________________________________ developer mailing list developer@open-zfs.org http://lists.open-zfs.org/mailman/listinfo/developer
