Disable 'zfs remap' command

The implementation of 'zfs remap' has proven to be problematic since
it modifies the objset (but not its logical contents) by dirtying
metadata without owning it.  The consequence of which is that
dmu_objset_remap_indirects() is vulnerable to certain races.

For example, if we are in the middle of receiving into the filesystem
while it is being remapped.  Then it is possible we could evict the
objset when the receive completes (see dsl_dataset_clone_swap_sync_impl,
or dmu_recv_end_sync), but dmu_objset_remap_indirects() may be still
using the objset.  The result of which would be a panic.

Extended runs of ztest(8) have exposed other possible races which
can occur when using 'zfs remap'.  Several of these have been fixed
but there may be others which have not yet been encountered and
diagnosed.

Furthermore, the ability to manually remap a filesystem is no longer
particularly useful now that the removal code can map large chunks.
Coupled with the fact that explaining what this command does and why
it may be useful requires a detailed understanding of the internals
of device removal.  These are details users should not be bothered
with.

Therefore, the 'zfs remap' command is being disabled but not entirely
removed.  It may be removed in the future or potentially reworked
to address the issues described above.  Since 'zfs remap' has never
been part of a tagged release its removal is expected to have
minimal impact.

The ZTS tests have been updated to continue to exercise the command
to prevent atrophy, but it has been removed entirely from ztest(8).

Reviewed by: Matt Ahrens <mahrens@delphix.com>
Reviewed by: Tom Caputi <tcaputi@datto.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #8238

diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c
index 6b08bb115e078d6f2cf2be6eb37a9db43b0979a5..b8258a9cf53be4c3ad634683748463831cdc0981 100644 (file)
--- a/cmd/zfs/zfs_main.c
+++ b/cmd/zfs/zfs_main.c
@@ -7083,6 +7083,21 @@ zfs_do_unshare(int argc, char **argv)
        return (unshare_unmount(OP_SHARE, argc, argv));
 }
 
        return (unshare_unmount(OP_SHARE, argc, argv));
 }
 

