[PATCH 0/5] external ioengine fixes

[PATCH 0/5] external ioengine fixes

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

This is resend of below which I believe hasn't been reviewed/commented yet.

The entire diff is the exact same as the original one, but [PATCH 3/4]
has been splitted into two ([PATCH 3/5] and [PATCH 4/5] in this series),
to make diffs more clear and obvious.

[PATCH 0/4] external ioengine fixes
http://www.spinics.net/lists/fio/msg06204.html

--
Tomohiro Kusumi (5):
  skeleton: add option example
  fix broken external ioengine option
  cleanup ioengine_load() (for the next commit)
  fix load_ioengine() not to support no "external:" prefix
  add __load_ioengine() to separate ioengine loading from td context

 HOWTO                       |  4 +++-
 engines/skeleton_external.c | 32 +++++++++++++++++++++++++++++++-
 fio.1                       |  4 +++-
 init.c                      | 21 ++-------------------
 ioengines.c                 | 30 ++++++++++++++++++------------
 ioengines.h                 |  2 +-
 options.c                   | 34 ++++++++++++++++++++++++++++++++++
 thread_options.h            |  1 +
 8 files changed, 93 insertions(+), 35 deletions(-)

-- 
2.9.5

[PATCH 1/5] skeleton: add option example

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

Add an example of ioengine specific options, which can/should exist
within each ioengine code. Also fix/add documentation.

Also see e59b9e11('move skip_bad= option to engines/mtd.c').

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 HOWTO                       |  4 +++-
 engines/skeleton_external.c | 32 +++++++++++++++++++++++++++++++-
 fio.1                       |  4 +++-
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/HOWTO b/HOWTO
index 3a720c3..2a70b7c 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1791,7 +1791,9 @@ I/O engine
 		**external**
 			Prefix to specify loading an external I/O engine object file. Append
 			the engine filename, e.g. ``ioengine=external:/tmp/foo.o`` to load
-			ioengine :file:`foo.o` in :file:`/tmp`.
+			ioengine :file:`foo.o` in :file:`/tmp`. The path can be either
+			absolute or relative. See :file:`engines/skeleton_external.c` for
+			details of writing an external I/O engine.
 
 
 I/O engine specific parameters
diff --git a/engines/skeleton_external.c b/engines/skeleton_external.c
index 4bebcc4..56f89f9 100644
--- a/engines/skeleton_external.c
+++ b/engines/skeleton_external.c
@@ -3,7 +3,8 @@
  *
  * Should be compiled with:
  *
- * gcc -Wall -O2 -g -shared -rdynamic -fPIC -o engine.o engine.c
+ * gcc -Wall -O2 -g -shared -rdynamic -fPIC -o skeleton_external.o skeleton_external.c
+ * (also requires -D_GNU_SOURCE -DCONFIG_STRSEP on Linux)
  *
  */
 #include <stdio.h>
@@ -13,6 +14,7 @@
 #include <assert.h>
 
 #include "../fio.h"
+#include "../optgroup.h"
 
 /*
  * The core of the module is identical to the ones included with fio,
@@ -21,6 +23,32 @@
  */
 
 /*
+ * The io engine can define its own options within the io engine source.
+ * The option member must not be at offset 0, due to the way fio parses
+ * the given option. Just add a padding pointer unless the io engine has
+ * something usable.
+ */
+struct fio_skeleton_options {
+	void *pad; /* avoid ->off1 of fio_option becomes 0 */
+	unsigned int dummy;
+};
+
+static struct fio_option options[] = {
+	{
+		.name	= "dummy",
+		.lname	= "ldummy",
+		.type	= FIO_OPT_STR_SET,
+		.off1	= offsetof(struct fio_skeleton_options, dummy),
+		.help	= "Set dummy",
+		.category = FIO_OPT_C_ENGINE, /* always use this */
+		.group	= FIO_OPT_G_INVALID, /* this can be different */
+	},
+	{
+		.name	= NULL,
+	},
+};
+
+/*
  * The ->event() hook is called to match an event number with an io_u.
  * After the core has called ->getevents() and it has returned eg 3,
  * the ->event() hook must return the 3 events that have completed for
@@ -140,4 +168,6 @@ struct ioengine_ops ioengine = {
 	.cleanup	= fio_skeleton_cleanup,
 	.open_file	= fio_skeleton_open,
 	.close_file	= fio_skeleton_close,
+	.options	= options,
+	.option_struct_size	= sizeof(struct fio_skeleton_options),
 };
diff --git a/fio.1 b/fio.1
index 5b63dfd..97133da 100644
--- a/fio.1
+++ b/fio.1
@@ -1572,7 +1572,9 @@ Read and write using device DAX to a persistent memory device (e.g.,
 .B external
 Prefix to specify loading an external I/O engine object file. Append
 the engine filename, e.g. `ioengine=external:/tmp/foo.o' to load
-ioengine `foo.o' in `/tmp'.
+ioengine `foo.o' in `/tmp'. The path can be either
+absolute or relative. See `engines/skeleton_external.c' in the fio source for
+details of writing an external I/O engine.
 .SS "I/O engine specific parameters"
 In addition, there are some parameters which are only valid when a specific
 \fBioengine\fR is in use. These are used identically to normal parameters,
-- 
2.9.5

[PATCH 2/5] fix broken external ioengine option

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

get_engine_name() function added to support external ioengine option
via "external:" prefix in below commits in 2007 has been broken.
 7b395ca5('Prefix external io engine loading with 'external'')
 8a7bd877('Document loading external io engines')

It seems to have been broken since below commit in 2011 which made
__handle_option() strip ':' and after while parsing the option.
 c44b1ff5('Add sub-option support (sort-of) and convert libaio_userspace_reap')

Above change made fio dlopen "external" instead of the path located
after "external:", since the path had already been stripped by
__handle_option() by the time get_engine_name() was called. This commit
fixes it by adding ->ioengine_so_path pointer to keep the path while
parsing using sub-option callback.

Note that removing get_engine_name() doesn't affect non "external:"
ioengine options, as the option parser also strips while parsing.

-- before this commit
 # ./fio --name=xxx --ioengine=external:./engines/skeleton_external.so
 fio: engine external not loadable
 fio: failed to load engine external
 fio: file:ioengines.c:91, func=dlopen, error=external: cannot open shared object file: No such file or directory

-- with this commit
 # ./fio --name=xxx --ioengine=external:./engines/skeleton_external.so
 xxx: (g=0): rw=read, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=engine_name, iodepth=1
 fio-3.0-32-g68f6f
 Starting 1 process
 xxx: you need to specify size=
 fio: pid=0, err=22/file:filesetup.c:973, func=total_file_size, error=Invalid argument

 Run status group 0 (all jobs):

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 init.c           | 19 ++++---------------
 options.c        | 34 ++++++++++++++++++++++++++++++++++
 thread_options.h |  1 +
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/init.c b/init.c
index 625c937..2da64ba 100644
--- a/init.c
+++ b/init.c
@@ -912,20 +912,6 @@ static int fixup_options(struct thread_data *td)
 	return ret;
 }
 
-/* External engines are specified by "external:name.o") */
-static const char *get_engine_name(const char *str)
-{
-	char *p = strstr(str, ":");
-
-	if (!p)
-		return str;
-
-	p++;
-	strip_blank_front(&p);
-	strip_blank_end(p);
-	return p;
-}
-
 static void init_rand_file_service(struct thread_data *td)
 {
 	unsigned long nranges = td->o.nr_files << FIO_FSERVICE_SHIFT;
@@ -1057,7 +1043,10 @@ int ioengine_load(struct thread_data *td)
 		free_ioengine(td);
 	}
 
