OpenZFS 8677 - Open-Context Channel Programs

Authored by: Serapheim Dimitropoulos <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Chris Williamson <[email protected]>
Reviewed by: Pavel Zakharov <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported-by: Don Brady <[email protected]>

We want to be able to run channel programs outside of synching
context. This would greatly improve performance for channel programs
that just gather information, as they won't have to wait for synching
context anymore.

=== What is implemented?

This feature introduces the following:
- A new command line flag in "zfs program" to specify our intention
  to run in open context. (The -n option)
- A new flag/option within the channel program ioctl which selects
  the context.
- Appropriate error handling whenever we try a channel program in
  open-context that contains zfs.sync* expressions.
- Documentation for the new feature in the manual pages.

=== How do we handle zfs.sync functions in open context?

When such a function is found by the interpreter and we are running
in open context we abort the script and we spit out a descriptive
runtime error. For example, given the script below ...

arg = ...
fs = arg["argv"][1]
err = zfs.sync.destroy(fs)
msg = "destroying " .. fs .. " err=" .. err
return msg

if we run it in open context, we will get back the following error:

Channel program execution failed:
[string "channel program"]:3: running functions from the zfs.sync
submodule requires passing sync=TRUE to lzc_channel_program()
(i.e. do not specify the "-n" command line argument)
stack traceback:
            [C]: in function 'destroy'
            [string "channel program"]:3: in main chunk

=== What about testing?

We've introduced new wrappers for all channel program tests that
run each channel program as both (startard & open-context) and
expect the appropriate behavior depending on the program using
the zfs.sync module.

OpenZFS-issue: https://www.illumos.org/issues/8677
OpenZFS-commit: https://github.com/openzfs/openzfs/commit/17a49e15
Closes #6558

4 files changed, 135 insertions, 59 deletions

diff --git a/module/zfs/dsl_destroy.c b/module/zfs/dsl_destroy.c
index 7b49de95f..d0fcacaed 100644
--- a/module/zfs/dsl_destroy.c
+++ b/module/zfs/dsl_destroy.c
@@ -20,7 +20,7 @@
  */
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2012, 2016 by Delphix. All rights reserved.
+ * Copyright (c) 2012, 2017 by Delphix. All rights reserved.
  * Copyright (c) 2013 Steven Hartland. All rights reserved.
  * Copyright (c) 2013 by Joyent, Inc. All rights reserved.
  * Copyright (c) 2016 Actifio, Inc. All rights reserved.
