Skip to content

Commit

Permalink
Fix some DEBUG problems, especially ASSERTs
Browse files Browse the repository at this point in the history 
If compiled with DEBUG ldi_iokit_io_intr would dereference
NULL after a device removal.

Fix numerous noisy ASSERTs:

Change abd_copy to abd_copy_off because of frequent ASSERT
on size in arc_write_ready

EQUIV in arc_buf_try_copy_decompressed_data was failing
because copied was true, but freeze was NULL.  Code
elsewhere guards freezing and thawing against zfs_debug &
ZFS_DEBUG_MODIFY.  We should do that here too.  Also allow
userland EQUIV checks again.  Similar change in
arc_buf_fill.

Catch abd_copy ASSERT:
              zfs`abd_copy+0xed
              zfs`zio_vdev_io_start+0x159
              zfs`zio_nowait+0x282
              zfs`vdev_mirror_io_start+0x349
              zfs`zio_vdev_io_start+0x65c
              zfs`__zio_execute+0x1d2
              spl`taskq_thread+0x205
              kernel`call_continuation+0x17
              524

Fix assert in vdev_uberblock_sync

Fix "abd.c abd_zero_off 1223 : ASSERT3( size > 0) failed (0 > 0)"

Take several ASSERTs from openzfs's and zol's arc.c

Many whitespace and indentation fixes in arc.c

Fix abd assertions in vdev_disk.c following approach taken
for the similar assertions in vdev_file.c.

Fix l2arc_feed_thread and other l2arc ASSERT and whitespace

Fix an ASSERT from upstream that results in sporadic
"kernel[0]: arc.c l2arc_read_done 8133 : ASSERT3(
arc_hdr_size(hdr) < zio->io_size) failed (444928 <
444928)", where the compared sizes are equal.
  • Loading branch information
@rottegift @lundman
rottegift authored and lundman committed Sep 8, 2017
1 parent aa814ff commit cedec90
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 55 deletions.
98 changes: 58 additions & 40 deletions module/zfs/arc.c
View file
Expand Up @@ -1595,6 +1595,8 @@ arc_cksum_verify(arc_buf_t *buf)
return;

if (ARC_BUF_COMPRESSED(buf)) {
ASSERT(hdr->b_l1hdr.b_freeze_cksum == NULL ||
arc_hdr_has_uncompressed_buf(hdr));
return;
}

Expand Down Expand Up @@ -1660,6 +1662,7 @@ arc_cksum_compute(arc_buf_t *buf)

mutex_enter(&buf->b_hdr->b_l1hdr.b_freeze_lock);
if (hdr->b_l1hdr.b_freeze_cksum != NULL) {
ASSERT(arc_hdr_has_uncompressed_buf(hdr));
mutex_exit(&hdr->b_l1hdr.b_freeze_lock);
return;
} else if (ARC_BUF_COMPRESSED(buf)) {
Expand Down Expand Up @@ -1776,6 +1779,8 @@ arc_buf_thaw(arc_buf_t *buf)
* allocate b_thawed.
*/
if (ARC_BUF_COMPRESSED(buf)) {
ASSERT(hdr->b_l1hdr.b_freeze_cksum == NULL ||
arc_hdr_has_uncompressed_buf(hdr));
return;
}

Expand Down Expand Up @@ -1805,6 +1810,8 @@ arc_buf_freeze(arc_buf_t *buf)
return;

if (ARC_BUF_COMPRESSED(buf)) {
ASSERT(hdr->b_l1hdr.b_freeze_cksum == NULL ||
arc_hdr_has_uncompressed_buf(hdr));
return;
}

Expand Down Expand Up @@ -1901,9 +1908,9 @@ arc_buf_try_copy_decompressed_data(arc_buf_t *buf)
* There were no decompressed bufs, so there should not be a
* checksum on the hdr either.
*/
#ifdef _KERNEL
// XXX: ifdef out of userland because it causes zdb to die
EQUIV(!copied, hdr->b_l1hdr.b_freeze_cksum == NULL);
#ifdef DEBUG
if (zfs_flags & ZFS_DEBUG_MODIFY)
EQUIV(!copied, hdr->b_l1hdr.b_freeze_cksum == NULL);
#endif

return (copied);
Expand Down Expand Up @@ -2294,9 +2301,9 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, uint64_t dsobj, arc_fill_flags_t flags)
*/
if (arc_buf_try_copy_decompressed_data(buf)) {
/* Skip byteswapping and checksumming (already done) */
#ifdef _KERNEL
// XXX #ifdef out of userland because it causes zdb to die
ASSERT3P(hdr->b_l1hdr.b_freeze_cksum, !=, NULL);
#ifdef DEBUG
if (zfs_flags & ZFS_DEBUG_MODIFY)
ASSERT3P(hdr->b_l1hdr.b_freeze_cksum, !=, NULL);
#endif
return (0);
} else {
Expand Down Expand Up @@ -2672,6 +2679,8 @@ arc_change_state(arc_state_t *new_state, arc_buf_hdr_t *hdr,
buf);
}
ASSERT3U(bufcnt, ==, buffers);
ASSERT(hdr->b_l1hdr.b_pabd != NULL ||
HDR_HAS_RABD(hdr));

