[PATCH] options: Add thinktime_iotime option

[PATCH] options: Add thinktime_iotime option

[Shin'ichiro Kawasaki]

The thinktime option allows stalling a job for a specified amount of
time. Using the thinktime_blocks option, periodic stalls can be added
every thinktime_blocks IOs. However, with this option, the periodic
stall may not be repeated at equal time intervals as the time to execute
thinktime_blocks IOs may vary.

To control the thinktime interval by time, introduce the option
thinktime_iotime. With this new option, the thinktime stall is repeated
after IOs are executed for thinktime_iotime. If this option is used
together with the thinktime_blocks option, the thinktime pause is
repeated after thinktime_iotime or after thinktime_blocks IOs, whichever
happens first.

To track the time and IO block count at the last stall, add
last_thinktime variable and last_thinktime_blocks variable to struct
thread_data. Also, introduce the helper function init_thinktime()
to group thinktime related preparations.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 HOWTO            |  9 ++++++++-
 backend.c        | 45 +++++++++++++++++++++++++++++++++++++++------
 cconv.c          |  2 ++
 fio.1            |  7 ++++++-
 fio.h            |  2 ++
 options.c        | 14 ++++++++++++++
 server.h         |  2 +-
 thread_options.h |  3 +++
 8 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/HOWTO b/HOWTO
index 9bfd38b4..cfb278ef 100644
--- a/HOWTO
+++ b/HOWTO
@@ -2710,7 +2710,7 @@ I/O rate
 	Stall the job for the specified period of time after an I/O has completed before issuing the
 	next. May be used to simulate processing being done by an application.
 	When the unit is omitted, the value is interpreted in microseconds.  See
-	:option:`thinktime_blocks` and :option:`thinktime_spin`.
+	:option:`thinktime_blocks`, :option:`thinktime_iotime` and :option:`thinktime_spin`.
 
 .. option:: thinktime_spin=time
 
@@ -2735,6 +2735,13 @@ I/O rate
 	:option:`thinktime_blocks` blocks. If this is set to `issue`, then the trigger happens
 	at the issue side.
 
+.. option:: thinktime_iotime=time
+
+	Only valid if :option:`thinktime` is set - Repeat job stall for
+	:option:`thinktime` after executing I/Os for `thinktime_iotime`. When
+	the unit is omitted, :option:`thinktime_iotime` is interpreted as a
+	number of seconds.
+
 .. option:: rate=int[,int][,int]
 
 	Cap the bandwidth used by this job. The number is in bytes/sec, the normal
diff --git a/backend.c b/backend.c
index 808e4362..5cdb8124 100644
--- a/backend.c
+++ b/backend.c
@@ -858,15 +858,47 @@ static long long usec_for_io(struct thread_data *td, enum fio_ddir ddir)
 	return 0;
 }
 
