[PATCH 1/7] Replace redundant TD_F_NOIO flag with td->io_ops_init

[PATCH 1/7] Replace redundant TD_F_NOIO flag with td->io_ops_init

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

The only reason TD_F_NOIO exists in addition to FIO_NOIO is because
fio_running_or_pending_io_threads() needs to know whether td has
successfully initialized ioengine, whereas FIO_NOIO is statically
set regardless of ioengines's ->init() result.

This commit adds a new td field ->io_ops_init to inidicate ->init()
result, so that td needs no extra bit field for each ioengine like
TD_F_NOIO. It was rather odd that td was unable to tell ->init()
result afterward when ->init() failure (returning non zero) doesn't
mean aborting fio itself. This commit also changes TD_F_NOIO to
TD_F_RESERVED as it's no longer used.

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

diff --git a/fio.h b/fio.h
index b573ac5..581512f 100644
--- a/fio.h
+++ b/fio.h
@@ -74,7 +74,7 @@ enum {
 	TD_F_VER_NONE		= 1U << 5,
 	TD_F_PROFILE_OPS	= 1U << 6,
 	TD_F_COMPRESS		= 1U << 7,
-	TD_F_NOIO		= 1U << 8,
+	TD_F_RESERVED		= 1U << 8, /* not used */
 	TD_F_COMPRESS_LOG	= 1U << 9,
 	TD_F_VSTATE_SAVED	= 1U << 10,
 	TD_F_NEED_LOCK		= 1U << 11,
@@ -231,6 +231,7 @@ struct thread_data {
 	 * to any of the available IO engines.
 	 */
 	struct ioengine_ops *io_ops;
+	int io_ops_init;
 
 	/*
 	 * IO engine private data and dlhandle.
diff --git a/init.c b/init.c
index b4b0974..4a72255 100644
--- a/init.c
+++ b/init.c
@@ -459,6 +459,7 @@ static struct thread_data *get_new_job(bool global, struct thread_data *parent,
 		copy_opt_list(td, parent);
 
 	td->io_ops = NULL;
+	td->io_ops_init = 0;
 	if (!preserve_eo)
 		td->eo = NULL;
 
diff --git a/ioengines.c b/ioengines.c
index 95013d1..c773f2e 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -368,17 +368,17 @@ int td_io_init(struct thread_data *td)
 
 	if (td->io_ops->init) {
 		ret = td->io_ops->init(td);
-		if (ret && td->o.iodepth > 1) {
-			log_err("fio: io engine init failed. Perhaps try"
-				" reducing io depth?\n");
-		}
+		if (ret)
+			log_err("fio: io engine %s init failed.%s\n",
+				td->io_ops->name,
+				td->o.iodepth > 1 ?
+				" Perhaps try reducing io depth?" : "");
+		else
+			td->io_ops_init = 1;
 		if (!td->error)
 			td->error = ret;
 	}
 
-	if (!ret && td_ioengine_flagged(td, FIO_NOIO))
-		td->flags |= TD_F_NOIO;
-
 	return ret;
 }
 
diff --git a/libfio.c b/libfio.c
index 4b53c92..8310708 100644
--- a/libfio.c
+++ b/libfio.c
@@ -276,7 +276,7 @@ int fio_running_or_pending_io_threads(void)
 	int nr_io_threads = 0;
 
 	for_each_td(td, i) {
-		if (td->flags & TD_F_NOIO)
+		if (td->io_ops_init && td_ioengine_flagged(td, FIO_NOIO))
 			continue;
 		nr_io_threads++;
 		if (td->runstate < TD_EXITED)
-- 
2.9.3

[PATCH 2/7] Define struct sk_out in server.h (not server.c)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

struct sk_out isn't local to server.c, so it should be visible
in its header (instead of having struct sk_out; in fio.h).

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 fio.h    |  1 -
 server.c | 11 -----------
 server.h | 11 +++++++++++
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/fio.h b/fio.h
index 581512f..52a9b75 100644
--- a/fio.h
+++ b/fio.h
@@ -121,7 +121,6 @@ enum {
  * Per-thread/process specific data. Only used for the network client
  * for now.
  */
-struct sk_out;
 void sk_out_assign(struct sk_out *);
 void sk_out_drop(void);
 
diff --git a/server.c b/server.c
index 6d5d4ea..1b3bc30 100644
--- a/server.c
+++ b/server.c
@@ -50,17 +50,6 @@ struct sk_entry {
 	struct flist_head next;	/* Other sk_entry's, if linked command */
 };
 
-struct sk_out {
-	unsigned int refs;	/* frees sk_out when it drops to zero.
-				 * protected by below ->lock */
-
-	int sk;			/* socket fd to talk to client */
-	struct fio_mutex lock;	/* protects ref and below list */
-	struct flist_head list;	/* list of pending transmit work */
-	struct fio_mutex wait;	/* wake backend when items added to list */
-	struct fio_mutex xmit;	/* held while sending data */
-};
-
 static char *fio_server_arg;
 static char *bind_sock;
 static struct sockaddr_in saddr_in;
diff --git a/server.h b/server.h
index 3a1d0b0..798d5a8 100644
--- a/server.h
+++ b/server.h
@@ -12,6 +12,17 @@
 
 #define FIO_NET_PORT 8765
 
+struct sk_out {
+	unsigned int refs;	/* frees sk_out when it drops to zero.
+				 * protected by below ->lock */
+
+	int sk;			/* socket fd to talk to client */
+	struct fio_mutex lock;	/* protects ref and below list */
+	struct flist_head list;	/* list of pending transmit work */
+	struct fio_mutex wait;	/* wake backend when items added to list */
+	struct fio_mutex xmit;	/* held while sending data */
+};
+
 /*
  * On-wire encoding is little endian
  */
-- 
2.9.3

[PATCH 3/7] HOWTO: Mention cpuload= is mandatory for cpuio

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

fio jobs using cpuio fail without this option value specified,
so mention it's mandatory (or have default value for this ?).

fio: pid=9320, err=22/file:engines/cpu.c:75, func=cpuio, error=cpu thread needs rate (cpuload=)
Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 HOWTO | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/HOWTO b/HOWTO
index 5d378f3..1ce1b0c 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1748,7 +1748,8 @@ caveat that when used on the command line, they must come after the
 
 .. option:: cpuload=int : [cpuio]
 
-	Attempt to use the specified percentage of CPU cycles.
+	Attempt to use the specified percentage of CPU cycles. This is a mandatory
+	option when using cpuio I/O engine.
 
 .. option:: cpuchunks=int : [cpuio]
 
-- 
2.9.3

[PATCH 4/7] HOWTO: Mention fsync=/fsyncdata= are set to 0 by default

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think people could think sync ioengine and variants would somehow
fsync(2) for writes from what their name sound like, while they do
not unless these options are explicitly set (default 0).

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 HOWTO | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/HOWTO b/HOWTO
index 1ce1b0c..cae95b7 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1100,13 +1100,15 @@ I/O type
 	blocks given. For example, if you give 32 as a parameter, fio will sync the
 	file for every 32 writes issued. If fio is using non-buffered I/O, we may
 	not sync the file. The exception is the sg I/O engine, which synchronizes
-	the disk cache anyway.
+	the disk cache anyway. Defaults to 0, which means no sync every certain
+	number of writes.
 
 .. option:: fdatasync=int
 
 	Like :option:`fsync` but uses :manpage:`fdatasync(2)` to only sync data and
 	not metadata blocks.  In Windows, FreeBSD, and DragonFlyBSD there is no
 	:manpage:`fdatasync(2)`, this falls back to using :manpage:`fsync(2)`.
+	Defaults to 0, which means no sync data every certain number of writes.
 
 .. option:: write_barrier=int
 
@@ -1571,6 +1573,7 @@ I/O engine
 		**sync**
 			Basic :manpage:`read(2)` or :manpage:`write(2)`
 			I/O. :manpage:`lseek(2)` is used to position the I/O location.
+			See :option:`fsync` and :option:`fdatasync` for syncing write I/Os.
 
 		**psync**
 			Basic :manpage:`pread(2)` or :manpage:`pwrite(2)` I/O.  Default on
-- 
2.9.3

[PATCH 5/7] Fix a comment after f227e2b6

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

Add "for regular files" since it's true only on regfiles.
Also see below.
http://www.spinics.net/lists/fio/msg05646.html

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 filesetup.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/filesetup.c b/filesetup.c
index c9f2b5f..bcf95bd 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -932,10 +932,11 @@ int setup_files(struct thread_data *td)
 			}
 
 			/*
-			 * We normally don't come here, but if the result is 0,
-			 * set it to the real file size. This could be size of
-			 * the existing one if it already exists, but otherwise
-			 * will be set to 0. A new file won't be created because
+			 * We normally don't come here for regular files, but
+			 * if the result is 0 for a regular file, set it to the
+			 * real file size. This could be size of the existing
+			 * one if it already exists, but otherwise will be set
+			 * to 0. A new file won't be created because
 			 * ->io_size + ->file_offset equals ->real_file_size.
 			 */
 			if (!f->io_size) {
-- 
2.9.3

[PATCH 6/7] Test uint,int before division uint/int for the next i/o

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

When selecting ddir for the next i/o, test td->io_issues[DDIR_WRITE]
before division which is more expensive.

if (x && y && !(y % x))
rather than
if (x && !(y % x) && y)

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

diff --git a/io_u.c b/io_u.c
index 5f01c1b..1de35c8 100644
--- a/io_u.c
+++ b/io_u.c
@@ -719,25 +719,25 @@ static enum fio_ddir get_rw_ddir(struct thread_data *td)
 	/*
 	 * see if it's time to fsync
 	 */
-	if (td->o.fsync_blocks &&
-	   !(td->io_issues[DDIR_WRITE] % td->o.fsync_blocks) &&
-	     td->io_issues[DDIR_WRITE] && should_fsync(td))
+	if (td->o.fsync_blocks && td->io_issues[DDIR_WRITE] &&
+	    !(td->io_issues[DDIR_WRITE] % td->o.fsync_blocks) &&
+	    should_fsync(td))
 		return DDIR_SYNC;
 
 	/*
 	 * see if it's time to fdatasync
 	 */
-	if (td->o.fdatasync_blocks &&
-	   !(td->io_issues[DDIR_WRITE] % td->o.fdatasync_blocks) &&
-	     td->io_issues[DDIR_WRITE] && should_fsync(td))
+	if (td->o.fdatasync_blocks && td->io_issues[DDIR_WRITE] &&
+	    !(td->io_issues[DDIR_WRITE] % td->o.fdatasync_blocks) &&
+	    should_fsync(td))
 		return DDIR_DATASYNC;
 
 	/*
 	 * see if it's time to sync_file_range
 	 */
-	if (td->sync_file_range_nr &&
-	   !(td->io_issues[DDIR_WRITE] % td->sync_file_range_nr) &&
-	     td->io_issues[DDIR_WRITE] && should_fsync(td))
+	if (td->sync_file_range_nr && td->io_issues[DDIR_WRITE] &&
+	    !(td->io_issues[DDIR_WRITE] % td->sync_file_range_nr) &&
+	    should_fsync(td))
 		return DDIR_SYNC_FILE_RANGE;
 
 	if (td_rw(td)) {
-- 
2.9.3

[PATCH 7/7] Test fsync/fdatasync/sync_file_range for the next i/o only if should_fsync(td)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

Checking an inline function should_fsync() first is better since
should_fsync() itself contains td_write() as a condition to return true,
and there's no need for non-write td to go through the rest of checks
when they result in false due to td->io_issues[DDIR_WRITE] being 0.

(This commit directly goes on top of the previous one)

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

diff --git a/io_u.c b/io_u.c
index 1de35c8..c6d814b 100644
--- a/io_u.c
+++ b/io_u.c
@@ -717,28 +717,22 @@ static enum fio_ddir get_rw_ddir(struct thread_data *td)
 	enum fio_ddir ddir;
 
 	/*
-	 * see if it's time to fsync
+	 * See if it's time to fsync/fdatasync/sync_file_range first,
+	 * and if not then move on to check regular I/Os.
 	 */
-	if (td->o.fsync_blocks && td->io_issues[DDIR_WRITE] &&
-	    !(td->io_issues[DDIR_WRITE] % td->o.fsync_blocks) &&
-	    should_fsync(td))
-		return DDIR_SYNC;
-
-	/*
-	 * see if it's time to fdatasync
-	 */
-	if (td->o.fdatasync_blocks && td->io_issues[DDIR_WRITE] &&
-	    !(td->io_issues[DDIR_WRITE] % td->o.fdatasync_blocks) &&
-	    should_fsync(td))
-		return DDIR_DATASYNC;
-
-	/*
-	 * see if it's time to sync_file_range
-	 */
-	if (td->sync_file_range_nr && td->io_issues[DDIR_WRITE] &&
-	    !(td->io_issues[DDIR_WRITE] % td->sync_file_range_nr) &&
-	    should_fsync(td))
-		return DDIR_SYNC_FILE_RANGE;
+	if (should_fsync(td)) {
+		if (td->o.fsync_blocks && td->io_issues[DDIR_WRITE] &&
+		    !(td->io_issues[DDIR_WRITE] % td->o.fsync_blocks))
+			return DDIR_SYNC;
+
+		if (td->o.fdatasync_blocks && td->io_issues[DDIR_WRITE] &&
+		    !(td->io_issues[DDIR_WRITE] % td->o.fdatasync_blocks))
+			return DDIR_DATASYNC;
+
+		if (td->sync_file_range_nr && td->io_issues[DDIR_WRITE] &&
+		    !(td->io_issues[DDIR_WRITE] % td->sync_file_range_nr))
+			return DDIR_SYNC_FILE_RANGE;
+	}
 
 	if (td_rw(td)) {
 		/*
-- 
2.9.3

Re: [PATCH 1/7] Replace redundant TD_F_NOIO flag with td->io_ops_init

[Jens Axboe]

On Mon, Mar 20 2017, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> The only reason TD_F_NOIO exists in addition to FIO_NOIO is because
> fio_running_or_pending_io_threads() needs to know whether td has
> successfully initialized ioengine, whereas FIO_NOIO is statically
> set regardless of ioengines's ->init() result.
> 
> This commit adds a new td field ->io_ops_init to inidicate ->init()
> result, so that td needs no extra bit field for each ioengine like
> TD_F_NOIO. It was rather odd that td was unable to tell ->init()
> result afterward when ->init() failure (returning non zero) doesn't
> mean aborting fio itself. This commit also changes TD_F_NOIO to
> TD_F_RESERVED as it's no longer used.

Thanks, applied 1-7.

-- 
Jens Axboe