OpenZFS 8677 - Open-Context Channel Programs

Authored by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed by: Matt Ahrens <mahrens@delphix.com>
Reviewed by: Chris Williamson <chris.williamson@delphix.com>
Reviewed by: Pavel Zakharov <pavel.zakharov@delphix.com>
Approved by: Robert Mustacchi <rm@joyent.com>
Ported-by: Don Brady <don.brady@delphix.com>

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

cmd/zfs/zfs_main.c

@@ -21,7 +21,7 @@
 
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2011, 2016 by Delphix. All rights reserved.
+ * Copyright (c) 2011, 2017 by Delphix. All rights reserved.
  * Copyright 2012 Milan Jurik. All rights reserved.
  * Copyright (c) 2012, Joyent, Inc. All rights reserved.
  * Copyright (c) 2013 Steven Hartland.  All rights reserved.
@@ -340,7 +340,7 @@ get_usage(zfs_help_t idx)
 	case HELP_BOOKMARK:
 		return (gettext("\tbookmark <snapshot> <bookmark>\n"));
 	case HELP_CHANNEL_PROGRAM:
-		return (gettext("\tprogram [-t <instruction limit>] "
+		return (gettext("\tprogram [-n] [-t <instruction limit>] "
 		    "[-m <memory limit (b)>] <pool> <program file> "
 		    "[lua args...]\n"));
 	case HELP_LOAD_KEY:
@@ -7098,10 +7098,11 @@ zfs_do_channel_program(int argc, char **argv)
 	nvlist_t *outnvl;
 	uint64_t instrlimit = ZCP_DEFAULT_INSTRLIMIT;
 	uint64_t memlimit = ZCP_DEFAULT_MEMLIMIT;
+	boolean_t sync_flag = B_TRUE;
 	zpool_handle_t *zhp;
 
 	/* check options */
-	while ((c = getopt(argc, argv, "t:m:")) != -1) {
+	while ((c = getopt(argc, argv, "nt:m:")) != -1) {
 		switch (c) {
 		case 't':
 		case 'm': {
@@ -7139,6 +7140,10 @@ zfs_do_channel_program(int argc, char **argv)
 			}
 			break;
 		}
+		case 'n': {
+			sync_flag = B_FALSE;
+			break;
+		}
 		case '?':
 			(void) fprintf(stderr, gettext("invalid option '%c'\n"),
 			    optopt);
@@ -7210,8 +7215,13 @@ zfs_do_channel_program(int argc, char **argv)
 	nvlist_t *argnvl = fnvlist_alloc();
 	fnvlist_add_string_array(argnvl, ZCP_ARG_CLIARGV, argv + 2, argc - 2);
 
-	ret = lzc_channel_program(poolname, progbuf, instrlimit, memlimit,
-	    argnvl, &outnvl);
+	if (sync_flag) {
+		ret = lzc_channel_program(poolname, progbuf,
+		    instrlimit, memlimit, argnvl, &outnvl);
+	} else {
+		ret = lzc_channel_program_nosync(poolname, progbuf,
+		    instrlimit, memlimit, argnvl, &outnvl);
+	}
 
 	if (ret != 0) {
 		/*

include/libzfs_core.h

@@ -20,7 +20,7 @@
  */
 
 /*
- * Copyright (c) 2012, 2016 by Delphix. All rights reserved.
+ * Copyright (c) 2012, 2017 by Delphix. All rights reserved.
  * Copyright (c) 2017 Datto Inc.
  * Copyright 2017 RackTop Systems.
  * Copyright (c) 2017 Open-E, Inc. All Rights Reserved.
@@ -101,8 +101,10 @@ boolean_t lzc_exists(const char *);
 int lzc_rollback(const char *, char *, int);
 int lzc_rollback_to(const char *, const char *);
 
-int lzc_channel_program(const char *, const char *, uint64_t, uint64_t,
-    nvlist_t *, nvlist_t **);
+int lzc_channel_program(const char *, const char *, uint64_t,
+    uint64_t, nvlist_t *, nvlist_t **);
+int lzc_channel_program_nosync(const char *, const char *, uint64_t,
+    uint64_t, nvlist_t *, nvlist_t **);
 
 int lzc_sync(const char *, nvlist_t *, nvlist_t **);
 int lzc_reopen(const char *, boolean_t);

include/sys/fs/zfs.h

@@ -21,7 +21,7 @@
 
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2011, 2016 by Delphix. All rights reserved.
+ * Copyright (c) 2011, 2017 by Delphix. All rights reserved.
  * Copyright 2011 Nexenta Systems, Inc.  All rights reserved.
  * Copyright (c) 2013, 2017 Joyent, Inc. All rights reserved.
  * Copyright (c) 2014 Integros [integros.com]
@@ -1204,6 +1204,7 @@ typedef enum {
  */
 #define	ZCP_ARG_PROGRAM		"program"
 #define	ZCP_ARG_ARGLIST		"arg"
+#define	ZCP_ARG_SYNC		"sync"
 #define	ZCP_ARG_INSTRLIMIT	"instrlimit"
 #define	ZCP_ARG_MEMLIMIT	"memlimit"
 

include/sys/zcp.h

@@ -38,20 +38,25 @@ extern uint64_t zfs_lua_max_memlimit;
 
 int zcp_argerror(lua_State *, int, const char *, ...);
 
-int zcp_eval(const char *, const char *, uint64_t, uint64_t, nvpair_t *,
-    nvlist_t *);
+int zcp_eval(const char *, const char *, boolean_t, uint64_t, uint64_t,
+    nvpair_t *, nvlist_t *);
 
 int zcp_load_list_lib(lua_State *);
 
 int zcp_load_synctask_lib(lua_State *, boolean_t);
 
 typedef void (zcp_cleanup_t)(void *);
+typedef struct zcp_cleanup_handler {
+	zcp_cleanup_t *zch_cleanup_func;
+	void *zch_cleanup_arg;
+	list_node_t zch_node;
+} zcp_cleanup_handler_t;
 
 typedef struct zcp_run_info {
 	dsl_pool_t	*zri_pool;
 
 	/*
-	 * An estimate of the total ammount of space consumed by all
+	 * An estimate of the total amount of space consumed by all
 	 * synctasks we have successfully performed so far in this
 	 * channel program. Used to generate ENOSPC errors for syncfuncs.
 	 */
@@ -89,16 +94,21 @@ typedef struct zcp_run_info {
 	boolean_t	zri_timed_out;
 
 	/*
-	 * The currently registered cleanup function, which will be called
-	 * with the stored argument if a fatal error occurs.
+	 * Boolean indicating whether or not we are running in syncing
+	 * context.
 	 */
-	zcp_cleanup_t	*zri_cleanup;
-	void		*zri_cleanup_arg;
+	boolean_t	zri_sync;
+
+	/*
+	 * List of currently registered cleanup handlers, which will be
+	 * triggered in the event of a fatal error.
+	 */
+	list_t		zri_cleanup_handlers;
 } zcp_run_info_t;
 
 zcp_run_info_t *zcp_run_info(lua_State *);
-void zcp_register_cleanup(lua_State *, zcp_cleanup_t, void *);
-void zcp_clear_cleanup(lua_State *);
+zcp_cleanup_handler_t *zcp_register_cleanup(lua_State *, zcp_cleanup_t, void *);
+void zcp_deregister_cleanup(lua_State *, zcp_cleanup_handler_t *);
 void zcp_cleanup(lua_State *);
 
 /*

lib/libzfs/libzfs_dataset.c

@@ -22,7 +22,7 @@
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2013, Joyent, Inc. All rights reserved.
- * Copyright (c) 2011, 2016 by Delphix. All rights reserved.
+ * Copyright (c) 2011, 2017 by Delphix. All rights reserved.
  * Copyright (c) 2012 DEY Storage Systems, Inc.  All rights reserved.
  * Copyright (c) 2012 Pawel Jakub Dawidek <pawel@dawidek.net>.
  * Copyright (c) 2013 Martin Matuska. All rights reserved.
@@ -2506,7 +2506,7 @@ zcp_check(zfs_handle_t *zhp, zfs_prop_t prop, uint64_t intval,
 	fnvlist_add_string(argnvl, "dataset", zhp->zfs_name);
 	fnvlist_add_string(argnvl, "property", zfs_prop_to_name(prop));
 
-	error = lzc_channel_program(poolname, program,
+	error = lzc_channel_program_nosync(poolname, program,
 	    10 * 1000 * 1000, 10 * 1024 * 1024, argnvl, &outnvl);
 
 	if (error == 0) {

lib/libzfs_core/libzfs_core.c

@@ -1063,6 +1063,25 @@ lzc_destroy_bookmarks(nvlist_t *bmarks, nvlist_t **errlist)
 	return (error);
 }
 
+static int
+lzc_channel_program_impl(const char *pool, const char *program, boolean_t sync,
+    uint64_t instrlimit, uint64_t memlimit, nvlist_t *argnvl, nvlist_t **outnvl)
+{
+	int error;
+	nvlist_t *args;
+
+	args = fnvlist_alloc();
+	fnvlist_add_string(args, ZCP_ARG_PROGRAM, program);
+	fnvlist_add_nvlist(args, ZCP_ARG_ARGLIST, argnvl);
+	fnvlist_add_boolean_value(args, ZCP_ARG_SYNC, sync);
+	fnvlist_add_uint64(args, ZCP_ARG_INSTRLIMIT, instrlimit);
+	fnvlist_add_uint64(args, ZCP_ARG_MEMLIMIT, memlimit);
+	error = lzc_ioctl(ZFS_IOC_CHANNEL_PROGRAM, pool, args, outnvl);
+	fnvlist_free(args);
+
+	return (error);
+}
+
 /*
  * Executes a channel program.
  *
@@ -1100,18 +1119,28 @@ int
 lzc_channel_program(const char *pool, const char *program, uint64_t instrlimit,
     uint64_t memlimit, nvlist_t *argnvl, nvlist_t **outnvl)
 {
-	int error;
-	nvlist_t *args;
+	return (lzc_channel_program_impl(pool, program, B_TRUE, instrlimit,
+	    memlimit, argnvl, outnvl));
+}
 
-	args = fnvlist_alloc();
-	fnvlist_add_string(args, ZCP_ARG_PROGRAM, program);
-	fnvlist_add_nvlist(args, ZCP_ARG_ARGLIST, argnvl);
-	fnvlist_add_uint64(args, ZCP_ARG_INSTRLIMIT, instrlimit);
-	fnvlist_add_uint64(args, ZCP_ARG_MEMLIMIT, memlimit);
-	error = lzc_ioctl(ZFS_IOC_CHANNEL_PROGRAM, pool, args, outnvl);
-	fnvlist_free(args);
-
-	return (error);
+/*
+ * Executes a read-only channel program.
+ *
+ * A read-only channel program works programmatically the same way as a
+ * normal channel program executed with lzc_channel_program(). The only
+ * difference is it runs exclusively in open-context and therefore can
+ * return faster. The downside to that, is that the program cannot change
+ * on-disk state by calling functions from the zfs.sync submodule.
+ *
+ * The return values of this function (and their meaning) are exactly the
+ * same as the ones described in lzc_channel_program().
+ */
+int
+lzc_channel_program_nosync(const char *pool, const char *program,
+    uint64_t timeout, uint64_t memlimit, nvlist_t *argnvl, nvlist_t **outnvl)
+{
+	return (lzc_channel_program_impl(pool, program, B_FALSE, timeout,
+	    memlimit, argnvl, outnvl));
 }
 
 /*

man/man8/zfs-program.8

@@ -17,7 +17,8 @@
 .Nm zfs program
 .Nd executes ZFS channel programs
 .Sh SYNOPSIS
-.Cm zfs program
+.Cm "zfs program"
+.Op Fl n
 .Op Fl t Ar instruction-limit
 .Op Fl m Ar memory-limit
 .Ar pool
@@ -45,6 +46,14 @@ will be run on
 and any attempts to access or modify other pools will cause an error.
 .Sh OPTIONS
 .Bl -tag -width "-t"
+.It Fl n
+Executes a read-only channel program, which runs faster.
+The program cannot change on-disk state by calling functions from the
+zfs.sync submodule.
+The program can be used to gather information such as properties and
+determining if changes would succeed (zfs.check.*).
+Without this flag, all pending changes must be synced to disk before a
+channel program can complete.
 .It Fl t Ar instruction-limit
 Execution time limit, in number of Lua instructions to execute.
 If a channel program executes more than the specified number of instructions,

man/man8/zfs.8

@@ -21,7 +21,7 @@
 .\"
 .\" Copyright (c) 2009 Sun Microsystems, Inc. All Rights Reserved.
 .\" Copyright 2011 Joshua M. Clulow <josh@sysmgr.org>
-.\" Copyright (c) 2011, 2016 by Delphix. All rights reserved.
+.\" Copyright (c) 2011, 2017 by Delphix. All rights reserved.
 .\" Copyright (c) 2013 by Saso Kiselkov. All rights reserved.
 .\" Copyright (c) 2014, Joyent, Inc. All rights reserved.
 .\" Copyright (c) 2014 by Adam Stevko. All rights reserved.
@@ -272,6 +272,7 @@
 .Ar snapshot Ar snapshot Ns | Ns Ar filesystem
 .Nm
 .Cm program
+.Op Fl n
 .Op Fl t Ar timeout
 .Op Fl m Ar memory_limit
 .Ar pool script
@@ -4042,6 +4043,7 @@ Display the path's inode change time as the first column of output.
 .It Xo
 .Nm
 .Cm program
+.Op Fl n
 .Op Fl t Ar timeout
 .Op Fl m Ar memory_limit
 .Ar pool script
@@ -4063,6 +4065,14 @@ For full documentation of the ZFS channel program interface, see the manual
 page for
 .Xr zfs-program 8 .
 .Bl -tag -width ""
+.It Fl n
+Executes a read-only channel program, which runs faster.
+The program cannot change on-disk state by calling functions from
+the zfs.sync submodule.
+The program can be used to gather information such as properties and
+determining if changes would succeed (zfs.check.*).
+Without this flag, all pending changes must be synced to disk before
+a channel program can complete.
 .It Fl t Ar timeout
 Execution time limit, in milliseconds.
 If a channel program executes for longer than the provided timeout, it will

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);

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);

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) {
@@ -233,6 +244,15 @@ zcp_synctask_snapshot(lua_State *state, boolean_t sync, nvlist_t *err_details)
 	const char *dsname = lua_tostring(state, 1);
 	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);

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));
 }
 

tests/zfs-tests/tests/functional/channel_program/channel_common.kshlib

@@ -11,22 +11,31 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 . $STF_SUITE/include/libtest.shlib
 
 ZCP_ROOT=$STF_SUITE/tests/functional/channel_program
 
-# <exitcode> <expected error string> <zfs program args>
-# e.g. log_program 0 $POOL foo.zcp arg1 arg2
+#
+# Note: In case of failure (log_fail) in this function
+# we delete the file passed as <input file> so the
+# test suite doesn't leak temp files on failures. So it
+# is expected that <input file> is a temp file and not
+# an installed file.
+#
+# <exitcode> <expected error string> <input file> <zfs program args>
+# e.g. log_program 0 "" tmp.7a12V $POOL foo.zcp arg1 arg2
 function log_program
 {
 	typeset expectexit=$1
 	shift
 	typeset expecterror=$1
 	shift
-	typeset cmdargs=$@ tmpout=$(mktemp) tmperr=$(mktemp) tmpin=$(mktemp)
+	typeset tmpin=$1
+	shift
+	typeset cmdargs=$@ tmpout=$(mktemp) tmperr=$(mktemp)
 
 	# Expected output/error filename is the same as the .zcp name
 	typeset basename
@@ -36,65 +45,195 @@ function log_program
 
 	log_note "running: zfs program $cmdargs:"
 
-	tee $tmpin | zfs program $cmdargs >$tmpout 2>$tmperr
+	zfs program $cmdargs >$tmpout 2>$tmperr
 	typeset ret=$?
 
 	log_note "input:\n$(cat $tmpin)"
 	log_note "output:\n$(cat $tmpout)"
 	log_note "error:\n$(cat $tmperr)"
-	# verify correct return value
+
+	#
+	# Verify correct return value
+	#
 	if [[ $ret -ne $expectexit ]]; then
+		rm $tmpout $tmperr $tmpin
 		log_fail "return mismatch: expected $expectexit, got $ret"
 	fi
 
 	#
 	# Check the output or reported error for successful or error returns,
 	# respectively.
+	#
 	if [[ -f "$basename.out" ]] && [[ $expectexit -eq 0 ]]; then
 
 		outdiff=$(diff "$basename.out" "$tmpout")
-		[[ $? -ne 0 ]] && log_fail "Output mismatch. Expected:\n" \
-		    "$(cat $basename.out)\nBut got:$(cat $tmpout)\n" \
+		if [[ $? -ne 0 ]]; then
+			output=$(cat $tmpout)
+			rm $tmpout $tmperr $tmpin
+			log_fail "Output mismatch. Expected:\n" \
+				"$(cat $basename.out)\nBut got:\n$output\n" \
 				"Diff:\n$outdiff"
+		fi
 
 	elif [[ -f "$basename.err" ]] && [[ $expectexit -ne 0 ]]; then
 
 		outdiff=$(diff "$basename.err" "$tmperr")
-		[[ $? -ne 0 ]] && log_fail "Error mismatch. Expected:\n" \
-		    "$(cat $basename.err)\nBut got:$(cat $tmpout)\n" \
+		if [[ $? -ne 0 ]]; then
+			outputerror=$(cat $tmperr)
+			rm $tmpout $tmperr $tmpin
+			log_fail "Error mismatch. Expected:\n" \
+				"$(cat $basename.err)\nBut got:\n$outputerror\n" \
 				"Diff:\n$outdiff"
+		fi
 
 	elif [[ -n $expecterror ]] && [[ $expectexit -ne 0 ]]; then
 
-		grep -q "$expecterror" $tmperr || \
+		grep -q "$expecterror" $tmperr
+		if [[ $? -ne 0 ]]; then
+			outputerror=$(cat $tmperr)
+			rm $tmpout $tmperr $tmpin
 			log_fail "Error mismatch. Expected to contain:\n" \
-		    "$expecterror\nBut got:$(cat $tmpout)\n"
+				"$expecterror\nBut got:\n$outputerror\n"
+		fi
 
 	elif [[ $expectexit -ne 0 ]]; then
 		#
 		# If there's no expected output, error reporting is allowed to
 		# vary, but ensure that we didn't fail silently.
 		#
-		[[ -z "$(cat $tmperr)" ]] && \
+		if [[ -z "$(cat $tmperr)" ]]; then
+			rm $tmpout $tmperr $tmpin
 			log_fail "error with no stderr output"
 		fi
+	fi
 
+	#
+	# Clean up all temp files except $tmpin which is
+	# reused for the second invocation of log_program.
+	#
 	rm $tmpout $tmperr
 }
 
+#
+# Even though the command's arguments are passed correctly
+# to the log_must_program family of wrappers the majority
+# of the time, zcp scripts passed as HERE documents can
+# make things trickier (see comment within the function
+# below) in the ordering of the commands arguments and how
+# they are passed. Thus, with this function we reconstruct
+# them to ensure that they are passed properly.
+#
+function log_program_construct_args
+{
+	typeset tmpin=$1
+	shift
+
+	args=""
+	i=0
+	while getopts "nt:m:" opt; do
+		case $opt in
+			t) args="$args -t $OPTARG"; i=$(($i + 2)) ;;
+			m) args="$args -m $OPTARG"; i=$(($i + 2)) ;;
+			n) args="$args -n"; i=$(($i + 1)) ;;
+		esac
+	done
+	shift $i
+
+	pool=$1
+	shift
+
+	#
+	# Catch HERE document if it exists and save it within our
+	# temp file. The reason we do this is that since the
+	# log_must_program wrapper calls zfs-program twice (once
+	# for open context and once for syncing) the HERE doc
+	# is consumed in the first invocation and the second one
+	# does not have a program to run.
+	#
+	test -s /dev/stdin && cat > $tmpin
+
+	#
+	# If $tmpin has contents it means that we consumed a HERE
+	# doc and $1 currently holds "-" (a dash). If there is no
+	# HERE doc and $tmpin is empty, then we copy the contents
+	# of the original channel program to $tmpin.
+	#
+	[[ -s $tmpin ]] || cp $1 $tmpin
+	shift
+
+	lua_args=$@
+
+	echo "$args $pool $tmpin $lua_args"
+}
+
+#
+# Program should complete successfully
+# when run in either context.
+#
 function log_must_program
 {
-	log_program 0 "" "$@"
-}
+	typeset tmpin=$(mktemp)
 
-function log_mustnot_program
-{
-	log_program 1 "" "$@"
-}
+	program_args=$(log_program_construct_args $tmpin $@)
 
+	log_program 0 "" $tmpin "-n $program_args"
+	log_program 0 "" $tmpin "$program_args"
+
+	rm $tmpin
+}
+#
+# Program should error as expected in
+# the same way in both contexts.
+#
 function log_mustnot_checkerror_program
 {
 	typeset expecterror=$1
 	shift
-	log_program 1 "$expecterror" "$@"
+	typeset tmpin=$(mktemp)
+
+	program_args=$(log_program_construct_args $tmpin $@)
+
+	log_program 1 "$expecterror" $tmpin "-n $program_args"
+	log_program 1 "$expecterror" $tmpin "$program_args"
+
+	rm $tmpin
+}
+
+#
+# Program should fail when run in either
+# context.
+#
+function log_mustnot_program
+{
+	log_mustnot_checkerror_program "" $@
+}
+
+
+#
+# Program should error as expected in
+# open context but complete successfully
+# in syncing context.
+#
+function log_mustnot_checkerror_program_open
+{
+	typeset expecterror=$1
+	shift
+	typeset tmpin=$(mktemp)
+
+	program_args=$(log_program_construct_args $tmpin $@)
+
+	log_program 1 "$expecterror" $tmpin "-n $program_args"
+	log_program 0 "" $tmpin "$program_args"
+
+	rm $tmpin
+}
+
+#
+# Program should complete successfully
+# when run in syncing context but fail
+# when attempted to run in open context.
+#
+function log_must_program_sync
+{
+	log_mustnot_checkerror_program_open "requires passing sync=TRUE" $@
 }

tests/zfs-tests/tests/functional/channel_program/lua_core/tst.return_large.ksh

@@ -47,7 +47,7 @@ output_lines=$(log_must zfs program $TESTPOOL \
 #
 # Make sure we fail if the return is over the memory limit
 #
-log_mustnot_program $TESTPOOL -m 10000 \
+log_mustnot_program -m 10000 $TESTPOOL \
     $ZCP_ROOT/lua_core/tst.return_large.zcp
 
 log_pass "Large return values work properly"

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.destroy_fs.ksh

@@ -11,7 +11,7 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 verify_runnable "global"
@@ -32,7 +32,7 @@ log_must zfs unmount $fs
 
 log_must datasetexists $fs
 
-log_must_program $TESTPOOL - $fs <<-EOF
+log_must_program_sync $TESTPOOL - $fs <<-EOF
 	arg = ...
 	fs = arg["argv"][1]
 	err = zfs.sync.destroy(fs)

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.destroy_snap.ksh

@@ -11,7 +11,7 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 verify_runnable "global"
@@ -31,7 +31,7 @@ create_snapshot $TESTPOOL/$TESTFS $TESTSNAP
 
 log_must snapexists $snap
 
-log_must_program $TESTPOOL - $snap <<-EOF
+log_must_program_sync $TESTPOOL - $snap <<-EOF
 	arg = ...
 	snap = arg["argv"][1]
 	err = zfs.sync.destroy(snap)

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.parse_args_neg.ksh

@@ -11,7 +11,7 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 . $STF_SUITE/tests/functional/channel_program/channel_common.kshlib
@@ -40,8 +40,10 @@ set -A progs "zfs.sync.destroy(\"foo\", \"bar\")" \
 typeset -i i=0
 while (( i < ${#progs[*]} )); do
 	log_note "running program: ${progs[i]}"
-	# output should contain the usage message, which starts with "destroy{"
-	echo ${progs[i]} | log_mustnot_checkerror_program "destroy{" $TESTPOOL -
+	# output should contain the usage message, which contains "destroy{"
+	log_mustnot_checkerror_program "destroy{" $TESTPOOL - <<-EOF
+		${progs[i]}
+	EOF
 	((i = i + 1))
 done
 

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.promote_conflict.ksh

@@ -11,7 +11,7 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 . $STF_SUITE/tests/functional/channel_program/channel_common.kshlib
@@ -49,7 +49,7 @@ log_must zfs snapshot $clone@$snap
 # code and description, which should be EEXIST (17) and the name of the
 # conflicting snapshot.
 #
-log_must_program $TESTPOOL \
+log_must_program_sync $TESTPOOL \
     $ZCP_ROOT/synctask_core/tst.promote_conflict.zcp $clone
 
 log_pass "Promoting a clone with a conflicting snapshot fails."

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.promote_multiple.ksh

@@ -11,7 +11,7 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 . $STF_SUITE/tests/functional/channel_program/channel_common.kshlib
@@ -62,7 +62,7 @@ log_must zfs clone $snap2 $clone2
 
 log_must zfs unmount -f $clone1
 
-log_must_program $TESTPOOL - <<-EOF
+log_must_program_sync $TESTPOOL - <<-EOF
     assert(zfs.sync.promote("$clone2") == 0)
     assert(zfs.sync.promote("$clone2") == 0)
     assert(zfs.sync.destroy("$clone1") == 0)

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.promote_simple.ksh

@@ -11,7 +11,7 @@
 #
 
 #
-# Copyright (c) 2016 by Delphix. All rights reserved.
+# Copyright (c) 2016, 2017 by Delphix. All rights reserved.
 #
 
 . $STF_SUITE/tests/functional/channel_program/channel_common.kshlib
@@ -40,7 +40,7 @@ log_must zfs create $fs
 log_must zfs snapshot $snap
 log_must zfs clone $snap $clone
 
-log_must_program $TESTPOOL - <<-EOF
+log_must_program_sync $TESTPOOL - <<-EOF
     assert(zfs.sync.promote("$clone") == 0)
 EOF
 

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.rollback_mult.ksh

@@ -39,7 +39,7 @@ log_must snapexists $snap1
 log_must snapexists $snap2
 log_must zfs unmount $fs
 
-log_must_program $TESTPOOL - $fs $snap2 <<-EOF
+log_must_program_sync $TESTPOOL - $fs $snap2 <<-EOF
 	arg = ...
 	fs = arg["argv"][1]
 	snap = arg["argv"][2]

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.rollback_one.ksh

@@ -37,7 +37,7 @@ log_must rm $file
 log_must snapexists $snap
 log_must zfs unmount $fs
 
-log_must_program $TESTPOOL - $fs <<-EOF
+log_must_program_sync $TESTPOOL - $fs <<-EOF
 	arg = ...
 	fs = arg["argv"][1]
 	err = zfs.sync.rollback(fs)

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.snapshot_destroy.ksh

@@ -33,7 +33,7 @@ log_onexit cleanup
 
 log_must zfs create $fs
 
-log_must_program $TESTPOOL \
+log_must_program_sync $TESTPOOL \
     $ZCP_ROOT/synctask_core/tst.snapshot_destroy.zcp $fs
 
 log_pass "Creating/destroying snapshots in one channel program works"

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.snapshot_neg.ksh

@@ -38,7 +38,8 @@ log_must zfs create $fs1
 log_must zfs create $fs2
 log_must zfs snapshot $fs1@snap1
 
-log_must_program $TESTPOOL $ZCP_ROOT/synctask_core/tst.snapshot_neg.zcp $fs1 $fs2
+log_must_program_sync $TESTPOOL \
+    $ZCP_ROOT/synctask_core/tst.snapshot_neg.zcp $fs1 $fs2
 
 log_pass "zfs.sync.snapshot returns correct errors on invalid input"
 

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.snapshot_recursive.ksh

@@ -46,7 +46,7 @@ for fs in $filesystems; do
     log_must zfs create $fs
 done
 
-log_must_program $TESTPOOL \
+log_must_program_sync $TESTPOOL \
     $ZCP_ROOT/synctask_core/tst.snapshot_recursive.zcp $rootfs $snapname
 
 #

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.snapshot_simple.ksh

@@ -34,7 +34,7 @@ log_onexit cleanup
 
 log_must zfs create $fs
 
-log_must_program $TESTPOOL \
+log_must_program_sync $TESTPOOL \
     $ZCP_ROOT/synctask_core/tst.snapshot_simple.zcp $fs $snapname
 
 log_pass "Simple snapshotting works"