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
for small pools preventing removal when it clearly should be possible.

This change reworks the check to drop the additional slop space from
the calculation.  This solves two problems:

1) If a large device is accidentally added to a small pool the slop
   requirement is dramatically increased.  This in turn can prevent
   removal of that larger device even if it has never been used.
   This was particularly unfortunate since a main motivation for
   this feature was to allow such mistakes to be corrected.

2) For very small pools with 256M vdevs the minimum slop size of
   128M represents a huge fraction of the total pool capacity.
   Requiring twice this amount of space to be available effectively
   prevents device removal from ever working with these tiny pools.

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

Author: behlendorf

module/zfs/vdev_removal.c

@@ -1995,17 +1995,18 @@ 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 */
+		/*
+		 * This is a normal device. There has to be enough free
+		 * space in the pool's normal class to remove the device
+		 * and leave 1.6% of the space available.  The available
+		 * space accounts for both reserved and "slop" space.
+		 */
 		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).
-			 */
+
+		if (available < vd->vdev_stat.vs_dspace ||
+		    available - vd->vdev_stat.vs_dspace <
+		    spa_get_dspace(spa) >> 6) {
 			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,56 @@
+#!/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=64
+
+# 2. Remove a top-level device
+log_must zpool remove $TESTPOOL $TEST_BASE_DIR/device-1
+log_must wait_for_removal $TESTPOOL
+
+log_note "Capacity $(get_pool_prop capacity $TESTPOOL)"
+
+log_pass "Device removal is possible for minimum sized vdevs"

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

@@ -0,0 +1,57 @@
+#!/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_note "Capacity $(get_pool_prop capacity $TESTPOOL)"
+
+log_pass "Device removal is possible for highly unbalanced vdevs"