[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi @ahrens, @grwilson, Are there news about that patch ? Best regards, Ganael. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-393063362 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/Ta70e14786d4a9936-M36fdcd9966239224fddf33dc Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi @grwilson, As a recall, the original problem that led to the patch is described here : https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=204622 > These knobs expose the internals of the product. Yes, but I don't think exposing the internals of the product is a problem here because it does not make the command more complex : for the average user, its default action is still the same (invalidating all four labels). We just provide an administrator the possibility to go *beyond* and avoid the hassle of using dd plus a seek argument (this *is* a kind of simplification, but for the administrator) as @jpaetzel explains. > why wouldn't I just run zpool labelclear and have the command figure out if > we need to clear label 2 and 3 or 0-3 or any combination? I do not always want to let ZFS choose the labels for me because I may want to minimally invalidate them at the beginning of a device (to avoid issues described at the link above) and totally wipe (zeroe) them at the end (e.g. to produce a clean disk image). The current implementation simply does not provide enough flexibility for that. > I still struggle with invalidating the nvlist encoding vs setting txg = 0 > [...] I wonder if we ever need to "wipe" the labels or do we just want zfs to > forget about this device? As @jpaetzel wrote, I think we want to be able to do both. The default behaviour is to minimally invalidate labels, but an option is provided to wipe them entirely if one needs it. Regarding the idea of invalidating a label using its encoding style, you're right, touching 1 byte can break another FS too, but it's always better than modifying 8 bytes (and even better than what is done currently : zeroing 4 * 256 KiB). Also, as you noticed, debugging tools could be expanded to handle that kind of invalid encoding. Best regards, Ganael. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-364754358 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-M83a3610d9da5a374f4d9e3c2 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
martymac commented on this pull request.
> + if (pread64(fd, , sizeof (vdev_label_t),
+ label_offset(size, l)) != sizeof (vdev_label_t))
+ return (-1);
+
+ if (!force) {
+ nvlist_t *config = NULL;
+ if (nvlist_unpack(buf, buflen, , 0) != 0)
+ return (-1);
+ nvlist_free(config);
+ }
+ }
+
+ if (wipe) {
+ (void) memset(, 0, sizeof (vdev_label_t));
+ } else {
+ if (nvlist_invalidate(buf, buflen) != 0)
The main goal of the patch is to allow wiping a single label with *minimal*
changes.
Acting on nvs encoding type (introducing a new type "invalid") allows touching
a single byte, which is great because it minimizes the risks of damaging
another FS that would have been created over the label. Acting on txg would
touch 8 bytes and greatly improve the chances to break something. Also, as a
side effect, acting on nvs encoding type keeps last txg value available for
debugging.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#discussion_r166851737
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-Mead329a69c4ddfb84cbab979
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
martymac commented on this pull request.
> @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv)
}
if (zpool_read_label(fd, ) != 0) {
- (void) fprintf(stderr,
- gettext("failed to read label from %s\n"), vdev);
+ if (force)
Hi George,
The original version of FreeBSD's labelclear command allowed to forcibly wipe a
label in any case (except when a pool is in use), see :
https://svnweb.freebsd.org/base?view=revision=224171
but that "feature" disappeared when the labelclear command was imported from
upstream :
https://svnweb.freebsd.org/base?view=revision=297760
So this is just a fix to re-add that possibility : if we really want to force a
wipe out, I think the command should not prevent us from doing so ; and that
becomes even more useful when working with individual labels, as that patch now
allows.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#discussion_r166851668
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-M96bfa58c87dac44031872719
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi and happy new year :) I've updated the patch with latest changes from master. If there are no more comments, can it be merged upstream ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-356023274 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tb5209b6a4a580249-M49de3cb80cbfb6b39d1f2732 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@martymac pushed 1 commit. 29d743b Merge branch 'master' of https://github.com/openzfs/openzfs into zpool-labelclear-improvements -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/424/files/7be4c748e5b99478657b945bbe10dc54936af5d5..29d743b2bb55917c72c4ac9d0a957091fe9e6511 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tb5209b6a4a580249-Mfa2f92526c45ad6c9e11a5d6 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@ahrens Have you had time to take a deeper look at latest changes ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-337288816 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tb5209b6a4a580249-M24d3428b4131095b774ef766 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Compilation warnings have been fixed and all checks have passed. @ahrens @rmustacc @yuripv Do you have additional comments regarding that PR ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-328590236 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M74b3b1f51d81e1899e3e35a4 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@martymac pushed 1 commit. 7be4c74 Move VDEV_LABELS definition to sys/fs/zfs.h -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/424/files/7485b1b72227ba2c14464c9a8f37b96c338388ca..7be4c748e5b99478657b945bbe10dc54936af5d5 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M569edc4bcf4377d30daa6901 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
martymac commented on this pull request. > @@ -47,6 +48,7 @@ #include #include #include +#include Yes, this is needed just for VDEV_LABELS. You mean moving VDEV_LABELS definition to sys/fs/zfs.h ? I don't know what that would imply for other parts of the code :/ Help would be welcome here :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#discussion_r137500550 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M4fe57a2f20c29e2d371687f3 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi Matthew, I've pushed fixes following your recommendations, thanks :) Only remains warnings about abd stuff. Any hint ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-327757235 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M76a27d61f16e62cb3b0fb458 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@ahrens Any thoughts on that new patch ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-326551259 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M2748ef86c95601c9cacced2e Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@ahrens Hi Matthew, Well, I have modified the patch to try to keep simplicity as well as flexibility : - As you suggest, always check label validity by default (but one can use the -f flag twice to override that behaviour) - Use the minimal mode by default but provide an option (-w) to wipe (zero) labels entirely as was done before - To maintain ZFS library compatible, zpool_clear_label() still wipes the full labels and does not check for label validity I think it is better that way : the base command remains simple but one can always bypass checks and wipe labels if necessary. What do you think ? Best regards, Ganael. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-325647528 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M331d0ad744f5d5e4c38cddb8 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@martymac pushed 4 commits. dc767f1 Check label validity by default and remove option '-c' 81bb4c6 Invalidate labels the minimal way by default 8c9eeb3 Rewrite boolean test for more readability e0dbeda Specify -f twice to treat invalid labels as valid -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/424/files/22fe1ea7d7b816181336970f6ba242d89bdda9ba..e0dbedafea472876343fc49402990c0930896712 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M887cd1621e46f4dce5a5f82b Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@ahrens Hi Matthew, what do you think about my previous comments ? Can we leave the default options as they are ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-324284867 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M6044f2c2055304c13214bf79 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi, Thanks for your feedback. C99LMODE seems to be derived from C99MODE (see Makefile.master) so setting C99MODE only should be enough ? I've modified the Makefile accordingly. Regards, Ganael. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-317183892 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Me11e3e6b297fcf1c50ddbc35 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@martymac pushed 6 commits. ccdede9 Use strtonum() instead of strtoll() 31452fc Add buflen check to nvlist_invalidate() c876bde Move nvlist_invalidate symbol to SUNWprivate_1.1 1561da9 Simplify boolean check syntax 72b508c Avoid compilation warning related to ignored return value of memset() c94b7fd Reformat zpool labelclear man page section -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/424/files/4e55a80a5b7a0a9521dfbf5b5a75d1e261027006..c94b7fd8fef2a1d5ff20f540e5b0c7e338a53d3d -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M32b68b93eae0ef9814c93d90 Powered by Topicbox: https://topicbox.com
