nopwrites on dmu_sync-ed blocks can result in a panic

After a device has been removed, any nopwrites for blocks on that
indirect vdev should be ignored and a new block should be allocated. The
original code attempted to handle this but used the wrong block pointer
when checking for indirect vdevs and failed to check all DVAs.

This change corrects both of these issues and modifies the test case
to ensure that it properly tests nopwrites with device removal.

Reviewed-by: Prakash Surya <prakash.surya@delphix.com>
Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: George Wilson <gwilson@delphix.com>
Closes #14235

diff --git a/module/zfs/zio.c b/module/zfs/zio.c
index 46904b800b7f34c94246aa5f87afa95ef2aaacbb..2ae42e2df4b68a6b8c92b05d8cf0b5f96b5d1ef8 100644 (file)
--- a/module/zfs/zio.c
+++ b/module/zfs/zio.c
@@ -20,7 +20,7 @@
  */
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2011, 2020 by Delphix. All rights reserved.
+ * Copyright (c) 2011, 2022 by Delphix. All rights reserved.
  * Copyright (c) 2011 Nexenta Systems, Inc. All rights reserved.
  * Copyright (c) 2017, Intel Corporation.
  * Copyright (c) 2019, Klara Inc.
@@ -2974,6 +2974,7 @@ zio_nop_write(zio_t *zio)
        blkptr_t *bp_orig = &zio->io_bp_orig;
        zio_prop_t *zp = &zio->io_prop;
 
+       ASSERT(BP_IS_HOLE(bp));
        ASSERT(BP_GET_LEVEL(bp) == 0);
        ASSERT(!(zio->io_flags & ZIO_FLAG_IO_REWRITE));
        ASSERT(zp->zp_nopwrite);
@@ -3007,8 +3008,7 @@ zio_nop_write(zio_t *zio)
                ASSERT3U(BP_GET_PSIZE(bp), ==, BP_GET_PSIZE(bp_orig));
                ASSERT3U(BP_GET_LSIZE(bp), ==, BP_GET_LSIZE(bp_orig));
                ASSERT(zp->zp_compress != ZIO_COMPRESS_OFF);
-               ASSERT(memcmp(&bp->blk_prop, &bp_orig->blk_prop,
-                   sizeof (uint64_t)) == 0);
+               ASSERT3U(bp->blk_prop, ==, bp_orig->blk_prop);
 
                /*
                 * If we're overwriting a block that is currently on an
@@ -3016,11 +3016,13 @@ zio_nop_write(zio_t *zio)
                 * allow a new block to be allocated on a concrete vdev.
                 */
                spa_config_enter(zio->io_spa, SCL_VDEV, FTAG, RW_READER);
-               vdev_t *tvd = vdev_lookup_top(zio->io_spa,
-                   DVA_GET_VDEV(&bp->blk_dva[0]));
-               if (tvd->vdev_ops == &vdev_indirect_ops) {
-                       spa_config_exit(zio->io_spa, SCL_VDEV, FTAG);
-                       return (zio);
+               for (int d = 0; d < BP_GET_NDVAS(bp_orig); d++) {
+                       vdev_t *tvd = vdev_lookup_top(zio->io_spa,
+                           DVA_GET_VDEV(&bp_orig->blk_dva[d]));
+                       if (tvd->vdev_ops == &vdev_indirect_ops) {
+                               spa_config_exit(zio->io_spa, SCL_VDEV, FTAG);
+                               return (zio);
+                       }
                }
                spa_config_exit(zio->io_spa, SCL_VDEV, FTAG);
 

diff --git a/tests/zfs-tests/tests/functional/nopwrite/nopwrite.shlib b/tests/zfs-tests/tests/functional/nopwrite/nopwrite.shlib
index 139b4b26e5ed0f46931b177e6593e85d7f3795ee..a6d1eb2c769f5a5ae0eebd2cacdd5bee635cbfd9 100644 (file)
--- a/tests/zfs-tests/tests/functional/nopwrite/nopwrite.shlib
+++ b/tests/zfs-tests/tests/functional/nopwrite/nopwrite.shlib
@@ -57,12 +57,12 @@ function verify_nopwrite
        within_percent $origin_used $snap_refer $high || return 1
 
        #
-       # The comparisons below should pass regardless of nopwrite. They're
-       # here for sanity.
+       # The comparisons below should be within 90% regardless of nopwrite.
+       # They're here for sanity.
        #
        typeset deadlist=$(zdb -Pddd $clone | awk '/Deadlist:/ {print $2}')
-       within_percent $deadlist $clone_written $high || return 1
-       within_percent $snap_refer $snap_written $high || return 1
+       within_percent $deadlist $clone_written 90 || return 1
+       within_percent $snap_refer $snap_written 90 || return 1
 
        return 0
 }

diff --git a/tests/zfs-tests/tests/functional/removal/removal_nopwrite.ksh b/tests/zfs-tests/tests/functional/removal/removal_nopwrite.ksh
index cede81ad60c251d379555fa9d8a31ac633da298c..3533c41cac2863e3811d66625d5c593887bf026b 100755 (executable)
--- a/tests/zfs-tests/tests/functional/removal/removal_nopwrite.ksh
+++ b/tests/zfs-tests/tests/functional/removal/removal_nopwrite.ksh
@@ -15,7 +15,7 @@
 #
 
 #