@@ -550,6 +550,7 @@ dsl_destroy_snapshots_nvl(nvlist_t *snaps, boolean_t defer,
 	nvlist_t *result = fnvlist_alloc();
 	int error = zcp_eval(nvpair_name(nvlist_next_nvpair(snaps, NULL)),
 	    program,
+	    B_TRUE,
 	    0,
 	    zfs_lua_max_memlimit,
 	    nvlist_next_nvpair(wrapper, NULL), result);
diff --git a/module/zfs/zcp.c b/module/zfs/zcp.c
index 1a39330d3..31230dc52 100644
--- a/module/zfs/zcp.c
+++ b/module/zfs/zcp.c
@@ -134,13 +134,6 @@ typedef struct zcp_eval_arg {
 	uint64_t	ea_instrlimit;
 } zcp_eval_arg_t;
 
-/*ARGSUSED*/
-static int
-zcp_eval_check(void *arg, dmu_tx_t *tx)
-{
-	return (0);
-}
-
 /*
  * The outer-most error callback handler for use with lua_pcall(). On
  * error Lua will call this callback with a single argument that
@@ -184,41 +177,45 @@ zcp_argerror(lua_State *state, int narg, const char *msg, ...)
  *
  * If an error occurs, the cleanup function will be invoked exactly once and
  * then unreigstered.
+ *
+ * Returns the registered cleanup handler so the caller can deregister it
+ * if no error occurs.
  */
-void
+zcp_cleanup_handler_t *
 zcp_register_cleanup(lua_State *state, zcp_cleanup_t cleanfunc, void *cleanarg)
 {
 	zcp_run_info_t *ri = zcp_run_info(state);
-	/*
-	 * A cleanup function should always be explicitly removed before
-	 * installing a new one to avoid accidental clobbering.
-	 */
-	ASSERT3P(ri->zri_cleanup, ==, NULL);
 
-	ri->zri_cleanup = cleanfunc;
-	ri->zri_cleanup_arg = cleanarg;
+	zcp_cleanup_handler_t *zch = kmem_alloc(sizeof (*zch), KM_SLEEP);
+	zch->zch_cleanup_func = cleanfunc;
+	zch->zch_cleanup_arg = cleanarg;
+	list_insert_head(&ri->zri_cleanup_handlers, zch);
+
+	return (zch);
 }
 
 void
-zcp_clear_cleanup(lua_State *state)
+zcp_deregister_cleanup(lua_State *state, zcp_cleanup_handler_t *zch)
 {
 	zcp_run_info_t *ri = zcp_run_info(state);
-
-	ri->zri_cleanup = NULL;
-	ri->zri_cleanup_arg = NULL;
+	list_remove(&ri->zri_cleanup_handlers, zch);
+	kmem_free(zch, sizeof (*zch));
 }
 
 /*
- * If it exists, execute the currently set cleanup function then unregister it.
+ * Execute the currently registered cleanup handlers then free them and
+ * destroy the handler list.
  */
 void
 zcp_cleanup(lua_State *state)
 {
 	zcp_run_info_t *ri = zcp_run_info(state);
 
-	if (ri->zri_cleanup != NULL) {
-		ri->zri_cleanup(ri->zri_cleanup_arg);
-		zcp_clear_cleanup(state);
+	for (zcp_cleanup_handler_t *zch =
+	    list_remove_head(&ri->zri_cleanup_handlers); zch != NULL;
+	    zch = list_remove_head(&ri->zri_cleanup_handlers)) {
+		zch->zch_cleanup_func(zch->zch_cleanup_arg);
+		kmem_free(zch, sizeof (*zch));
 	}
 }
 
@@ -819,19 +816,12 @@ zcp_panic_cb(lua_State *state)
 }
 
 static void
-zcp_eval_sync(void *arg, dmu_tx_t *tx)
+zcp_eval_impl(dmu_tx_t *tx, boolean_t sync, zcp_eval_arg_t *evalargs)
 {
 	int err;
 	zcp_run_info_t ri;
-	zcp_eval_arg_t *evalargs = arg;
 	lua_State *state = evalargs->ea_state;
 
-	/*
-	 * Open context should have setup the stack to contain:
-	 * 1: Error handler callback
-	 * 2: Script to run (converted to a Lua function)
-	 * 3: nvlist input to function (converted to Lua table or nil)
-	 */
 	VERIFY3U(3, ==, lua_gettop(state));
 
 	/*
@@ -844,8 +834,9 @@ zcp_eval_sync(void *arg, dmu_tx_t *tx)
 	ri.zri_cred = evalargs->ea_cred;
 	ri.zri_tx = tx;
 	ri.zri_timed_out = B_FALSE;
-	ri.zri_cleanup = NULL;
-	ri.zri_cleanup_arg = NULL;
+	ri.zri_sync = sync;
+	list_create(&ri.zri_cleanup_handlers, sizeof (zcp_cleanup_handler_t),
+	    offsetof(zcp_cleanup_handler_t, zch_node));
 	ri.zri_curinstrs = 0;
 	ri.zri_maxinstrs = evalargs->ea_instrlimit;
 
@@ -882,10 +873,10 @@ zcp_eval_sync(void *arg, dmu_tx_t *tx)
 
 	/*
 	 * Remove the error handler callback from the stack. At this point,
-	 * if there is a cleanup function registered, then it was registered
-	 * but never run or removed, which should never occur.
+	 * there shouldn't be any cleanup handler registered in the handler
+	 * list (zri_cleanup_handlers), regardless of whether it ran or not.
 	 */
-	ASSERT3P(ri.zri_cleanup, ==, NULL);
+	list_destroy(&ri.zri_cleanup_handlers);
 	lua_remove(state, 1);
 
 	switch (err) {
@@ -973,9 +964,73 @@ zcp_eval_sync(void *arg, dmu_tx_t *tx)
 	}
 }
 
+static void
+zcp_pool_error(zcp_eval_arg_t *evalargs, const char *poolname)
+{
+	evalargs->ea_result = SET_ERROR(ECHRNG);
+	(void) lua_pushfstring(evalargs->ea_state, "Could not open pool: %s",
+	    poolname);
+	zcp_convert_return_values(evalargs->ea_state, evalargs->ea_outnvl,
+	    ZCP_RET_ERROR, evalargs);
+
+}
+
+static void
+zcp_eval_sync(void *arg, dmu_tx_t *tx)
+{
+	zcp_eval_arg_t *evalargs = arg;
+
+	/*
+	 * Open context should have setup the stack to contain:
+	 * 1: Error handler callback
+	 * 2: Script to run (converted to a Lua function)
+	 * 3: nvlist input to function (converted to Lua table or nil)
+	 */
+	VERIFY3U(3, ==, lua_gettop(evalargs->ea_state));
+
+	zcp_eval_impl(tx, B_TRUE, evalargs);
+}
+
+static void
+zcp_eval_open(zcp_eval_arg_t *evalargs, const char *poolname)
+{
+
+	int error;
+	dsl_pool_t *dp;
+	dmu_tx_t *tx;
+
+	/*
+	 * See comment from the same assertion in zcp_eval_sync().
+	 */
+	VERIFY3U(3, ==, lua_gettop(evalargs->ea_state));
+
+	error = dsl_pool_hold(poolname, FTAG, &dp);
+	if (error != 0) {
+		zcp_pool_error(evalargs, poolname);
+		return;
+	}
+
+	/*
+	 * As we are running in open-context, we have no transaction associated
+	 * with the channel program. At the same time, functions from the
+	 * zfs.check submodule need to be associated with a transaction as
+	 * they are basically dry-runs of their counterparts in the zfs.sync
+	 * submodule. These functions should be able to run in open-context.
+	 * Therefore we create a new transaction that we later abort once
+	 * the channel program has been evaluated.
+	 */
+	tx = dmu_tx_create_dd(dp->dp_mos_dir);
+
+	zcp_eval_impl(tx, B_FALSE, evalargs);
+
+	dmu_tx_abort(tx);
+
+	dsl_pool_rele(dp, FTAG);
+}
+
 int
-zcp_eval(const char *poolname, const char *program, uint64_t instrlimit,
-    uint64_t memlimit, nvpair_t *nvarg, nvlist_t *outnvl)
+zcp_eval(const char *poolname, const char *program, boolean_t sync,
+    uint64_t instrlimit, uint64_t memlimit, nvpair_t *nvarg, nvlist_t *outnvl)
 {
 	int err;
 	lua_State *state;
@@ -1086,9 +1141,14 @@ zcp_eval(const char *poolname, const char *program, uint64_t instrlimit,
 	evalargs.ea_outnvl = outnvl;
 	evalargs.ea_result = 0;
 
-	VERIFY0(dsl_sync_task(poolname, zcp_eval_check,
-	    zcp_eval_sync, &evalargs, 0, ZFS_SPACE_CHECK_NONE));
-
+	if (sync) {
+		err = dsl_sync_task(poolname, NULL,
+		    zcp_eval_sync, &evalargs, 0, ZFS_SPACE_CHECK_NONE);
+		if (err != 0)
+			zcp_pool_error(&evalargs, poolname);
+	} else {
+		zcp_eval_open(&evalargs, poolname);
+	}
 	lua_close(state);
 
 	return (evalargs.ea_result);
diff --git a/module/zfs/zcp_synctask.c b/module/zfs/zcp_synctask.c
index ad9bffacb..5a7066407 100644
--- a/module/zfs/zcp_synctask.c
+++ b/module/zfs/zcp_synctask.c
@@ -55,6 +55,10 @@ typedef struct zcp_synctask_info {
  *
  * If 'sync' is false, executes a dry run and returns the error code.
  *
+ * If we are not running in syncing context and we are not doing a dry run
+ * (meaning we are running a zfs.sync function in open-context) then we
+ * return a Lua error.
+ *
  * This function also handles common fatal error cases for channel program
  * library functions. If a fatal error occurs, err_dsname will be the dataset
  * name reported in error messages, if supplied.
@@ -70,6 +74,13 @@ zcp_sync_task(lua_State *state, dsl_checkfunc_t *checkfunc,
 	if (!sync)
 		return (err);
 
+	if (!ri->zri_sync) {
+		return (luaL_error(state, "running functions from the zfs.sync "
+		    "submodule requires passing sync=TRUE to "
+		    "lzc_channel_program() (i.e. do not specify the \"-n\" "
+		    "command line argument)"));
+	}
+
 	if (err == 0) {
 		syncfunc(arg, ri->zri_tx);
 	} else if (err == EIO) {
@@ -234,6 +245,15 @@ zcp_synctask_snapshot(lua_State *state, boolean_t sync, nvlist_t *err_details)
 	zcp_run_info_t *ri = zcp_run_info(state);
 
 	/*
+	 * On old pools, the ZIL must not be active when a snapshot is created,
+	 * but we can't suspend the ZIL because we're already in syncing
+	 * context.
+	 */
+	if (spa_version(ri->zri_pool->dp_spa) < SPA_VERSION_FAST_SNAP) {
+		return (ENOTSUP);
+	}
+
+	/*
 	 * We only allow for a single snapshot rather than a list, so the
 	 * error list output is unnecessary.
 	 */
@@ -243,33 +263,23 @@ zcp_synctask_snapshot(lua_State *state, boolean_t sync, nvlist_t *err_details)
 	ddsa.ddsa_snaps = fnvlist_alloc();
 	fnvlist_add_boolean(ddsa.ddsa_snaps, dsname);
 
-	/*
-	 * On old pools, the ZIL must not be active when a snapshot is created,
-	 * but we can't suspend the ZIL because we're already in syncing
-	 * context.
-	 */
-	if (spa_version(ri->zri_pool->dp_spa) < SPA_VERSION_FAST_SNAP) {
-		return (ENOTSUP);
-	}
+	zcp_cleanup_handler_t *zch = zcp_register_cleanup(state,
+	    (zcp_cleanup_t *)&fnvlist_free, ddsa.ddsa_snaps);
 
 	err = zcp_sync_task(state, dsl_dataset_snapshot_check,
 	    dsl_dataset_snapshot_sync, &ddsa, sync, dsname);
 
+	zcp_deregister_cleanup(state, zch);
 	fnvlist_free(ddsa.ddsa_snaps);
 
 	return (err);
 }
 
-void
-zcp_synctask_wrapper_cleanup(void *arg)
-{
-	fnvlist_free(arg);
-}
-
 static int
 zcp_synctask_wrapper(lua_State *state)
 {
 	int err;
+	zcp_cleanup_handler_t *zch;
 	int num_ret = 1;
 	nvlist_t *err_details = fnvlist_alloc();
 
@@ -277,7 +287,8 @@ zcp_synctask_wrapper(lua_State *state)
 	 * Make sure err_details is properly freed, even if a fatal error is
 	 * thrown during the synctask.
 	 */
-	zcp_register_cleanup(state, &zcp_synctask_wrapper_cleanup, err_details);
+	zch = zcp_register_cleanup(state,
+	    (zcp_cleanup_t *)&fnvlist_free, err_details);
 
 	zcp_synctask_info_t *info = lua_touserdata(state, lua_upvalueindex(1));
 	boolean_t sync = lua_toboolean(state, lua_upvalueindex(2));
@@ -317,7 +328,7 @@ zcp_synctask_wrapper(lua_State *state)
 		num_ret++;
 	}
 
-	zcp_clear_cleanup(state);
+	zcp_deregister_cleanup(state, zch);
 	fnvlist_free(err_details);
 
 	return (num_ret);
diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index 443005165..cc7155a78 100644
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -3673,11 +3673,15 @@ zfs_ioc_channel_program(const char *poolname, nvlist_t *innvl,
 {
 	char *program;
 	uint64_t instrlimit, memlimit;
+	boolean_t sync_flag;
 	nvpair_t *nvarg = NULL;
 
 	if (0 != nvlist_lookup_string(innvl, ZCP_ARG_PROGRAM, &program)) {
 		return (EINVAL);
 	}
+	if (0 != nvlist_lookup_boolean_value(innvl, ZCP_ARG_SYNC, &sync_flag)) {
+		sync_flag = B_TRUE;
+	}
 	if (0 != nvlist_lookup_uint64(innvl, ZCP_ARG_INSTRLIMIT, &instrlimit)) {
 		instrlimit = ZCP_DEFAULT_INSTRLIMIT;
 	}
@@ -3693,7 +3697,7 @@ zfs_ioc_channel_program(const char *poolname, nvlist_t *innvl,
 	if (memlimit == 0 || memlimit > zfs_lua_max_memlimit)
 		return (EINVAL);
 
-	return (zcp_eval(poolname, program, instrlimit, memlimit,
+	return (zcp_eval(poolname, program, sync_flag, instrlimit, memlimit,
 	    nvarg, outnvl));
 }