[PATCH v2] options: Add thinktime_iotime option

[PATCH v2] 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 support the new option, add a new member thinktime_iotime in the
struct thread_options and the struct thread_options_pack. Avoid size
increase of the struct thread_options_pack by replacing a padding 'pad5'
with the new member. To keep thinktime related members close, move the
members near the position where the padding was placed. Make same
changes to the struct thread_option also for consistency.

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>
---
Changes from v1:
* Moved thinktime related members in thread_options_pack to avoid padding

 HOWTO            |  9 ++++++++-
 backend.c        | 45 +++++++++++++++++++++++++++++++++++++++------
 cconv.c          |  2 ++
 fio.1            |  7 ++++++-
 fio.h            |  2 ++
 options.c        | 14 ++++++++++++++
 server.h         |  2 +-
 thread_options.h | 21 ++++++++++++---------
 8 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/HOWTO b/HOWTO
index a2cf20f6..321a4402 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..60f63f81 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -185,10 +185,6 @@ struct thread_options {
 
 	unsigned int hugepage_size;
 	unsigned long long rw_min_bs;
-	unsigned int thinktime;
-	unsigned int thinktime_spin;
-	unsigned int thinktime_blocks;
-	unsigned int thinktime_blocks_type;
 	unsigned int fsync_blocks;
 	unsigned int fdatasync_blocks;
 	unsigned int barrier_blocks;
@@ -297,6 +293,12 @@ struct thread_options {
 	char *exec_prerun;
 	char *exec_postrun;
 
+	unsigned int thinktime;
+	unsigned int thinktime_spin;
+	unsigned int thinktime_blocks;
+	unsigned int thinktime_blocks_type;
+	unsigned int thinktime_iotime;
+
 	uint64_t rate[DDIR_RWDIR_CNT];
 	uint64_t ratemin[DDIR_RWDIR_CNT];
 	unsigned int ratecycle;
@@ -496,10 +498,6 @@ struct thread_options_pack {
 
 	uint32_t hugepage_size;
 	uint64_t rw_min_bs;
-	uint32_t thinktime;
-	uint32_t thinktime_spin;
-	uint32_t thinktime_blocks;
-	uint32_t thinktime_blocks_type;
 	uint32_t fsync_blocks;
 	uint32_t fdatasync_blocks;
 	uint32_t barrier_blocks;
@@ -604,6 +602,12 @@ struct thread_options_pack {
 	uint8_t exec_prerun[FIO_TOP_STR_MAX];
 	uint8_t exec_postrun[FIO_TOP_STR_MAX];
 
+	uint32_t thinktime;
+	uint32_t thinktime_spin;
+	uint32_t thinktime_blocks;
+	uint32_t thinktime_blocks_type;
+	uint32_t thinktime_iotime;
+
 	uint64_t rate[DDIR_RWDIR_CNT];
 	uint64_t ratemin[DDIR_RWDIR_CNT];
 	uint32_t ratecycle;
@@ -643,7 +647,6 @@ struct thread_options_pack {
 	uint64_t latency_target;
 	uint64_t latency_window;
 	uint64_t max_latency[DDIR_RWDIR_CNT];
-	uint32_t pad5;
 	fio_fp64_t latency_percentile;
 	uint32_t latency_run;
 
-- 
2.31.1

Re: [PATCH v2] options: Add thinktime_iotime option

[Jens Axboe]

On 9/3/21 2:37 AM, Shin'ichiro Kawasaki wrote:
> 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,
> +	},

Please line these up nicely as is done elsewhere.

Apart from that, this looks fine. Could really use an example job
though, and an improvement in the explanation. The commit message looks
great, but then the doc additions are pretty lackluster. Maybe steal a
bit of the text you wrote from the commit message and put it in the
shipped documentation?

-- 
Jens Axboe

Re: [PATCH v2] options: Add thinktime_iotime option

[Shinichiro Kawasaki]

On Sep 03, 2021 / 06:38, Jens Axboe wrote:
> On 9/3/21 2:37 AM, Shin'ichiro Kawasaki wrote:
> > 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,
> > +	},
> 
> Please line these up nicely as is done elsewhere.

Ah, the order of the members are diffferent from others. I will move a few
members to make the order same as others.

> 
> Apart from that, this looks fine. Could really use an example job
> though, and an improvement in the explanation. The commit message looks
> great, but then the doc additions are pretty lackluster. Maybe steal a
> bit of the text you wrote from the commit message and put it in the
> shipped documentation?

I will improve the descriptions of the new option in fio.1 and HOWTO, copying
some of the commit message. Also I will add "for example" option usage in the
descriptions.

Thank you for your review.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH v2] options: Add thinktime_iotime option

[Jens Axboe]

On 9/3/21 3:38 PM, Shinichiro Kawasaki wrote:
> On Sep 03, 2021 / 06:38, Jens Axboe wrote:
>> On 9/3/21 2:37 AM, Shin'ichiro Kawasaki wrote:
>>> 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,
>>> +	},
>>
>> Please line these up nicely as is done elsewhere.
> 
> Ah, the order of the members are diffferent from others. I will move a
> few members to make the order same as others.

I didn't notice the ordering, but just to be clear, I'm just referring
to the tabbing so the '=' line up. But if they are out of order, please
do correct that too.

>> Apart from that, this looks fine. Could really use an example job
>> though, and an improvement in the explanation. The commit message looks
>> great, but then the doc additions are pretty lackluster. Maybe steal a
>> bit of the text you wrote from the commit message and put it in the
>> shipped documentation?
> 
> I will improve the descriptions of the new option in fio.1 and HOWTO,
> copying some of the commit message. Also I will add "for example"
> option usage in the descriptions.

Sounds good, thanks!

-- 
Jens Axboe

Re: [PATCH v2] options: Add thinktime_iotime option

[Shinichiro Kawasaki]

On Sep 03, 2021 / 18:58, Jens Axboe wrote:
> On 9/3/21 3:38 PM, Shinichiro Kawasaki wrote:
> > On Sep 03, 2021 / 06:38, Jens Axboe wrote:
> >> On 9/3/21 2:37 AM, Shin'ichiro Kawasaki wrote:
> >>> 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,
> >>> +	},
> >>
> >> Please line these up nicely as is done elsewhere.
> > 
> > Ah, the order of the members are diffferent from others. I will move a
> > few members to make the order same as others.
> 
> I didn't notice the ordering, but just to be clear, I'm just referring
> to the tabbing so the '=' line up. But if they are out of order, please
> do correct that too.