-	engine = get_engine_name(td->o.ioengine);
+	/*
+	 * Use ->ioengine_so_path if an external ioengine is specified.
+	 */
+	engine = td->o.ioengine_so_path ?: td->o.ioengine;
 	td->io_ops = load_ioengine(td, engine);
 	if (!td->io_ops) {
 		log_err("fio: failed to load engine %s\n", engine);
diff --git a/options.c b/options.c
index 443791a..54fa4ee 100644
--- a/options.c
+++ b/options.c
@@ -1462,6 +1462,39 @@ static int str_write_hist_log_cb(void *data, const char *str)
 	return 0;
 }
 
+/*
+ * str is supposed to be a substring of the strdup'd original string,
+ * and is valid only if it's a regular file path.
+ * This function keeps the pointer to the path as needed later.
+ *
+ * "external:/path/to/so\0" <- original pointer updated with strdup'd
+ * "external\0"             <- above pointer after parsed, i.e. ->ioengine
+ *          "/path/to/so\0" <- str argument, i.e. ->ioengine_so_path
+ */
+static int str_ioengine_external_cb(void *data, const char *str)
+{
+	struct thread_data *td = cb_data_to_td(data);
+	struct stat sb;
+	char *p;
+
+	if (!str) {
+		log_err("fio: null external ioengine path\n");
+		return 1;
+	}
+
+	p = (char *)str; /* str is mutable */
+	strip_blank_front(&p);
+	strip_blank_end(p);
+
+	if (stat(p, &sb) || !S_ISREG(sb.st_mode)) {
+		log_err("fio: invalid external ioengine path \"%s\"\n", p);
+		return 1;
+	}
+
+	td->o.ioengine_so_path = p;
+	return 0;
+}
+
 static int rw_verify(struct fio_option *o, void *data)
 {
 	struct thread_data *td = cb_data_to_td(data);
@@ -1812,6 +1845,7 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 #endif
 			  { .ival = "external",
 			    .help = "Load external engine (append name)",
+			    .cb = str_ioengine_external_cb,
 			  },
 		},
 	},
