7584 Improve 'zpool labelclear' command #424

martymac · 2017-07-13T10:34:54Z

Hi,

This patch improves the 'zpool labelclear' command.

See :

https://www.illumos.org/issues/7584

for a detailed description and test cases.

Best regards,

Ganael.

ghost · 2017-07-13T10:39:54Z

usr/src/lib/libnvpair/mapfile-vers

@@ -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.

ghost · 2017-07-13T10:40:41Z

usr/src/lib/libzfs/common/libzfs_import.c

-			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).

ghost · 2017-07-13T10:45:07Z

usr/src/cmd/zpool/zpool_main.c

+			n = VDEV_LABELS / 2;
+			break;
+		case 'i':
+			index = strtoll(optarg, &end, 10);


how about using our new friend, strtonum(), here? :-)

ghost · 2017-07-13T10:47:48Z

usr/src/man/man1m/zpool.1m

@@ -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?

rmustacc · 2017-07-13T14:49:50Z

usr/src/common/nvpair/nvpair.c

@@ -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.

martymac · 2017-07-15T08:54:13Z

Thanks for your feedback! I've just committed changes. Can you check them out ?

ghost

This looks good to me except for the small nit in the Makefile, thanks!

ghost · 2017-07-22T08:34:37Z

usr/src/cmd/zpool/Makefile

 INCS += -I../../common/zfs -I$(STATCOMMONDIR)

+C99MODE=	-xc99=%all


Please use the $(C99_ENABLE) macro instead for both.

martymac · 2017-07-22T13:30:50Z

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.

ahrens · 2017-07-25T04:09:09Z

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.

martymac · 2017-07-25T10:27:56Z

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 :)

ahrens · 2017-07-25T20:47:56Z

@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.

martymac · 2017-07-26T10:13:02Z

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.

martymac · 2017-08-23T10:11:36Z

@ahrens Hi Matthew, what do you think about my previous comments ? Can we leave the default options as they are ?

ahrens · 2017-08-23T21:44:49Z

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.

(one can still override checks using option '-f')

(option '-m' becomes option '-w' to zero labels entirely as was done before)

(an exported pool already needs option -f to clear labels: we cannot use a single -f to handle invalid labels as well)

martymac · 2017-08-29T12:25:25Z

@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.

martymac · 2017-09-01T10:51:11Z

@ahrens Any thoughts on that new patch ?

ahrens · 2017-09-06T22:52:04Z

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?

ahrens · 2017-09-06T22:57:01Z

usr/src/cmd/zpool/zpool_main.c

@@ -47,6 +48,7 @@
 #include <zone.h>
 #include <zfs_prop.h>
 #include <sys/fs/zfs.h>
+#include <sys/vdev_impl.h>


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)

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 :)

ahrens · 2017-09-06T22:57:50Z

usr/src/cmd/zpool/zpool_main.c

+ *
+ *	-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"?

ahrens · 2017-09-06T22:58:17Z

usr/src/cmd/zpool/zpool_main.c

+			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) {

ahrens · 2017-09-06T22:59:32Z

usr/src/cmd/zpool/zpool_main.c

 		case 'f':
+			if(force)


add space after if, and add braces for multi-line body (even if only one statement)

ahrens · 2017-09-06T23:02:56Z

usr/src/man/man1m/zpool.1m

 .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.

martymac · 2017-09-07T10:14:31Z

Hi Matthew,

I've pushed fixes following your recommendations, thanks :)

Only remains warnings about abd stuff. Any hint ?

ahrens · 2017-09-07T16:34:26Z

You mean moving VDEV_LABELS definition to sys/fs/zfs.h ?

Yes.

- this removes compilation warnings about static unused abd_* functions related to inclusion of sys/vdev_impl.h. - while here, also remove definition of -unused- VDEV_BEST_LABEL.

martymac · 2017-09-08T10:12:04Z

@ahrens My last commit should fix compilation warnings.

martymac · 2017-09-11T16:52:20Z

Compilation warnings have been fixed and all checks have passed.

@ahrens @rmustacc @yuripv Do you have additional comments regarding that PR ?

ahrens · 2017-09-11T21:25:43Z

@martymac I'll take a look at your latest changes.

martymac · 2017-10-17T16:30:10Z

@ahrens Have you had time to take a deeper look at latest changes ?

…l-labelclear-improvements

martymac · 2018-01-08T16:49:00Z

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 ?

grwilson

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.

grwilson · 2018-02-07T15:20:02Z

usr/src/cmd/zpool/zpool_main.c

@@ -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.

Hi @grwilson ,

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.

grwilson · 2018-02-07T16:08:42Z

usr/src/lib/libzfs/common/libzfs_import.c

+		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.

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.

grwilson · 2018-02-09T15:18:15Z

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?

jpaetzel · 2018-02-09T15:37:45Z

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)