Thanks for the clarification. I will fix the order. As for tabbing, I found that
'=' is not lined up in existing members of fio_options[] either. Here I quote an
example from "startdelay" option.

	{
		.name	= "startdelay",
		.lname	= "Start delay",
		.type	= FIO_OPT_STR_VAL_TIME,
		.off1	= offsetof(struct thread_options, start_delay),
		.off2	= offsetof(struct thread_options, start_delay_high),
		.help	= "Only start job when this period has passed",
		.def	= "0",
		.is_seconds = 1,
		.is_time = 1,
		.category = FIO_OPT_C_GENERAL,
		.group	= FIO_OPT_G_RUNTIME,
	},

The number of tab in front of '=' is single for all members. On the other hand,
some of the members have too long name: such as "is_time", "is_seconds" or
"category". So '=' can not be lined up for them. I think it would be the better
to allow unaligned '=' style for new options if they have the long name members.

After fixing the order of the members of the new option, that part will look as
follows:

+		.name	= "thinktime_iotime",
+		.lname	= "Thinktime interval",
+		.type	= FIO_OPT_INT,
+		.off1	= offsetof(struct thread_options, thinktime_iotime),
+		.help	= "IO time interval between 'thinktime'",
+		.def	= "0",
+		.parent	= "thinktime",
+		.hide	= 1,
+		.is_seconds = 1,
+		.is_time = 1,
+		.category = FIO_OPT_C_IO,
+		.group	= FIO_OPT_G_THINKTIME,

The unaligned '='s will be gathered at the last part, so I wish it does not look
so messy.

> 
> >> Apart from that, this looks fine. Could really use an example job
> >> though, and an improvement in the explanation. The commit message looks
> >> great, but then the doc additions are pretty lackluster. Maybe steal a
> >> bit of the text you wrote from the commit message and put it in the
> >> shipped documentation?
> > 
> > I will improve the descriptions of the new option in fio.1 and HOWTO,
> > copying some of the commit message. Also I will add "for example"
> > option usage in the descriptions.
> 
> Sounds good, thanks!
> 
> -- 
> Jens Axboe
> 

-- 
Best Regards,
Shin'ichiro Kawasaki