diff --git a/thread_options.h b/thread_options.h
index 26a3e0e..fd6576e 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -53,6 +53,7 @@ struct thread_options {
 	char *filename_format;
 	char *opendir;
 	char *ioengine;
+	char *ioengine_so_path;
 	char *mmapfile;
 	enum td_ddir td_ddir;
 	unsigned int rw_seq;
-- 
2.9.5

[PATCH 3/5] cleanup ioengine_load() (for the next commit)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

This commit removes name argument from ioengine_load(), as both
ioengine name and path are self contained in td.
No functional changes.

This makes the next commit's diff more clear by separating non
functional noise.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 init.c      | 10 ++--------
 ioengines.c |  8 ++++++--
 ioengines.h |  2 +-
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/init.c b/init.c
index 2da64ba..cf5c646 100644
--- a/init.c
+++ b/init.c
@@ -1023,8 +1023,6 @@ void td_fill_rand_seeds(struct thread_data *td)
  */
 int ioengine_load(struct thread_data *td)
 {
-	const char *engine;
-
 	if (!td->o.ioengine) {
 		log_err("fio: internal fault, no IO engine specified\n");
 		return 1;
@@ -1043,13 +1041,9 @@ int ioengine_load(struct thread_data *td)
 		free_ioengine(td);
 	}
 
-	/*
-	 * Use ->ioengine_so_path if an external ioengine is specified.
-	 */
-	engine = td->o.ioengine_so_path ?: td->o.ioengine;
-	td->io_ops = load_ioengine(td, engine);
+	td->io_ops = load_ioengine(td);
 	if (!td->io_ops) {
-		log_err("fio: failed to load engine %s\n", engine);
+		log_err("fio: failed to load engine\n");
 		return 1;
 	}
 
diff --git a/ioengines.c b/ioengines.c
index 919781c..54aa5a6 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -123,10 +123,13 @@ static struct ioengine_ops *dlopen_ioengine(struct thread_data *td,
 	return ops;
 }
 
-struct ioengine_ops *load_ioengine(struct thread_data *td, const char *name)
+struct ioengine_ops *load_ioengine(struct thread_data *td)
 {
 	struct ioengine_ops *ops;
 	char engine[64];
+	char *name;
+
+	name = td->o.ioengine_so_path ?: td->o.ioengine;
 
 	dprint(FD_IO, "load ioengine %s\n", name);
 
@@ -573,7 +576,8 @@ int fio_show_ioengine_help(const char *engine)
 
 	memset(&td, 0, sizeof(td));
 
-	io_ops = load_ioengine(&td, engine);
+	td.o.ioengine = (char *)engine;
+	io_ops = load_ioengine(&td);
 	if (!io_ops) {
 		log_info("IO engine %s not found\n", engine);
 		return 1;
diff --git a/ioengines.h b/ioengines.h
index f24f4df..177cbc0 100644
--- a/ioengines.h
+++ b/ioengines.h
@@ -79,7 +79,7 @@ extern int td_io_close_file(struct thread_data *, struct fio_file *);
 extern int td_io_unlink_file(struct thread_data *, struct fio_file *);
 extern int __must_check td_io_get_file_size(struct thread_data *, struct fio_file *);
 
-extern struct ioengine_ops *load_ioengine(struct thread_data *, const char *);
+extern struct ioengine_ops *load_ioengine(struct thread_data *);
 extern void register_ioengine(struct ioengine_ops *);
 extern void unregister_ioengine(struct ioengine_ops *);
 extern void free_ioengine(struct thread_data *);
-- 
2.9.5

[PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

To load ioengines, it should be done by either
 1) find the ioengine from the existing (static linked) ioengines or
 2) dlopen the path in case of external ioengine,

but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
dynamically loaded unless with "external:" prefix by design.

The current implementation (i.e. do 2 if 1 failed) happened to have
been able to load an external ioengine with below syntax without
"external:" prefix, but this is a bug rather than a feature.

(--)ioengine=./engines/skeleton_external.so

The design of the external ioengine option since below commits in 2007
 7b395ca5('Prefix external io engine loading with 'external'')
 8a7bd877('Document loading external io engines')
is to use "external:/path/to/so" syntax.

This commit fixes above bug, which also potentially avoids the case
where "/path/to/so" within the given "external:/path/to/so" happens
to match the existing name, though this is normally unlikely to happen.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 ioengines.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/ioengines.c b/ioengines.c
index 54aa5a6..0c631e8 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -125,26 +125,29 @@ static struct ioengine_ops *dlopen_ioengine(struct thread_data *td,
 
 struct ioengine_ops *load_ioengine(struct thread_data *td)
 {
-	struct ioengine_ops *ops;
-	char engine[64];
-	char *name;
-
-	name = td->o.ioengine_so_path ?: td->o.ioengine;
+	struct ioengine_ops *ops = NULL;
+	const char *name = NULL;
 
-	dprint(FD_IO, "load ioengine %s\n", name);
+	if (strcmp(td->o.ioengine, "external")) {
+		char engine[64];
 
-	engine[sizeof(engine) - 1] = '\0';
-	strncpy(engine, name, sizeof(engine) - 1);
+		name = td->o.ioengine;
+		engine[sizeof(engine) - 1] = '\0';
+		strncpy(engine, name, sizeof(engine) - 1);
 
-	/*
-	 * linux libaio has alias names, so convert to what we want
-	 */
-	if (!strncmp(engine, "linuxaio", 8) || !strncmp(engine, "aio", 3))
-		strcpy(engine, "libaio");
+		/*
+		 * linux libaio has alias names, so convert to what we want
+		 */
+		if (!strncmp(engine, "linuxaio", 8) || !strncmp(engine, "aio", 3))
+			strcpy(engine, "libaio");
 
-	ops = find_ioengine(engine);
-	if (!ops)
+		dprint(FD_IO, "load ioengine %s\n", engine);
+		ops = find_ioengine(engine);
+	} else if (td->o.ioengine_so_path) {
+		name = td->o.ioengine_so_path;
 		ops = dlopen_ioengine(td, name);
+	} else
+		log_err("fio: missing external ioengine path\n");
 
 	if (!ops) {
 		log_err("fio: engine %s not loadable\n", name);
-- 
2.9.5

[PATCH 5/5] add __load_ioengine() to separate ioengine loading from td context

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

Add a sub function __load_ioengine(), which only takes name argument,
to be called from load_ioengine(). No functional changes.

This lets fio_show_ioengine_help() get rid of a local variable td
which only existed to call load_ioengine(&td, ...) while this td had
no actual thread context thus unneeded.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 ioengines.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/ioengines.c b/ioengines.c
index 0c631e8..e78b4f7 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -123,26 +123,31 @@ static struct ioengine_ops *dlopen_ioengine(struct thread_data *td,
 	return ops;
 }
 
+static struct ioengine_ops *__load_ioengine(const char *name)
+{
+	char engine[64];
+
+	engine[sizeof(engine) - 1] = '\0';
+	strncpy(engine, name, sizeof(engine) - 1);
+
+	/*
+	 * linux libaio has alias names, so convert to what we want
+	 */
+	if (!strncmp(engine, "linuxaio", 8) || !strncmp(engine, "aio", 3))
+		strcpy(engine, "libaio");
+
+	dprint(FD_IO, "load ioengine %s\n", engine);
+	return find_ioengine(engine);
+}
+
 struct ioengine_ops *load_ioengine(struct thread_data *td)
 {
 	struct ioengine_ops *ops = NULL;
 	const char *name = NULL;
 
 	if (strcmp(td->o.ioengine, "external")) {
-		char engine[64];
-
 		name = td->o.ioengine;
-		engine[sizeof(engine) - 1] = '\0';
-		strncpy(engine, name, sizeof(engine) - 1);
-
-		/*
-		 * linux libaio has alias names, so convert to what we want
-		 */
-		if (!strncmp(engine, "linuxaio", 8) || !strncmp(engine, "aio", 3))
-			strcpy(engine, "libaio");
-
-		dprint(FD_IO, "load ioengine %s\n", engine);
-		ops = find_ioengine(engine);
+		ops = __load_ioengine(name);
 	} else if (td->o.ioengine_so_path) {
 		name = td->o.ioengine_so_path;
 		ops = dlopen_ioengine(td, name);
@@ -558,7 +563,6 @@ int td_io_get_file_size(struct thread_data *td, struct fio_file *f)
 int fio_show_ioengine_help(const char *engine)
 {
 	struct flist_head *entry;
-	struct thread_data td;
 	struct ioengine_ops *io_ops;
 	char *sep;
 	int ret = 1;
@@ -577,10 +581,7 @@ int fio_show_ioengine_help(const char *engine)
 		sep++;
 	}
 
-	memset(&td, 0, sizeof(td));
-
-	td.o.ioengine = (char *)engine;
-	io_ops = load_ioengine(&td);
+	io_ops = __load_ioengine(engine);
 	if (!io_ops) {
 		log_info("IO engine %s not found\n", engine);
 		return 1;
@@ -591,7 +592,5 @@ int fio_show_ioengine_help(const char *engine)
 	else
 		log_info("IO engine %s has no options\n", io_ops->name);
 
-	free_ioengine(&td);
-
 	return ret;
 }
-- 
2.9.5

Re: [PATCH 0/5] external ioengine fixes

[Jens Axboe]

On Thu, Aug 31 2017, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> This is resend of below which I believe hasn't been reviewed/commented yet.
> 
> The entire diff is the exact same as the original one, but [PATCH 3/4]
> has been splitted into two ([PATCH 3/5] and [PATCH 4/5] in this series),
> to make diffs more clear and obvious.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Sitsofe Wheeler]

On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>
> To load ioengines, it should be done by either
>  1) find the ioengine from the existing (static linked) ioengines or
>  2) dlopen the path in case of external ioengine,
>
> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
> dynamically loaded unless with "external:" prefix by design.
>
> The current implementation (i.e. do 2 if 1 failed) happened to have
> been able to load an external ioengine with below syntax without
> "external:" prefix, but this is a bug rather than a feature.
>
> (--)ioengine=./engines/skeleton_external.so
>
> The design of the external ioengine option since below commits in 2007
>  7b395ca5('Prefix external io engine loading with 'external'')
>  8a7bd877('Document loading external io engines')
> is to use "external:/path/to/so" syntax.
>
> This commit fixes above bug, which also potentially avoids the case
> where "/path/to/so" within the given "external:/path/to/so" happens
> to match the existing name, though this is normally unlikely to happen.

People have already started depending on the "wrong" behaviour e.g.
ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
. Because they're external it's hard to find out just how prevalent
this behaviour is.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Tomohiro Kusumi]

2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>
>> To load ioengines, it should be done by either
>>  1) find the ioengine from the existing (static linked) ioengines or
>>  2) dlopen the path in case of external ioengine,
>>
>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>> dynamically loaded unless with "external:" prefix by design.
>>
>> The current implementation (i.e. do 2 if 1 failed) happened to have
>> been able to load an external ioengine with below syntax without
>> "external:" prefix, but this is a bug rather than a feature.
>>
>> (--)ioengine=./engines/skeleton_external.so
>>
>> The design of the external ioengine option since below commits in 2007
>>  7b395ca5('Prefix external io engine loading with 'external'')
>>  8a7bd877('Document loading external io engines')
>> is to use "external:/path/to/so" syntax.
>>
>> This commit fixes above bug, which also potentially avoids the case
>> where "/path/to/so" within the given "external:/path/to/so" happens
>> to match the existing name, though this is normally unlikely to happen.
>
> People have already started depending on the "wrong" behaviour e.g.
> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
> . Because they're external it's hard to find out just how prevalent
> this behaviour is.

Then they can add "external:".
If it has ever been official (in docs/etc), then the program should
maintain such behavior, like fio keeps kb_base=1024 by default.

This isn't the first time fio broke external ioengines to begin with,
and fio doesn't guarantee external ioengines.
e.g. this commit more than a year ago broke ABI.
565e784df05c2529479eed8a38701a33b01894bd

It's essentially the same as non mainline'd kernel modules are on
their own to be uptodate with upstream.

>
> --
> Sitsofe | http://sucs.org/~sits/

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Jens Axboe]

On 08/31/2017 11:38 PM, Tomohiro Kusumi wrote:
> 2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>
>>> To load ioengines, it should be done by either
>>>  1) find the ioengine from the existing (static linked) ioengines or
>>>  2) dlopen the path in case of external ioengine,
>>>
>>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>>> dynamically loaded unless with "external:" prefix by design.
>>>
>>> The current implementation (i.e. do 2 if 1 failed) happened to have
>>> been able to load an external ioengine with below syntax without
>>> "external:" prefix, but this is a bug rather than a feature.
>>>
>>> (--)ioengine=./engines/skeleton_external.so
>>>
>>> The design of the external ioengine option since below commits in 2007
>>>  7b395ca5('Prefix external io engine loading with 'external'')
>>>  8a7bd877('Document loading external io engines')
>>> is to use "external:/path/to/so" syntax.
>>>
>>> This commit fixes above bug, which also potentially avoids the case
>>> where "/path/to/so" within the given "external:/path/to/so" happens
>>> to match the existing name, though this is normally unlikely to happen.
>>
>> People have already started depending on the "wrong" behaviour e.g.
>> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
>> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
>> . Because they're external it's hard to find out just how prevalent
>> this behaviour is.
> 
> Then they can add "external:".
> If it has ever been official (in docs/etc), then the program should
> maintain such behavior, like fio keeps kb_base=1024 by default.
> 
> This isn't the first time fio broke external ioengines to begin with,
> and fio doesn't guarantee external ioengines.
> e.g. this commit more than a year ago broke ABI.
> 565e784df05c2529479eed8a38701a33b01894bd
> 
> It's essentially the same as non mainline'd kernel modules are on
> their own to be uptodate with upstream.

