Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple zdb bugs #7099

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 51 additions & 34 deletions cmd/zdb/zdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -3388,7 +3388,7 @@ dump_block_stats(spa_t *spa)
int flags = TRAVERSE_PRE | TRAVERSE_PREFETCH_METADATA |
TRAVERSE_NO_DECRYPT | TRAVERSE_HARD;
boolean_t leaks = B_FALSE;
int e, c;
int e, c, err;
bp_embedded_type_t i;

bzero(&zcb, sizeof (zcb));
Expand Down Expand Up @@ -3430,7 +3430,7 @@ dump_block_stats(spa_t *spa)

zcb.zcb_totalasize = metaslab_class_get_alloc(spa_normal_class(spa));
zcb.zcb_start = zcb.zcb_lastprint = gethrtime();
zcb.zcb_haderrors |= traverse_pool(spa, 0, flags, zdb_blkptr_cb, &zcb);
err = traverse_pool(spa, 0, flags, zdb_blkptr_cb, &zcb);

/*
* If we've traversed the data blocks then we need to wait for those
Expand All @@ -3446,6 +3446,12 @@ dump_block_stats(spa_t *spa)
}
}

/*
* Done after zio_wait() since zcb_haderrors is modified in
* zdb_blkptr_done()
*/
zcb.zcb_haderrors |= err;

if (zcb.zcb_haderrors) {
(void) printf("\nError counts:\n\n");
(void) printf("\t%5s %s\n", "errno", "count");
Expand Down Expand Up @@ -3978,13 +3984,6 @@ zdb_vdev_lookup(vdev_t *vdev, const char *path)
return (NULL);
}

/* ARGSUSED */
static int
random_get_pseudo_bytes_cb(void *buf, size_t len, void *unused)
{
return (random_get_pseudo_bytes(buf, len));
}

/*
* Read a block from a pool and print it out. The syntax of the
* block descriptor is:
Expand Down Expand Up @@ -4154,17 +4153,8 @@ zdb_read_block(char *thing, spa_t *spa)
* every decompress function at every inflated blocksize.
*/
enum zio_compress c;
void *pbuf2 = umem_alloc(SPA_MAXBLOCKSIZE, UMEM_NOFAIL);
void *lbuf2 = umem_alloc(SPA_MAXBLOCKSIZE, UMEM_NOFAIL);

abd_copy_to_buf(pbuf2, pabd, psize);

VERIFY0(abd_iterate_func(pabd, psize, SPA_MAXBLOCKSIZE - psize,
random_get_pseudo_bytes_cb, NULL));

VERIFY0(random_get_pseudo_bytes((uint8_t *)pbuf2 + psize,
SPA_MAXBLOCKSIZE - psize));

/*
* XXX - On the one hand, with SPA_MAXBLOCKSIZE at 16MB,
* this could take a while and we should let the user know
Expand All @@ -4174,25 +4164,39 @@ zdb_read_block(char *thing, spa_t *spa)
for (lsize = psize + SPA_MINBLOCKSIZE;
lsize <= SPA_MAXBLOCKSIZE; lsize += SPA_MINBLOCKSIZE) {
for (c = 0; c < ZIO_COMPRESS_FUNCTIONS; c++) {
/*
* ZLE can easily decompress non zle stream.
* So have an option to disable it.
*/
if (c == ZIO_COMPRESS_ZLE &&
getenv("ZDB_NO_ZLE"))
continue;

(void) fprintf(stderr,
"Trying %05llx -> %05llx (%s)\n",
(u_longlong_t)psize, (u_longlong_t)lsize,
zio_compress_table[c].ci_name);

/*
* We randomize lbuf2, and decompress to both
* lbuf and lbuf2. This way, we will know if
* decompression fill exactly to lsize.
*/
VERIFY0(random_get_pseudo_bytes(lbuf2, lsize));

if (zio_decompress_data(c, pabd,
lbuf, psize, lsize) == 0 &&
zio_decompress_data_buf(c, pbuf2,
zio_decompress_data(c, pabd,
lbuf2, psize, lsize) == 0 &&
bcmp(lbuf, lbuf2, lsize) == 0)
break;
}
if (c != ZIO_COMPRESS_FUNCTIONS)
break;
}

umem_free(pbuf2, SPA_MAXBLOCKSIZE);
umem_free(lbuf2, SPA_MAXBLOCKSIZE);

