Tighten up 'zpool remove' space check

The available space check in spa_vdev_remove_top_check() is intended
to verify there is enough available space on the other devices before
starting the removal process.  This is obviously a good idea but the
current check can significantly overestimate the space requirements
and often prevent removal when it clearly should be possible.  This
change reworks the check slightly.

First, we reduce the slop space requirement to be inline with with
the capacity of the pool after the device has been removed.  This
way if a large device is accidentally added to a small pool the
vastly increased slop requirement of the larger pool won't prevent
removal.  For example, it was previously possible that even newly
added empty vdevs couldn't be removed due to the increased slop
requirement.  This was particularly unfortunate since one of the
main motivations for this feature was to allow such mistakes to be
corrected.

Second, we handle the case of very small pools where the minimum
slop size of 128M represents a large fraction (>25%) of the total
pool capacity.  In this case, it's reasonable to reduce the extra
slop requirement.

Lastly it's worth mentioning that by allowing the removal to start
with close to the minimum required free space it is more likely that
an active pool close to capacity may fail the removal process.  This
failure case has always been possible since we can't know what new
data will be written during the removal process.  It is correctly
handled and there's no danger to the pool.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue #11356

Author: behlendorf

include/sys/spa.h

@@ -1041,7 +1041,7 @@ extern uint64_t spa_freeze_txg(spa_t *spa);
 extern uint64_t spa_get_worst_case_asize(spa_t *spa, uint64_t lsize);
 extern uint64_t spa_get_dspace(spa_t *spa);
 extern uint64_t spa_get_checkpoint_space(spa_t *spa);
-extern uint64_t spa_get_slop_space(spa_t *spa);
+extern uint64_t spa_get_slop_space(spa_t *spa, int64_t);
 extern void spa_update_dspace(spa_t *spa);
 extern uint64_t spa_version(spa_t *spa);
 extern boolean_t spa_deflate(spa_t *spa);

module/zfs/dsl_pool.c

@@ -861,7 +861,7 @@ dsl_pool_adjustedsize(dsl_pool_t *dp, zfs_space_check_t slop_policy)
 
 	space = spa_get_dspace(spa)
 	    - spa_get_checkpoint_space(spa) - spa_deferred_frees;