No, those two are VERY different. Fio doesn't make any guarantees wrt
keeping the same ABI (or API, even) for external engines. If they break,
then you get to keep the pieces and glue them back together. We have
the engine version number as well to guard against those kinds of
changes.

But we don't break job files, at least not knowingly, and definitely
not just in the name of a cleanup.

-- 
Jens Axboe

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Tomohiro Kusumi]

2017-09-01 17:43 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
> On 08/31/2017 11:38 PM, Tomohiro Kusumi wrote:
>> 2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>
>>>> To load ioengines, it should be done by either
>>>>  1) find the ioengine from the existing (static linked) ioengines or
>>>>  2) dlopen the path in case of external ioengine,
>>>>
>>>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>>>> dynamically loaded unless with "external:" prefix by design.
>>>>
>>>> The current implementation (i.e. do 2 if 1 failed) happened to have
>>>> been able to load an external ioengine with below syntax without
>>>> "external:" prefix, but this is a bug rather than a feature.
>>>>
>>>> (--)ioengine=./engines/skeleton_external.so
>>>>
>>>> The design of the external ioengine option since below commits in 2007
>>>>  7b395ca5('Prefix external io engine loading with 'external'')
>>>>  8a7bd877('Document loading external io engines')
>>>> is to use "external:/path/to/so" syntax.
>>>>
>>>> This commit fixes above bug, which also potentially avoids the case
>>>> where "/path/to/so" within the given "external:/path/to/so" happens
>>>> to match the existing name, though this is normally unlikely to happen.
>>>
>>> People have already started depending on the "wrong" behaviour e.g.
>>> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
>>> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
>>> . Because they're external it's hard to find out just how prevalent
>>> this behaviour is.
>>
>> Then they can add "external:".
>> If it has ever been official (in docs/etc), then the program should
>> maintain such behavior, like fio keeps kb_base=1024 by default.
>>
>> This isn't the first time fio broke external ioengines to begin with,
>> and fio doesn't guarantee external ioengines.
>> e.g. this commit more than a year ago broke ABI.
>> 565e784df05c2529479eed8a38701a33b01894bd
>>
>> It's essentially the same as non mainline'd kernel modules are on
>> their own to be uptodate with upstream.
>
> No, those two are VERY different. Fio doesn't make any guarantees wrt
> keeping the same ABI (or API, even) for external engines. If they break,
> then you get to keep the pieces and glue them back together. We have
> the engine version number as well to guard against those kinds of
> changes.
>
> But we don't break job files, at least not knowingly, and definitely
> not just in the name of a cleanup.