-# Copyright (c) 2019 by Delphix. All rights reserved.
+# Copyright (c) 2019, 2022 by Delphix. All rights reserved.
 #
 
 . $STF_SUITE/include/libtest.shlib
@@ -24,19 +24,35 @@
 
 default_setup_noexit "$DISKS"
 log_onexit default_cleanup_noexit
-BLOCKSIZE=8192
+
+#
+# Randomly pick a device to remove
+#
+DISK_TOKENS=( $DISKS )
+DISK_INDEX_TO_REMOVE=$((RANDOM%${#DISK_TOKENS[@]}))
+DISK_TO_REMOVE=${DISK_TOKENS[${DISK_INDEX_TO_REMOVE}]}
 
 origin="$TESTPOOL/$TESTFS"
 
 log_must zfs set compress=on $origin
 log_must zfs set checksum=skein $origin
 
+log_must zfs set copies=1 $origin
 log_must zfs set recordsize=8k $origin
 dd if=/dev/urandom of=$TESTDIR/file_8k bs=1024k count=$MEGS oflag=sync \
     conv=notrunc >/dev/null 2>&1 || log_fail "dd into $TESTDIR/file failed."
+log_must zfs set copies=3 $origin
+dd if=/dev/urandom of=$TESTDIR/file_8k_copies bs=1024k count=$MEGS oflag=sync \
+    conv=notrunc >/dev/null 2>&1 || log_fail "dd into $TESTDIR/file failed."
+
+log_must zfs set copies=1 $origin
 log_must zfs set recordsize=128k $origin
 dd if=/dev/urandom of=$TESTDIR/file_128k bs=1024k count=$MEGS oflag=sync \
     conv=notrunc >/dev/null 2>&1 || log_fail "dd into $TESTDIR/file failed."
+log_must zfs set copies=3 $origin
+dd if=/dev/urandom of=$TESTDIR/file_128k_copies bs=1024k \
+    count=$MEGS oflag=sync conv=notrunc >/dev/null 2>&1 || \
+    log_fail "dd into $TESTDIR/file failed."
 
 zfs snapshot $origin@a || log_fail "zfs snap failed"
 log_must zfs clone $origin@a $origin/clone
@@ -44,22 +60,33 @@ log_must zfs clone $origin@a $origin/clone
 #
 # Verify that nopwrites work prior to removal
 #
+log_must zfs set copies=1 $origin/clone
 log_must zfs set recordsize=8k $origin/clone
 dd if=/$TESTDIR/file_8k of=/$TESTDIR/clone/file_8k bs=1024k \
      oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
 log_must verify_nopwrite $origin $origin@a $origin/clone
+log_must zfs set copies=3 $origin/clone
+dd if=/$TESTDIR/file_8k_copies of=/$TESTDIR/clone/file_8k_copies bs=1024k \
+     oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
+log_must verify_nopwrite $origin $origin@a $origin/clone
 
+log_must zfs set copies=1 $origin/clone
 log_must zfs set recordsize=128k $origin/clone
 dd if=/$TESTDIR/file_128k of=/$TESTDIR/clone/file_128k bs=1024k \
      oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
 log_must verify_nopwrite $origin $origin@a $origin/clone
+log_must zfs set copies=3 $origin/clone
+dd if=/$TESTDIR/file_128k_copies of=/$TESTDIR/clone/file_128k_copies bs=1024k \
+     oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
+log_must verify_nopwrite $origin $origin@a $origin/clone
 
 #
 # Remove a device before testing nopwrites again
 #
-log_must zpool remove $TESTPOOL $REMOVEDISK
+log_note "Removing: $DISK_TO_REMOVE"
+log_must zpool remove $TESTPOOL $DISK_TO_REMOVE
 log_must wait_for_removal $TESTPOOL
-log_mustnot vdevs_in_pool $TESTPOOL $REMOVEDISK
+log_mustnot vdevs_in_pool $TESTPOOL $DISK_TO_REMOVE
 
 #
 # Normally, we expect nopwrites to avoid allocating new blocks, but
@@ -71,17 +98,27 @@ log_mustnot vdevs_in_pool $TESTPOOL $REMOVEDISK
 #
 # Perform a direct zil nopwrite test
 #
+log_must zfs set copies=1 $origin/clone
 log_must zfs set recordsize=8k $origin/clone
 dd if=/$TESTDIR/file_8k of=/$TESTDIR/clone/file_8k bs=1024k \
      oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
 log_mustnot verify_nopwrite $origin $origin@a $origin/clone
+log_must zfs set copies=3 $origin/clone
+dd if=/$TESTDIR/file_8k_copies of=/$TESTDIR/clone/file_8k_copies bs=1024k \
+     oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
+log_mustnot verify_nopwrite $origin $origin@a $origin/clone
 
 #
 # Perform an indirect zil nopwrite test
 #
+log_must zfs set copies=1 $origin/clone
 log_must zfs set recordsize=128k $origin/clone
 dd if=/$TESTDIR/file_128k of=/$TESTDIR/clone/file_128k bs=1024k \
      oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
 log_mustnot verify_nopwrite $origin $origin@a $origin/clone
+log_must zfs set copies=3 $origin/clone
+dd if=/$TESTDIR/file_128k_copies of=/$TESTDIR/clone/file_128k_copies bs=1024k \
+     oflag=sync conv=notrunc >/dev/null 2>&1 || log_fail "dd failed."
+log_mustnot verify_nopwrite $origin $origin@a $origin/clone
 
 log_pass "Remove works with nopwrite."