-	resv = spa_get_slop_space(spa);
+	resv = spa_get_slop_space(spa, 0);
 
 	switch (slop_policy) {
 	case ZFS_SPACE_CHECK_NORMAL:

module/zfs/metaslab.c

@@ -4261,7 +4261,7 @@ metaslab_sync_done(metaslab_t *msp, uint64_t txg)
 
 	uint64_t free_space = metaslab_class_get_space(spa_normal_class(spa)) -
 	    metaslab_class_get_alloc(spa_normal_class(spa));
-	if (free_space <= spa_get_slop_space(spa) || vd->vdev_removing) {
+	if (free_space <= spa_get_slop_space(spa, 0) || vd->vdev_removing) {
 		defer_allowed = B_FALSE;
 	}
 

module/zfs/spa_misc.c

@@ -1778,14 +1778,16 @@ spa_get_worst_case_asize(spa_t *spa, uint64_t lsize)
 /*
  * Return the amount of slop space in bytes.  It is 1/32 of the pool (3.2%),
  * or at least 128MB, unless that would cause it to be more than half the
- * pool size.
+ * pool size.  An adjustment may be passed to apply the calculation to a
+ * modified pool size.  For example, in the case of top-level device removal
+ * where the total pool capacity will be reduced.
  *
  * See the comment above spa_slop_shift for details.
  */
 uint64_t
-spa_get_slop_space(spa_t *spa)
+spa_get_slop_space(spa_t *spa, int64_t adjustment)
 {
-	uint64_t space = spa_get_dspace(spa);
+	uint64_t space = spa_get_dspace(spa) + adjustment;
 	return (MAX(space >> spa_slop_shift, MIN(space >> 1, spa_min_slop)));
 }
 

module/zfs/vdev_removal.c

@@ -1995,19 +1995,44 @@ spa_vdev_remove_top_check(vdev_t *vd)
 		if (available < vd->vdev_stat.vs_alloc)
 			return (SET_ERROR(ENOSPC));
 	} else {
-		/* available space in the pool's normal class */
-		uint64_t available = dsl_dir_space_available(
-		    spa->spa_dsl_pool->dp_root_dir, NULL, 0, B_TRUE);
-		if (available <
-		    vd->vdev_stat.vs_dspace + spa_get_slop_space(spa)) {
-			/*
-			 * This is a normal device. There has to be enough free
-			 * space to remove the device and leave double the
-			 * "slop" space (i.e. we must leave at least 3% of the
-			 * pool free, in addition to the normal slop space).
-			 */
+		vdev_stat_t *rs = &spa->spa_root_vdev->vdev_stat;
+		vdev_stat_t *vs = &vd->vdev_stat;
+
+		/*
+		 * This is a normal device. There has to be enough free
+		 * space in the pool's normal class to copy the allocated
+		 * space from the device being removed, this will account
+		 * for both reserved and "slop" space.  Any available
+		 * capacity on the device being removed is deducted from
+		 * the total available pool capacity.
+		 */
+		int64_t available = dsl_dir_space_available(
+		    spa->spa_dsl_pool->dp_root_dir, NULL, 0, B_TRUE) -
+		    (vs->vs_dspace - vs->vs_alloc);
+
+		/*
+		 * Calculate the "slop" space requirement based on the pool
+		 * capacity after the device has been removed.  This helps
+		 * avoid the case where removal is prevented because a large
+		 * vdev is added to a small pool resulting in a vastly
+		 * inflated slop requirement.  Furthermore, if the "slop"
+		 * space exceeds a quarter of the total pool capacity then
+		 * the pool must be tiny and this requirement is reduced.
+		 */
+		int64_t slop = spa_get_slop_space(spa, -vs->vs_dspace);
+		int64_t factor = 4;
+
+		if (slop > (rs->vs_dspace - vs->vs_dspace) / 4)
+			factor = 1;
+
+		/*
+		 * There has to be enough free space to remove the device
+		 * and leave double the "slop" space (i.e. we must leave
+		 * at least 3% of the pool free, in addition to the normal
+		 * slop space).
+		 */
+		if (available < (int64_t)vs->vs_alloc + (factor * slop / 2))
 			return (SET_ERROR(ENOSPC));
-		}
 	}
 
 	/*

tests/runfiles/common.run

@@ -756,7 +756,8 @@ tests = ['removal_all_vdev', 'removal_cancel', 'removal_check_space',
     'removal_with_send_recv', 'removal_with_snapshot',
     'removal_with_write', 'removal_with_zdb', 'remove_expanded',
     'remove_mirror', 'remove_mirror_sanity', 'remove_raidz',
-    'remove_indirect', 'remove_attach_mirror']
+    'remove_indirect', 'remove_attach_mirror', 'remove_minimum',
+    'remove_unbalanced']
 tags = ['functional', 'removal']
 
 [tests/functional/rename_dirs]

tests/zfs-tests/tests/functional/removal/Makefile.am

@@ -30,7 +30,7 @@ dist_pkgdata_SCRIPTS = \
 	removal_with_snapshot.ksh removal_with_write.ksh \
 	removal_with_zdb.ksh remove_mirror.ksh remove_mirror_sanity.ksh \
 	remove_raidz.ksh remove_expanded.ksh remove_indirect.ksh \
-	remove_attach_mirror.ksh
+	remove_attach_mirror.ksh remove_minimum.ksh remove_unbalanced.ksh
 
 dist_pkgdata_DATA = \
 	removal.kshlib

tests/zfs-tests/tests/functional/removal/remove_minimum.ksh

@@ -0,0 +1,54 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# 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.
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2020 by Lawrence Livermore National Security, LLC.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/removal/removal.kshlib
+
+#
+# DESCRIPTION:
+# Device removal is possible for minimum sized vdevs.
+#
+# STRATEGY:
+# 1. Create a pool with minimum sized removable devices
+# 2. Remove a top-level device
+#
+
+verify_runnable "global"
+
+function cleanup
+{
+	default_cleanup_noexit
+	log_must rm -f $TEST_BASE_DIR/device-{1,2}
+}
+
+log_assert "Device removal is possible for minimum sized vdevs."
+log_onexit cleanup
+
+# 1. Create a pool with minimum sized removable devices
+log_must truncate -s $MINVDEVSIZE $TEST_BASE_DIR/device-{1,2}
+log_must default_setup_noexit "$TEST_BASE_DIR/device-1 $TEST_BASE_DIR/device-2"
+
+log_must dd if=/dev/urandom of=$TESTDIR/$TESTFILE0 bs=1M count=32
+
+# 2. Remove a top-level device
+log_must zpool remove $TESTPOOL $TEST_BASE_DIR/device-1
+log_must wait_for_removal $TESTPOOL
+
+log_pass "Device removal is possible for minimum sized vdevs"

tests/zfs-tests/tests/functional/removal/remove_unbalanced.ksh

@@ -0,0 +1,55 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# 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.
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2020 by Lawrence Livermore National Security, LLC.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/removal/removal.kshlib
+
+#
+# DESCRIPTION:
+# Device removal is possible for highly unbalanced vdevs.
+#
+# STRATEGY:
+# 1. Create a pool with unbalanced removable devices
+# 2. Remove the large top-level device
+#
+
+verify_runnable "global"
+
+function cleanup
+{
+	default_cleanup_noexit
+	log_must rm -f $TEST_BASE_DIR/device-{1,2}
+}
+
+log_assert "Device removal is possible for highly unbalanced vdevs."
+log_onexit cleanup
+
+# 1. Create a pool with unbalanced removable devices
+log_must truncate -s 10G $TEST_BASE_DIR/device-1
+log_must truncate -s 1T $TEST_BASE_DIR/device-2
+log_must default_setup_noexit "$TEST_BASE_DIR/device-1 $TEST_BASE_DIR/device-2"
+
+log_must dd if=/dev/urandom of=$TESTDIR/$TESTFILE0 bs=1M count=32
+
+# 2. Remove the large top-level device
+log_must zpool remove $TESTPOOL $TEST_BASE_DIR/device-1
+log_must wait_for_removal $TESTPOOL
+
+log_pass "Device removal is possible for highly unbalanced vdevs"