Yes, but you took this patch which breaks job files *wrongly* using
external engine option syntax.
By "job files wrongly using", I mean job files somehow found
unofficial (i.e. undocumented, not intended) way to dlopen it.

If breaking this case is not okay, shouldn't this commit be reverted
or the design be expand to allow both with/without prefix ?

> --
> Jens Axboe
>

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Jens Axboe]

On 09/01/2017 09:03 AM, Tomohiro Kusumi wrote:
> 2017-09-01 17:43 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
>> On 08/31/2017 11:38 PM, Tomohiro Kusumi wrote:
>>> 2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>>> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>>
>>>>> To load ioengines, it should be done by either
>>>>>  1) find the ioengine from the existing (static linked) ioengines or
>>>>>  2) dlopen the path in case of external ioengine,
>>>>>
>>>>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>>>>> dynamically loaded unless with "external:" prefix by design.
>>>>>
>>>>> The current implementation (i.e. do 2 if 1 failed) happened to have
>>>>> been able to load an external ioengine with below syntax without
>>>>> "external:" prefix, but this is a bug rather than a feature.
>>>>>
>>>>> (--)ioengine=./engines/skeleton_external.so
>>>>>
>>>>> The design of the external ioengine option since below commits in 2007
>>>>>  7b395ca5('Prefix external io engine loading with 'external'')
>>>>>  8a7bd877('Document loading external io engines')
>>>>> is to use "external:/path/to/so" syntax.
>>>>>
>>>>> This commit fixes above bug, which also potentially avoids the case
>>>>> where "/path/to/so" within the given "external:/path/to/so" happens
>>>>> to match the existing name, though this is normally unlikely to happen.
>>>>
>>>> People have already started depending on the "wrong" behaviour e.g.
>>>> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
>>>> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
>>>> . Because they're external it's hard to find out just how prevalent
>>>> this behaviour is.
>>>
>>> Then they can add "external:".
>>> If it has ever been official (in docs/etc), then the program should
>>> maintain such behavior, like fio keeps kb_base=1024 by default.
>>>
>>> This isn't the first time fio broke external ioengines to begin with,
>>> and fio doesn't guarantee external ioengines.
>>> e.g. this commit more than a year ago broke ABI.
>>> 565e784df05c2529479eed8a38701a33b01894bd
>>>
>>> It's essentially the same as non mainline'd kernel modules are on
>>> their own to be uptodate with upstream.
>>
>> No, those two are VERY different. Fio doesn't make any guarantees wrt
>> keeping the same ABI (or API, even) for external engines. If they break,
>> then you get to keep the pieces and glue them back together. We have
>> the engine version number as well to guard against those kinds of
>> changes.
>>
>> But we don't break job files, at least not knowingly, and definitely
>> not just in the name of a cleanup.
> 
> Yes, but you took this patch which breaks job files *wrongly* using
> external engine option syntax.

