[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
yuripv commented on this pull request.
> @@ -198,6 +198,7 @@ SYMBOL_VERSION SUNW_1.1 {
nvlist_alloc;
nvlist_dup;
nvlist_free;
+ nvlist_invalidate;
You can't add the symbols to already existing public version, either move it to
SUNWprivate_1.1, or provide new public version.
> return (-1);
- for (l = 0; l < VDEV_LABELS; l++) {
- if (pwrite64(fd, label, sizeof (vdev_label_t),
- label_offset(size, l)) != sizeof (vdev_label_t)) {
- free(label);
- return (-1);
+ for (l = start; l < end; l++) {
+ if ((check == B_TRUE) || (cherry == B_TRUE)) {
treat these as booleans that they are, i.e., ```if (check || cherry)```.
> switch (c) {
+ case 'b':
+ start = 0;
+ n = VDEV_LABELS / 2;
+ break;
+ case 'e':
+ start = VDEV_LABELS / 2;
+ n = VDEV_LABELS / 2;
+ break;
+ case 'i':
+ index = strtoll(optarg, &end, 10);
how about using our new friend, ```strtonum()``, here? :-)
> @@ -105,6 +105,9 @@
.Op Ar interval Op Ar count
.Nm
.Cm labelclear
+.Op Fl b | Fl e | Fl i Ar index
can we please make it (here, usage, description) ```zfs labelclear [-cfm]
[-b|-e|-i index] device```?
--
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#pullrequestreview-49740365
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Mb1f31cd3e6b5baa1ce303894
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
rmustacc commented on this pull request. > @@ -2355,6 +2355,18 @@ nvlist_common(nvlist_t *nvl, char *buf, size_t > *buflen, int encoding, } int +nvlist_invalidate(char *buf) Shouldn't this function be taking the size of the buffer in question that we're modifying? I realize that we're only modifying the first byte at this time which in theory should always be valid for a char *, but seems like if this ever changes, that'll be helpful as the function is making an implicit assumption about the size. -- 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#pullrequestreview-49803070 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M5aed2960c23eae7ddd81cff6 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
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Thanks for your feedback! I've just committed changes. Can you check them out ? -- 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-315520516 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Mcfdaf219348fb4bf0e09b407 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
yuripv approved this pull request. This looks good to me except for the small nit in the Makefile, thanks! > INCS += -I../../common/zfs -I$(STATCOMMONDIR) +C99MODE= -xc99=%all Please use the $(C99_ENABLE) macro instead for both. -- 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#pullrequestreview-51618787 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M1a0c96d64427cdec95c6d272 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@martymac pushed 1 commit. 22fe1ea Use C99_ENABLE macro -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/424/files/c94b7fd8fef2a1d5ff20f540e5b0c7e338a53d3d..22fe1ea7d7b816181336970f6ba242d89bdda9ba -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Mcff2dc73073b66060745d70a 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)
This adds quite a lot of flexibility to an already obscure subcommand, making it even harder to know how to use correctly. Is there any reason you wouldn't always want the `-cm` behavior? What would you think about changing `zpool labelclear` (without any flags) to always make the minimal modification, and only to labels which we detect are actual ZFS labels? That way it would always behave like your `-cm`, without the need for 2 additional options. -- 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-317623358 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M7dddb3accd941cbb17483fab Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi Matthew, Thanks for your feedback. My original idea was to extend the command while keeping its default behaviour. The reason is simple : that command is quite new in OpenZFS world but has already existed on FreeBSD for about 6 years now, see : https://svnweb.freebsd.org/base?view=revision&revision=224171 People are already used to it and changing its default behaviour will probably break things (scripts, brains, ...). That's just my 2 cents by I would personally prefer keeping it that way to avoid -more- confusion :) -- 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-317696729 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M41e3c1feeb996f862885d165 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
@martymac What could break if we change the default behavior? It seems like it would still have the same semantic of leaving the disk without any valid labels. -- 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-317867356 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Me194c97617e347a6fef8aabb Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
What I mean is that people are probably used to the fact that this command blindly zeroes 512KiB at the beginning and the end of the disk and may (ab)use that property in a way or another (e.g. in an install script to wipe other kinds of metadata - shame on me, I remember having done that myself). Also, if you make -cm the default (and only) available methods, there will be no easy way of "full cleaning" labels. Yes, this can always be replaced by a dd pass but the end of the disk is trickier to reach than a simple labelclear. We could revert options meaning : i.e. make -cm the default and provide 2 options, one for '*not* checking before erasing' and another for the 'full mode', but that will leave the same complexity while changing users habits. -- 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-318011439 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M4f86a97af743e9dae3b7807d 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)
I'm not convinced that people are "used to the fact that this command blindly zeroes 512KiB at the beginning and the end of the disk". I think that this command is seldom-used, and when it's used it's for the documented purpose: "Removes ZFS label information." That said, I'm open to evidence to the contrary. I agree that with my suggestion, "there will be no easy way of "full cleaning" labels", but I don't know of the use case for that. I don't consider "abusing ... to wipe other kinds of metadata" a legitimate use case. -- 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-324471369 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M3640d8d8f37a86c59a4860fc 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, 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)
@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)
Sorry for the latency, I was on vacation last week. That design sounds OK to me. I (or someone else familiar with this code) still need to review the code. @yuripv do you want to take another look? -- 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-327633621 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M42e75ee272e14e07c0b209e0 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
ahrens commented on this pull request.
> @@ -47,6 +48,7 @@
#include
#include
#include
+#include
It's possible that this is causing the compilation (lint) error (see
http://jenkins.open-zfs.org/blue/organizations/jenkins/openzfs%2Fopenzfs/detail/PR-424/4/pipeline).
Is this needed just for `VDEV_LABELS`? Maybe we should move that to a
different header file (e.g. zfs.h)
> *
- * Verifies that the vdev is not active and zeros out the label information
+ * -e Only work on labels located at the end of the device.
+ *
+ * -i indexOnly work on labels located at specified index.
+ *
+ * -w Wipe label area entirely and replace it with zeroes.
+ *
+ * -f Force clearing the label for the vdevs which are
+ * members of the exported or foreign pools. Also consider
+ * seemingly invalid labels as valid ones.
should this now say, "if specified twice, clear even seemingly invalid labels"?
> switch (c) {
+ case 'b':
+ start = 0;
+ n = VDEV_LABELS / 2;
+ break;
+ case 'e':
+ start = VDEV_LABELS / 2;
+ n = VDEV_LABELS / 2;
+ break;
+ case 'i':
+ index = strtonum(optarg, 0, VDEV_LABELS - 1, &errstr);
+ if(errstr) {
cstyle: add space after `if`, and add explicit `!= NULL`:
`if (strerr != NULL) {`
> case 'f':
+ if(force)
add space after `if`, and add braces for multi-line body (even if only one
statement)
> .Bl -tag -width Ds
.It Fl f
-Treat exported or foreign devices as inactive.
+Force mode: treat exported or foreign devices as inactive.
+Specify twice to treat invalid labels as valid ones.
How about, `Specify twice to clear even seemingly-invalid labels.`
--
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#pullrequestreview-61073830
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M7670a84d321b47c1bae929fa
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)
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)
@martymac pushed 1 commit. 7485b1b Follow Matthew Ahrens' recommendations -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/424/files/e0dbedafea472876343fc49402990c0930896712..7485b1b72227ba2c14464c9a8f37b96c338388ca -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Mb0a1987ec4be746e9c6624c4 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
> You mean moving VDEV_LABELS definition to sys/fs/zfs.h ? Yes. -- 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-327854271 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M253d768b2b440d6d8222a575 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)
@ahrens My last commit should fix compilation warnings. -- 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-328064611 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M2c3258e58d4fbbd0ecd04be5 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 I'll take a look at your 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-328663255 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-Ma79dc925b8fd451a39f4ff98 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)
@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)
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)
ahrens approved this pull request. -- 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#pullrequestreview-93366574 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-Mc81bc9144244f8805ec8ad91 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
grwilson commented on this pull request.
I have concerns about the ultimate goal for this command. It's unclear if we
really want to "wipe" or just "forget" about this device.
> @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv)
}
if (zpool_read_label(fd, &config) != 0) {
- (void) fprintf(stderr,
- gettext("failed to read label from %s\n"), vdev);
+ if (force)
Shouldn't this check force_invalid since it's possible for zpool_read_label to
return -1 for reasons other than the inability to read the label. If just force
is used then we could wipe out the label of a legit pool.
> + if (pread64(fd, &label, 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, &config, 0) != 0)
+ return (-1);
+ nvlist_free(config);
+ }
+ }
+
+ if (wipe) {
+ (void) memset(&label, 0, sizeof (vdev_label_t));
+ } else {
+ if (nvlist_invalidate(buf, buflen) != 0)
If the goal is to prevent zpool import from showing the pool, then we should
set the TXG value to 0 which is how ZFS clears a label while still leaving some
information around 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#pullrequestreview-94746416
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-Mb71b9d84a4c53f6d4b400daf
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, &config) != 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&revision=224171
but that "feature" disappeared when the labelclear command was imported from
upstream :
https://svnweb.freebsd.org/base?view=revision&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)
martymac commented on this pull request.
> + if (pread64(fd, &label, 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, &config, 0) != 0)
+ return (-1);
+ nvlist_free(config);
+ }
+ }
+
+ if (wipe) {
+ (void) memset(&label, 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)
One of the guiding principles for zfs is simple administration and it seems like we're exposing way too many knobs to the administrator. These knobs expose the internals of the product. For example, if I want to clear the labels, 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? In other words if the user wants to clear the labels then clear whatever valid labels we find. If the user specifies -f then clear them all. I like that this change is trying to protect the user but it seems like we can accomplish this without having to make the user figure out the internal details of the product. This is less of a concern but one that I want to make sure we can discuss -- I still struggle with invalidating the nvlist encoding vs setting txg = 0. Yes, it's 1 byte vs 8 bytes so we have a smaller chance of impacting any software that is using that disk but the chance is not 0% in either case. I would argue that setting the value of txg=0 at least provides some diagnostics with existing tools and possibly some recovery opportunities that are not available with the nvlist invalidate case. For example, if invalidating the nvlist impacts the software running on that disk, how would you ever know that the disk was once used by zfs and has been invalidated? You would end up with corruption with no way of diagnosing what caused it. We could enhance the tools to look for the invalid encoding making this less of an issue. I recognize this is not a new problem since the current implementation "wipes" the labels. This is why I wonder if we ever need to "wipe" the labels or do we just want zfs to forget about this device? Do we know of cases where we have needed to "wipe" the label? -- 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-364462555 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-Mdb7922ecbe8217cb2bbd1d04 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
I'll chime in here. There have been a number of cases in the past where I have wanted the moral equivalent to dd if=/dev/zero of=/dev/da0 bs=1M Typically, and this may be FreeBSD specific, it's a case of ZFS thinks the device is unused, then geom attaches to the device, then ZFS magically sees that oh yes, there is a label there. An example might be where you GPT partition and put a UFS filesystem on a disk that used to have ZFS on it. >From a ZFS user perspective: I'd like it if there was a ZF safe and friendly "forget about this device" as well as a "nuke it from orbit" option that was faster than dd. (Yes, you can avoid running dd on the whole disk if you calculate the number of sectors on the disk and use the appropriate seek argument, but sometimes that takes enough time to figure out that I just let the dd run and go do something else for a while) -- 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-364468308 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-Mdf28cd67aecf404323daeeb2 Powered by Topicbox: https://topicbox.com
[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)
Hi @grwilson, Do you have any other concerns / comments about that 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-372652991 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T59e810f5492df392-Md20d60417fd24c53e3999857 Delivery options: https://openzfs.topicbox.com/groups
[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
