svn commit: r337025 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
Alexander Motin
mav at FreeBSD.org
Wed Aug 1 02:39:45 UTC 2018
Author: mav
Date: Wed Aug 1 02:39:44 2018
New Revision: 337025
URL: https://svnweb.freebsd.org/changeset/base/337025
Log:
MFV r337022:
9403 assertion failed in arc_buf_destroy() when concurrently reading block with checksum error
This assertion (VERIFY) failure was reported when reading a block. Turns out
the problem is that if we get an i/o error (ECKSUM in this case), and there
are multiple concurrent ARC reads of the same block (from different clones),
then the ARC will put multiple buf's on the same ANON hdr, which isn't
supposed to happen, and then causes a panic when we try to arc_buf_destroy()
the buf.
illumos/illumos-gate at fa98e487a9619b7902f218663be219e787a57dad
Reviewed by: George Wilson <george.wilson at delphix.com>
Reviewed by: Paul Dagnelie <pcd at delphix.com>
Reviewed by: Pavel Zakharov <pavel.zakharov at delphix.com>
Approved by: Matt Ahrens <mahrens at delphix.com>
Author: Matthew Ahrens <mahrens at delphix.com>
Modified:
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio_compress.c
Directory Properties:
head/sys/cddl/contrib/opensolaris/ (props changed)
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c Wed Aug 1 00:41:02 2018 (r337024)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c Wed Aug 1 02:39:44 2018 (r337025)
@@ -21,7 +21,7 @@
/*
* Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, Joyent, Inc.
- * Copyright (c) 2011, 2017 by Delphix. All rights reserved.
+ * Copyright (c) 2011, 2018 by Delphix. All rights reserved.
* Copyright (c) 2014 by Saso Kiselkov. All rights reserved.
* Copyright 2017 Nexenta Systems, Inc. All rights reserved.
*/
@@ -5148,12 +5148,13 @@ arc_getbuf_func(zio_t *zio, const zbookmark_phys_t *zb
arc_buf_t *buf, void *arg)
{
arc_buf_t **bufp = arg;
-
if (buf == NULL) {
+ ASSERT(zio == NULL || zio->io_error != 0);
*bufp = NULL;
} else {
+ ASSERT(zio == NULL || zio->io_error == 0);
*bufp = buf;
- ASSERT(buf->b_data);
+ ASSERT(buf->b_data != NULL);
}
}
@@ -5181,6 +5182,7 @@ arc_read_done(zio_t *zio)
arc_callback_t *callback_list;
arc_callback_t *acb;
boolean_t freeable = B_FALSE;
+ boolean_t no_zio_error = (zio->io_error == 0);
/*
* The hdr was inserted into hash-table and removed from lists
@@ -5206,7 +5208,7 @@ arc_read_done(zio_t *zio)
ASSERT3P(hash_lock, !=, NULL);
}
- if (zio->io_error == 0) {
+ if (no_zio_error) {
/* byteswap if necessary */
if (BP_SHOULD_BYTESWAP(zio->io_bp)) {
if (BP_GET_LEVEL(zio->io_bp) > 0) {
@@ -5227,8 +5229,7 @@ arc_read_done(zio_t *zio)
callback_list = hdr->b_l1hdr.b_acb;
ASSERT3P(callback_list, !=, NULL);
- if (hash_lock && zio->io_error == 0 &&
- hdr->b_l1hdr.b_state == arc_anon) {
+ if (hash_lock && no_zio_error && hdr->b_l1hdr.b_state == arc_anon) {
/*
* Only call arc_access on anonymous buffers. This is because
* if we've issued an I/O for an evicted buffer, we've already
@@ -5251,20 +5252,38 @@ arc_read_done(zio_t *zio)
callback_cnt++;
- if (zio->io_error != 0)
- continue;
-
- int error = arc_buf_alloc_impl(hdr, acb->acb_private,
- acb->acb_compressed,
- B_TRUE, &acb->acb_buf);
- if (error != 0) {
- arc_buf_destroy(acb->acb_buf, acb->acb_private);
- acb->acb_buf = NULL;
+ if (no_zio_error) {
+ int error = arc_buf_alloc_impl(hdr, acb->acb_private,
+ acb->acb_compressed, zio->io_error == 0,
+ &acb->acb_buf);
+ if (error != 0) {
+ /*
+ * Decompression failed. Set io_error
+ * so that when we call acb_done (below),
+ * we will indicate that the read failed.
+ * Note that in the unusual case where one
+ * callback is compressed and another
+ * uncompressed, we will mark all of them
+ * as failed, even though the uncompressed
+ * one can't actually fail. In this case,
+ * the hdr will not be anonymous, because
+ * if there are multiple callbacks, it's
+ * because multiple threads found the same
+ * arc buf in the hash table.
+ */
+ zio->io_error = error;
+ }
}
-
- if (zio->io_error == 0)
- zio->io_error = error;
}
+ /*
+ * If there are multiple callbacks, we must have the hash lock,
+ * because the only way for multiple threads to find this hdr is
+ * in the hash table. This ensures that if there are multiple
+ * callbacks, the hdr is not anonymous. If it were anonymous,
+ * we couldn't use arc_buf_destroy() in the error case below.
+ */
+ ASSERT(callback_cnt < 2 || hash_lock != NULL);
+
hdr->b_l1hdr.b_acb = NULL;
arc_hdr_clear_flags(hdr, ARC_FLAG_IO_IN_PROGRESS);
if (callback_cnt == 0) {
@@ -5276,7 +5295,7 @@ arc_read_done(zio_t *zio)
ASSERT(refcount_is_zero(&hdr->b_l1hdr.b_refcnt) ||
callback_list != NULL);
- if (zio->io_error == 0) {
+ if (no_zio_error) {
arc_hdr_verify(hdr, zio->io_bp);
} else {
arc_hdr_set_flags(hdr, ARC_FLAG_IO_ERROR);
@@ -5309,7 +5328,16 @@ arc_read_done(zio_t *zio)
/* execute each callback and free its structure */
while ((acb = callback_list) != NULL) {
- if (acb->acb_done) {
+ if (acb->acb_done != NULL) {
+ if (zio->io_error != 0 && acb->acb_buf != NULL) {
+ /*
+ * If arc_buf_alloc_impl() fails during
+ * decompression, the buf will still be
+ * allocated, and needs to be freed here.
+ */
+ arc_buf_destroy(acb->acb_buf, acb->acb_private);
+ acb->acb_buf = NULL;
+ }
acb->acb_done(zio, &zio->io_bookmark, zio->io_bp,
acb->acb_buf, acb->acb_private);
}
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c Wed Aug 1 00:41:02 2018 (r337024)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c Wed Aug 1 02:39:44 2018 (r337025)
@@ -21,7 +21,7 @@
/*
* Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
* Copyright 2011 Nexenta Systems, Inc. All rights reserved.
- * Copyright (c) 2012, 2017 by Delphix. All rights reserved.
+ * Copyright (c) 2012, 2018 by Delphix. All rights reserved.
* Copyright (c) 2013 by Saso Kiselkov. All rights reserved.
* Copyright (c) 2013, Joyent, Inc. All rights reserved.
* Copyright (c) 2014 Spectra Logic Corporation, All rights reserved.
@@ -1000,8 +1000,15 @@ dbuf_read_done(zio_t *zio, const zbookmark_phys_t *zb,
ASSERT(refcount_count(&db->db_holds) > 0);
ASSERT(db->db_buf == NULL);
ASSERT(db->db.db_data == NULL);
- if (db->db_level == 0 && db->db_freed_in_flight) {
- /* we were freed in flight; disregard any error */
+ if (buf == NULL) {
+ /* i/o error */
+ ASSERT(zio == NULL || zio->io_error != 0);
+ ASSERT(db->db_blkid != DMU_BONUS_BLKID);
+ ASSERT3P(db->db_buf, ==, NULL);
+ db->db_state = DB_UNCACHED;
+ } else if (db->db_level == 0 && db->db_freed_in_flight) {
+ /* freed in flight */
+ ASSERT(zio == NULL || zio->io_error == 0);
if (buf == NULL) {
buf = arc_alloc_buf(db->db_objset->os_spa,
db, DBUF_GET_BUFC_TYPE(db), db->db.db_size);
@@ -1012,13 +1019,11 @@ dbuf_read_done(zio_t *zio, const zbookmark_phys_t *zb,
db->db_freed_in_flight = FALSE;
dbuf_set_data(db, buf);
db->db_state = DB_CACHED;
- } else if (buf != NULL) {
+ } else {
+ /* success */
+ ASSERT(zio == NULL || zio->io_error == 0);
dbuf_set_data(db, buf);
db->db_state = DB_CACHED;
- } else {
- ASSERT(db->db_blkid != DMU_BONUS_BLKID);
- ASSERT3P(db->db_buf, ==, NULL);
- db->db_state = DB_UNCACHED;
}
cv_broadcast(&db->db_changed);
dbuf_rele_and_unlock(db, NULL);
@@ -2431,6 +2436,13 @@ dbuf_prefetch_indirect_done(zio_t *zio, const zbookmar
ASSERT3S(dpa->dpa_zb.zb_level, <, dpa->dpa_curlevel);
ASSERT3S(dpa->dpa_curlevel, >, 0);
+
+ if (abuf == NULL) {
+ ASSERT(zio == NULL || zio->io_error != 0);
+ kmem_free(dpa, sizeof (*dpa));
+ return;
+ }
+ ASSERT(zio == NULL || zio->io_error == 0);
/*
* The dpa_dnode is only valid if we are called with a NULL
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio_compress.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio_compress.c Wed Aug 1 00:41:02 2018 (r337024)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio_compress.c Wed Aug 1 02:39:44 2018 (r337025)
@@ -25,7 +25,7 @@
*/
/*
* Copyright (c) 2013 by Saso Kiselkov. All rights reserved.
- * Copyright (c) 2013, 2016 by Delphix. All rights reserved.
+ * Copyright (c) 2013, 2018 by Delphix. All rights reserved.
*/
#include <sys/zfs_context.h>
@@ -56,6 +56,12 @@ static zcomp_stats_t zcomp_stats = {
kstat_t *zcomp_ksp;
/*
+ * If nonzero, every 1/X decompression attempts will fail, simulating
+ * an undetected memory error.
+ */
+uint64_t zio_decompress_fail_fraction = 0;
+
+/*
* Compression vectors.
*/
zio_compress_info_t zio_compress_table[ZIO_COMPRESS_FUNCTIONS] = {
@@ -171,6 +177,16 @@ zio_decompress_data(enum zio_compress c, abd_t *src, v
void *tmp = abd_borrow_buf_copy(src, s_len);
int ret = zio_decompress_data_buf(c, tmp, dst, s_len, d_len);
abd_return_buf(src, tmp, s_len);
+
+ /*
+ * Decompression shouldn't fail, because we've already verifyied
+ * the checksum. However, for extra protection (e.g. against bitflips
+ * in non-ECC RAM), we handle this error (and test it).
+ */
+ ASSERT0(ret);
+ if (zio_decompress_fail_fraction != 0 &&
+ spa_get_random(zio_decompress_fail_fraction) == 0)
+ ret = SET_ERROR(EINVAL);
return (ret);
}
More information about the svn-src-head
mailing list