Right, but it's not in a release yet, so it's not impossible to just
revert it.

> By "job files wrongly using", I mean job files somehow found
> unofficial (i.e. undocumented, not intended) way to dlopen it.

This case is a bit difficult, since it isn't black and white. I took a
look at some of the older documentation, and we have been documenting
the need for 'external' for a long time (forever?). So I'd be inclined
to let this one stay in.

> If breaking this case is not okay, shouldn't this commit be reverted
> or the design be expand to allow both with/without prefix ?

The design (or intent, at least) has always been that if you do:

ioengine=something

then we first try and load 'something' as an internal engine. If that
fails, we look for an external engine of that name. That makes sense to
me, since it means you don't have to do anything special to load an
external engine. I think we should try and retain that behavior, if we
can.

-- 
Jens Axboe

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Tomohiro Kusumi]

2017-09-01 18:08 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
> On 09/01/2017 09:03 AM, Tomohiro Kusumi wrote:
>> 2017-09-01 17:43 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
>>> On 08/31/2017 11:38 PM, Tomohiro Kusumi wrote:
>>>> 2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>>>> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>>>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>>>
>>>>>> To load ioengines, it should be done by either
>>>>>>  1) find the ioengine from the existing (static linked) ioengines or
>>>>>>  2) dlopen the path in case of external ioengine,
>>>>>>
>>>>>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>>>>>> dynamically loaded unless with "external:" prefix by design.
>>>>>>
>>>>>> The current implementation (i.e. do 2 if 1 failed) happened to have
>>>>>> been able to load an external ioengine with below syntax without
>>>>>> "external:" prefix, but this is a bug rather than a feature.
>>>>>>
>>>>>> (--)ioengine=./engines/skeleton_external.so
>>>>>>
>>>>>> The design of the external ioengine option since below commits in 2007
>>>>>>  7b395ca5('Prefix external io engine loading with 'external'')
>>>>>>  8a7bd877('Document loading external io engines')
>>>>>> is to use "external:/path/to/so" syntax.
>>>>>>
>>>>>> This commit fixes above bug, which also potentially avoids the case
>>>>>> where "/path/to/so" within the given "external:/path/to/so" happens
>>>>>> to match the existing name, though this is normally unlikely to happen.
>>>>>
>>>>> People have already started depending on the "wrong" behaviour e.g.
>>>>> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
>>>>> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
>>>>> . Because they're external it's hard to find out just how prevalent
>>>>> this behaviour is.
>>>>
>>>> Then they can add "external:".
>>>> If it has ever been official (in docs/etc), then the program should
>>>> maintain such behavior, like fio keeps kb_base=1024 by default.
>>>>
>>>> This isn't the first time fio broke external ioengines to begin with,
>>>> and fio doesn't guarantee external ioengines.
>>>> e.g. this commit more than a year ago broke ABI.
>>>> 565e784df05c2529479eed8a38701a33b01894bd
>>>>
>>>> It's essentially the same as non mainline'd kernel modules are on
>>>> their own to be uptodate with upstream.
>>>
>>> No, those two are VERY different. Fio doesn't make any guarantees wrt
>>> keeping the same ABI (or API, even) for external engines. If they break,
>>> then you get to keep the pieces and glue them back together. We have
>>> the engine version number as well to guard against those kinds of
>>> changes.
>>>
>>> But we don't break job files, at least not knowingly, and definitely
>>> not just in the name of a cleanup.
>>
>> Yes, but you took this patch which breaks job files *wrongly* using
>> external engine option syntax.
>
> Right, but it's not in a release yet, so it's not impossible to just
> revert it.
>
>> By "job files wrongly using", I mean job files somehow found
>> unofficial (i.e. undocumented, not intended) way to dlopen it.
>
> This case is a bit difficult, since it isn't black and white. I took a
> look at some of the older documentation, and we have been documenting
> the need for 'external' for a long time (forever?). So I'd be inclined
> to let this one stay in.
>
>> If breaking this case is not okay, shouldn't this commit be reverted
>> or the design be expand to allow both with/without prefix ?
>
> The design (or intent, at least) has always been that if you do:
>
> ioengine=something
>
> then we first try and load 'something' as an internal engine. If that
> fails, we look for an external engine of that name. That makes sense to
> me, since it means you don't have to do anything special to load an
> external engine. I think we should try and retain that behavior, if we
> can.