if (lsize <= psize) {
if (lsize > SPA_MAXBLOCKSIZE) {
(void) printf("Decompress of %s failed\n", thing);
goto out;
}
Expand Down Expand Up @@ -4231,9 +4235,11 @@ zdb_embedded_block(char *thing)
{
blkptr_t bp;
unsigned long long *words = (void *)&bp;
char buf[SPA_MAXBLOCKSIZE];
char *buf;
int err;

buf = umem_alloc(SPA_MAXBLOCKSIZE, UMEM_NOFAIL);

bzero(&bp, sizeof (bp));
err = sscanf(thing, "%llx:%llx:%llx:%llx:%llx:%llx:%llx:%llx:"
"%llx:%llx:%llx:%llx:%llx:%llx:%llx:%llx",
Expand All @@ -4252,6 +4258,7 @@ zdb_embedded_block(char *thing)
exit(1);
}
zdb_dump_block_raw(buf, BPE_GET_LSIZE(&bp), 0);
umem_free(buf, SPA_MAXBLOCKSIZE);
}

int
Expand All @@ -4266,7 +4273,7 @@ main(int argc, char **argv)
int error = 0;
char **searchdirs = NULL;
int nsearch = 0;
char *target;
char *target, *target_pool;
nvlist_t *policy = NULL;
uint64_t max_txg = UINT64_MAX;
int flags = ZFS_IMPORT_MISSING_LOG;
Expand Down Expand Up @@ -4469,6 +4476,20 @@ main(int argc, char **argv)
error = 0;
target = argv[0];

if (strpbrk(target, "/@") != NULL) {
size_t targetlen;

target_pool = strdup(target);
*strpbrk(target_pool, "/@") = '\0';

target_is_spa = B_FALSE;
targetlen = strlen(target);
if (targetlen && target[targetlen - 1] == '/')
target[targetlen - 1] = '\0';
} else {
target_pool = target;
}

if (dump_opt['e']) {
importargs_t args = { 0 };
nvlist_t *cfg = NULL;
Expand All @@ -4477,8 +4498,10 @@ main(int argc, char **argv)
args.path = searchdirs;
args.can_be_active = B_TRUE;

error = zpool_tryimport(g_zfs, target, &cfg, &args);
error = zpool_tryimport(g_zfs, target_pool, &cfg, &args);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little concerning we didn't have any test coverage for this. What do you think about adding a very basic testcase to tests/zfs-tests/tests/functional/cli_root/zdb/ which covers passing the pool/dataset for imported/exported pools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can use the error message set by zfs_error_aux() in zpool_tryimport() to provide a more descriptive message here?

4568	if (error)
4569	 	fatal("can't open '%s': %s", target, strerror(error));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave this for later.


if (error == 0) {

if (nvlist_add_nvlist(cfg,
ZPOOL_REWIND_POLICY, policy) != 0) {
fatal("can't open '%s': %s",
Expand All @@ -4493,19 +4516,13 @@ main(int argc, char **argv)
(void) printf("\nConfiguration for import:\n");
dump_nvlist(cfg, 8);
}
error = spa_import(target, cfg, NULL,
error = spa_import(target_pool, cfg, NULL,
flags | ZFS_IMPORT_SKIP_MMP);
}
}

if (strpbrk(target, "/@") != NULL) {
size_t targetlen;

target_is_spa = B_FALSE;
targetlen = strlen(target);
if (targetlen && target[targetlen - 1] == '/')
target[targetlen - 1] = '\0';
}
if (target_pool != target)
free(target_pool);

if (error == 0) {
if (target_is_spa || dump_opt['R']) {
Expand Down
4 changes: 3 additions & 1 deletion man/man8/zdb.8
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,9 @@ and, optionally,
.It Sy b Ar offset
Print block pointer
.It Sy d
Decompress the block
Decompress the block. Set environment variable
.Nm ZBD_NO_ZLE
to skip zle when guessing.
.It Sy e
Byte swap the block
.It Sy g
Expand Down
20 changes: 14 additions & 6 deletions module/zfs/dmu_traverse.c
Original file line number Diff line number Diff line change
Expand Up @@ -634,12 +634,20 @@ traverse_impl(spa_t *spa, dsl_dataset_t *ds, uint64_t objset, blkptr_t *rootbp,

err = arc_read(NULL, td->td_spa, rootbp, arc_getbuf_func,
&buf, ZIO_PRIORITY_ASYNC_READ, zio_flags, &flags, czb);
if (err != 0)
return (err);

osp = buf->b_data;
traverse_zil(td, &osp->os_zil_header);
arc_buf_destroy(buf, &buf);
if (err != 0) {
/*
* If both TRAVERSE_HARD and TRAVERSE_PRE are set,
* continue to visitbp so that td_func can be called
* in pre stage, and err will reset to zero.
*/
if (!(td->td_flags & TRAVERSE_HARD) ||
!(td->td_flags & TRAVERSE_PRE))
return (err);
} else {
osp = buf->b_data;
traverse_zil(td, &osp->os_zil_header);
arc_buf_destroy(buf, &buf);
}
}

if (!(flags & TRAVERSE_PREFETCH_DATA) ||
Expand Down
4 changes: 4 additions & 0 deletions module/zfs/zle.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,14 @@ zle_decompress(void *s_start, void *d_start, size_t s_len, size_t d_len, int n)
while (src < s_end && dst < d_end) {
int len = 1 + *src++;
if (len <= n) {
if (src + len > s_end || dst + len > d_end)
Copy link
Contributor

@behlendorf behlendorf Feb 3, 2018

Choose a reason for hiding this comment

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

Check my math on this, but shouldn't this be >= and the same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming src + len == s_end, after running *dst++ = *src++ len times, src becomes s_end. Which is fine, because *src++ dereference before addition.

return (-1);
while (len-- != 0)
*dst++ = *src++;
} else {
len -= n;
if (dst + len > d_end)
return (-1);
while (len-- != 0)
*dst++ = 0;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/runfiles/linux.run
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ tags = ['functional', 'clean_mirror']

[tests/functional/cli_root/zdb]
tests = ['zdb_001_neg', 'zdb_002_pos', 'zdb_003_pos', 'zdb_004_pos',
'zdb_005_pos']
'zdb_005_pos', 'zdb_006_pos']
pre =
post =
tags = ['functional', 'cli_root', 'zdb']
Expand Down
4 changes: 2 additions & 2 deletions tests/zfs-tests/tests/functional/clean_mirror/cleanup.ksh
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ df -F zfs -h | grep "$TESTFS " >/dev/null
[[ $? == 0 ]] && log_must zfs umount -f $TESTDIR
destroy_pool $TESTPOOL

if is_mpath_device $MIRROR_PRIMARY; then
if ( is_mpath_device $MIRROR_PRIMARY || is_loop_device $MIRROR_SECONDARY); then
parted $DEV_DSKDIR/$MIRROR_PRIMARY -s rm 1
fi
if is_mpath_device $MIRROR_SECONDARY; then
if ( is_mpath_device $MIRROR_SECONDARY || is_loop_device $MIRROR_SECONDARY); then
parted $DEV_DSKDIR/$MIRROR_SECONDARY -s rm 1
fi
# recreate and destroy a zpool over the disks to restore the partitions to
Expand Down
3 changes: 2 additions & 1 deletion tests/zfs-tests/tests/functional/cli_root/zdb/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ dist_pkgdata_SCRIPTS = \
zdb_002_pos.ksh \
zdb_003_pos.ksh \
zdb_004_pos.ksh \
zdb_005_pos.ksh
zdb_005_pos.ksh \
zdb_006_pos.ksh
64 changes: 64 additions & 0 deletions tests/zfs-tests/tests/functional/cli_root/zdb/zdb_006_pos.ksh
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#!/bin/ksh

#
# This file and its contents are supplied under the terms of the
# Common Development and Distribution License ("CDDL"), version 1.0.
# You may only use this file in accordance with the terms of version
# 1.0 of the CDDL.
#
# A full copy of the text of the CDDL should have accompanied this
# source. A copy of the CDDL is also available via the Internet at
# http://www.illumos.org/license/CDDL.
#

#
# Copyright (c) 2018 by Nutanix. All rights reserved.
#

. $STF_SUITE/include/libtest.shlib

#
# Description:
# zdb -d will work on imported/exported pool with pool/dataset argument
#
# Strategy:
# 1. Create a pool
# 2. Run zdb -d with pool and dataset arguments.
# 3. Export the pool
# 4. Run zdb -ed with pool and dataset arguments.
#

function cleanup
{
datasetexists $TESTPOOL && destroy_pool $TESTPOOL
Copy link
Contributor

Choose a reason for hiding this comment

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

destroy_pool uses poolexists internally, no need to datasetexists $TESTPOOL && here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copy this from other zdb tests for consistency.

for DISK in $DISKS; do
zpool labelclear -f $DEV_RDSKDIR/$DISK
done
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: need to destroy $TESTPOOL/$TESTFS@snap here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? The pool is already destroyed.


log_assert "Verify zdb -d works on imported/exported pool with pool/dataset argument"
log_onexit cleanup

verify_runnable "global"
verify_disk_count "$DISKS" 2

default_mirror_setup_noexit $DISKS
log_must zfs snap $TESTPOOL/$TESTFS@snap

log_must zdb -d $TESTPOOL
log_must zdb -d $TESTPOOL/
log_must zdb -d $TESTPOOL/$TESTFS
log_must zdb -d $TESTPOOL/$TESTFS@snap

log_must zpool export $TESTPOOL

log_must zdb -ed $TESTPOOL
log_must zdb -ed $TESTPOOL/
log_must zdb -ed $TESTPOOL/$TESTFS
log_must zdb -ed $TESTPOOL/$TESTFS@snap

log_must zpool import $TESTPOOL

cleanup

log_pass "zdb -d works on imported/exported pool with pool/dataset argument"