+static int
+disable_command_idx(char *command)
+{
+       for (int i = 0; i < NCOMMAND; i++) {
+               if (command_table[i].name == NULL)
+                       continue;
+
+               if (strcmp(command, command_table[i].name) == 0) {
+                       command_table[i].name = NULL;
+                       return (0);
+               }
+       }
+       return (1);
+}
+

 static int
 find_command_idx(char *command, int *idx)
 {
 static int
 find_command_idx(char *command, int *idx)
 {


@@ -7180,7 +7195,22 @@ zfs_do_diff(int argc, char **argv)
 /*
  * zfs remap <filesystem | volume>
  *
 /*
  * zfs remap <filesystem | volume>
  *

- * Remap the indirect blocks in the given fileystem or volume.
+ * N.B. The remap command has been disabled and may be removed in the future.
+ *
+ * Remap the indirect blocks in the given filesystem or volume so that they no
+ * longer reference blocks on previously removed vdevs and we can eventually
+ * shrink the size of the indirect mapping objects for the previously removed
+ * vdevs. Note that remapping all blocks might not be possible and that
+ * references from snapshots will still exist and cannot be remapped.
+ *
+ * This functionality is no longer particularly useful now that the removal
+ * code can map large chunks.  Furthermore, explaining what this command
+ * does and why it may be useful requires a detailed understanding of the
+ * internals of device removal.  These are details users should not be
+ * bothered with.  If required, the remap command can be re-enabled by
+ * setting the ZFS_REMAP_ENABLED environment variable.
+ *
+ * > ZFS_REMAP_ENABLED=yes zfs remap <filesystem | volume>

  */
 static int
 zfs_do_remap(int argc, char **argv)
  */
 static int
 zfs_do_remap(int argc, char **argv)


@@ -8006,6 +8036,13 @@ main(int argc, char **argv)
        if (strcmp(cmdname, "snap") == 0)
                cmdname = "snapshot";
 
        if (strcmp(cmdname, "snap") == 0)
                cmdname = "snapshot";
 

+       /*
+        * The 'remap' command has been disabled and may be removed in the
+        * future.  See the comment above zfs_do_remap() for details.
+        */
+       if (!libzfs_envvar_is_set("ZFS_REMAP_ENABLED"))
+               disable_command_idx("remap");
+

        /*
         * Special case '-?'
         */
        /*
         * Special case '-?'
         */

diff --git a/cmd/ztest/ztest.c b/cmd/ztest/ztest.c
index f7f584358cd8e731bb376d29700b78c06470076d..678e29ff6681e45f9583bf502d07f515f2510066 100644 (file)
--- a/cmd/ztest/ztest.c
+++ b/cmd/ztest/ztest.c
@@ -216,7 +216,6 @@ extern int dmu_object_alloc_chunk_shift;
 extern boolean_t zfs_force_some_double_word_sm_entries;
 extern unsigned long zio_decompress_fail_fraction;
 extern unsigned long zfs_reconstruct_indirect_damage_fraction;
 extern boolean_t zfs_force_some_double_word_sm_entries;
 extern unsigned long zio_decompress_fail_fraction;
 extern unsigned long zfs_reconstruct_indirect_damage_fraction;

-extern int zfs_object_remap_one_indirect_delay_ms;

 
 
 static ztest_shared_opts_t *ztest_shared_opts;
 
 
 static ztest_shared_opts_t *ztest_shared_opts;


@@ -373,7 +372,6 @@ ztest_func_t ztest_split_pool;
 ztest_func_t ztest_reguid;
 ztest_func_t ztest_spa_upgrade;
 ztest_func_t ztest_device_removal;
 ztest_func_t ztest_reguid;
 ztest_func_t ztest_spa_upgrade;
 ztest_func_t ztest_device_removal;

-ztest_func_t ztest_remap_blocks;

 ztest_func_t ztest_spa_checkpoint_create_discard;
 ztest_func_t ztest_initialize;
 ztest_func_t ztest_fletcher;
 ztest_func_t ztest_spa_checkpoint_create_discard;
 ztest_func_t ztest_initialize;
 ztest_func_t ztest_fletcher;


@@ -427,7 +425,6 @@ ztest_info_t ztest_info[] = {
        ZTI_INIT(ztest_vdev_class_add, 1, &ztest_opts.zo_vdevtime),
        ZTI_INIT(ztest_vdev_aux_add_remove, 1, &ztest_opts.zo_vdevtime),
        ZTI_INIT(ztest_device_removal, 1, &zopt_sometimes),
        ZTI_INIT(ztest_vdev_class_add, 1, &ztest_opts.zo_vdevtime),
        ZTI_INIT(ztest_vdev_aux_add_remove, 1, &ztest_opts.zo_vdevtime),
        ZTI_INIT(ztest_device_removal, 1, &zopt_sometimes),

-       ZTI_INIT(ztest_remap_blocks, 1, &zopt_sometimes),

        ZTI_INIT(ztest_spa_checkpoint_create_discard, 1, &zopt_rarely),
        ZTI_INIT(ztest_initialize, 1, &zopt_sometimes),
        ZTI_INIT(ztest_fletcher, 1, &zopt_rarely),
        ZTI_INIT(ztest_spa_checkpoint_create_discard, 1, &zopt_rarely),
        ZTI_INIT(ztest_initialize, 1, &zopt_sometimes),
        ZTI_INIT(ztest_fletcher, 1, &zopt_rarely),


@@ -5550,20 +5547,6 @@ ztest_dsl_prop_get_set(ztest_ds_t *zd, uint64_t id)
        (void) pthread_rwlock_unlock(&ztest_name_lock);
 }
 
        (void) pthread_rwlock_unlock(&ztest_name_lock);
 }
 

-/* ARGSUSED */
-void
-ztest_remap_blocks(ztest_ds_t *zd, uint64_t id)
-{
-       (void) pthread_rwlock_rdlock(&ztest_name_lock);
-
-       int error = dmu_objset_remap_indirects(zd->zd_name);
-       if (error == ENOSPC)
-               error = 0;
-       ASSERT0(error);
-
-       (void) pthread_rwlock_unlock(&ztest_name_lock);
-}
-

 /* ARGSUSED */
 void
 ztest_spa_prop_get_set(ztest_ds_t *zd, uint64_t id)
 /* ARGSUSED */
 void
 ztest_spa_prop_get_set(ztest_ds_t *zd, uint64_t id)


@@ -6629,12 +6612,6 @@ ztest_resume_thread(void *arg)
                 */
                if (ztest_random(10) == 0)
                        zfs_abd_scatter_enabled = ztest_random(2);
                 */
                if (ztest_random(10) == 0)
                        zfs_abd_scatter_enabled = ztest_random(2);

-
-               /*
-                * Periodically inject remapping delays (10% of the time).
-                */
-               zfs_object_remap_one_indirect_delay_ms =
-                   ztest_random(10) == 0 ? ztest_random(1000) + 1 : 0;

        }
 
        thread_exit();
        }
 
        thread_exit();

