OpenZFS 9079 - race condition in starting and ending condensing thread for indirect vdevs

The timeline of the race condition is the following:

[1] Thread A is about to finish condesing the first vdev in
    spa_condense_indirect_thread(), so it calls the
    spa_condense_indirect_complete_sync() sync task which sets
    the spa_condensing_indirect field to NULL. Waiting for the
    sync task to finish, thread A sleeps until the txg is done.
    When this happens, thread A will acquire spa_async_lock and
    set spa_condense_thread to NULL.

[2] While thread A waits for the txg to finish, thread B which is
    running spa_sync() checks whether it should condense the
    second vdev in vdev_indirect_should_condense() by checking the
    spa_condensing_indirect field which was set to NULL by
    spa_condense_indirect_thread() from thread A. So it goes on
    and tries to spawn a new condensing thread in
    spa_condense_indirect_start_sync() and the aforementioned
    assertions fails because thread A has not set spa_condense_thread
    to NULL (which is basically the last thing it does before returning).

The main issue here is that we rely on both spa_condensing_indirect
and spa_condense_thread to signify whether a condensing thread is
running. Ideally we would only use one throughout the codebase. In
addition, for managing spa_condense_thread we currently use
spa_async_lock which basically tights condensing to scrubing when
it comes to pausing and resuming those actions during spa export.

This commit introduces the ZTHR infrastructure, which is basically
threads created during spa_load()/spa_create() and exist until we
export or destroy the pool. ZTHRs sleep the majority of the time,
until they are notified to wake up and do some predefined type of work.

In the context of the current bug, a zthr to does the condensing of
indirect mappings replacing the older code that used bare kthreads.
When a pool is created, the condensing zthr is spawned but sleeps
right away, until it is awaken by a signal from spa_sync(). If an
existing pool is loaded, the condensing zthr looks if there is
anything to condense before going to sleep, in case we were condensing
mappings in the pool before it got exported.

The benefits of this solution are the following:
- The current bug is fixed
- spa_condensing_indirect is the sole indicator of whether we are
  currently condensing or not
- condensing is more decoupled from the spa_async_thread related
  functionality.

As a final note, this commit also sets up the path on upstreaming
other features that use the ZTHR code like zpool checkpoint and
fast clone deletion.

Authored by: Serapheim Dimitropoulos <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Pavel Zakharov <[email protected]>
Approved by: Hans Rosenfeld <[email protected]>
Ported-by: Tim Chase <[email protected]>

OpenZFS-issue: https://illumos.org/issues/9079
OpenZFS-commit: https://github.com/openzfs/openzfs/commit/3dc606ee
Closes #6900

1 files changed, 319 insertions, 0 deletions