martymac · 2018-02-11T14:10:26Z

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.

martymac · 2018-03-13T12:46:24Z

Hi @grwilson,
Do you have any other concerns / comments about that patch ?

martymac · 2018-05-30T07:45:38Z

Hi @ahrens, @grwilson,

Are there news about that patch ?

Best regards,
Ganael.

behlendorf · 2019-03-19T18:48:04Z

@martymac @ahrens @grwilson @yuripv this is an issue we've definitely seen on the Linux side as well and would like to see resolved.

I've read though the comments above it sounds like there's still some needed discussion over exactly which command lines options to support. However, there is general agreement that zpool labelclear should not be allowed to zero an offset without verifying it contains a valid label.

In order to help move this issue forward I've opened PR openzfs/zfs#8500 against ZFS on Linux which solely addresses that concern. No new options are added and only valid labels are zeroed. Additional improvements are left as future work. This change is in need of reviewers if you have the time. Thanks!

That patch follows : openzfs#8500 and is a port to ZoL of: openzfs/openzfs#424 It adds the following: - ability to clear a specific label (options -b, -e and -i) - label invalidation using a single-byte modification (wipe with option -w) - option -ff to force invalidation even for invalid labels

That patch follows: openzfs#8500 and is a port to ZoL of: openzfs/openzfs#424 It adds the following: - ability to clear a specific label (options -b, -e and -i) - label invalidation using a single-byte modification (wipe with option -w) - option -ff to force invalidation even for invalid labels OpenZFS-issue: https://www.illumos.org/issues/7584

martymac · 2019-03-29T11:30:53Z

Hi @behlendorf,

Sorry for lagging. If you are still interested in other parts of my patch, here is a port to ZoL :

openzfs/zfs#8548

There are probably still things to discuss about, probably :

do we want to handle single-byte invalidation ?
merge -f and -ff options together ?

but that PR can serve as a basis.

Note : I could not review your code in time but it introduces a small mistake : pwrite64() should also skip (2 * VDEV_PAD_SIZE) when reading from label, but as you write zeroes, that does not matter in your patch. That needed to be fixed in my patch to get the single-byte invalidation work.

Best regards,

ahrens · 2019-10-31T04:13:06Z

