[PATCH] time_based: Avoid restarting main I/O loop

[PATCH] time_based: Avoid restarting main I/O loop

[Dan Ehrenberg]

Previously, when fio had written a volume of I/O equal to the size
argument, it restarted the main do_io loop.

This patch allows time_based tests to be run for longer than one
cycle in the do_io main loop. This has a couple of advantages:
 * The random number generator is not reset on each iteration
   of the loop, so running longer will reach different locations.
 * There is not a throughput-reducing point where all operations
   must be reaped before new operations are submitted.

The implementation consists of two minor changes:
 * In the do_io loop, a time_based test will not exit the loop for
   reading or writing too much data.
 * When reading or writing sequentially, the operations wrap around
   to the beginning after reading the end within the
   get_next_seq_block function.
---
 backend.c |    3 ++-
 io_u.c    |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/backend.c b/backend.c
index 7343286..1d9b0a2 100644
--- a/backend.c
+++ b/backend.c
@@ -555,7 +555,8 @@ static void do_io(struct thread_data *td)
 		td_set_runstate(td, TD_RUNNING);
 
 	while ((td->o.read_iolog_file && !flist_empty(&td->io_log_list)) ||
-		(!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td)) {
+		(!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td) ||
+		td->o.time_based) {
 		struct timeval comp_time;
 		unsigned long bytes_done[2] = { 0, 0 };
 		int min_evts = 0;
diff --git a/io_u.c b/io_u.c
index 20794c3..a3ea43d 100644
--- a/io_u.c
+++ b/io_u.c
@@ -252,6 +252,9 @@ static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
 {
 	assert(ddir_rw(ddir));
 
+       if (f->last_pos >= f->io_size && td->o.time_based)
+               f->last_pos = f->last_pos - f->io_size;
+
 	if (f->last_pos < f->real_file_size) {
 		unsigned long long pos;
 
-- 
1.7.7.3

Re: [PATCH] time_based: Avoid restarting main I/O loop

[Jens Axboe]

On 03/16/2012 04:12 AM, Dan Ehrenberg wrote:
> Previously, when fio had written a volume of I/O equal to the size
> argument, it restarted the main do_io loop.
> 
> This patch allows time_based tests to be run for longer than one
> cycle in the do_io main loop. This has a couple of advantages:
>  * The random number generator is not reset on each iteration
>    of the loop, so running longer will reach different locations.
>  * There is not a throughput-reducing point where all operations
>    must be reaped before new operations are submitted.
> 
> The implementation consists of two minor changes:
>  * In the do_io loop, a time_based test will not exit the loop for
>    reading or writing too much data.
>  * When reading or writing sequentially, the operations wrap around
>    to the beginning after reading the end within the
>    get_next_seq_block function.

This looks good, but one question - does it really behave with random
IO, when the random map is enabled? I set write_iolog and looked at the
patterns. From the beginning:

foo.1.0 add
foo.1.0 open 
foo.1.0 read 8093696 4096
foo.1.0 read 99356672 4096
foo.1.0 read 113164288 4096
[...]
foo.1.0 close
foo.1.0 open 
foo.1.0 read 8093696 4096
foo.1.0 read 99356672 4096
foo.1.0 read 113164288 4096
[...]

etc. So it's definitely repeating the same sequence there. We don't want
to close/open the file for this case either, how does the below
look/work for you? It's your patch, and an update in
get_next_rand_block() to handle this case too.

diff --git a/backend.c b/backend.c
index 7343286..1d9b0a2 100644
--- a/backend.c
+++ b/backend.c
@@ -555,7 +555,8 @@ static void do_io(struct thread_data *td)
 		td_set_runstate(td, TD_RUNNING);
 
 	while ((td->o.read_iolog_file && !flist_empty(&td->io_log_list)) ||
-		(!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td)) {
+		(!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td) ||
+		td->o.time_based) {
 		struct timeval comp_time;
 		unsigned long bytes_done[2] = { 0, 0 };
 		int min_evts = 0;
diff --git a/io_u.c b/io_u.c
index 20794c3..3bda0e6 100644
--- a/io_u.c
+++ b/io_u.c
@@ -238,13 +238,18 @@ ret:
 static int get_next_rand_block(struct thread_data *td, struct fio_file *f,
 			       enum fio_ddir ddir, unsigned long long *b)
 {
-	if (get_next_rand_offset(td, f, ddir, b)) {
-		dprint(FD_IO, "%s: rand offset failed, last=%llu, size=%llu\n",
-				f->file_name, f->last_pos, f->real_file_size);
-		return 1;
+	if (!get_next_rand_offset(td, f, ddir, b))
+		return 0;
+
+	if (td->o.time_based) {
+		fio_file_reset(f);
+		if (!get_next_rand_offset(td, f, ddir, b))
+			return 0;
 	}
 
-	return 0;
+	dprint(FD_IO, "%s: rand offset failed, last=%llu, size=%llu\n",
+			f->file_name, f->last_pos, f->real_file_size);
+	return 1;
 }
 
 static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
@@ -252,6 +257,9 @@ static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
 {
 	assert(ddir_rw(ddir));
 
+	if (f->last_pos >= f->io_size && td->o.time_based)
+		f->last_pos = f->last_pos - f->io_size;
+
 	if (f->last_pos < f->real_file_size) {
 		unsigned long long pos;
 


-- 
Jens Axboe

Re: [PATCH] time_based: Avoid restarting main I/O loop

[Daniel Ehrenberg]

On Fri, Mar 16, 2012 at 3:07 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 03/16/2012 04:12 AM, Dan Ehrenberg wrote:
>> Previously, when fio had written a volume of I/O equal to the size
>> argument, it restarted the main do_io loop.
>>
>> This patch allows time_based tests to be run for longer than one
>> cycle in the do_io main loop. This has a couple of advantages:
>>  * The random number generator is not reset on each iteration
>>    of the loop, so running longer will reach different locations.
>>  * There is not a throughput-reducing point where all operations
>>    must be reaped before new operations are submitted.
>>
>> The implementation consists of two minor changes:
>>  * In the do_io loop, a time_based test will not exit the loop for
>>    reading or writing too much data.
>>  * When reading or writing sequentially, the operations wrap around
>>    to the beginning after reading the end within the
>>    get_next_seq_block function.
>
> This looks good, but one question - does it really behave with random
> IO, when the random map is enabled? I set write_iolog and looked at the
> patterns. From the beginning:
>
> foo.1.0 add
> foo.1.0 open
> foo.1.0 read 8093696 4096
> foo.1.0 read 99356672 4096
> foo.1.0 read 113164288 4096
> [...]
> foo.1.0 close
> foo.1.0 open
> foo.1.0 read 8093696 4096
> foo.1.0 read 99356672 4096
> foo.1.0 read 113164288 4096
> [...]
>
> etc. So it's definitely repeating the same sequence there. We don't want
> to close/open the file for this case either, how does the below
> look/work for you? It's your patch, and an update in
> get_next_rand_block() to handle this case too.
>
> diff --git a/backend.c b/backend.c
> index 7343286..1d9b0a2 100644
> --- a/backend.c
> +++ b/backend.c
> @@ -555,7 +555,8 @@ static void do_io(struct thread_data *td)
>                td_set_runstate(td, TD_RUNNING);
>
>        while ((td->o.read_iolog_file && !flist_empty(&td->io_log_list)) ||
> -               (!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td)) {
> +               (!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td) ||
> +               td->o.time_based) {
>                struct timeval comp_time;
>                unsigned long bytes_done[2] = { 0, 0 };
>                int min_evts = 0;
> diff --git a/io_u.c b/io_u.c
> index 20794c3..3bda0e6 100644
> --- a/io_u.c
> +++ b/io_u.c
> @@ -238,13 +238,18 @@ ret:
>  static int get_next_rand_block(struct thread_data *td, struct fio_file *f,
>                               enum fio_ddir ddir, unsigned long long *b)
>  {
> -       if (get_next_rand_offset(td, f, ddir, b)) {
> -               dprint(FD_IO, "%s: rand offset failed, last=%llu, size=%llu\n",
> -                               f->file_name, f->last_pos, f->real_file_size);
> -               return 1;
> +       if (!get_next_rand_offset(td, f, ddir, b))
> +               return 0;
> +
> +       if (td->o.time_based) {
> +               fio_file_reset(f);
> +               if (!get_next_rand_offset(td, f, ddir, b))
> +                       return 0;
>        }
>
> -       return 0;
> +       dprint(FD_IO, "%s: rand offset failed, last=%llu, size=%llu\n",
> +                       f->file_name, f->last_pos, f->real_file_size);
> +       return 1;
>  }
>
>  static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
> @@ -252,6 +257,9 @@ static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
>  {
>        assert(ddir_rw(ddir));
>
> +       if (f->last_pos >= f->io_size && td->o.time_based)
> +               f->last_pos = f->last_pos - f->io_size;
> +
>        if (f->last_pos < f->real_file_size) {
>                unsigned long long pos;
>
>
>
> --
> Jens Axboe
>

Thanks for the correction, Jens. I only tested my code with
norandommap, forgetting about the default case. Your modified patch
seems to fix the issue in a nice simple way. I would be happy to see
this new patch committed.

Dan

Re: [PATCH] time_based: Avoid restarting main I/O loop

[Jens Axboe]

On 2012-03-16 18:46, Daniel Ehrenberg wrote:
> On Fri, Mar 16, 2012 at 3:07 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 03/16/2012 04:12 AM, Dan Ehrenberg wrote:
>>> Previously, when fio had written a volume of I/O equal to the size
>>> argument, it restarted the main do_io loop.
>>>
>>> This patch allows time_based tests to be run for longer than one
>>> cycle in the do_io main loop. This has a couple of advantages:
>>>  * The random number generator is not reset on each iteration
>>>    of the loop, so running longer will reach different locations.
>>>  * There is not a throughput-reducing point where all operations
>>>    must be reaped before new operations are submitted.
>>>
>>> The implementation consists of two minor changes:
>>>  * In the do_io loop, a time_based test will not exit the loop for
>>>    reading or writing too much data.
>>>  * When reading or writing sequentially, the operations wrap around
>>>    to the beginning after reading the end within the
>>>    get_next_seq_block function.
>>
>> This looks good, but one question - does it really behave with random
>> IO, when the random map is enabled? I set write_iolog and looked at the
>> patterns. From the beginning:
>>
>> foo.1.0 add
>> foo.1.0 open
>> foo.1.0 read 8093696 4096
>> foo.1.0 read 99356672 4096
>> foo.1.0 read 113164288 4096
>> [...]
>> foo.1.0 close
>> foo.1.0 open
>> foo.1.0 read 8093696 4096
>> foo.1.0 read 99356672 4096
>> foo.1.0 read 113164288 4096
>> [...]
>>
>> etc. So it's definitely repeating the same sequence there. We don't want
>> to close/open the file for this case either, how does the below
>> look/work for you? It's your patch, and an update in
>> get_next_rand_block() to handle this case too.
>>
>> diff --git a/backend.c b/backend.c
>> index 7343286..1d9b0a2 100644
>> --- a/backend.c
>> +++ b/backend.c
>> @@ -555,7 +555,8 @@ static void do_io(struct thread_data *td)
>>                td_set_runstate(td, TD_RUNNING);
>>
>>        while ((td->o.read_iolog_file && !flist_empty(&td->io_log_list)) ||
>> -               (!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td)) {
>> +               (!flist_empty(&td->trim_list)) || !io_bytes_exceeded(td) ||
>> +               td->o.time_based) {
>>                struct timeval comp_time;
>>                unsigned long bytes_done[2] = { 0, 0 };
>>                int min_evts = 0;
>> diff --git a/io_u.c b/io_u.c
>> index 20794c3..3bda0e6 100644
>> --- a/io_u.c
>> +++ b/io_u.c
>> @@ -238,13 +238,18 @@ ret:
>>  static int get_next_rand_block(struct thread_data *td, struct fio_file *f,
>>                               enum fio_ddir ddir, unsigned long long *b)
>>  {
>> -       if (get_next_rand_offset(td, f, ddir, b)) {
>> -               dprint(FD_IO, "%s: rand offset failed, last=%llu, size=%llu\n",
>> -                               f->file_name, f->last_pos, f->real_file_size);
>> -               return 1;
>> +       if (!get_next_rand_offset(td, f, ddir, b))
>> +               return 0;
>> +
>> +       if (td->o.time_based) {
>> +               fio_file_reset(f);
>> +               if (!get_next_rand_offset(td, f, ddir, b))
>> +                       return 0;
>>        }
>>
>> -       return 0;
>> +       dprint(FD_IO, "%s: rand offset failed, last=%llu, size=%llu\n",
>> +                       f->file_name, f->last_pos, f->real_file_size);
>> +       return 1;
>>  }
>>
>>  static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
>> @@ -252,6 +257,9 @@ static int get_next_seq_block(struct thread_data *td, struct fio_file *f,
>>  {
>>        assert(ddir_rw(ddir));
>>
>> +       if (f->last_pos >= f->io_size && td->o.time_based)
>> +               f->last_pos = f->last_pos - f->io_size;
>> +
>>        if (f->last_pos < f->real_file_size) {
>>                unsigned long long pos;
>>
>>
>>
>> --
>> Jens Axboe
>>
> 
> Thanks for the correction, Jens. I only tested my code with
> norandommap, forgetting about the default case. Your modified patch
> seems to fix the issue in a nice simple way. I would be happy to see
> this new patch committed.

Thanks for confirming, I will commit the above variant.

-- 
Jens Axboe