OK, thank you for the explanation.
I don't mind reverting this at all, since now that "external:" prefix
works as documented (after [PATCH 2/5]).

> --
> Jens Axboe
>

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Jens Axboe]

On 09/01/2017 09:18 AM, Tomohiro Kusumi wrote:
> 2017-09-01 18:08 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
>> On 09/01/2017 09:03 AM, Tomohiro Kusumi wrote:
>>> 2017-09-01 17:43 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
>>>> On 08/31/2017 11:38 PM, Tomohiro Kusumi wrote:
>>>>> 2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>>>>> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>>>>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>>>>
>>>>>>> To load ioengines, it should be done by either
>>>>>>>  1) find the ioengine from the existing (static linked) ioengines or
>>>>>>>  2) dlopen the path in case of external ioengine,
>>>>>>>
>>>>>>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>>>>>>> dynamically loaded unless with "external:" prefix by design.
>>>>>>>
>>>>>>> The current implementation (i.e. do 2 if 1 failed) happened to have
>>>>>>> been able to load an external ioengine with below syntax without
>>>>>>> "external:" prefix, but this is a bug rather than a feature.
>>>>>>>
>>>>>>> (--)ioengine=./engines/skeleton_external.so
>>>>>>>
>>>>>>> The design of the external ioengine option since below commits in 2007
>>>>>>>  7b395ca5('Prefix external io engine loading with 'external'')
>>>>>>>  8a7bd877('Document loading external io engines')
>>>>>>> is to use "external:/path/to/so" syntax.
>>>>>>>
>>>>>>> This commit fixes above bug, which also potentially avoids the case
>>>>>>> where "/path/to/so" within the given "external:/path/to/so" happens
>>>>>>> to match the existing name, though this is normally unlikely to happen.
>>>>>>
>>>>>> People have already started depending on the "wrong" behaviour e.g.
>>>>>> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
>>>>>> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
>>>>>> . Because they're external it's hard to find out just how prevalent
>>>>>> this behaviour is.
>>>>>
>>>>> Then they can add "external:".
>>>>> If it has ever been official (in docs/etc), then the program should
>>>>> maintain such behavior, like fio keeps kb_base=1024 by default.
>>>>>
>>>>> This isn't the first time fio broke external ioengines to begin with,
>>>>> and fio doesn't guarantee external ioengines.
>>>>> e.g. this commit more than a year ago broke ABI.
>>>>> 565e784df05c2529479eed8a38701a33b01894bd
>>>>>
>>>>> It's essentially the same as non mainline'd kernel modules are on
>>>>> their own to be uptodate with upstream.
>>>>
>>>> No, those two are VERY different. Fio doesn't make any guarantees wrt
>>>> keeping the same ABI (or API, even) for external engines. If they break,
>>>> then you get to keep the pieces and glue them back together. We have
>>>> the engine version number as well to guard against those kinds of
>>>> changes.
>>>>
>>>> But we don't break job files, at least not knowingly, and definitely
>>>> not just in the name of a cleanup.
>>>
>>> Yes, but you took this patch which breaks job files *wrongly* using
>>> external engine option syntax.
>>
>> Right, but it's not in a release yet, so it's not impossible to just
>> revert it.
>>
>>> By "job files wrongly using", I mean job files somehow found
>>> unofficial (i.e. undocumented, not intended) way to dlopen it.
>>
>> This case is a bit difficult, since it isn't black and white. I took a
>> look at some of the older documentation, and we have been documenting
>> the need for 'external' for a long time (forever?). So I'd be inclined
>> to let this one stay in.
>>
>>> If breaking this case is not okay, shouldn't this commit be reverted
>>> or the design be expand to allow both with/without prefix ?
>>
>> The design (or intent, at least) has always been that if you do:
>>
>> ioengine=something
>>
>> then we first try and load 'something' as an internal engine. If that
>> fails, we look for an external engine of that name. That makes sense to
>> me, since it means you don't have to do anything special to load an
>> external engine. I think we should try and retain that behavior, if we
>> can.
> 
> OK, thank you for the explanation.
> I don't mind reverting this at all, since now that "external:" prefix
> works as documented (after [PATCH 2/5]).