diff --git a/module/zfs/zthr.c b/module/zfs/zthr.c
new file mode 100644
index 000000000..9beb7e128
--- /dev/null
+++ b/module/zfs/zthr.c
@@ -0,0 +1,319 @@
+/*
+ * 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) 2017 by Delphix. All rights reserved.
+ */
+
+/*
+ * ZTHR Infrastructure
+ * ===================
+ *
+ * ZTHR threads are used for isolated operations that span multiple txgs
+ * within a SPA. They generally exist from SPA creation/loading and until
+ * the SPA is exported/destroyed. The ideal requirements for an operation
+ * to be modeled with a zthr are the following:
+ *
+ * 1] The operation needs to run over multiple txgs.
+ * 2] There is be a single point of reference in memory or on disk that
+ *    indicates whether the operation should run/is running or is
+ *    stopped.
+ *
+ * If the operation satisfies the above then the following rules guarantee
+ * a certain level of correctness:
+ *
+ * 1] Any thread EXCEPT the zthr changes the work indicator from stopped
+ *    to running but not the opposite.
+ * 2] Only the zthr can change the work indicator from running to stopped
+ *    (e.g. when it is done) but not the opposite.
+ *
+ * This way a normal zthr cycle should go like this:
+ *
+ * 1] An external thread changes the work indicator from stopped to
+ *    running and wakes up the zthr.
+ * 2] The zthr wakes up, checks the indicator and starts working.
+ * 3] When the zthr is done, it changes the indicator to stopped, allowing
+ *    a new cycle to start.
+ *
+ * == ZTHR creation
+ *
+ * Every zthr needs three inputs to start running:
+ *
+ * 1] A user-defined checker function (checkfunc) that decides whether
+ *    the zthr should start working or go to sleep. The function should
+ *    return TRUE when the zthr needs to work or FALSE to let it sleep,
+ *    and should adhere to the following signature:
+ *    boolean_t checkfunc_name(void *args, zthr_t *t);
+ *
+ * 2] A user-defined ZTHR function (func) which the zthr executes when
+ *    it is not sleeping. The function should adhere to the following
+ *    signature type:
+ *    int func_name(void *args, zthr_t *t);
+ *
+ * 3] A void args pointer that will be passed to checkfunc and func
+ *    implicitly by the infrastructure.
+ *
+ * The reason why the above API needs two different functions,
+ * instead of one that both checks and does the work, has to do with
+ * the zthr's internal lock (zthr_lock) and the allowed cancellation
+ * windows. We want to hold the zthr_lock while running checkfunc
+ * but not while running func. This way the zthr can be cancelled
+ * while doing work and not while checking for work.
+ *
+ * To start a zthr:
+ *     zthr_t *zthr_pointer = zthr_create(checkfunc, func, args);
+ *
+ * After that you should be able to wakeup, cancel, and resume the
+ * zthr from another thread using zthr_pointer.
+ *
+ * NOTE: ZTHR threads could potentially wake up spuriously and the
+ * user should take this into account when writing a checkfunc.
+ * [see ZTHR state transitions]
+ *
+ * == ZTHR cancellation
+ *
+ * ZTHR threads must be cancelled when their SPA is being exported
+ * or when they need to be paused so they don't interfere with other
+ * operations.
+ *
+ * To cancel a zthr:
+ *     zthr_cancel(zthr_pointer);
+ *
+ * To resume it:
+ *     zthr_resume(zthr_pointer);
+ *
+ * A zthr will implicitly check if it has received a cancellation
+ * signal every time func returns and everytime it wakes up [see ZTHR
+ * state transitions below].
+ *
+ * At times, waiting for the zthr's func to finish its job may take
+ * time. This may be very time-consuming for some operations that
+ * need to cancel the SPA's zthrs (e.g spa_export). For this scenario
+ * the user can explicitly make their ZTHR function aware of incoming
+ * cancellation signals using zthr_iscancelled(). A common pattern for
+ * that looks like this:
+ *
+ * int
+ * func_name(void *args, zthr_t *t)
+ * {
+ *     ... <unpack args> ...
+ *     while (!work_done && !zthr_iscancelled(t)) {
+ *         ... <do more work> ...
+ *     }
+ *     return (0);
+ * }
+ *
+ * == ZTHR exit
+ *
+ * For the rare cases where the zthr wants to stop running voluntarily
+ * while running its ZTHR function (func), we provide zthr_exit().
+ * When a zthr has voluntarily stopped running, it can be resumed with
+ * zthr_resume(), just like it would if it was cancelled by some other
+ * thread.
+ *
+ * == ZTHR cleanup
+ *
+ * Cancelling a zthr doesn't clean up its metadata (internal locks,
+ * function pointers to func and checkfunc, etc..). This is because
+ * we want to keep them around in case we want to resume the execution
+ * of the zthr later. Similarly for zthrs that exit themselves.
+ *
+ * To completely cleanup a zthr, cancel it first to ensure that it
+ * is not running and then use zthr_destroy().
+ *
+ * == ZTHR state transitions
+ *
+ *    zthr creation
+ *      +
+ *      |
+ *      |      woke up
+ *      |   +--------------+ sleep
+ *      |   |                  ^
+ *      |   |                  |
+ *      |   |                  | FALSE
+ *      |   |                  |
+ *      v   v     FALSE        +
+ *   cancelled? +---------> checkfunc?
+ *      +   ^                  +
+ *      |   |                  |
+ *      |   |                  | TRUE
+ *      |   |                  |
+ *      |   |  func returned   v
+ *      |   +---------------+ func
+ *      |
+ *      | TRUE
+ *      |
+ *      v
+ *   zthr stopped running
+ *
+ */
+
+#include <sys/zfs_context.h>
+#include <sys/zthr.h>
+
+void
+zthr_exit(zthr_t *t, int rc)
+{
+	ASSERT3P(t->zthr_thread, ==, curthread);
+	mutex_enter(&t->zthr_lock);
+	t->zthr_thread = NULL;
+	t->zthr_rc = rc;
+	cv_broadcast(&t->zthr_cv);
+	mutex_exit(&t->zthr_lock);
+	thread_exit();
+}
+
+static void
+zthr_procedure(void *arg)
+{
+	zthr_t *t = arg;
+	int rc = 0;
+
+	mutex_enter(&t->zthr_lock);
+	while (!t->zthr_cancel) {
+		if (t->zthr_checkfunc(t->zthr_arg, t)) {
+			mutex_exit(&t->zthr_lock);
+			rc = t->zthr_func(t->zthr_arg, t);
+			mutex_enter(&t->zthr_lock);
+		} else {
+			/* go to sleep */
+			cv_wait(&t->zthr_cv, &t->zthr_lock);
+		}
+	}
+	mutex_exit(&t->zthr_lock);
+
+	zthr_exit(t, rc);
+}
+
+zthr_t *
+zthr_create(zthr_checkfunc_t *checkfunc, zthr_func_t *func, void *arg)
+{
+	zthr_t *t = kmem_zalloc(sizeof (*t), KM_SLEEP);
+	mutex_init(&t->zthr_lock, NULL, MUTEX_DEFAULT, NULL);
+	cv_init(&t->zthr_cv, NULL, CV_DEFAULT, NULL);
+
+	mutex_enter(&t->zthr_lock);
+	t->zthr_checkfunc = checkfunc;
+	t->zthr_func = func;
+	t->zthr_arg = arg;
+
+	t->zthr_thread = thread_create(NULL, 0, zthr_procedure, t,
+	    0, &p0, TS_RUN, minclsyspri);
+	mutex_exit(&t->zthr_lock);
+
+	return (t);
+}
+
+void
+zthr_destroy(zthr_t *t)
+{
+	VERIFY3P(t->zthr_thread, ==, NULL);
+	mutex_destroy(&t->zthr_lock);
+	cv_destroy(&t->zthr_cv);
+	kmem_free(t, sizeof (*t));
+}
+
+/*
+ * Note: If the zthr is not sleeping and misses the wakeup
+ * (e.g it is running its ZTHR function), it will check if
+ * there is work to do before going to sleep using its checker
+ * function [see ZTHR state transition in ZTHR block comment].
+ * Thus, missing the wakeup still yields the expected behavior.
+ */
+void
+zthr_wakeup(zthr_t *t)
+{
+	ASSERT3P(t->zthr_thread, !=, NULL);
+
+	mutex_enter(&t->zthr_lock);
+	cv_broadcast(&t->zthr_cv);
+	mutex_exit(&t->zthr_lock);
+}
+
+/*
+ * Note: If the zthr is not running (e.g. has been cancelled
+ * already), this is a no-op.
+ */
+int
+zthr_cancel(zthr_t *t)
+{
+	int rc = 0;
+
+	mutex_enter(&t->zthr_lock);
+
+	/* broadcast in case the zthr is sleeping */
+	cv_broadcast(&t->zthr_cv);
+
+	t->zthr_cancel = B_TRUE;
+	while (t->zthr_thread != NULL)
+		cv_wait(&t->zthr_cv, &t->zthr_lock);
+	t->zthr_cancel = B_FALSE;
+	rc = t->zthr_rc;
+	mutex_exit(&t->zthr_lock);
+
+	return (rc);
+}
+
+void
+zthr_resume(zthr_t *t)
+{
+	ASSERT3P(t->zthr_thread, ==, NULL);
+
+	mutex_enter(&t->zthr_lock);
+
+	ASSERT3P(&t->zthr_checkfunc, !=, NULL);
+	ASSERT3P(&t->zthr_func, !=, NULL);
+	ASSERT(!t->zthr_cancel);
+
+	t->zthr_thread = thread_create(NULL, 0, zthr_procedure, t,
+	    0, &p0, TS_RUN, minclsyspri);
+
+	mutex_exit(&t->zthr_lock);
+}
+
+/*
+ * This function is intended to be used by the zthr itself
+ * to check if another thread has signal it to stop running.
+ *
+ * returns TRUE if we are in the middle of trying to cancel
+ *     this thread.
+ *
+ * returns FALSE otherwise.
+ */
+boolean_t
+zthr_iscancelled(zthr_t *t)
+{
+	boolean_t cancelled;
+
+	ASSERT3P(t->zthr_thread, ==, curthread);
+
+	mutex_enter(&t->zthr_lock);
+	cancelled = t->zthr_cancel;
+	mutex_exit(&t->zthr_lock);
+
+	return (cancelled);
+}
+
+boolean_t
+zthr_isrunning(zthr_t *t)
+{
+	boolean_t running;
+
+	mutex_enter(&t->zthr_lock);
+	running = (t->zthr_thread != NULL);
+	mutex_exit(&t->zthr_lock);
+
+	return (running);
+}