+static void init_thinktime(struct thread_data *td)
+{
+	if (td->o.thinktime_blocks_type == THINKTIME_BLOCKS_TYPE_COMPLETE)
+		td->thinktime_blocks_counter = td->io_blocks;
+	else
+		td->thinktime_blocks_counter = td->io_issues;
+	td->last_thinktime = td->epoch;
+	td->last_thinktime_blocks = 0;
+}
+
 static void handle_thinktime(struct thread_data *td, enum fio_ddir ddir,
 			     struct timespec *time)
 {
 	unsigned long long b;
 	uint64_t total;
 	int left;
+	struct timespec now;
+	bool stall = false;
+
+	if (td->o.thinktime_iotime) {
+		fio_gettime(&now, NULL);
+		if (utime_since(&td->last_thinktime, &now)
+		    >= td->o.thinktime_iotime + td->o.thinktime) {
+			stall = true;
+		} else if (!fio_option_is_set(&td->o, thinktime_blocks)) {
+			/*
+			 * When thinktime_iotime is set and thinktime_blocks is
+			 * not set, skip the thinktime_blocks check, since
+			 * thinktime_blocks default value 1 does not work
+			 * together with thinktime_iotime.
+			 */
+			return;
+		}
+
+	}
 
 	b = ddir_rw_sum(td->thinktime_blocks_counter);
-	if (b % td->o.thinktime_blocks || !b)
+	if (b >= td->last_thinktime_blocks + td->o.thinktime_blocks)
+		stall = true;
+
+	if (!stall)
 		return;
 
 	io_u_quiesce(td);
@@ -902,6 +934,10 @@ static void handle_thinktime(struct thread_data *td, enum fio_ddir ddir,
 
 	if (time && should_check_rate(td))
 		fio_gettime(time, NULL);
+
+	td->last_thinktime_blocks = b;
+	if (td->o.thinktime_iotime)
+		td->last_thinktime = now;
 }
 
 /*
@@ -1791,17 +1827,14 @@ static void *thread_main(void *data)
 	if (rate_submit_init(td, sk_out))
 		goto err;
 
-	if (td->o.thinktime_blocks_type == THINKTIME_BLOCKS_TYPE_COMPLETE)
-		td->thinktime_blocks_counter = td->io_blocks;
-	else
-		td->thinktime_blocks_counter = td->io_issues;
-
 	set_epoch_time(td, o->log_unix_epoch);
 	fio_getrusage(&td->ru_start);
 	memcpy(&td->bw_sample_time, &td->epoch, sizeof(td->epoch));
 	memcpy(&td->iops_sample_time, &td->epoch, sizeof(td->epoch));
 	memcpy(&td->ss.prev_time, &td->epoch, sizeof(td->epoch));
 
+	init_thinktime(td);
+
 	if (o->ratemin[DDIR_READ] || o->ratemin[DDIR_WRITE] ||
 			o->ratemin[DDIR_TRIM]) {
 	        memcpy(&td->lastrate[DDIR_READ], &td->bw_sample_time,
diff --git a/cconv.c b/cconv.c
index e3a8c27c..92491ddb 100644
--- a/cconv.c
+++ b/cconv.c
@@ -213,6 +213,7 @@ void convert_thread_options_to_cpu(struct thread_options *o,
 	o->thinktime_spin = le32_to_cpu(top->thinktime_spin);
 	o->thinktime_blocks = le32_to_cpu(top->thinktime_blocks);
 	o->thinktime_blocks_type = le32_to_cpu(top->thinktime_blocks_type);
+	o->thinktime_iotime = le32_to_cpu(top->thinktime_iotime);
 	o->fsync_blocks = le32_to_cpu(top->fsync_blocks);
 	o->fdatasync_blocks = le32_to_cpu(top->fdatasync_blocks);
 	o->barrier_blocks = le32_to_cpu(top->barrier_blocks);
@@ -438,6 +439,7 @@ void convert_thread_options_to_net(struct thread_options_pack *top,
 	top->thinktime_spin = cpu_to_le32(o->thinktime_spin);
 	top->thinktime_blocks = cpu_to_le32(o->thinktime_blocks);
 	top->thinktime_blocks_type = __cpu_to_le32(o->thinktime_blocks_type);
+	top->thinktime_iotime = __cpu_to_le32(o->thinktime_iotime);
 	top->fsync_blocks = cpu_to_le32(o->fsync_blocks);
 	top->fdatasync_blocks = cpu_to_le32(o->fdatasync_blocks);
 	top->barrier_blocks = cpu_to_le32(o->barrier_blocks);
diff --git a/fio.1 b/fio.1
index 382cebfc..3cae15b3 100644
--- a/fio.1
+++ b/fio.1
@@ -2471,7 +2471,7 @@ problem). Note that this option cannot reliably be used with async IO engines.
 Stall the job for the specified period of time after an I/O has completed before issuing the
 next. May be used to simulate processing being done by an application.
 When the unit is omitted, the value is interpreted in microseconds. See
-\fBthinktime_blocks\fR and \fBthinktime_spin\fR.
+\fBthinktime_blocks\fR, \fBthinktime_iotime\fR and \fBthinktime_spin\fR.
 .TP
 .BI thinktime_spin \fR=\fPtime
 Only valid if \fBthinktime\fR is set - pretend to spend CPU time doing
@@ -2493,6 +2493,11 @@ The default is `complete', which triggers \fBthinktime\fR when fio completes
 \fBthinktime_blocks\fR blocks. If this is set to `issue', then the trigger happens
 at the issue side.
 .TP
+.BI thinktime_iotime \fR=\fPtime
+Only valid if \fBthinktime\fR is set - Repeat job stall for \fBthinktime\fR
+after executing I/Os for \fBthinktime_iotime\fR. When the unit is omitted,
+\fBthinktime_iotime\fR is interpreted as a number of seconds.
+.TP
 .BI rate \fR=\fPint[,int][,int]
 Cap the bandwidth used by this job. The number is in bytes/sec, the normal
 suffix rules apply. Comma-separated values may be specified for reads,
diff --git a/fio.h b/fio.h
index 6f6b211b..85ed60fc 100644
--- a/fio.h
+++ b/fio.h
@@ -365,6 +365,8 @@ struct thread_data {
 	uint64_t bytes_done[DDIR_RWDIR_CNT];
 
 	uint64_t *thinktime_blocks_counter;
+	struct timespec last_thinktime;
+	uint64_t last_thinktime_blocks;
 
 	/*
 	 * State for random io, a bitmap of blocks done vs not done
diff --git a/options.c b/options.c
index 8c2ab7cc..3fe1e6bd 100644
--- a/options.c
+++ b/options.c
@@ -3688,6 +3688,20 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		},
 		.parent = "thinktime",
 	},
+	{
+		.name	= "thinktime_iotime",
+		.lname	= "Thinktime interval",
+		.type	= FIO_OPT_INT,
+		.off1	= offsetof(struct thread_options, thinktime_iotime),
+		.help	= "IO time interval between 'thinktime'",
+		.is_time = 1,
+		.is_seconds = 1,
+		.def	= "0",
+		.parent	= "thinktime",
+		.hide	= 1,
+		.category = FIO_OPT_C_IO,
+		.group	= FIO_OPT_G_THINKTIME,
+	},
 	{
 		.name	= "rate",
 		.lname	= "I/O rate",
diff --git a/server.h b/server.h
index daed057a..7ce2ddb1 100644
--- a/server.h
+++ b/server.h
@@ -48,7 +48,7 @@ struct fio_net_cmd_reply {
 };
 
 enum {
-	FIO_SERVER_VER			= 92,
+	FIO_SERVER_VER			= 93,
 
 	FIO_SERVER_MAX_FRAGMENT_PDU	= 1024,
 	FIO_SERVER_MAX_CMD_MB		= 2048,
diff --git a/thread_options.h b/thread_options.h
index 4b4ecfe1..6fe1cad7 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -189,6 +189,7 @@ struct thread_options {
 	unsigned int thinktime_spin;
 	unsigned int thinktime_blocks;
 	unsigned int thinktime_blocks_type;
+	unsigned int thinktime_iotime;
 	unsigned int fsync_blocks;
 	unsigned int fdatasync_blocks;
 	unsigned int barrier_blocks;
@@ -500,6 +501,8 @@ struct thread_options_pack {
 	uint32_t thinktime_spin;
 	uint32_t thinktime_blocks;
 	uint32_t thinktime_blocks_type;
+	uint32_t thinktime_iotime;
+	uint32_t pad6;
 	uint32_t fsync_blocks;
 	uint32_t fdatasync_blocks;
 	uint32_t barrier_blocks;
-- 
2.31.1

Re: [PATCH] options: Add thinktime_iotime option

[Damien Le Moal]

On 2021/08/20 20:25, Shin'ichiro Kawasaki wrote:
> The thinktime option allows stalling a job for a specified amount of
> time. Using the thinktime_blocks option, periodic stalls can be added
> every thinktime_blocks IOs. However, with this option, the periodic
> stall may not be repeated at equal time intervals as the time to execute
> thinktime_blocks IOs may vary.
> 
> To control the thinktime interval by time, introduce the option
> thinktime_iotime. With this new option, the thinktime stall is repeated
> after IOs are executed for thinktime_iotime. If this option is used
> together with the thinktime_blocks option, the thinktime pause is
> repeated after thinktime_iotime or after thinktime_blocks IOs, whichever
> happens first.
> 
> To track the time and IO block count at the last stall, add
> last_thinktime variable and last_thinktime_blocks variable to struct
> thread_data. Also, introduce the helper function init_thinktime()
> to group thinktime related preparations.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  HOWTO            |  9 ++++++++-
>  backend.c        | 45 +++++++++++++++++++++++++++++++++++++++------
>  cconv.c          |  2 ++
>  fio.1            |  7 ++++++-
>  fio.h            |  2 ++
>  options.c        | 14 ++++++++++++++
>  server.h         |  2 +-
>  thread_options.h |  3 +++
>  8 files changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/HOWTO b/HOWTO
> index 9bfd38b4..cfb278ef 100644
> --- a/HOWTO
> +++ b/HOWTO
> @@ -2710,7 +2710,7 @@ I/O rate
>  	Stall the job for the specified period of time after an I/O has completed before issuing the
>  	next. May be used to simulate processing being done by an application.
>  	When the unit is omitted, the value is interpreted in microseconds.  See
> -	:option:`thinktime_blocks` and :option:`thinktime_spin`.
> +	:option:`thinktime_blocks`, :option:`thinktime_iotime` and :option:`thinktime_spin`.
>  
>  .. option:: thinktime_spin=time
>  
> @@ -2735,6 +2735,13 @@ I/O rate
>  	:option:`thinktime_blocks` blocks. If this is set to `issue`, then the trigger happens
>  	at the issue side.
>  
> +.. option:: thinktime_iotime=time
> +
> +	Only valid if :option:`thinktime` is set - Repeat job stall for
> +	:option:`thinktime` after executing I/Os for `thinktime_iotime`. When
> +	the unit is omitted, :option:`thinktime_iotime` is interpreted as a
> +	number of seconds.
> +
>  .. option:: rate=int[,int][,int]
>  
>  	Cap the bandwidth used by this job. The number is in bytes/sec, the normal
> diff --git a/backend.c b/backend.c
> index 808e4362..5cdb8124 100644
> --- a/backend.c
> +++ b/backend.c
> @@ -858,15 +858,47 @@ static long long usec_for_io(struct thread_data *td, enum fio_ddir ddir)
>  	return 0;
>  }
>  
> +static void init_thinktime(struct thread_data *td)
> +{
> +	if (td->o.thinktime_blocks_type == THINKTIME_BLOCKS_TYPE_COMPLETE)
> +		td->thinktime_blocks_counter = td->io_blocks;
> +	else
> +		td->thinktime_blocks_counter = td->io_issues;
> +	td->last_thinktime = td->epoch;
> +	td->last_thinktime_blocks = 0;
> +}
> +
>  static void handle_thinktime(struct thread_data *td, enum fio_ddir ddir,
>  			     struct timespec *time)
>  {
>  	unsigned long long b;
>  	uint64_t total;
>  	int left;
> +	struct timespec now;
> +	bool stall = false;
> +
> +	if (td->o.thinktime_iotime) {
> +		fio_gettime(&now, NULL);
> +		if (utime_since(&td->last_thinktime, &now)
> +		    >= td->o.thinktime_iotime + td->o.thinktime) {
> +			stall = true;
> +		} else if (!fio_option_is_set(&td->o, thinktime_blocks)) {
> +			/*
> +			 * When thinktime_iotime is set and thinktime_blocks is
> +			 * not set, skip the thinktime_blocks check, since
> +			 * thinktime_blocks default value 1 does not work
> +			 * together with thinktime_iotime.
> +			 */
> +			return;
> +		}
> +
> +	}
>  
>  	b = ddir_rw_sum(td->thinktime_blocks_counter);
> -	if (b % td->o.thinktime_blocks || !b)
> +	if (b >= td->last_thinktime_blocks + td->o.thinktime_blocks)
> +		stall = true;
> +
> +	if (!stall)
>  		return;
>  
>  	io_u_quiesce(td);
> @@ -902,6 +934,10 @@ static void handle_thinktime(struct thread_data *td, enum fio_ddir ddir,
>  
>  	if (time && should_check_rate(td))
>  		fio_gettime(time, NULL);
> +
> +	td->last_thinktime_blocks = b;
> +	if (td->o.thinktime_iotime)
> +		td->last_thinktime = now;
>  }
>  
>  /*
> @@ -1791,17 +1827,14 @@ static void *thread_main(void *data)
>  	if (rate_submit_init(td, sk_out))
>  		goto err;
>  
> -	if (td->o.thinktime_blocks_type == THINKTIME_BLOCKS_TYPE_COMPLETE)
> -		td->thinktime_blocks_counter = td->io_blocks;
> -	else
> -		td->thinktime_blocks_counter = td->io_issues;
> -
>  	set_epoch_time(td, o->log_unix_epoch);
>  	fio_getrusage(&td->ru_start);
>  	memcpy(&td->bw_sample_time, &td->epoch, sizeof(td->epoch));
>  	memcpy(&td->iops_sample_time, &td->epoch, sizeof(td->epoch));
>  	memcpy(&td->ss.prev_time, &td->epoch, sizeof(td->epoch));
>  
> +	init_thinktime(td);
> +
>  	if (o->ratemin[DDIR_READ] || o->ratemin[DDIR_WRITE] ||
>  			o->ratemin[DDIR_TRIM]) {
>  	        memcpy(&td->lastrate[DDIR_READ], &td->bw_sample_time,
> diff --git a/cconv.c b/cconv.c
> index e3a8c27c..92491ddb 100644
> --- a/cconv.c
> +++ b/cconv.c
> @@ -213,6 +213,7 @@ void convert_thread_options_to_cpu(struct thread_options *o,
>  	o->thinktime_spin = le32_to_cpu(top->thinktime_spin);
>  	o->thinktime_blocks = le32_to_cpu(top->thinktime_blocks);
>  	o->thinktime_blocks_type = le32_to_cpu(top->thinktime_blocks_type);
> +	o->thinktime_iotime = le32_to_cpu(top->thinktime_iotime);
>  	o->fsync_blocks = le32_to_cpu(top->fsync_blocks);
>  	o->fdatasync_blocks = le32_to_cpu(top->fdatasync_blocks);
>  	o->barrier_blocks = le32_to_cpu(top->barrier_blocks);
> @@ -438,6 +439,7 @@ void convert_thread_options_to_net(struct thread_options_pack *top,
>  	top->thinktime_spin = cpu_to_le32(o->thinktime_spin);
>  	top->thinktime_blocks = cpu_to_le32(o->thinktime_blocks);
>  	top->thinktime_blocks_type = __cpu_to_le32(o->thinktime_blocks_type);
> +	top->thinktime_iotime = __cpu_to_le32(o->thinktime_iotime);
>  	top->fsync_blocks = cpu_to_le32(o->fsync_blocks);
>  	top->fdatasync_blocks = cpu_to_le32(o->fdatasync_blocks);
>  	top->barrier_blocks = cpu_to_le32(o->barrier_blocks);
> diff --git a/fio.1 b/fio.1
> index 382cebfc..3cae15b3 100644
> --- a/fio.1
> +++ b/fio.1
> @@ -2471,7 +2471,7 @@ problem). Note that this option cannot reliably be used with async IO engines.
>  Stall the job for the specified period of time after an I/O has completed before issuing the
>  next. May be used to simulate processing being done by an application.
>  When the unit is omitted, the value is interpreted in microseconds. See
> -\fBthinktime_blocks\fR and \fBthinktime_spin\fR.
> +\fBthinktime_blocks\fR, \fBthinktime_iotime\fR and \fBthinktime_spin\fR.
>  .TP
>  .BI thinktime_spin \fR=\fPtime
>  Only valid if \fBthinktime\fR is set - pretend to spend CPU time doing
> @@ -2493,6 +2493,11 @@ The default is `complete', which triggers \fBthinktime\fR when fio completes
>  \fBthinktime_blocks\fR blocks. If this is set to `issue', then the trigger happens
>  at the issue side.
>  .TP
> +.BI thinktime_iotime \fR=\fPtime
> +Only valid if \fBthinktime\fR is set - Repeat job stall for \fBthinktime\fR
> +after executing I/Os for \fBthinktime_iotime\fR. When the unit is omitted,
> +\fBthinktime_iotime\fR is interpreted as a number of seconds.
> +.TP
>  .BI rate \fR=\fPint[,int][,int]
>  Cap the bandwidth used by this job. The number is in bytes/sec, the normal
>  suffix rules apply. Comma-separated values may be specified for reads,
> diff --git a/fio.h b/fio.h
> index 6f6b211b..85ed60fc 100644
> --- a/fio.h
> +++ b/fio.h
> @@ -365,6 +365,8 @@ struct thread_data {
>  	uint64_t bytes_done[DDIR_RWDIR_CNT];
>  
>  	uint64_t *thinktime_blocks_counter;
> +	struct timespec last_thinktime;
> +	uint64_t last_thinktime_blocks;
>  
>  	/*
>  	 * State for random io, a bitmap of blocks done vs not done
> diff --git a/options.c b/options.c
> index 8c2ab7cc..3fe1e6bd 100644
> --- a/options.c
> +++ b/options.c
> @@ -3688,6 +3688,20 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
>  		},
>  		.parent = "thinktime",
>  	},
> +	{
> +		.name	= "thinktime_iotime",
> +		.lname	= "Thinktime interval",
> +		.type	= FIO_OPT_INT,
> +		.off1	= offsetof(struct thread_options, thinktime_iotime),
> +		.help	= "IO time interval between 'thinktime'",
> +		.is_time = 1,
> +		.is_seconds = 1,
> +		.def	= "0",
> +		.parent	= "thinktime",
> +		.hide	= 1,
> +		.category = FIO_OPT_C_IO,
> +		.group	= FIO_OPT_G_THINKTIME,
> +	},
>  	{
>  		.name	= "rate",
>  		.lname	= "I/O rate",
> diff --git a/server.h b/server.h
> index daed057a..7ce2ddb1 100644
> --- a/server.h
> +++ b/server.h
> @@ -48,7 +48,7 @@ struct fio_net_cmd_reply {
>  };
>  
>  enum {
> -	FIO_SERVER_VER			= 92,
> +	FIO_SERVER_VER			= 93,
>  
>  	FIO_SERVER_MAX_FRAGMENT_PDU	= 1024,
>  	FIO_SERVER_MAX_CMD_MB		= 2048,
> diff --git a/thread_options.h b/thread_options.h
> index 4b4ecfe1..6fe1cad7 100644
> --- a/thread_options.h
> +++ b/thread_options.h
> @@ -189,6 +189,7 @@ struct thread_options {
>  	unsigned int thinktime_spin;
>  	unsigned int thinktime_blocks;
>  	unsigned int thinktime_blocks_type;
> +	unsigned int thinktime_iotime;
>  	unsigned int fsync_blocks;
>  	unsigned int fdatasync_blocks;
>  	unsigned int barrier_blocks;
> @@ -500,6 +501,8 @@ struct thread_options_pack {
>  	uint32_t thinktime_spin;
>  	uint32_t thinktime_blocks;
>  	uint32_t thinktime_blocks_type;
> +	uint32_t thinktime_iotime;
> +	uint32_t pad6;

Why is this needed ? Some alignement warning ? If yes, have you tried moving
this declaration in the struct ?

>  	uint32_t fsync_blocks;
>  	uint32_t fdatasync_blocks;
>  	uint32_t barrier_blocks;
> 

Apart from the question above, this looks good to me.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] options: Add thinktime_iotime option

[Shinichiro Kawasaki]

On Aug 23, 2021 / 01:10, Damien Le Moal wrote:
> On 2021/08/20 20:25, Shin'ichiro Kawasaki wrote:
> > The thinktime option allows stalling a job for a specified amount of
> > time. Using the thinktime_blocks option, periodic stalls can be added
> > every thinktime_blocks IOs. However, with this option, the periodic
> > stall may not be repeated at equal time intervals as the time to execute
> > thinktime_blocks IOs may vary.
> > 
> > To control the thinktime interval by time, introduce the option
> > thinktime_iotime. With this new option, the thinktime stall is repeated
> > after IOs are executed for thinktime_iotime. If this option is used
> > together with the thinktime_blocks option, the thinktime pause is
> > repeated after thinktime_iotime or after thinktime_blocks IOs, whichever
> > happens first.
> > 
> > To track the time and IO block count at the last stall, add
> > last_thinktime variable and last_thinktime_blocks variable to struct
> > thread_data. Also, introduce the helper function init_thinktime()
> > to group thinktime related preparations.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  HOWTO            |  9 ++++++++-
> >  backend.c        | 45 +++++++++++++++++++++++++++++++++++++++------
> >  cconv.c          |  2 ++
> >  fio.1            |  7 ++++++-
> >  fio.h            |  2 ++
> >  options.c        | 14 ++++++++++++++
> >  server.h         |  2 +-
> >  thread_options.h |  3 +++
> >  8 files changed, 75 insertions(+), 9 deletions(-)
> > 
> > diff --git a/HOWTO b/HOWTO
> > index 9bfd38b4..cfb278ef 100644
> > --- a/HOWTO
> > +++ b/HOWTO
> > @@ -2710,7 +2710,7 @@ I/O rate
> >  	Stall the job for the specified period of time after an I/O has completed before issuing the
> >  	next. May be used to simulate processing being done by an application.
> >  	When the unit is omitted, the value is interpreted in microseconds.  See
> > -	:option:`thinktime_blocks` and :option:`thinktime_spin`.
> > +	:option:`thinktime_blocks`, :option:`thinktime_iotime` and :option:`thinktime_spin`.
> >  
> >  .. option:: thinktime_spin=time
> >  
> > @@ -2735,6 +2735,13 @@ I/O rate
> >  	:option:`thinktime_blocks` blocks. If this is set to `issue`, then the trigger happens
> >  	at the issue side.
> >  
> > +.. option:: thinktime_iotime=time
> > +
> > +	Only valid if :option:`thinktime` is set - Repeat job stall for
> > +	:option:`thinktime` after executing I/Os for `thinktime_iotime`. When
> > +	the unit is omitted, :option:`thinktime_iotime` is interpreted as a
> > +	number of seconds.
> > +
> >  .. option:: rate=int[,int][,int]
> >  
> >  	Cap the bandwidth used by this job. The number is in bytes/sec, the normal
> > diff --git a/backend.c b/backend.c
> > index 808e4362..5cdb8124 100644
> > --- a/backend.c
> > +++ b/backend.c
> > @@ -858,15 +858,47 @@ static long long usec_for_io(struct thread_data *td, enum fio_ddir ddir)
> >  	return 0;
> >  }
> >  
> > +static void init_thinktime(struct thread_data *td)
> > +{
> > +	if (td->o.thinktime_blocks_type == THINKTIME_BLOCKS_TYPE_COMPLETE)
> > +		td->thinktime_blocks_counter = td->io_blocks;
> > +	else
> > +		td->thinktime_blocks_counter = td->io_issues;
> > +	td->last_thinktime = td->epoch;
> > +	td->last_thinktime_blocks = 0;
> > +}
> > +
> >  static void handle_thinktime(struct thread_data *td, enum fio_ddir ddir,
> >  			     struct timespec *time)
> >  {
> >  	unsigned long long b;
> >  	uint64_t total;
> >  	int left;
> > +	struct timespec now;
> > +	bool stall = false;
> > +
> > +	if (td->o.thinktime_iotime) {
> > +		fio_gettime(&now, NULL);
> > +		if (utime_since(&td->last_thinktime, &now)
> > +		    >= td->o.thinktime_iotime + td->o.thinktime) {
> > +			stall = true;
> > +		} else if (!fio_option_is_set(&td->o, thinktime_blocks)) {
> > +			/*
> > +			 * When thinktime_iotime is set and thinktime_blocks is
> > +			 * not set, skip the thinktime_blocks check, since
> > +			 * thinktime_blocks default value 1 does not work
> > +			 * together with thinktime_iotime.
> > +			 */
> > +			return;
> > +		}
> > +
> > +	}
> >  
> >  	b = ddir_rw_sum(td->thinktime_blocks_counter);
> > -	if (b % td->o.thinktime_blocks || !b)
> > +	if (b >= td->last_thinktime_blocks + td->o.thinktime_blocks)
> > +		stall = true;
> > +
> > +	if (!stall)
> >  		return;
> >  
> >  	io_u_quiesce(td);
> > @@ -902,6 +934,10 @@ static void handle_thinktime(struct thread_data *td, enum fio_ddir ddir,
> >  
> >  	if (time && should_check_rate(td))
> >  		fio_gettime(time, NULL);
> > +
> > +	td->last_thinktime_blocks = b;
> > +	if (td->o.thinktime_iotime)
> > +		td->last_thinktime = now;
> >  }
> >  
> >  /*
> > @@ -1791,17 +1827,14 @@ static void *thread_main(void *data)
> >  	if (rate_submit_init(td, sk_out))
> >  		goto err;
> >  
> > -	if (td->o.thinktime_blocks_type == THINKTIME_BLOCKS_TYPE_COMPLETE)
> > -		td->thinktime_blocks_counter = td->io_blocks;
> > -	else
> > -		td->thinktime_blocks_counter = td->io_issues;
> > -
> >  	set_epoch_time(td, o->log_unix_epoch);
> >  	fio_getrusage(&td->ru_start);
> >  	memcpy(&td->bw_sample_time, &td->epoch, sizeof(td->epoch));
> >  	memcpy(&td->iops_sample_time, &td->epoch, sizeof(td->epoch));
> >  	memcpy(&td->ss.prev_time, &td->epoch, sizeof(td->epoch));
> >  
> > +	init_thinktime(td);
> > +
> >  	if (o->ratemin[DDIR_READ] || o->ratemin[DDIR_WRITE] ||
> >  			o->ratemin[DDIR_TRIM]) {
> >  	        memcpy(&td->lastrate[DDIR_READ], &td->bw_sample_time,
> > diff --git a/cconv.c b/cconv.c
> > index e3a8c27c..92491ddb 100644
> > --- a/cconv.c
> > +++ b/cconv.c
> > @@ -213,6 +213,7 @@ void convert_thread_options_to_cpu(struct thread_options *o,
> >  	o->thinktime_spin = le32_to_cpu(top->thinktime_spin);
> >  	o->thinktime_blocks = le32_to_cpu(top->thinktime_blocks);
> >  	o->thinktime_blocks_type = le32_to_cpu(top->thinktime_blocks_type);
> > +	o->thinktime_iotime = le32_to_cpu(top->thinktime_iotime);
> >  	o->fsync_blocks = le32_to_cpu(top->fsync_blocks);
> >  	o->fdatasync_blocks = le32_to_cpu(top->fdatasync_blocks);
> >  	o->barrier_blocks = le32_to_cpu(top->barrier_blocks);
> > @@ -438,6 +439,7 @@ void convert_thread_options_to_net(struct thread_options_pack *top,
> >  	top->thinktime_spin = cpu_to_le32(o->thinktime_spin);
> >  	top->thinktime_blocks = cpu_to_le32(o->thinktime_blocks);
> >  	top->thinktime_blocks_type = __cpu_to_le32(o->thinktime_blocks_type);
> > +	top->thinktime_iotime = __cpu_to_le32(o->thinktime_iotime);
> >  	top->fsync_blocks = cpu_to_le32(o->fsync_blocks);
> >  	top->fdatasync_blocks = cpu_to_le32(o->fdatasync_blocks);
> >  	top->barrier_blocks = cpu_to_le32(o->barrier_blocks);
> > diff --git a/fio.1 b/fio.1
> > index 382cebfc..3cae15b3 100644
> > --- a/fio.1
> > +++ b/fio.1
> > @@ -2471,7 +2471,7 @@ problem). Note that this option cannot reliably be used with async IO engines.
> >  Stall the job for the specified period of time after an I/O has completed before issuing the
> >  next. May be used to simulate processing being done by an application.
> >  When the unit is omitted, the value is interpreted in microseconds. See
> > -\fBthinktime_blocks\fR and \fBthinktime_spin\fR.
> > +\fBthinktime_blocks\fR, \fBthinktime_iotime\fR and \fBthinktime_spin\fR.
> >  .TP
> >  .BI thinktime_spin \fR=\fPtime
> >  Only valid if \fBthinktime\fR is set - pretend to spend CPU time doing
> > @@ -2493,6 +2493,11 @@ The default is `complete', which triggers \fBthinktime\fR when fio completes
> >  \fBthinktime_blocks\fR blocks. If this is set to `issue', then the trigger happens
> >  at the issue side.
> >  .TP
> > +.BI thinktime_iotime \fR=\fPtime
> > +Only valid if \fBthinktime\fR is set - Repeat job stall for \fBthinktime\fR
> > +after executing I/Os for \fBthinktime_iotime\fR. When the unit is omitted,
> > +\fBthinktime_iotime\fR is interpreted as a number of seconds.
> > +.TP
> >  .BI rate \fR=\fPint[,int][,int]
> >  Cap the bandwidth used by this job. The number is in bytes/sec, the normal
> >  suffix rules apply. Comma-separated values may be specified for reads,
> > diff --git a/fio.h b/fio.h
> > index 6f6b211b..85ed60fc 100644
> > --- a/fio.h
> > +++ b/fio.h
> > @@ -365,6 +365,8 @@ struct thread_data {
> >  	uint64_t bytes_done[DDIR_RWDIR_CNT];
> >  
> >  	uint64_t *thinktime_blocks_counter;
> > +	struct timespec last_thinktime;
> > +	uint64_t last_thinktime_blocks;
> >  
> >  	/*
> >  	 * State for random io, a bitmap of blocks done vs not done
> > diff --git a/options.c b/options.c
> > index 8c2ab7cc..3fe1e6bd 100644
> > --- a/options.c
> > +++ b/options.c
> > @@ -3688,6 +3688,20 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
> >  		},
> >  		.parent = "thinktime",
> >  	},
> > +	{
> > +		.name	= "thinktime_iotime",
> > +		.lname	= "Thinktime interval",
> > +		.type	= FIO_OPT_INT,
> > +		.off1	= offsetof(struct thread_options, thinktime_iotime),
> > +		.help	= "IO time interval between 'thinktime'",
> > +		.is_time = 1,
> > +		.is_seconds = 1,
> > +		.def	= "0",
> > +		.parent	= "thinktime",
> > +		.hide	= 1,
> > +		.category = FIO_OPT_C_IO,
> > +		.group	= FIO_OPT_G_THINKTIME,
> > +	},
> >  	{
> >  		.name	= "rate",
> >  		.lname	= "I/O rate",
> > diff --git a/server.h b/server.h
> > index daed057a..7ce2ddb1 100644
> > --- a/server.h
> > +++ b/server.h
> > @@ -48,7 +48,7 @@ struct fio_net_cmd_reply {
> >  };
> >  
> >  enum {
> > -	FIO_SERVER_VER			= 92,
> > +	FIO_SERVER_VER			= 93,
> >  
> >  	FIO_SERVER_MAX_FRAGMENT_PDU	= 1024,
> >  	FIO_SERVER_MAX_CMD_MB		= 2048,
> > diff --git a/thread_options.h b/thread_options.h
> > index 4b4ecfe1..6fe1cad7 100644
> > --- a/thread_options.h
> > +++ b/thread_options.h
> > @@ -189,6 +189,7 @@ struct thread_options {
> >  	unsigned int thinktime_spin;
> >  	unsigned int thinktime_blocks;
> >  	unsigned int thinktime_blocks_type;
> > +	unsigned int thinktime_iotime;
> >  	unsigned int fsync_blocks;
> >  	unsigned int fdatasync_blocks;
> >  	unsigned int barrier_blocks;
> > @@ -500,6 +501,8 @@ struct thread_options_pack {
> >  	uint32_t thinktime_spin;
> >  	uint32_t thinktime_blocks;
> >  	uint32_t thinktime_blocks_type;
> > +	uint32_t thinktime_iotime;
> > +	uint32_t pad6;
> 
> Why is this needed ? Some alignement warning ?

Yes. Without the pad, I observe build errors as follows:

In file included from fio.h:17,
                 from libfio.c:31:
libfio.c: In function ‘initialize_fio’:
compiler/compiler.h:31:44: error: static assertion failed: "percentile_list"
   31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
      |                                            ^~~~~~~~~~~~~~
libfio.c:372:9: note: in expansion of macro ‘compiletime_assert’
  372 |         compiletime_assert((offsetof(struct thread_options_pack, percentile_list) % 8) == 0, "percentile_list");
      |         ^~~~~~~~~~~~~~~~~~
compiler/compiler.h:31:44: error: static assertion failed: "latency_percentile"
   31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
      |                                            ^~~~~~~~~~~~~~
libfio.c:373:9: note: in expansion of macro ‘compiletime_assert’
  373 |         compiletime_assert((offsetof(struct thread_options_pack, latency_percentile) % 8) == 0, "latency_percentile");
      |         ^~~~~~~~~~~~~~~~~~
make: *** [Makefile:496: libfio.o] Error 1
make: *** Waiting for unfinished jobs....

> If yes, have you tried moving
> this declaration in the struct ?

Yes. I moved the new field thinktime_iotime to the end of struct
thread_options_pack then the errors were avoided. But I wanted
to place the new field at the same place as other thinktime related
fields. For that purpose, I needed to add the padding pad6. I tried to
utilize other pads such as pad2 or pad5, but it didn't work.

To place the related fields at same place with padding, or to place the new
field at different place without padding, which way to go?

> 
> >  	uint32_t fsync_blocks;
> >  	uint32_t fdatasync_blocks;
> >  	uint32_t barrier_blocks;
> > 
> 
> Apart from the question above, this looks good to me.
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH] options: Add thinktime_iotime option

[Damien Le Moal]

On 2021/08/23 10:35, Shinichiro Kawasaki wrote:
[...]
>>> diff --git a/thread_options.h b/thread_options.h
>>> index 4b4ecfe1..6fe1cad7 100644
>>> --- a/thread_options.h
>>> +++ b/thread_options.h
>>> @@ -189,6 +189,7 @@ struct thread_options {
>>>  	unsigned int thinktime_spin;
>>>  	unsigned int thinktime_blocks;
>>>  	unsigned int thinktime_blocks_type;
>>> +	unsigned int thinktime_iotime;
>>>  	unsigned int fsync_blocks;
>>>  	unsigned int fdatasync_blocks;
>>>  	unsigned int barrier_blocks;
>>> @@ -500,6 +501,8 @@ struct thread_options_pack {
>>>  	uint32_t thinktime_spin;
>>>  	uint32_t thinktime_blocks;
>>>  	uint32_t thinktime_blocks_type;
>>> +	uint32_t thinktime_iotime;
>>> +	uint32_t pad6;
>>
>> Why is this needed ? Some alignement warning ?
> 
> Yes. Without the pad, I observe build errors as follows:
> 
> In file included from fio.h:17,
>                  from libfio.c:31:
> libfio.c: In function ‘initialize_fio’:
> compiler/compiler.h:31:44: error: static assertion failed: "percentile_list"
>    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
>       |                                            ^~~~~~~~~~~~~~
> libfio.c:372:9: note: in expansion of macro ‘compiletime_assert’
>   372 |         compiletime_assert((offsetof(struct thread_options_pack, percentile_list) % 8) == 0, "percentile_list");
>       |         ^~~~~~~~~~~~~~~~~~
> compiler/compiler.h:31:44: error: static assertion failed: "latency_percentile"
>    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
>       |                                            ^~~~~~~~~~~~~~
> libfio.c:373:9: note: in expansion of macro ‘compiletime_assert’
>   373 |         compiletime_assert((offsetof(struct thread_options_pack, latency_percentile) % 8) == 0, "latency_percentile");
>       |         ^~~~~~~~~~~~~~~~~~
> make: *** [Makefile:496: libfio.o] Error 1
> make: *** Waiting for unfinished jobs....
> 
>> If yes, have you tried moving
>> this declaration in the struct ?
> 
> Yes. I moved the new field thinktime_iotime to the end of struct
> thread_options_pack then the errors were avoided. But I wanted
> to place the new field at the same place as other thinktime related
> fields. For that purpose, I needed to add the padding pad6. I tried to
> utilize other pads such as pad2 or pad5, but it didn't work.
> 
> To place the related fields at same place with padding, or to place the new
> field at different place without padding, which way to go?

I think that is a question for Jens...

Jens,

Which way do you prefer ?


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] options: Add thinktime_iotime option

[Shinichiro Kawasaki]

On Aug 23, 2021 / 03:02, Damien Le Moal wrote:
> On 2021/08/23 10:35, Shinichiro Kawasaki wrote:
> [...]
> >>> diff --git a/thread_options.h b/thread_options.h
> >>> index 4b4ecfe1..6fe1cad7 100644
> >>> --- a/thread_options.h
> >>> +++ b/thread_options.h
> >>> @@ -189,6 +189,7 @@ struct thread_options {
> >>>  	unsigned int thinktime_spin;
> >>>  	unsigned int thinktime_blocks;
> >>>  	unsigned int thinktime_blocks_type;
> >>> +	unsigned int thinktime_iotime;
> >>>  	unsigned int fsync_blocks;
> >>>  	unsigned int fdatasync_blocks;
> >>>  	unsigned int barrier_blocks;
> >>> @@ -500,6 +501,8 @@ struct thread_options_pack {
> >>>  	uint32_t thinktime_spin;
> >>>  	uint32_t thinktime_blocks;
> >>>  	uint32_t thinktime_blocks_type;
> >>> +	uint32_t thinktime_iotime;
> >>> +	uint32_t pad6;
> >>
> >> Why is this needed ? Some alignement warning ?
> > 
> > Yes. Without the pad, I observe build errors as follows:
> > 
> > In file included from fio.h:17,
> >                  from libfio.c:31:
> > libfio.c: In function ‘initialize_fio’:
> > compiler/compiler.h:31:44: error: static assertion failed: "percentile_list"
> >    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
> >       |                                            ^~~~~~~~~~~~~~
> > libfio.c:372:9: note: in expansion of macro ‘compiletime_assert’
> >   372 |         compiletime_assert((offsetof(struct thread_options_pack, percentile_list) % 8) == 0, "percentile_list");
> >       |         ^~~~~~~~~~~~~~~~~~
> > compiler/compiler.h:31:44: error: static assertion failed: "latency_percentile"
> >    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
> >       |                                            ^~~~~~~~~~~~~~
> > libfio.c:373:9: note: in expansion of macro ‘compiletime_assert’
> >   373 |         compiletime_assert((offsetof(struct thread_options_pack, latency_percentile) % 8) == 0, "latency_percentile");
> >       |         ^~~~~~~~~~~~~~~~~~
> > make: *** [Makefile:496: libfio.o] Error 1
> > make: *** Waiting for unfinished jobs....
> > 
> >> If yes, have you tried moving
> >> this declaration in the struct ?
> > 
> > Yes. I moved the new field thinktime_iotime to the end of struct
> > thread_options_pack then the errors were avoided. But I wanted
> > to place the new field at the same place as other thinktime related
> > fields. For that purpose, I needed to add the padding pad6. I tried to
> > utilize other pads such as pad2 or pad5, but it didn't work.
> > 
> > To place the related fields at same place with padding, or to place the new
> > field at different place without padding, which way to go?
> 
> I think that is a question for Jens...
> 
> Jens,
> 
> Which way do you prefer ?

Jens,

May I ask your comment on the new pad in the struct thread_options_pack? If the
new pad is not good, I will move the new field thinktime_iotime to the end of
the struct.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH] options: Add thinktime_iotime option

[Jens Axboe]

On 9/2/21 7:26 PM, Shinichiro Kawasaki wrote:
> On Aug 23, 2021 / 03:02, Damien Le Moal wrote:
>> On 2021/08/23 10:35, Shinichiro Kawasaki wrote:
>> [...]
>>>>> diff --git a/thread_options.h b/thread_options.h
>>>>> index 4b4ecfe1..6fe1cad7 100644
>>>>> --- a/thread_options.h
>>>>> +++ b/thread_options.h
>>>>> @@ -189,6 +189,7 @@ struct thread_options {
>>>>>  	unsigned int thinktime_spin;
>>>>>  	unsigned int thinktime_blocks;
>>>>>  	unsigned int thinktime_blocks_type;
>>>>> +	unsigned int thinktime_iotime;
>>>>>  	unsigned int fsync_blocks;
>>>>>  	unsigned int fdatasync_blocks;
>>>>>  	unsigned int barrier_blocks;
>>>>> @@ -500,6 +501,8 @@ struct thread_options_pack {
>>>>>  	uint32_t thinktime_spin;
>>>>>  	uint32_t thinktime_blocks;
>>>>>  	uint32_t thinktime_blocks_type;
>>>>> +	uint32_t thinktime_iotime;
>>>>> +	uint32_t pad6;
>>>>
>>>> Why is this needed ? Some alignement warning ?
>>>
>>> Yes. Without the pad, I observe build errors as follows:
>>>
>>> In file included from fio.h:17,
>>>                  from libfio.c:31:
>>> libfio.c: In function ‘initialize_fio’:
>>> compiler/compiler.h:31:44: error: static assertion failed: "percentile_list"
>>>    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
>>>       |                                            ^~~~~~~~~~~~~~
>>> libfio.c:372:9: note: in expansion of macro ‘compiletime_assert’
>>>   372 |         compiletime_assert((offsetof(struct thread_options_pack, percentile_list) % 8) == 0, "percentile_list");
>>>       |         ^~~~~~~~~~~~~~~~~~
>>> compiler/compiler.h:31:44: error: static assertion failed: "latency_percentile"
>>>    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
>>>       |                                            ^~~~~~~~~~~~~~
>>> libfio.c:373:9: note: in expansion of macro ‘compiletime_assert’
>>>   373 |         compiletime_assert((offsetof(struct thread_options_pack, latency_percentile) % 8) == 0, "latency_percentile");
>>>       |         ^~~~~~~~~~~~~~~~~~
>>> make: *** [Makefile:496: libfio.o] Error 1
>>> make: *** Waiting for unfinished jobs....
>>>
>>>> If yes, have you tried moving
>>>> this declaration in the struct ?
>>>
>>> Yes. I moved the new field thinktime_iotime to the end of struct
>>> thread_options_pack then the errors were avoided. But I wanted
>>> to place the new field at the same place as other thinktime related
>>> fields. For that purpose, I needed to add the padding pad6. I tried to
>>> utilize other pads such as pad2 or pad5, but it didn't work.
>>>
>>> To place the related fields at same place with padding, or to place the new
>>> field at different place without padding, which way to go?
>>
>> I think that is a question for Jens...
>>
>> Jens,
>>
>> Which way do you prefer ?
> 
> Jens,
> 
> May I ask your comment on the new pad in the struct thread_options_pack? If the
> new pad is not good, I will move the new field thinktime_iotime to the end of
> the struct.

I tend to add new elements where I can remove a padding field. Or moving
things around to make it saner. If you need to add pad6, then that's
a clear indication that things can be moved around and padding reduced.

-- 
Jens Axboe

Re: [PATCH] options: Add thinktime_iotime option

[Shinichiro Kawasaki]

On Sep 02, 2021 / 20:57, Jens Axboe wrote:
> On 9/2/21 7:26 PM, Shinichiro Kawasaki wrote:
> > On Aug 23, 2021 / 03:02, Damien Le Moal wrote:
> >> On 2021/08/23 10:35, Shinichiro Kawasaki wrote:
> >> [...]
> >>>>> diff --git a/thread_options.h b/thread_options.h
> >>>>> index 4b4ecfe1..6fe1cad7 100644
> >>>>> --- a/thread_options.h
> >>>>> +++ b/thread_options.h
> >>>>> @@ -189,6 +189,7 @@ struct thread_options {
> >>>>>  	unsigned int thinktime_spin;
> >>>>>  	unsigned int thinktime_blocks;
> >>>>>  	unsigned int thinktime_blocks_type;
> >>>>> +	unsigned int thinktime_iotime;
> >>>>>  	unsigned int fsync_blocks;
> >>>>>  	unsigned int fdatasync_blocks;
> >>>>>  	unsigned int barrier_blocks;
> >>>>> @@ -500,6 +501,8 @@ struct thread_options_pack {
> >>>>>  	uint32_t thinktime_spin;
> >>>>>  	uint32_t thinktime_blocks;
> >>>>>  	uint32_t thinktime_blocks_type;
> >>>>> +	uint32_t thinktime_iotime;
> >>>>> +	uint32_t pad6;
> >>>>
> >>>> Why is this needed ? Some alignement warning ?
> >>>
> >>> Yes. Without the pad, I observe build errors as follows:
> >>>
> >>> In file included from fio.h:17,
> >>>                  from libfio.c:31:
> >>> libfio.c: In function ‘initialize_fio’:
> >>> compiler/compiler.h:31:44: error: static assertion failed: "percentile_list"
> >>>    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
> >>>       |                                            ^~~~~~~~~~~~~~
> >>> libfio.c:372:9: note: in expansion of macro ‘compiletime_assert’
> >>>   372 |         compiletime_assert((offsetof(struct thread_options_pack, percentile_list) % 8) == 0, "percentile_list");
> >>>       |         ^~~~~~~~~~~~~~~~~~
> >>> compiler/compiler.h:31:44: error: static assertion failed: "latency_percentile"
> >>>    31 | #define compiletime_assert(condition, msg) _Static_assert(condition, msg)
> >>>       |                                            ^~~~~~~~~~~~~~
> >>> libfio.c:373:9: note: in expansion of macro ‘compiletime_assert’
> >>>   373 |         compiletime_assert((offsetof(struct thread_options_pack, latency_percentile) % 8) == 0, "latency_percentile");
> >>>       |         ^~~~~~~~~~~~~~~~~~
> >>> make: *** [Makefile:496: libfio.o] Error 1
> >>> make: *** Waiting for unfinished jobs....
> >>>
> >>>> If yes, have you tried moving
> >>>> this declaration in the struct ?
> >>>
> >>> Yes. I moved the new field thinktime_iotime to the end of struct
> >>> thread_options_pack then the errors were avoided. But I wanted
> >>> to place the new field at the same place as other thinktime related
> >>> fields. For that purpose, I needed to add the padding pad6. I tried to
> >>> utilize other pads such as pad2 or pad5, but it didn't work.
> >>>
> >>> To place the related fields at same place with padding, or to place the new
> >>> field at different place without padding, which way to go?
> >>
> >> I think that is a question for Jens...
> >>
> >> Jens,
> >>
> >> Which way do you prefer ?
> > 
> > Jens,
> > 
> > May I ask your comment on the new pad in the struct thread_options_pack? If the
> > new pad is not good, I will move the new field thinktime_iotime to the end of
> > the struct.
> 
> I tend to add new elements where I can remove a padding field. Or moving
> things around to make it saner. If you need to add pad6, then that's
> a clear indication that things can be moved around and padding reduced.

Jens, thank you for the comment. I will move thinktime related elements and
remove an existing padding.

-- 
Best Regards,
Shin'ichiro Kawasaki