Would you mind doing a revert patch with an explanation? Since there are
changes on top, it doesn't revert cleanly.

-- 
Jens Axboe

Re: [PATCH 4/5] fix load_ioengine() not to support no "external:" prefix

[Tomohiro Kusumi]

2017-09-01 18:21 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
> On 09/01/2017 09:18 AM, Tomohiro Kusumi wrote:
>> 2017-09-01 18:08 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
>>> On 09/01/2017 09:03 AM, Tomohiro Kusumi wrote:
>>>> 2017-09-01 17:43 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
>>>>> On 08/31/2017 11:38 PM, Tomohiro Kusumi wrote:
>>>>>> 2017-09-01 7:52 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>>>>>> On 31 August 2017 at 21:13,  <kusumi.tomohiro@gmail.com> wrote:
>>>>>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>>>>>
>>>>>>>> To load ioengines, it should be done by either
>>>>>>>>  1) find the ioengine from the existing (static linked) ioengines or
>>>>>>>>  2) dlopen the path in case of external ioengine,
>>>>>>>>
>>>>>>>> but not to do 2 if 1 failed, as fio doesn't expect an ioengine to be
>>>>>>>> dynamically loaded unless with "external:" prefix by design.
>>>>>>>>
>>>>>>>> The current implementation (i.e. do 2 if 1 failed) happened to have
>>>>>>>> been able to load an external ioengine with below syntax without
>>>>>>>> "external:" prefix, but this is a bug rather than a feature.
>>>>>>>>
>>>>>>>> (--)ioengine=./engines/skeleton_external.so
>>>>>>>>
>>>>>>>> The design of the external ioengine option since below commits in 2007
>>>>>>>>  7b395ca5('Prefix external io engine loading with 'external'')
>>>>>>>>  8a7bd877('Document loading external io engines')
>>>>>>>> is to use "external:/path/to/so" syntax.
>>>>>>>>
>>>>>>>> This commit fixes above bug, which also potentially avoids the case
>>>>>>>> where "/path/to/so" within the given "external:/path/to/so" happens
>>>>>>>> to match the existing name, though this is normally unlikely to happen.
>>>>>>>
>>>>>>> People have already started depending on the "wrong" behaviour e.g.
>>>>>>> ioengine=libfio_ceph_objectstore.so # must be found in your LD_LIBRARY_PATH
>>>>>>> from https://github.com/ceph/ceph/blob/master/src/test/fio/ceph-bluestore.fio
>>>>>>> . Because they're external it's hard to find out just how prevalent
>>>>>>> this behaviour is.
>>>>>>
>>>>>> Then they can add "external:".
>>>>>> If it has ever been official (in docs/etc), then the program should
>>>>>> maintain such behavior, like fio keeps kb_base=1024 by default.
>>>>>>
>>>>>> This isn't the first time fio broke external ioengines to begin with,
>>>>>> and fio doesn't guarantee external ioengines.
>>>>>> e.g. this commit more than a year ago broke ABI.
>>>>>> 565e784df05c2529479eed8a38701a33b01894bd
>>>>>>
>>>>>> It's essentially the same as non mainline'd kernel modules are on
>>>>>> their own to be uptodate with upstream.
>>>>>
>>>>> No, those two are VERY different. Fio doesn't make any guarantees wrt
>>>>> keeping the same ABI (or API, even) for external engines. If they break,
>>>>> then you get to keep the pieces and glue them back together. We have
>>>>> the engine version number as well to guard against those kinds of
>>>>> changes.
>>>>>
>>>>> But we don't break job files, at least not knowingly, and definitely
>>>>> not just in the name of a cleanup.
>>>>
>>>> Yes, but you took this patch which breaks job files *wrongly* using
>>>> external engine option syntax.
>>>
>>> Right, but it's not in a release yet, so it's not impossible to just
>>> revert it.
>>>
>>>> By "job files wrongly using", I mean job files somehow found
>>>> unofficial (i.e. undocumented, not intended) way to dlopen it.
>>>
>>> This case is a bit difficult, since it isn't black and white. I took a
>>> look at some of the older documentation, and we have been documenting
>>> the need for 'external' for a long time (forever?). So I'd be inclined
>>> to let this one stay in.
>>>
>>>> If breaking this case is not okay, shouldn't this commit be reverted
>>>> or the design be expand to allow both with/without prefix ?
>>>
>>> The design (or intent, at least) has always been that if you do:
>>>
>>> ioengine=something
>>>
>>> then we first try and load 'something' as an internal engine. If that
>>> fails, we look for an external engine of that name. That makes sense to
>>> me, since it means you don't have to do anything special to load an
>>> external engine. I think we should try and retain that behavior, if we
>>> can.
>>
>> OK, thank you for the explanation.
>> I don't mind reverting this at all, since now that "external:" prefix
>> works as documented (after [PATCH 2/5]).
>
> Would you mind doing a revert patch with an explanation? Since there are
> changes on top, it doesn't revert cleanly.

Sure. I'll send it later.

> --
> Jens Axboe
>