Any chance this gets pushed across the finish line? I have a real need for this in my environment as migrating datasets across zpools is a frequent task. This patch would make this possible without downtime.
Cheers! -Chip On Sun, Nov 1, 2015 at 7:57 PM, Matthew Ahrens <[email protected]> wrote: > > > On Sun, Nov 1, 2015 at 4:36 PM, Alexandre Lécuyer < > [email protected]> wrote: > >> Thank you for reviewing the patch. Sorry about the style and indentation >> issues, I have corrected the ones you mentionned and ran cstyle on all >> changed files. >> >> The webrev is updated, and I have uploaded the diff on the illumos review >> board, which should make it easier to review :) >> https://www.illumos.org/rb/r/111/ >> >> Comments about other changes below : >> >> Le 30/10/2015 23:53, Matthew Ahrens a écrit : >> >>> Aaah webrevs. Makes even github code review seem sophisticated and >>> painless :-) >>> >>> zfs_prop.c: I don't think the new prop is applicable to bookmarks. >>> >> Right, I have removed it. >> >> >>> zfs_create(), libzfs_changelist.c: Indentation looks wrong. Make sure >>> you are using tabs, and continuation lines are indented with tabs + 4 >>> spaces. >>> >>> zfs_create(): what error does lzc_create() if you try to create a >>> filesystem that already exists? >>> >> lzc_create() is not called in this case, we stop at line 3191, >> zfs_dataset_exists() is true and an error is returned ("dataset already >> exists") > > > OK, but: > > A. What if the dataset doesn't exist when zfs_dataset_exists() is called, > and then another thread creates it before the lzc/ioctl is made? > > B. The lzc_* interfaces should provide a good interface for all consumers, > not just this one. If we use the same error code for 2 totally different > types of error, it's hard to use. > > >> >> >> >>> dsl_dataset.c: >>> 3510: put a blank line after the end of the function, before the typedef. >>> 3514: only need one blank line here >>> 3566: only one blank line >>> 3567: "int" should be on its own line >>> 3576: this should be ZFS_SPACE_CHECK_RESERVED. See the comments in >>> dsl_synctask.h. >>> >> Done >> >> 3546: I assume it's OK to not manipulate the unique avl because we know >>> this is not mounted. Can you add an assertion to that effect? >>> >> On line 3552 there is : >> VERIFY0(dsl_dataset_hold(dp, ddsfg->ddsfg_name, FTAG, &ds)); >> Is this OK ? > > > Holding doesn't ensure that it is not mounted. (Checking if it's owned > would) > > Actually, even not being mounted is not strong enough. It will be in the > avl tree if the dataset_t exists at all (see where we call > unique_insert()). You might need to manipulate the AVL after all, unless > you can ensure that the dataset_t doesn't exist. > > --matt > > >> >> >> the _check and _sync funcs do not seem to be used outside this file, >>> they should be declared "static". >>> >> Done >> >> >>> lines 3517 - 3563 are indented incorrectly. >>> >>> additional cstyle errors on lines 3532, 3539, 3540, 3534 >>> >>> dsl_dataset.h: >>> the _check and _sync funcs do not seem to be used outside dsl_datset.c, >>> they should not be declared here. >>> >> Done >> >> >>> unique.c: >>> changes to this file are incorrectly indented >>> additional cstyle error line 113-114. >>> 110: prefer to explicitly convert pointer to boolean with "!= NULL", and >>> number to boolean with "!= 0". >>> is line 110 <=80 columns? >>> this might be easier to read if you do: >>> boolean_t rv = (value & ... == 0 && avl_find(...) == NULL); >>> mutex_exit(...); >>> return (rv); >>> >> Changed as suggested >> >> >>> zfs_ioctl.c: >>> changes to this file are incorrectly indented >>> >>> zfs.h: >>> prefer to add new props to end of the enum, to maintain user/kernel >>> compatibility for platforms that do not keep them in sync (linux, >>> freebsd) >>> >> Done >> >> Thanks, >> -- >> alex >> >>> >>> >>> Let me know if any of the cstyle errors are not obvious (or look at the >>> rest of the file and see how it's done). The cstyle tool can catch some >>> of these. >>> >>> --matt >>> >>> >>> >>> >>> >>> On Wed, Oct 28, 2015 at 6:10 AM, Alexandre Lécuyer >>> <[email protected] <mailto:[email protected]>> wrote: >>> >>> Hello, >>> >>> Here's a patch which allows modification of the fsid_guid of a >>> dataset : >>> zfs create -o fsid_guid=xxx tank/test ; or zfs set fsid_guid=xxx >>> tank/test >>> >>> This is useful if you need to migrate NFS clients from a target >>> system to another, without having to umount/remount all clients. >>> >>> I have made changes that were suggested at the hackathon last week, >>> thanks to all for your suggestions. >>> If this is of interest to others, would you please review it ? I >>> have uploaded a webrev here : >>> >>> http://cr.illumos.org/~webrev/alecuyer/6333/ >>> >>> thanks, >>> -- >>> alex >>> >>> >>> http://www.listbox.com >>> >>> >>> > *illumos-developer* | Archives > <https://www.listbox.com/member/archive/182179/=now> > <https://www.listbox.com/member/archive/rss/182179/26619341-53733119> | > Modify > <https://www.listbox.com/member/?member_id=26619341&id_secret=26619341-9d39953a> > Your Subscription <http://www.listbox.com> >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
