Skip to content
This repository has been archived by the owner on Nov 7, 2019. It is now read-only.

7584 Improve 'zpool labelclear' command #424

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions usr/src/cmd/zpool/Makefile
Expand Up @@ -41,8 +41,12 @@ SRCS += $(STAT_COMMON_SRCS)

LDLIBS += -lzfs -lnvpair -ldevid -lefi -ldiskmgt -luutil -lumem

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

C99MODE= -xc99=%all
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

C99LMODE= -Xc99=%all

CPPFLAGS += -D_LARGEFILE64_SOURCE=1 -D_REENTRANT $(INCS)
$(NOT_RELEASE_BUILD)CPPFLAGS += -DDEBUG

Expand Down
46 changes: 41 additions & 5 deletions usr/src/cmd/zpool/zpool_main.c
Expand Up @@ -36,6 +36,7 @@
#include <libgen.h>
#include <libintl.h>
#include <libuutil.h>
#include <limits.h>
#include <locale.h>
#include <stdio.h>
#include <stdlib.h>
Expand All @@ -47,6 +48,7 @@
#include <zone.h>
#include <zfs_prop.h>
#include <sys/fs/zfs.h>
#include <sys/vdev_impl.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#include <sys/stat.h>

#include <libzfs.h>
Expand Down Expand Up @@ -234,7 +236,8 @@ get_usage(zpool_help_t idx)
return (gettext("\tiostat [-v] [-T d|u] [pool] ... [interval "
"[count]]\n"));
case HELP_LABELCLEAR:
return (gettext("\tlabelclear [-f] <vdev>\n"));
return (gettext("\tlabelclear [-b | -e | -i index] [-c] [-m] "
"[-f] <vdev>\n"));
case HELP_LIST:
return (gettext("\tlist [-Hp] [-o property[,...]] "
"[-T d|u] [pool] ... [interval [count]]\n"));
Expand Down Expand Up @@ -652,10 +655,39 @@ zpool_do_labelclear(int argc, char **argv)
pool_state_t state;
boolean_t inuse = B_FALSE;
boolean_t force = B_FALSE;
boolean_t check = B_FALSE;
boolean_t cherry = B_FALSE;
unsigned int start = 0, n = VDEV_LABELS;
long long index = 0;
const char *errstr;

/* check options */
while ((c = getopt(argc, argv, "f")) != -1) {
while ((c = getopt(argc, argv, "bei:cmf")) != -1) {
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cstyle: add space after if, and add explicit != NULL:
if (strerr != NULL) {

(void) fprintf(stderr,
gettext("Invalid index value provided\n"));
return (1);
}
start = (unsigned int)index;
n = 1;
break;
case 'c':
check = B_TRUE;
break;
case 'm':
cherry = B_TRUE;
break;
case 'f':
force = B_TRUE;
break;
Expand Down Expand Up @@ -708,8 +740,12 @@ zpool_do_labelclear(int argc, char **argv)
}

if (zpool_read_label(fd, &config) != 0 || config == NULL) {
(void) fprintf(stderr,
gettext("failed to read label from %s\n"), vdev);
if (force)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@martymac martymac Feb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

goto wipe_label;
(void) fprintf(stderr, gettext(
"use '-f' to override the following error:\n"
"failed to read label from \"%s\"\n"),
vdev);
return (1);
}
nvlist_free(config);
Expand Down Expand Up @@ -762,7 +798,7 @@ zpool_do_labelclear(int argc, char **argv)
}