diff --git a/man/man5/zpool-features.5 b/man/man5/zpool-features.5
index c1f6ee6e4e6d09c907d8ed405a6c4e817fa92803..4769c4eac335587d1c8d6136ef8a358b46502c0f 100644 (file)
--- a/man/man5/zpool-features.5
+++ b/man/man5/zpool-features.5
@@ -475,8 +475,7 @@ DEPENDENCIES        device_removal
 This feature is an enhancement of device_removal, which will over time
 reduce the memory used to track removed devices.  When indirect blocks
 are freed or remapped, we note that their part of the indirect mapping
 This feature is an enhancement of device_removal, which will over time
 reduce the memory used to track removed devices.  When indirect blocks
 are freed or remapped, we note that their part of the indirect mapping

-is "obsolete", i.e. no longer needed.  See also the \fBzfs remap\fR
-subcommand in \fBzfs\fR(1M).
+is "obsolete", i.e. no longer needed.

 
 This feature becomes \fBactive\fR when the "zpool remove" command is
 used on a top-level vdev, and will never return to being \fBenabled\fR.
 
 This feature becomes \fBactive\fR when the "zpool remove" command is
 used on a top-level vdev, and will never return to being \fBenabled\fR.

diff --git a/man/man8/zfs.8 b/man/man8/zfs.8
index df904d0bdb202c71a2d8896e57d6d1024f489a90..be28f2b62a86bdeae9e698b30ae2b2b07a339a49 100644 (file)
--- a/man/man8/zfs.8
+++ b/man/man8/zfs.8
@@ -103,9 +103,6 @@
 .Oo Fl t Ar type Ns Oo , Ns Ar type Oc Ns ... Oc
 .Oo Ar filesystem Ns | Ns Ar volume Ns | Ns Ar snapshot Oc Ns ...
 .Nm
 .Oo Fl t Ar type Ns Oo , Ns Ar type Oc Ns ... Oc
 .Oo Ar filesystem Ns | Ns Ar volume Ns | Ns Ar snapshot Oc Ns ...
 .Nm

-.Cm remap
-.Ar filesystem Ns | Ns Ar volume
-.Nm

 .Cm set
 .Ar property Ns = Ns Ar value Oo Ar property Ns = Ns Ar value Oc Ns ...
 .Ar filesystem Ns | Ns Ar volume Ns | Ns Ar snapshot Ns ...
 .Cm set
 .Ar property Ns = Ns Ar value Oo Ar property Ns = Ns Ar value Oc Ns ...
 .Ar filesystem Ns | Ns Ar volume Ns | Ns Ar snapshot Ns ...


@@ -3037,16 +3034,6 @@ option was not specified.
 .El
 .It Xo
 .Nm
 .El
 .It Xo
 .Nm

-.Cm remap
-.Ar filesystem Ns | Ns Ar volume
-.Xc
-Remap the indirect blocks in the given fileystem or volume so that they no
-longer reference blocks on previously removed vdevs and we can eventually
-shrink the size of the indirect mapping objects for the previously removed
-vdevs. Note that remapping all blocks might not be possible and that
-references from snapshots will still exist and cannot be remapped.
-.It Xo
-.Nm

 .Cm upgrade
 .Xc
 Displays a list of file systems that are not the most recent version.
 .Cm upgrade
 .Xc
 Displays a list of file systems that are not the most recent version.

diff --git a/tests/zfs-tests/tests/functional/alloc_class/alloc_class_012_pos.ksh b/tests/zfs-tests/tests/functional/alloc_class/alloc_class_012_pos.ksh
index b03a8b4c87933f34afd225dd6b6c33a81893b853..2371c5a2689327646b95543bf789c15e2ceae7cb 100755 (executable)
--- a/tests/zfs-tests/tests/functional/alloc_class/alloc_class_012_pos.ksh
+++ b/tests/zfs-tests/tests/functional/alloc_class/alloc_class_012_pos.ksh
@@ -55,7 +55,10 @@ log_must zpool list -v $TESTPOOL
 
 #
 # remove a special allocation vdev and force a remapping
 
 #
 # remove a special allocation vdev and force a remapping

+# N.B. The 'zfs remap' command has been disabled and may be removed.

 #
 #

+export ZFS_REMAP_ENABLED=YES
+

 log_must zpool remove $TESTPOOL $CLASS_DISK0
 log_must zfs remap $TESTPOOL/$TESTFS
 
 log_must zpool remove $TESTPOOL $CLASS_DISK0
 log_must zfs remap $TESTPOOL/$TESTFS
 

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_cliargs.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_cliargs.ksh
index 9a0b7d6192093369ce0c69fd8c54fa8ad2c344e6..80a5e6e0d9d94f59a10ee613e4b27eb5a41c78e2 100755 (executable)
--- a/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_cliargs.ksh
+++ b/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_cliargs.ksh
@@ -27,6 +27,9 @@
 # 3. Verify other unsupported parameters raise an error
 #
 
 # 3. Verify other unsupported parameters raise an error
 #
 