This repo will be archived in the coming week, and therefore this PR is being closed. Alternate processes for contributing ZFS changes (including those in open PR's) include following the illumos procedures, or opening a PR against ZFSonLinux.

The reasoning behind this are outlined in an email to the developer@open-zfs.org mailing list, which is reproduced below:

The OpenZFS code repository on github (http://github.com/openzfs/openzfs) is a clone of the illumos repo, with basically identical code. The OpenZFS repo made it easier to contribute ZFS code to illumos, by leveraging the github pull request and code review processes, and by automatically building illumos and running the ZFS Test Suite.

Unfortunately, the automated systems have atrophied and we lack the effort and interest to maintain it. Meanwhile, the illumos code review and contribution process has been working well for a lot of ZFS changes (notably including ports from Linux).

Since the utility of this repo has decreased, and the volunteer workforce isn't available to maintain it, we will be archiving http://github.com/openzfs/openzfs in the coming week. Thank you to everyone who helped maintain this infrastructure, and to those who leveraged it to contribute over 500 commits to ZFS on illumos! Alternate processes for contributing ZFS changes (including those in open PR's) include following the illumos procedures, or opening a PR against ZFSonLinux.

The OpenZFS github organization (http://github.com/openzfs) will continue to exist. I will make an announcement about its future role at the OpenZFS Developer Summit on Monday.

Let me know if you have any questions,
--matt

7584 Improve 'zpool labelclear' command

4e55a80

ghost reviewed Jul 13, 2017

View reviewed changes

rmustacc reviewed Jul 13, 2017

View reviewed changes

martymac added 6 commits July 15, 2017 10:43

Use strtonum() instead of strtoll()

ccdede9

Add buflen check to nvlist_invalidate()

31452fc

Move nvlist_invalidate symbol to SUNWprivate_1.1

c876bde

Simplify boolean check syntax

1561da9

Avoid compilation warning related to ignored return value of memset()

72b508c

Reformat zpool labelclear man page section

c94b7fd

ghost approved these changes Jul 22, 2017

View reviewed changes

Use C99_ENABLE macro

22fe1ea

martymac added 4 commits August 25, 2017 13:41

Check label validity by default and remove option '-c'

dc767f1

(one can still override checks using option '-f')

Invalidate labels the minimal way by default

81bb4c6

(option '-m' becomes option '-w' to zero labels entirely as was done before)

Rewrite boolean test for more readability

8c9eeb3

Specify -f twice to treat invalid labels as valid

e0dbeda

(an exported pool already needs option -f to clear labels: we cannot use a single -f to handle invalid labels as well)

ahrens reviewed Sep 6, 2017

View reviewed changes

Follow Matthew Ahrens' recommendations

7485b1b

Move VDEV_LABELS definition to sys/fs/zfs.h

7be4c74

- this removes compilation warnings about static unused abd_* functions related to inclusion of sys/vdev_impl.h. - while here, also remove definition of -unused- VDEV_BEST_LABEL.

ahrens added the test pass label Sep 11, 2017

Merge branch 'master' of https://github.com/openzfs/openzfs into zpoo…

29d743b

…l-labelclear-improvements

ahrens approved these changes Feb 1, 2018

View reviewed changes

ahrens added reviewed ready to push labels Feb 1, 2018

grwilson reviewed Feb 7, 2018

View reviewed changes

ahrens removed the ready to push label Mar 14, 2018

mailinglists35 mentioned this pull request Feb 4, 2019

"zpool labelclear" should only clear valid labels openzfs/zfs#8373

Open

behlendorf mentioned this pull request Mar 19, 2019

Additional safe guards for zpool labelclear openzfs/zfs#8500

Merged

12 tasks

martymac mentioned this pull request Mar 29, 2019

Further improve 'zpool labelclear' command openzfs/zfs#8548

Closed

12 tasks

ahrens closed this Oct 31, 2019

		INCS += -I../../common/zfs -I$(STATCOMMONDIR)

		C99MODE= -xc99=%all

7584 Improve 'zpool labelclear' command #424

7584 Improve 'zpool labelclear' command #424

Conversation

martymac commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 13, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martymac commented Jul 15, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martymac commented Jul 22, 2017

ahrens commented Jul 25, 2017

martymac commented Jul 25, 2017

ahrens commented Jul 25, 2017

martymac commented Jul 26, 2017

martymac commented Aug 23, 2017

ahrens commented Aug 23, 2017

martymac commented Aug 29, 2017

martymac commented Sep 1, 2017

ahrens commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martymac commented Sep 7, 2017

ahrens commented Sep 7, 2017

martymac commented Sep 8, 2017

martymac commented Sep 11, 2017

ahrens commented Sep 11, 2017

martymac commented Oct 17, 2017

martymac commented Jan 8, 2018

grwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martymac Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grwilson commented Feb 9, 2018

jpaetzel commented Feb 9, 2018

martymac commented Feb 11, 2018

martymac commented Mar 13, 2018

martymac commented May 30, 2018

behlendorf commented Mar 19, 2019

martymac commented Mar 29, 2019

ahrens commented Oct 31, 2019

ghost Jul 13, 2017 •

edited by ghost

martymac Feb 8, 2018 •

edited