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 1 commit
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
47 changes: 42 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,40 @@ 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;
char *end = NULL;
long long index = 0;

/* 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 = strtoll(optarg, &end, 10);
Copy link

@ghost ghost Jul 13, 2017

Choose a reason for hiding this comment

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

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

if((end == optarg) || (*end != '\0') ||
(index < 0) || (index >= VDEV_LABELS)) {
(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 +741,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 +799,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)

Choose a reason for hiding this comment

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

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.

{
if (buf == NULL)
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 @@ -198,6 +198,7 @@ SYMBOL_VERSION SUNW_1.1 {
nvlist_alloc;
nvlist_dup;
nvlist_free;
nvlist_invalidate;
Copy link

Choose a reason for hiding this comment

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

You can't add the symbols to already existing public version, either move it to SUNWprivate_1.1, or provide new public version.

nvlist_lookup_boolean;
nvlist_lookup_byte;
nvlist_lookup_byte_array;
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 == B_TRUE) || (cherry == B_TRUE)) {
Copy link

Choose a reason for hiding this comment

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

treat these as booleans that they are, i.e., 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) != 0)
return (-1);
} else {
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: 29 additions & 0 deletions usr/src/man/man1m/zpool.1m
Expand Up @@ -105,6 +105,9 @@
.Op Ar interval Op Ar count
.Nm
.Cm labelclear
.Op Fl b | Fl e | Fl i Ar index
Copy link

Choose a reason for hiding this comment

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

can we please make it (here, usage, description) zfs labelclear [-cfm] [-b|-e|-i index] device?

.Op Fl c
.Op Fl m
.Op Fl f
.Ar device
.Nm
Expand Down Expand Up @@ -1275,6 +1278,9 @@ pool, in addition to the pool-wide statistics.
.It Xo
.Nm
.Cm labelclear
.Op Fl b | Fl e | Fl i Ar index
.Op Fl c
.Op Fl m
.Op Fl f
.Ar device
.Xc
Expand All @@ -1283,7 +1289,30 @@ Removes ZFS label information from the specified
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 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 .
.It Fl c
Check label integrity before doing anything.
.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 f
Treat exported or foreign devices as inactive.
.El
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 *);
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