if (hdr->b_l1hdr.b_pabd != NULL) {
(void) refcount_remove_many(
Expand Down Expand Up @@ -5824,7 +5833,7 @@ arc_read_done(zio_t *zio)
arc_hdr_clear_flags(hdr, ARC_FLAG_IO_IN_PROGRESS);
if (callback_cnt == 0) {
ASSERT(HDR_PREFETCH(hdr) || HDR_HAS_RABD(hdr));
ASSERT(hdr->b_l1hdr.b_pabd != NULL || HDR_HAS_RABD(hdr));
ASSERT(hdr->b_l1hdr.b_pabd != NULL || HDR_HAS_RABD(hdr));
}

ASSERT(refcount_is_zero(&hdr->b_l1hdr.b_refcnt) ||
Expand Down Expand Up @@ -6139,7 +6148,7 @@ arc_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
* For authenticated bp's, we do not ask the ZIO layer
* to authenticate them since this will cause the entire
* IO to fail if the key isn't loaded. Instead, we
* defer authentication until arc_buf_fill(), which will
* defer authentication until arc_buf_fill(), which will
* verify the data when the key is available.
*/
if (BP_IS_AUTHENTICATED(bp))
Expand Down Expand Up @@ -6630,6 +6639,7 @@ arc_write_ready(zio_t *zio)
arc_hdr_free_abd(hdr, B_TRUE);
}
ASSERT3P(hdr->b_l1hdr.b_pabd, ==, NULL);
ASSERT(!HDR_HAS_RABD(hdr));
ASSERT(!HDR_SHARED_DATA(hdr));
ASSERT(!arc_buf_is_shared(buf));

Expand Down Expand Up @@ -6692,31 +6702,33 @@ arc_write_ready(zio_t *zio)
* written. Therefore, if they're allowed then we allocate one and copy
* the data into it; otherwise, we share the data directly if we can.
*/
if (ARC_BUF_ENCRYPTED(buf)) {
ASSERT(ARC_BUF_COMPRESSED(buf));
arc_hdr_alloc_abd(hdr, B_TRUE);
abd_copy(hdr->b_crypt_hdr.b_rabd, zio->io_abd, psize);
} else if (zfs_abd_scatter_enabled || !arc_can_share(hdr, buf)) {
if (ARC_BUF_ENCRYPTED(buf)) {
ASSERT(ARC_BUF_COMPRESSED(buf));
arc_hdr_alloc_abd(hdr, B_TRUE);
abd_copy(hdr->b_crypt_hdr.b_rabd, zio->io_abd, psize);
} else if (zfs_abd_scatter_enabled || !arc_can_share(hdr, buf)) {
/*
* Ideally, we would always copy the io_abd into b_pabd, but the
* user may have disabled compressed ARC, thus we must check the
* hdr's compression setting rather than the io_bp's.
*/
if (BP_IS_ENCRYPTED(bp)) {
ASSERT3U(psize, >, 0);
arc_hdr_alloc_abd(hdr, B_TRUE);
abd_copy(hdr->b_crypt_hdr.b_rabd, zio->io_abd, psize);
} else if (arc_hdr_get_compress(hdr) != ZIO_COMPRESS_OFF &&
!ARC_BUF_COMPRESSED(buf)) {
ASSERT3U(psize, >, 0);
arc_hdr_alloc_abd(hdr, B_FALSE);
abd_copy(hdr->b_l1hdr.b_pabd, zio->io_abd, psize);
} else {
ASSERT3U(zio->io_orig_size, ==, arc_hdr_size(hdr));
arc_hdr_alloc_abd(hdr, B_FALSE);
abd_copy_from_buf(hdr->b_l1hdr.b_pabd, buf->b_data,
ASSERT3U(psize, >, 0);
arc_hdr_alloc_abd(hdr, B_TRUE);
abd_copy(hdr->b_crypt_hdr.b_rabd, zio->io_abd, psize);
} else if (arc_hdr_get_compress(hdr) != ZIO_COMPRESS_OFF &&
!ARC_BUF_COMPRESSED(buf)) {
ASSERT3U(psize, >, 0);
arc_hdr_alloc_abd(hdr, B_FALSE);
ASSERT3U(psize, <=, hdr->b_l1hdr.b_pabd->abd_size);
ASSERT3U(psize, <=, zio->io_abd->abd_size);
abd_copy_off(hdr->b_l1hdr.b_pabd, zio->io_abd, 0, 0, psize);
} else {
ASSERT3U(zio->io_orig_size, ==, arc_hdr_size(hdr));
arc_hdr_alloc_abd(hdr, B_FALSE);
abd_copy_from_buf(hdr->b_l1hdr.b_pabd, buf->b_data,
arc_buf_size(buf));
}
}
} else {
ASSERT3P(buf->b_data, ==, abd_to_buf_ephemeral(zio->io_orig_abd));
ASSERT3U(zio->io_orig_size, ==, arc_buf_size(buf));
Expand Down Expand Up @@ -8118,10 +8130,12 @@ l2arc_read_done(zio_t *zio)
* move it and free the buffer.
*/
if (cb->l2rcb_abd != NULL) {
ASSERT3U(arc_hdr_size(hdr), <, zio->io_size);
ASSERT3U(arc_hdr_size(hdr), <=, zio->io_size);
ASSERT3U(arc_hdr_size(hdr), <=, hdr->b_l1hdr.b_pabd->abd_size);
ASSERT3U(arc_hdr_size(hdr), <=, cb->l2rcb_abd->abd_size);
if (zio->io_error == 0) {
abd_copy(hdr->b_l1hdr.b_pabd, cb->l2rcb_abd,
arc_hdr_size(hdr));
abd_copy_off(hdr->b_l1hdr.b_pabd, cb->l2rcb_abd,
0, 0, arc_hdr_size(hdr));
}

/*
Expand All @@ -8145,8 +8159,8 @@ l2arc_read_done(zio_t *zio)
/*
* Check this survived the L2ARC journey.
*/
ASSERT(zio->io_abd == hdr->b_l1hdr.b_pabd ||
(HDR_HAS_RABD(hdr) && zio->io_abd == hdr->b_crypt_hdr.b_rabd));
ASSERT(zio->io_abd == hdr->b_l1hdr.b_pabd ||
(HDR_HAS_RABD(hdr) && zio->io_abd == hdr->b_crypt_hdr.b_rabd));
zio->io_bp_copy = cb->l2rcb_bp; /* XXX fix in L2ARC 2.0 */
zio->io_bp = &zio->io_bp_copy; /* XXX fix in L2ARC 2.0 */

Expand Down Expand Up @@ -8371,7 +8385,8 @@ l2arc_apply_transforms(spa_t *spa, arc_buf_hdr_t *hdr, uint64_t asize,

ASSERT((HDR_GET_COMPRESS(hdr) != ZIO_COMPRESS_OFF &&
!HDR_COMPRESSION_ENABLED(hdr)) ||
HDR_ENCRYPTED(hdr) || HDR_SHARED_DATA(hdr));
HDR_ENCRYPTED(hdr) || HDR_SHARED_DATA(hdr) || psize != asize);
ASSERT3U(psize, <=, asize);

/*
* If this is just a shared buffer, we simply copy the data. Otherwise
Expand All @@ -8382,10 +8397,11 @@ l2arc_apply_transforms(spa_t *spa, arc_buf_hdr_t *hdr, uint64_t asize,
!HDR_ENCRYPTED(hdr)) {
ASSERT3U(size, ==, psize);
to_write = abd_alloc_for_io(asize, ismd);
abd_copy(to_write, hdr->b_l1hdr.b_pabd, size);
if (size != asize)
abd_zero_off(to_write, size, asize - size);
goto out;
ASSERT3S(size,<=,hdr->b_l1hdr.b_pabd->abd_size);
abd_copy_off(to_write, hdr->b_l1hdr.b_pabd, 0, 0, size);
if (size != asize)
abd_zero_off(to_write, size, asize - size);
goto out;
}

if (compress != ZIO_COMPRESS_OFF && !HDR_COMPRESSION_ENABLED(hdr)) {
Expand Down Expand Up @@ -8550,7 +8566,7 @@ l2arc_write_buffers(spa_t *spa, l2arc_dev_t *dev, uint64_t target_sz)

ASSERT3U(HDR_GET_PSIZE(hdr), >, 0);
ASSERT3U(arc_hdr_size(hdr), >, 0);
ASSERT(hdr->b_l1hdr.b_pabd != NULL ||
ASSERT(hdr->b_l1hdr.b_pabd != NULL ||
HDR_HAS_RABD(hdr));
uint64_t psize = HDR_GET_PSIZE(hdr);
uint64_t asize = vdev_psize_to_asize(dev->l2ad_vdev,
Expand All @@ -8572,8 +8588,9 @@ l2arc_write_buffers(spa_t *spa, l2arc_dev_t *dev, uint64_t target_sz)
ASSERT(HDR_HAS_L1HDR(hdr));

ASSERT3U(HDR_GET_PSIZE(hdr), >, 0);
ASSERT(hdr->b_l1hdr.b_pabd != NULL ||
HDR_HAS_RABD(hdr));
ASSERT(hdr->b_l1hdr.b_pabd != NULL ||
HDR_HAS_RABD(hdr));
ASSERT3U(arc_hdr_size(hdr), >, 0);

/*
* If this header has b_rabd, we can use this since it
Expand Down Expand Up @@ -8662,7 +8679,8 @@ l2arc_write_buffers(spa_t *spa, l2arc_dev_t *dev, uint64_t target_sz)
} else {
to_write = abd_alloc_for_io(asize,
HDR_ISTYPE_METADATA(hdr));
abd_copy(to_write, hdr->b_l1hdr.b_pabd, psize);
ASSERT3S(psize, <=, hdr->b_l1hdr.b_pabd->abd_size);
abd_copy_off(to_write, hdr->b_l1hdr.b_pabd, 0, 0, psize);
if (asize != psize) {
abd_zero_off(to_write, psize,
asize - psize);
Expand Down
31 changes: 23 additions & 8 deletions module/zfs/ldi_iokit.cpp
View file
Expand Up @@ -1193,20 +1193,35 @@ ldi_iokit_io_intr(void *target, void *parameter,
/* In debug builds, verify buffer pointers */
ASSERT3U(lbp, !=, 0);
ASSERT3U(iobp, !=, 0);

if (!iobp || !lbp) {
printf("%s missing a buffer\n", __func__);
return;
}

ASSERT3U(iobp->iomem, !=, 0);

if (!iobp->iomem) {
printf("%s missing iobp->iomem\n", __func__);
return;
}

// this is very very very noisy in --enable-boot
//ASSERT3U(ldi_zfs_handle, !=, 0);

if (actualByteCount == 0 ||
actualByteCount != lbp->b_bcount ||
status != kIOReturnSuccess) {
dprintf("%s %s %llx / %llx\n", __func__,
printf("%s %s %llx / %llx\n", __func__,
"actualByteCount != lbp->b_bcount",
actualByteCount, lbp->b_bcount);
dprintf("%s status %d %d %s\n", __func__, status,
ldi_zfs_handle->errnoFromReturn(status),
ldi_zfs_handle->stringFromReturn(status));
}
if (!iobp || !lbp) {
dprintf("%s missing a buffer\n", __func__);
return;
if (ldi_zfs_handle)
printf("%s status %d %d %s\n", __func__, status,
ldi_zfs_handle->errnoFromReturn(status),
ldi_zfs_handle->stringFromReturn(status));
else
printf("%s status %d ldi_zfs_handle is NULL\n",
__func__, status);
}
#endif

Expand Down
14 changes: 10 additions & 4 deletions module/zfs/vdev_disk.c
View file
Expand Up @@ -695,9 +695,13 @@ vdev_disk_io_intr(ldi_buf_t *bp)
zio->io_error = SET_ERROR(EIO);

if (zio->io_type == ZIO_TYPE_READ) {
abd_return_buf_copy(zio->io_abd, bp->b_un.b_addr, zio->io_size);
VERIFY3S(zio->io_abd->abd_size,>=,zio->io_size);
abd_return_buf_copy_off(zio->io_abd, bp->b_un.b_addr,
0, zio->io_size, zio->io_abd->abd_size);
} else {
abd_return_buf(zio->io_abd, bp->b_un.b_addr, zio->io_size);
VERIFY3S(zio->io_abd->abd_size,>=,zio->io_size);
abd_return_buf_off(zio->io_abd, bp->b_un.b_addr,
0, zio->io_size, zio->io_abd->abd_size);
}

kmem_free(vb, sizeof (vdev_buf_t));
Expand Down Expand Up @@ -843,11 +847,13 @@ vdev_disk_io_start(zio_t *zio)
bp->b_bcount = zio->io_size;

if (zio->io_type == ZIO_TYPE_READ) {
ASSERT3S(zio->io_abd->abd_size,>=,zio->io_size);
bp->b_un.b_addr =
abd_borrow_buf(zio->io_abd, zio->io_size);
abd_borrow_buf(zio->io_abd, zio->io_abd->abd_size);
} else {
ASSERT3S(zio->io_abd->abd_size,>=,zio->io_size);
bp->b_un.b_addr =
abd_borrow_buf_copy(zio->io_abd, zio->io_size);
abd_borrow_buf_copy(zio->io_abd, zio->io_abd->abd_size);
}

bp->b_lblkno = lbtodb(zio->io_offset);
Expand Down
3 changes: 2 additions & 1 deletion module/zfs/vdev_label.c
View file
Expand Up @@ -1166,7 +1166,8 @@ vdev_uberblock_sync(zio_t *zio, uberblock_t *ub, vdev_t *vd, int flags)
/* Copy the uberblock_t into the ABD */
abd_t *ub_abd = abd_alloc_for_io(VDEV_UBERBLOCK_SIZE(vd), B_TRUE);
abd_zero(ub_abd, VDEV_UBERBLOCK_SIZE(vd));
abd_copy_from_buf(ub_abd, ub, sizeof (uberblock_t));
ASSERT3U(sizeof (uberblock_t), <=, ub_abd->abd_size);
abd_copy_from_buf_off(ub_abd, ub, 0, sizeof (uberblock_t));

for (int l = 0; l < VDEV_LABELS; l++)
vdev_label_write(zio, vd, l, ub_abd,
Expand Down
10 changes: 8 additions & 2 deletions module/zfs/zio.c
View file
Expand Up @@ -1540,7 +1540,11 @@ zio_write_compress(zio_t *zio)
} else {
abd_t *cdata = abd_get_from_buf(cbuf, lsize);
abd_take_ownership_of_buf(cdata, B_TRUE);
abd_zero_off(cdata, psize, rounded - psize);
if (rounded != psize) {
// sometimes rounded - psize == 0, bad in DEBUG abd.c
ASSERT3U(rounded, >, psize);
abd_zero_off(cdata, psize, rounded - psize);
}
psize = rounded;
zio_push_transform(zio, cdata,
psize, lsize, NULL);
Expand Down Expand Up @@ -3362,7 +3366,9 @@ zio_vdev_io_start(zio_t *zio)
abd_t *abuf = abd_alloc_sametype(zio->io_abd, asize);
ASSERT(vd == vd->vdev_top);
if (zio->io_type == ZIO_TYPE_WRITE) {
abd_copy(abuf, zio->io_abd, zio->io_size);
ASSERT3U(zio->io_size, <=, zio->io_abd->abd_size);
ASSERT3U(zio->io_size, <=, abuf->abd_size);
abd_copy_off(abuf, zio->io_abd, 0, 0, zio->io_size);
abd_zero_off(abuf, zio->io_size, asize - zio->io_size);
}
zio_push_transform(zio, abuf, asize, asize, zio_subblock);
Expand Down

0 comments on commit cedec90

Please sign in to comment.