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 to use the per-vdev vs_stats.  This
is sufficiently accurate and is convenient because it allows a direct
comparison to the allocated space.  If using dsl_dir_space_available()
then the available space of the device being removed is also included
in the return value and must somehow be accounted for.

Additionally, 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.

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 danager 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

@@ -1783,9 +1783,9 @@ spa_get_worst_case_asize(spa_t *spa, uint64_t lsize)
  * 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,30 @@ 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 on the other pool devices to copy the allocated
+		 * space from the device being removed plus double the
+		 * "slop" space (i.e. we must leave at least 3% of the
+		 * pool free, in addition to the normal slop space).
+		 */
+		uint64_t available = (rs->vs_dspace - rs->vs_alloc) -
+		    (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.
+		 */
+		uint64_t slop = spa_get_slop_space(spa, -vs->vs_dspace);
+
+		if (available < vd->vdev_stat.vs_alloc + 2 * slop)
 			return (SET_ERROR(ENOSPC));
-		}
 	}
 
 	/*

tests/runfiles/common.run

@@ -756,7 +756,7 @@ 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_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_unbalanced.ksh
 
 dist_pkgdata_DATA = \
 	removal.kshlib

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

@@ -0,0 +1,67 @@
+#!/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
+{
+	destroy_pool $TESTPOOL
+	log_must rm -f $TEST_BASE_DIR/device-{1,2,3,4}
+}
+
+log_assert "Device removal is possible for highly unbalanced vdevs."
+log_onexit cleanup
+
+# 1. Create a pool with unbalanced removable devices
+truncate -s 1G $TEST_BASE_DIR/device-{1,2}
+truncate -s 1T $TEST_BASE_DIR/device-{3,4}
+zpool create $TESTPOOL mirror $TEST_BASE_DIR/device-{1,2} \
+    mirror $TEST_BASE_DIR/device-{3,4}
+
+WORDS_FILE1="/usr/dict/words"
+WORDS_FILE2="/usr/share/dict/words"
+FILE_CONTENTS="Leeloo Dallas mul-ti-pass."
+
+if [[ -f $WORDS_FILE1 ]]; then
+    log_must cp $WORDS_FILE1 $TESTDIR
+elif [[ -f $WORDS_FILE2 ]]; then
+    log_must cp $WORDS_FILE2 $TESTDIR
+else
+    echo $FILE_CONTENTS  >$TESTDIR/$TESTFILE0
+    log_must [ "x$(<$TESTDIR/$TESTFILE0)" = "x$FILE_CONTENTS" ]
+fi
+
+# 2. Remove the large top-level device
+log_must zpool remove $TESTPOOL mirror-1
+log_must wait_for_removal $TESTPOOL
+
+log_pass "Device removal is possible for highly unbalanced vdevs"