wipe_label:
ret = zpool_clear_label(fd);
ret = zpool_clear_n_labels(fd, start, n, check, cherry);
if (ret != 0) {
(void) fprintf(stderr,
gettext("failed to clear label for %s\n"), vdev);
Expand Down
12 changes: 12 additions & 0 deletions usr/src/common/nvpair/nvpair.c
Expand Up @@ -2354,6 +2354,18 @@ nvlist_common(nvlist_t *nvl, char *buf, size_t *buflen, int encoding,
return (err);
}

int
nvlist_invalidate(char *buf, size_t buflen)
{
if (buf == NULL || buflen < sizeof (nvs_header_t))
return (EINVAL);

nvs_header_t *nvh = (void *)buf;
nvh->nvh_encoding = NV_ENCODE_INVALID;

return (0);
}

int
nvlist_size(nvlist_t *nvl, size_t *size, int encoding)
{
Expand Down
1 change: 1 addition & 0 deletions usr/src/lib/libnvpair/mapfile-vers
Expand Up @@ -243,6 +243,7 @@ SYMBOL_VERSION SUNWprivate_1.1 {
global:
dump_nvlist;
nvlist_add_hrtime;
nvlist_invalidate;
nvlist_lookup_hrtime;
nvlist_print;
nvlist_print_json;
Expand Down
2 changes: 2 additions & 0 deletions usr/src/lib/libzfs/common/libzfs.h
Expand Up @@ -763,6 +763,8 @@ extern int zpool_in_use(libzfs_handle_t *, int, pool_state_t *, char **,
* Label manipulation.
*/
extern int zpool_read_label(int, nvlist_t **);
extern int zpool_clear_n_labels(int, unsigned int, unsigned int, boolean_t,
boolean_t);
extern int zpool_clear_label(int);

/* is this zvol valid for use as a dump device? */
Expand Down
54 changes: 43 additions & 11 deletions usr/src/lib/libzfs/common/libzfs_import.c
Expand Up @@ -1063,35 +1063,67 @@ zpool_open_func(void *arg)
}

/*
* Given a file descriptor, clear (zero) the label information.
* Given a file descriptor, a starting label and a number of labels to clear,
* clear (zero) the label information.
*/
int
zpool_clear_label(int fd)
zpool_clear_n_labels(int fd, unsigned int start, unsigned int n,
boolean_t check, boolean_t cherry)
{
struct stat64 statbuf;
int l;
vdev_label_t *label;
unsigned int l, end;
vdev_label_t label;
uint64_t size;

char *buf = label.vl_vdev_phys.vp_nvlist;
size_t buflen = sizeof (label.vl_vdev_phys.vp_nvlist);

if (fstat64(fd, &statbuf) == -1)
return (0);
size = P2ALIGN_TYPED(statbuf.st_size, sizeof (vdev_label_t), uint64_t);

if ((label = calloc(sizeof (vdev_label_t), 1)) == NULL)
end = start + n;
if (end > VDEV_LABELS)
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 || cherry) {
if (pread64(fd, &label, sizeof (vdev_label_t),
label_offset(size, l)) != sizeof (vdev_label_t))
return (-1);

if (check == B_TRUE) {
nvlist_t *config = NULL;
if (nvlist_unpack(buf, buflen, &config, 0) != 0)
return (-1);
nvlist_free(config);
}
}

if (cherry == B_TRUE) {
if (nvlist_invalidate(buf, buflen) != 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (-1);
} else {
(void) memset(&label, 0, sizeof (vdev_label_t));
}

if (pwrite64(fd, &label, sizeof (vdev_label_t),
label_offset(size, l)) != sizeof (vdev_label_t))
return (-1);
}

free(label);
return (0);
}

/*
* Given a file descriptor, clear (zero) the label information.
*/
int
zpool_clear_label(int fd)
{
return (zpool_clear_n_labels(fd, 0, VDEV_LABELS, B_FALSE, B_FALSE));
}

/*
* Given a list of directories to search, find all pools stored on disk. This
* includes partial pools which are not available to import. If no args are
Expand Down
1 change: 1 addition & 0 deletions usr/src/lib/libzfs/common/mapfile-vers
Expand Up @@ -180,6 +180,7 @@ SYMBOL_VERSION SUNWprivate_1.1 {
zfs_zpl_version_map;
zpool_add;
zpool_clear;
zpool_clear_n_labels;
zpool_clear_label;
zpool_close;
zpool_create;
Expand Down
29 changes: 27 additions & 2 deletions usr/src/man/man1m/zpool.1m
Expand Up @@ -105,7 +105,8 @@
.Op Ar interval Op Ar count
.Nm
.Cm labelclear
.Op Fl f
.Op Fl cfm
.Op Fl Sy b Ns | Ns Sy e Ns | Ns Sy i Ar index
.Ar device
.Nm
.Cm list
Expand Down Expand Up @@ -1275,17 +1276,41 @@ pool, in addition to the pool-wide statistics.
.It Xo
.Nm
.Cm labelclear
.Op Fl f
.Op Fl cfm
.Op Fl Sy b Ns | Ns Sy e Ns | Ns Sy i Ar index
.Ar device
.Xc
Removes ZFS label information from the specified
.Ar device .
The
.Ar device
must not be part of an active pool configuration.
Options
.Fl b ,
.Fl e
and
.Fl i
can be used for clearing specific labels.
They are mutually exclusive and the last one will supersede others, if any.
.Bl -tag -width Ds
.It Fl c
Check label integrity before doing anything.
.It Fl f
Treat exported or foreign devices as inactive.
.It Fl m
Perform minimal operation needed to invalidate a label instead of blindly
erasing every single byte.
This reduces (but does not annihilate) the risks of overriding important
on-disk data.
.It Fl b
Only remove ZFS labels located at the beginning of
.Ar device .
.It Fl e
Only remove ZFS labels located at the end of
.Ar device .
.It Fl i Ar index
Remove a single label entry located at position
.Ar index .
.El
.It Xo
.Nm
Expand Down
2 changes: 2 additions & 0 deletions usr/src/uts/common/sys/nvpair.h
Expand Up @@ -98,6 +98,7 @@ typedef struct nvlist {
#define NV_VERSION 0

/* nvlist pack encoding */
#define NV_ENCODE_INVALID (-1)
#define NV_ENCODE_NATIVE 0
#define NV_ENCODE_XDR 1

Expand Down Expand Up @@ -153,6 +154,7 @@ void nv_alloc_fini(nv_alloc_t *);
/* list management */
int nvlist_alloc(nvlist_t **, uint_t, int);
void nvlist_free(nvlist_t *);
int nvlist_invalidate(char *, size_t);
int nvlist_size(nvlist_t *, size_t *, int);
int nvlist_pack(nvlist_t *, char **, size_t *, int, int);
int nvlist_unpack(char *, size_t, nvlist_t **, int);
Expand Down