+# The 'zfs remap' command has been disabled and may be removed.
+export ZFS_REMAP_ENABLED=YES
+

 verify_runnable "both"
 
 function cleanup
 verify_runnable "both"
 
 function cleanup

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_obsolete_counts.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_obsolete_counts.ksh
index 15d3ae493ea38670d94d50b1ff1122e6a2005224..1f0e0e85d82cf9f9dd2a76c3807af9033f8ebb7d 100755 (executable)
--- a/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_obsolete_counts.ksh
+++ b/tests/zfs-tests/tests/functional/cli_root/zfs_remap/zfs_remap_obsolete_counts.ksh
@@ -29,6 +29,9 @@
 #    feature@obsolete_counts is enabled
 #
 
 #    feature@obsolete_counts is enabled
 #
 

+# N.B. The 'zfs remap' command has been disabled and may be removed.
+export ZFS_REMAP_ENABLED=YES
+

 verify_runnable "both"
 
 function cleanup
 verify_runnable "both"
 
 function cleanup

diff --git a/tests/zfs-tests/tests/functional/removal/removal_remap.ksh b/tests/zfs-tests/tests/functional/removal/removal_remap.ksh
index 04d0c50e48f7aee8e97559c3e5cc66ba17ed13a2..5239ef3a5e19395d6d2e9abc47fa3d6a33e9af00 100755 (executable)
--- a/tests/zfs-tests/tests/functional/removal/removal_remap.ksh
+++ b/tests/zfs-tests/tests/functional/removal/removal_remap.ksh
@@ -21,6 +21,9 @@
 . $STF_SUITE/include/libtest.shlib
 . $STF_SUITE/tests/functional/removal/removal.kshlib
 
 . $STF_SUITE/include/libtest.shlib
 . $STF_SUITE/tests/functional/removal/removal.kshlib
 

+# N.B. The 'zfs remap' command has been disabled and may be removed.
+export ZFS_REMAP_ENABLED=YES
+

 default_setup_noexit "$DISKS"
 
 
 default_setup_noexit "$DISKS"
 
 

diff --git a/tests/zfs-tests/tests/functional/removal/removal_remap_deadlists.ksh b/tests/zfs-tests/tests/functional/removal/removal_remap_deadlists.ksh
index 6c630f2f5355639d9f37a3f5aa4e9dacafc64744..a2f6580b4f14b3d4d707fad42baf0a59c3565a1d 100755 (executable)
--- a/tests/zfs-tests/tests/functional/removal/removal_remap_deadlists.ksh
+++ b/tests/zfs-tests/tests/functional/removal/removal_remap_deadlists.ksh
@@ -21,6 +21,9 @@
 . $STF_SUITE/include/libtest.shlib
 . $STF_SUITE/tests/functional/removal/removal.kshlib
 
 . $STF_SUITE/include/libtest.shlib
 . $STF_SUITE/tests/functional/removal/removal.kshlib
 

+# N.B. The 'zfs remap' command has been disabled and may be removed.
+export ZFS_REMAP_ENABLED=YES
+

 default_setup_noexit "$DISKS"
 log_onexit default_cleanup_noexit
 
 default_setup_noexit "$DISKS"
 log_onexit default_cleanup_noexit
 

diff --git a/tests/zfs-tests/tests/functional/removal/removal_with_remap.ksh b/tests/zfs-tests/tests/functional/removal/removal_with_remap.ksh
index d3a53e40b30ddaf23a198ff27bab4d661be35721..6f56740b82fa2beaa3ed54dc0531c275bbb66c35 100755 (executable)
--- a/tests/zfs-tests/tests/functional/removal/removal_with_remap.ksh
+++ b/tests/zfs-tests/tests/functional/removal/removal_with_remap.ksh
@@ -21,6 +21,9 @@
 . $STF_SUITE/include/libtest.shlib
 . $STF_SUITE/tests/functional/removal/removal.kshlib
 
 . $STF_SUITE/include/libtest.shlib
 . $STF_SUITE/tests/functional/removal/removal.kshlib
 

+# N.B. The 'zfs remap' command has been disabled and may be removed.
+export ZFS_REMAP_ENABLED=YES
+

 default_setup_noexit "$DISKS"
 log_onexit default_cleanup_noexit
 
 default_setup_noexit "$DISKS"
 log_onexit default_cleanup_noexit