Add extra_buff_count flag

Add extra_buff_count flag

[Radha Ramachandran]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

Hi,
I was trying to use the async_verify option. Looks like if we use this
option for synchronous I/O, then it really doesnt do anything as after
every I/O is completed, we still need the asynchronous verify thread
to complete the verification and release the io_u so the main thread
can allocate this io_u for the next I/O. So to remove this bottle neck
I added a new option extra_buff_count. This takes an integer and the
code will allocate that many more extra io_us. This way when the main
thread has completed the I/O it will have extra io_u and buffers to
issue more I/Os while the asynchronous verify threads do their job.
This can be used with both synchronous and libaio contexts.
Thanks
-radha

[-- Attachment #2: extra_buff_count.patch --]
[-- Type: text/x-diff, Size: 3305 bytes --]

diff --git a/HOWTO b/HOWTO
index cdd6473..71efd10 100644
--- a/HOWTO
+++ b/HOWTO
@@ -872,7 +872,12 @@ verify_async=int	Fio will normally verify IO inline from the submitting
 verify_async_cpus=str	Tell fio to set the given CPU affinity on the
 		async IO verification threads. See cpus_allowed for the
 		format used.
-		
+
+extra_buff_count=int	Allocate extra I/O buffers to continue issuing
+		IO to the file/device. This can be used to keep the device
+		busy with I/Os while the data verification is being
+		done asynchronously.
+
 stonewall	Wait for preceeding jobs in the job file to exit, before
 		starting this one. Can be used to insert serialization
 		points in the job file. A stone wall also implies starting
diff --git a/fio.1 b/fio.1
index 4445d0a..8bee99d 100644
--- a/fio.1
+++ b/fio.1
@@ -627,6 +627,11 @@ allows them to have IO in flight while verifies are running.
 Tell fio to set the given CPU affinity on the async IO verification threads.
 See \fBcpus_allowed\fP for the format used.
 .TP
+.BI extra_buff_count \fR=\fPint
+Allocate extra I/O buffers to continue issuing IO to the file/device.
+This can be used to keep the device busy with I/Os while the data verification
+is being done asynchronously.
+.TP
 .B stonewall
 Wait for preceeding jobs in the job file to exit before starting this one.
 \fBstonewall\fR implies \fBnew_group\fR.
diff --git a/fio.c b/fio.c
index 8f8bb56..ff8d0e7 100644
--- a/fio.c
+++ b/fio.c
@@ -534,7 +534,8 @@ sync_done:
 		 * if we can queue more, do so. but check if there are
 		 * completed io_u's first.
 		 */
-		full = queue_full(td) || ret == FIO_Q_BUSY;
+		full = queue_full(td) || ret == FIO_Q_BUSY ||
+			td->cur_depth == td->o.iodepth;
 		if (full || !td->o.iodepth_batch_complete) {
 			min_events = td->o.iodepth_batch_complete;
 			if (full && !min_events)
@@ -686,7 +687,8 @@ sync_done:
 		/*
 		 * See if we need to complete some commands
 		 */
-		full = queue_full(td) || ret == FIO_Q_BUSY;
+		full = queue_full(td) || ret == FIO_Q_BUSY ||
+			td->cur_depth == td->o.iodepth;
 		if (full || !td->o.iodepth_batch_complete) {
 			min_evts = td->o.iodepth_batch_complete;
 			if (full && !min_evts)
@@ -787,7 +789,7 @@ static int init_io_u(struct thread_data *td)
 	int cl_align, i, max_units;
 	char *p;
 
-	max_units = td->o.iodepth;
+	max_units = td->o.iodepth + td->o.extra_buff_count;
 	max_bs = max(td->o.max_bs[DDIR_READ], td->o.max_bs[DDIR_WRITE]);
 	td->orig_buffer_size = (unsigned long long) max_bs
 					* (unsigned long long) max_units;
diff --git a/fio.h b/fio.h
index e4ed76f..833a03b 100644
--- a/fio.h
+++ b/fio.h
@@ -181,6 +181,7 @@ struct thread_options {
 	unsigned int verify_pattern_bytes;
 	unsigned int verify_fatal;
 	unsigned int verify_async;
+	unsigned int extra_buff_count;
 	unsigned int use_thread;
 	unsigned int unlink;
 	unsigned int do_disk_util;
diff --git a/options.c b/options.c
index 5b88255..71f26fb 100644
--- a/options.c
+++ b/options.c
@@ -1337,6 +1337,13 @@ static struct fio_option options[] = {
 	},
 #endif
 	{
+		.name	= "extra_buff_count",
+		.type	= FIO_OPT_INT,
+		.off1	= td_var_offset(extra_buff_count),
+		.def	= "0",
+		.help	= "Number of extra buffers to allocate",
+	},
+	{
 		.name	= "write_iolog",
 		.type	= FIO_OPT_STR_STORE,
 		.off1	= td_var_offset(write_iolog_file),

Re: Add extra_buff_count flag

[Jens Axboe]

On Tue, Nov 03 2009, Radha Ramachandran wrote:
> Hi,
> I was trying to use the async_verify option. Looks like if we use this
> option for synchronous I/O, then it really doesnt do anything as after
> every I/O is completed, we still need the asynchronous verify thread
> to complete the verification and release the io_u so the main thread
> can allocate this io_u for the next I/O. So to remove this bottle neck
> I added a new option extra_buff_count. This takes an integer and the
> code will allocate that many more extra io_us. This way when the main
> thread has completed the I/O it will have extra io_u and buffers to
> issue more I/Os while the asynchronous verify threads do their job.
> This can be used with both synchronous and libaio contexts.

Does iodepth=x not work for that? If not, I suggest we fix that instead
of adding a new parameter for it.

-- 
Jens Axboe

Re: Add extra_buff_count flag

[Radha Ramachandran]

Yes, iodepth=x works. I did not realise that I could use iodepth with
sync engine as well.
Thanks
-radha

On Tue, Nov 3, 2009 at 11:35 PM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Tue, Nov 03 2009, Radha Ramachandran wrote:
>> Hi,
>> I was trying to use the async_verify option. Looks like if we use this
>> option for synchronous I/O, then it really doesnt do anything as after
>> every I/O is completed, we still need the asynchronous verify thread
>> to complete the verification and release the io_u so the main thread
>> can allocate this io_u for the next I/O. So to remove this bottle neck
>> I added a new option extra_buff_count. This takes an integer and the
>> code will allocate that many more extra io_us. This way when the main
>> thread has completed the I/O it will have extra io_u and buffers to
>> issue more I/Os while the asynchronous verify threads do their job.
>> This can be used with both synchronous and libaio contexts.
>
> Does iodepth=x not work for that? If not, I suggest we fix that instead
> of adding a new parameter for it.
>
> --
> Jens Axboe
>
> --
> To unsubscribe from this list: send the line "unsubscribe fio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Add extra_buff_count flag

[Jens Axboe]

On Wed, Nov 04 2009, Radha Ramachandran wrote:
> Yes, iodepth=x works. I did not realise that I could use iodepth with
> sync engine as well.

Good, we should probably default iodepth to verify_async + 1, if a
higher number isn't given. And/or document that you probably want to do
this.

-- 
Jens Axboe

Re: Add extra_buff_count flag

[Radha Ramachandran]

I would rather document it than add it by default in case it starts
hitting memory constraints because its allocating more memory buffers.

Also Iam having difficulty running verify_async/libaio engine/iodepth
and iodepth_batch_complete options together, I will look into it to
see whats causing the issue.

thanks
-radha


On Wed, Nov 4, 2009 at 9:29 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Wed, Nov 04 2009, Radha Ramachandran wrote:
>> Yes, iodepth=x works. I did not realise that I could use iodepth with
>> sync engine as well.
>
> Good, we should probably default iodepth to verify_async + 1, if a
> higher number isn't given. And/or document that you probably want to do
> this.
>
> --
> Jens Axboe
>
>

Re: Add extra_buff_count flag

[Jens Axboe]

On Wed, Nov 04 2009, Radha Ramachandran wrote:
> I would rather document it than add it by default in case it starts
> hitting memory constraints because its allocating more memory buffers.

Yes I agree, I usually don't like having one option implying changes for
another either.

> Also Iam having difficulty running verify_async/libaio engine/iodepth
> and iodepth_batch_complete options together, I will look into it to
> see whats causing the issue.

OK, I saw some issues on what appears to be races on io_u->flags today,
but I haven't investigated that yet.

-- 
Jens Axboe

Re: Add extra_buff_count flag

[Jens Axboe]

On Wed, Nov 04 2009, Jens Axboe wrote:
> On Wed, Nov 04 2009, Radha Ramachandran wrote:
> > I would rather document it than add it by default in case it starts
> > hitting memory constraints because its allocating more memory buffers.
> 
> Yes I agree, I usually don't like having one option implying changes for
> another either.

So the problem with documentation is that usually nobody reads it. This
is what the HOWTO/man page currently has:

verify_async=int        Fio will normally verify IO inline from the submitting
                thread. This option takes an integer describing how many
                async offload threads to create for IO verification instead,
                causing fio to offload the duty of verifying IO contents
                to one or more separate threads. If using this offload
                option, even sync IO engines can benefit from using an
                iodepth setting higher than 1, as it allows them to have
                IO in flight while verifies are running.

It's already documented...

-- 
Jens Axboe

Re: Add extra_buff_count flag

[Radha Ramachandran]

Thank you for the documentation update.

You also mentioned that you saw some kind of a race on io_u->flags
today, do you by any chance know if you were using iodepth_low or
iodepth_batch_complete or libaio engine options.
I think I see an issue when using them and understand why it happens,
but dont have a clean fix yet, will hopefully have one soon. I was
wondering if its the same issue you are seeing.
Basically the issue is we might think the queue is full (because we
cannot allocate any more io_u (they are probably doing async verify)),
but the code assumes that if the queue is full, then there is atleast
one I/O that we can do "io_getevents" on. And that will cause a hang
in the code.

thanks
-radha


On Wed, Nov 4, 2009 at 11:22 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Wed, Nov 04 2009, Jens Axboe wrote:
>> On Wed, Nov 04 2009, Radha Ramachandran wrote:
>> > I would rather document it than add it by default in case it starts
>> > hitting memory constraints because its allocating more memory buffers.
>>
>> Yes I agree, I usually don't like having one option implying changes for
>> another either.
>
> So the problem with documentation is that usually nobody reads it. This
> is what the HOWTO/man page currently has:
>
> verify_async=int        Fio will normally verify IO inline from the submitting
>                thread. This option takes an integer describing how many
>                async offload threads to create for IO verification instead,
>                causing fio to offload the duty of verifying IO contents
>                to one or more separate threads. If using this offload
>                option, even sync IO engines can benefit from using an
>                iodepth setting higher than 1, as it allows them to have
>                IO in flight while verifies are running.
>
> It's already documented...
>
> --
> Jens Axboe
>
>

Re: Add extra_buff_count flag

[Jens Axboe]

On Wed, Nov 04 2009, Radha Ramachandran wrote:
> Thank you for the documentation update.

I haven't updated it, it was already there :-)

> You also mentioned that you saw some kind of a race on io_u->flags
> today, do you by any chance know if you were using iodepth_low or
> iodepth_batch_complete or libaio engine options.
> I think I see an issue when using them and understand why it happens,
> but dont have a clean fix yet, will hopefully have one soon. I was
> wondering if its the same issue you are seeing.
> Basically the issue is we might think the queue is full (because we
> cannot allocate any more io_u (they are probably doing async verify)),
> but the code assumes that if the queue is full, then there is atleast
> one I/O that we can do "io_getevents" on. And that will cause a hang
> in the code.

I didn't use libaio, I can reproduce it with the sync engine directly
and much easier if using fast "null" verifies. It triggers this assert
in put_io_u():

        assert((io_u->flags & IO_U_F_FREE) == 0);

and this in __get_io_u():

                assert(io_u->flags & IO_U_F_FREE);

The former I think is just a bug, it's likely a reput or something, but
not sure yet. The latter looks like a race on the flags, since it isn't
always locked down when manipulated.

So it sounds like it's two separate issues, I'd urge you to continue
debugging yours and I'll fix this one tomorrow.

-- 
Jens Axboe

Re: Add extra_buff_count flag

[Jens Axboe]

On Wed, Nov 04 2009, Jens Axboe wrote:
> > You also mentioned that you saw some kind of a race on io_u->flags
> > today, do you by any chance know if you were using iodepth_low or
> > iodepth_batch_complete or libaio engine options.
> > I think I see an issue when using them and understand why it happens,
> > but dont have a clean fix yet, will hopefully have one soon. I was
> > wondering if its the same issue you are seeing.
> > Basically the issue is we might think the queue is full (because we
> > cannot allocate any more io_u (they are probably doing async verify)),
> > but the code assumes that if the queue is full, then there is atleast
> > one I/O that we can do "io_getevents" on. And that will cause a hang
> > in the code.
> 
> I didn't use libaio, I can reproduce it with the sync engine directly
> and much easier if using fast "null" verifies. It triggers this assert
> in put_io_u():
> 
>         assert((io_u->flags & IO_U_F_FREE) == 0);
> 
> and this in __get_io_u():
> 
>                 assert(io_u->flags & IO_U_F_FREE);
> 
> The former I think is just a bug, it's likely a reput or something, but
> not sure yet. The latter looks like a race on the flags, since it isn't
> always locked down when manipulated.

I think this is fixed now, committed a patch for it.

-- 
Jens Axboe

Re: Add extra_buff_count flag

[Radha Ramachandran]

Hi,
This is the patch to fix the race condition when
iodepth_batch_complete and verify_async are used. The fix prevents the
code from waiting for more I/Os than were actually submitted.

diff --git a/fio.c b/fio.c
index debcac5..22eba49 100644
--- a/fio.c
+++ b/fio.c
@@ -536,7 +536,8 @@ sync_done:
                 */
                full = queue_full(td) || ret == FIO_Q_BUSY;
                if (full || !td->o.iodepth_batch_complete) {
-                       min_events = td->o.iodepth_batch_complete;
+                       min_events = min(td->o.iodepth_batch_complete,
+                                        td->cur_depth);
                        if (full && !min_events)
                                min_events = 1;

@@ -688,7 +689,8 @@ sync_done:
                 */
                full = queue_full(td) || ret == FIO_Q_BUSY;
                if (full || !td->o.iodepth_batch_complete) {
-                       min_evts = td->o.iodepth_batch_complete;
+                       min_evts = min(td->o.iodepth_batch_complete,
+                                        td->cur_depth);
                        if (full && !min_evts)
                                min_evts = 1;

thanks
-radha



On Wed, Nov 4, 2009 at 12:01 PM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Wed, Nov 04 2009, Jens Axboe wrote:
>> > You also mentioned that you saw some kind of a race on io_u->flags
>> > today, do you by any chance know if you were using iodepth_low or
>> > iodepth_batch_complete or libaio engine options.
>> > I think I see an issue when using them and understand why it happens,
>> > but dont have a clean fix yet, will hopefully have one soon. I was
>> > wondering if its the same issue you are seeing.
>> > Basically the issue is we might think the queue is full (because we
>> > cannot allocate any more io_u (they are probably doing async verify)),
>> > but the code assumes that if the queue is full, then there is atleast
>> > one I/O that we can do "io_getevents" on. And that will cause a hang
>> > in the code.
>>
>> I didn't use libaio, I can reproduce it with the sync engine directly
>> and much easier if using fast "null" verifies. It triggers this assert
>> in put_io_u():
>>
>>         assert((io_u->flags & IO_U_F_FREE) == 0);
>>
>> and this in __get_io_u():
>>
>>                 assert(io_u->flags & IO_U_F_FREE);
>>
>> The former I think is just a bug, it's likely a reput or something, but
>> not sure yet. The latter looks like a race on the flags, since it isn't
>> always locked down when manipulated.
>
> I think this is fixed now, committed a patch for it.
>
> --
> Jens Axboe
>
>

Re: Add extra_buff_count flag

[Jens Axboe]

On Wed, Nov 04 2009, Radha Ramachandran wrote:
> Hi,
> This is the patch to fix the race condition when
> iodepth_batch_complete and verify_async are used. The fix prevents the
> code from waiting for more I/Os than were actually submitted.

Ah thanks. So it wasn't quite a race, just a bug. I will apply it asap.

-- 
Jens Axboe