Rip out verify_backlog support?

Rip out verify_backlog support?

[Grant Grundler]

Today, fio has two distinct phases of operation: workload and then verify.

But there is this hack which is in-between those two: verify_backlog
which makes things a lot more complicated. This hack was added to
limit the amount of memory needed to track the IOs that needed to be
verify. I'm going to argue "verify_each_loop" could do the same thing
and keep fio internals simpler (strictly two phases). If the goal is
to have longer running, well defined workloads that can be verified,
then verifying after each iteration makes more sense.  In other words,
the jobs should define a workload limit (amount of IO or time) and
then iterate that constraint as many times as they want to reach the
duration they want.

Thoughts?

thanks,
grant

Re: Rip out verify_backlog support?

[Jens Axboe]

On Wed, Feb 05 2014, Grant Grundler wrote:
> Today, fio has two distinct phases of operation: workload and then verify.
> 
> But there is this hack which is in-between those two: verify_backlog
> which makes things a lot more complicated. This hack was added to
> limit the amount of memory needed to track the IOs that needed to be
> verify. I'm going to argue "verify_each_loop" could do the same thing
> and keep fio internals simpler (strictly two phases). If the goal is
> to have longer running, well defined workloads that can be verified,
> then verifying after each iteration makes more sense.  In other words,
> the jobs should define a workload limit (amount of IO or time) and
> then iterate that constraint as many times as they want to reach the
> duration they want.
> 
> Thoughts?

We've actually caught actual bugs with the verify_backlog in the past,
where you want verify closer to when the write has happened. So I'd
prefer if we fix those up instead.

For memory reduction, I think the experimental_verify is the way to go.
Basically have roll back support for any of the generated offsets etc,
so we don't need to track it at all.

-- 
Jens Axboe

Re: Rip out verify_backlog support?

[Grant Grundler]

On Wed, Feb 5, 2014 at 11:32 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On Wed, Feb 05 2014, Grant Grundler wrote:
>> Today, fio has two distinct phases of operation: workload and then verify.
>>
>> But there is this hack which is in-between those two: verify_backlog
>> which makes things a lot more complicated. This hack was added to
>> limit the amount of memory needed to track the IOs that needed to be
>> verify. I'm going to argue "verify_each_loop" could do the same thing
>> and keep fio internals simpler (strictly two phases). If the goal is
>> to have longer running, well defined workloads that can be verified,
>> then verifying after each iteration makes more sense.  In other words,
>> the jobs should define a workload limit (amount of IO or time) and
>> then iterate that constraint as many times as they want to reach the
>> duration they want.
>>
>> Thoughts?
>
> We've actually caught actual bugs with the verify_backlog in the past,
> where you want verify closer to when the write has happened.

Couldn't one shorten the defined workload and use bigger loop= instead?

> So I'd prefer if we fix those up instead.

Ok.

> For memory reduction, I think the experimental_verify is the way to go.
> Basically have roll back support for any of the generated offsets etc,
> so we don't need to track it at all.

But in order to have strong verification without tracking IOs we need
to log the order that the IOs are issued, not the order they complete.
Verify_backlog today requires logging IO in the order they are
completed. The problem is synchronizing the thread(s) that perform IO
vs the thread(s) that perform verification so verification isn't
attempted on IO that isn't complete (but is "issued" and thus logged
"in order issued"). The complication is IOs generally don't complete
in the order issued.

Make more sense why I don't like verify backlog?

And if we get "verify backlog" working correctly, then we don't need
two distinct phases anymore. So perhaps remove that instead.

thanks,
grant

Re: Rip out verify_backlog support?

[Jens Axboe]

On Wed, Feb 05 2014, Grant Grundler wrote:
> On Wed, Feb 5, 2014 at 11:32 AM, Jens Axboe <axboe@kernel.dk> wrote:
> > On Wed, Feb 05 2014, Grant Grundler wrote:
> >> Today, fio has two distinct phases of operation: workload and then verify.
> >>
> >> But there is this hack which is in-between those two: verify_backlog
> >> which makes things a lot more complicated. This hack was added to
> >> limit the amount of memory needed to track the IOs that needed to be
> >> verify. I'm going to argue "verify_each_loop" could do the same thing
> >> and keep fio internals simpler (strictly two phases). If the goal is
> >> to have longer running, well defined workloads that can be verified,
> >> then verifying after each iteration makes more sense.  In other words,
> >> the jobs should define a workload limit (amount of IO or time) and
> >> then iterate that constraint as many times as they want to reach the
> >> duration they want.
> >>
> >> Thoughts?
> >
> > We've actually caught actual bugs with the verify_backlog in the past,
> > where you want verify closer to when the write has happened.
> 
> Couldn't one shorten the defined workload and use bigger loop= instead?

That's not really the same, typically you'd end up re-running the same
thing over and over then.

> > So I'd prefer if we fix those up instead.
> 
> Ok.
> 
> > For memory reduction, I think the experimental_verify is the way to go.
> > Basically have roll back support for any of the generated offsets etc,
> > so we don't need to track it at all.
> 
> But in order to have strong verification without tracking IOs we need
> to log the order that the IOs are issued, not the order they complete.

My point is that you don't need to log it at all. Lets say you have a
backlog of 500. After you have issued 500 writes, you simply reset your
LFSR (or random generators) and re-run the same sequence as reads.

> Verify_backlog today requires logging IO in the order they are
> completed. The problem is synchronizing the thread(s) that perform IO
> vs the thread(s) that perform verification so verification isn't
> attempted on IO that isn't complete (but is "issued" and thus logged
> "in order issued"). The complication is IOs generally don't complete
> in the order issued.

For verify_backlog, obviously we cannot rollback the generators without
having other synchronization between the writers and readers. So logging
still makes more sense for that, I think. We'll just have them log in
issue order, that doesn't seem like a big deal.

> Make more sense why I don't like verify backlog?
>
> And if we get "verify backlog" working correctly, then we don't need
> two distinct phases anymore. So perhaps remove that instead.

That'd be fine.

-- 
Jens Axboe

Re: Rip out verify_backlog support?

[Grant Grundler]

On Wed, Feb 5, 2014 at 11:53 AM, Jens Axboe <axboe@kernel.dk> wrote:
...
>> Couldn't one shorten the defined workload and use bigger loop= instead?
>
> That's not really the same, typically you'd end up re-running the same
> thing over and over then.

Yes...unless we use a different seed on each loop. Or don't even
attempt to loop in FIO - but rather outside of FIO by the test
framework - e.g. autotest or other scripting language.

>> But in order to have strong verification without tracking IOs we need
>> to log the order that the IOs are issued, not the order they complete.
>
> My point is that you don't need to log it at all. Lets say you have a
> backlog of 500. After you have issued 500 writes, you simply reset your
> LFSR (or random generators) and re-run the same sequence as reads.

This is effectively "logging in the order issued" - just don't need to
record the LBA. We still need to track when the IO was issued and
completed. Once those stats are recorded elsewhere, we don't need to
remember the "order" since it can be reproduced.

>> Verify_backlog today requires logging IO in the order they are
>> completed. The problem is synchronizing the thread(s) that perform IO
>> vs the thread(s) that perform verification so verification isn't
>> attempted on IO that isn't complete (but is "issued" and thus logged
>> "in order issued"). The complication is IOs generally don't complete
>> in the order issued.
>
> For verify_backlog, obviously we cannot rollback the generators without
> having other synchronization between the writers and readers. So logging
> still makes more sense for that, I think. We'll just have them log in
> issue order, that doesn't seem like a big deal.

Agreed - but that synchronization doesn't exist today AFAICT.

>> And if we get "verify backlog" working correctly, then we don't need
>> two distinct phases anymore. So perhaps remove that instead.
>
> That'd be fine.

Excellent. I'll see if I can make time to add the synchronization
described above. Seems worth it to me.

thanks for the feedback!
grant

Re: Rip out verify_backlog support?

[Jens Axboe]

On Wed, Feb 05 2014, Grant Grundler wrote:
> On Wed, Feb 5, 2014 at 11:53 AM, Jens Axboe <axboe@kernel.dk> wrote:
> ...
> >> Couldn't one shorten the defined workload and use bigger loop= instead?
> >
> > That's not really the same, typically you'd end up re-running the same
> > thing over and over then.
> 
> Yes...unless we use a different seed on each loop. Or don't even
> attempt to loop in FIO - but rather outside of FIO by the test
> framework - e.g. autotest or other scripting language.

Lots of stuff happens at setup/teardown. It's a lot more efficient to
handle it internally.

So, to summarize, I'd much rather get rid of do_verify() and handle
everything in do_io(), which is what the verify_backlog does. The fact
that fio has two nearly identical loops for IO is weird and fragile. A
"normal" verify_backlog=0 is just a variant of verify_backlog=x with
unlimited backlog, it should fall out nicely. Lets move in that
direction, not kill verify_backlog.

> >> But in order to have strong verification without tracking IOs we need
> >> to log the order that the IOs are issued, not the order they complete.
> >
> > My point is that you don't need to log it at all. Lets say you have a
> > backlog of 500. After you have issued 500 writes, you simply reset your
> > LFSR (or random generators) and re-run the same sequence as reads.
> 
> This is effectively "logging in the order issued" - just don't need to
> record the LBA. We still need to track when the IO was issued and
> completed. Once those stats are recorded elsewhere, we don't need to
> remember the "order" since it can be reproduced.
> 
> >> Verify_backlog today requires logging IO in the order they are
> >> completed. The problem is synchronizing the thread(s) that perform IO
> >> vs the thread(s) that perform verification so verification isn't
> >> attempted on IO that isn't complete (but is "issued" and thus logged
> >> "in order issued"). The complication is IOs generally don't complete
> >> in the order issued.
> >
> > For verify_backlog, obviously we cannot rollback the generators without
> > having other synchronization between the writers and readers. So logging
> > still makes more sense for that, I think. We'll just have them log in
> > issue order, that doesn't seem like a big deal.
> 
> Agreed - but that synchronization doesn't exist today AFAICT.

It does - that's the log_io_piece() mechanism. The writer will generate
on, and verify will read those and verify. We just have to ensure that
it is correct in the way that it is logged. The alternative would be to
rely purely on the generator rollback, and for that you would then need
some specific notification on how far the reader could proceed, if async
verify_backlog is used.

-- 
Jens Axboe

Re: Rip out verify_backlog support?

[Grant Grundler]

On Wed, Feb 5, 2014 at 1:28 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On Wed, Feb 05 2014, Grant Grundler wrote:
>> On Wed, Feb 5, 2014 at 11:53 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> ...
>> >> Couldn't one shorten the defined workload and use bigger loop= instead?
>> >
>> > That's not really the same, typically you'd end up re-running the same
>> > thing over and over then.
>>
>> Yes...unless we use a different seed on each loop. Or don't even
>> attempt to loop in FIO - but rather outside of FIO by the test
>> framework - e.g. autotest or other scripting language.
>
> Lots of stuff happens at setup/teardown. It's a lot more efficient to
> handle it internally.
>
> So, to summarize, I'd much rather get rid of do_verify() and handle
> everything in do_io(), which is what the verify_backlog does. The fact
> that fio has two nearly identical loops for IO is weird and fragile.

Totally agree on the "two nearly identical loops" - I've complained
about that before.

> A "normal" verify_backlog=0 is just a variant of verify_backlog=x with
> unlimited backlog, it should fall out nicely. Lets move in that
> direction, not kill verify_backlog.

Yup - excellent. I've got two other fires right now...but I'll get
back to this later.

>
>> >> But in order to have strong verification without tracking IOs we need
>> >> to log the order that the IOs are issued, not the order they complete.
>> >
>> > My point is that you don't need to log it at all. Lets say you have a
>> > backlog of 500. After you have issued 500 writes, you simply reset your
>> > LFSR (or random generators) and re-run the same sequence as reads.
>>
>> This is effectively "logging in the order issued" - just don't need to
>> record the LBA. We still need to track when the IO was issued and
>> completed. Once those stats are recorded elsewhere, we don't need to
>> remember the "order" since it can be reproduced.
>>
>> >> Verify_backlog today requires logging IO in the order they are
>> >> completed. The problem is synchronizing the thread(s) that perform IO
>> >> vs the thread(s) that perform verification so verification isn't
>> >> attempted on IO that isn't complete (but is "issued" and thus logged
>> >> "in order issued"). The complication is IOs generally don't complete
>> >> in the order issued.
>> >
>> > For verify_backlog, obviously we cannot rollback the generators without
>> > having other synchronization between the writers and readers. So logging
>> > still makes more sense for that, I think. We'll just have them log in
>> > issue order, that doesn't seem like a big deal.
>>
>> Agreed - but that synchronization doesn't exist today AFAICT.
>
> It does - that's the log_io_piece() mechanism. The writer will generate
> on, and verify will read those and verify. We just have to ensure that
> it is correct in the way that it is logged. The alternative would be to
> rely purely on the generator rollback, and for that you would then need
> some specific notification on how far the reader could proceed, if async
> verify_backlog is used.

Yes - I was referring to the "rely purely on generator rollback".

Once log_io_piece() is called, the verify code assumes the IO is
complete...which isn't true if log_io_piece() is used to record "order
issued".

thanks,
grant

>
> --
> Jens Axboe
>

Re: Rip out verify_backlog support?

[Jens Axboe]

On Wed, Feb 05 2014, Grant Grundler wrote:
> > It does - that's the log_io_piece() mechanism. The writer will generate
> > on, and verify will read those and verify. We just have to ensure that
> > it is correct in the way that it is logged. The alternative would be to
> > rely purely on the generator rollback, and for that you would then need
> > some specific notification on how far the reader could proceed, if async
> > verify_backlog is used.
> 
> Yes - I was referring to the "rely purely on generator rollback".
> 
> Once log_io_piece() is called, the verify code assumes the IO is
> complete...which isn't true if log_io_piece() is used to record "order
> issued".

Right, we'd need to ensure the state is accurately known to the
verifier.

-- 
Jens Axboe

Re: Rip out verify_backlog support?

[Jens Axboe]

On Wed, Feb 05 2014, Jens Axboe wrote:
> On Wed, Feb 05 2014, Grant Grundler wrote:
> > > It does - that's the log_io_piece() mechanism. The writer will generate
> > > on, and verify will read those and verify. We just have to ensure that
> > > it is correct in the way that it is logged. The alternative would be to
> > > rely purely on the generator rollback, and for that you would then need
> > > some specific notification on how far the reader could proceed, if async
> > > verify_backlog is used.
> > 
> > Yes - I was referring to the "rely purely on generator rollback".
> > 
> > Once log_io_piece() is called, the verify code assumes the IO is
> > complete...which isn't true if log_io_piece() is used to record "order
> > issued".
> 
> Right, we'd need to ensure the state is accurately known to the
> verifier.

Something like this should work, though I don't like the extra
overhead... Totally untested.

diff --git a/backend.c b/backend.c
index 62fa17c3a209..9ececaa1d5af 100644
--- a/backend.c
+++ b/backend.c
@@ -731,8 +731,7 @@ static uint64_t do_io(struct thread_data *td)
 		if (td_write(td) && io_u->ddir == DDIR_WRITE &&
 		    td->o.do_verify &&
 		    td->o.verify != VERIFY_NONE &&
-		    !td->o.experimental_verify &&
-		    !(td->flags & TD_F_VER_BACKLOG))
+		    !td->o.experimental_verify)
 			log_io_piece(td, io_u);
 
 		ret = td_io_queue(td, io_u);
diff --git a/io_u.c b/io_u.c
index 4264cd54115c..64ff73cd5555 100644
--- a/io_u.c
+++ b/io_u.c
@@ -1285,6 +1285,7 @@ again:
 		io_u->acct_ddir = -1;
 		td->cur_depth++;
 		io_u->flags |= IO_U_F_IN_CUR_DEPTH;
+		io_u->ipo = NULL;
 	} else if (td->o.verify_async) {
 		/*
 		 * We ran out, wait for async verify threads to finish and
@@ -1568,6 +1569,15 @@ static void io_completed(struct thread_data *td, struct io_u *io_u,
 	td_io_u_lock(td);
 	assert(io_u->flags & IO_U_F_FLIGHT);
 	io_u->flags &= ~(IO_U_F_FLIGHT | IO_U_F_BUSY_OK);
+
+	/*
+	 * Mark IO ok to verify
+	 */
+	if (io_u->ipo) {
+		io_u->ipo->flags &= ~IP_F_IN_FLIGHT;
+		write_barrier();
+	}
+
 	td_io_u_unlock(td);
 
 	if (ddir_sync(io_u->ddir)) {
@@ -1623,17 +1633,6 @@ static void io_completed(struct thread_data *td, struct io_u *io_u,
 					 utime_since_now(&td->start));
 		}
 
-		/*
-		 * Verify_backlog enable: We need to log the write job after
-		 * finishing it to prevent verifying before finish writing.
-		 */
-		if (td_write(td) && idx == DDIR_WRITE &&
-		    td->o.do_verify &&
-		    td->o.verify != VERIFY_NONE &&
-		    !td->o.experimental_verify &&
-		    (td->flags & TD_F_VER_BACKLOG))
-			log_io_piece(td, io_u);
-
 		icd->bytes_done[idx] += bytes;
 
 		if (io_u->end_io) {
diff --git a/ioengine.h b/ioengine.h
index 0756bc7e6c13..37627bb1dc76 100644
--- a/ioengine.h
+++ b/ioengine.h
@@ -71,6 +71,8 @@ struct io_u {
 	 */
 	unsigned long buf_filled_len;
 
+	struct io_piece *ipo;
+
 	union {
 #ifdef CONFIG_LIBAIO
 		struct iocb iocb;
diff --git a/iolog.c b/iolog.c
index 017b235c217a..5fd9416c036e 100644
--- a/iolog.c
+++ b/iolog.c
@@ -189,6 +189,9 @@ void log_io_piece(struct thread_data *td, struct io_u *io_u)
 	ipo->offset = io_u->offset;
 	ipo->len = io_u->buflen;
 	ipo->numberio = io_u->numberio;
+	ipo->flags = IP_F_IN_FLIGHT;
+
+	io_u->ipo = ipo;
 
 	if (io_u_should_trim(td, io_u)) {
 		flist_add_tail(&ipo->trim_list, &td->trim_list);
diff --git a/iolog.h b/iolog.h
index 321576dbe611..3ec48f2100fe 100644
--- a/iolog.h
+++ b/iolog.h
@@ -67,6 +67,7 @@ enum {
 	IP_F_ONRB	= 1,
 	IP_F_ONLIST	= 2,
 	IP_F_TRIMMED	= 4,
+	IP_F_IN_FLIGHT	= 8,
 };
 
 /*
diff --git a/verify.c b/verify.c
index 90cd093add1f..93731228f1b6 100644
--- a/verify.c
+++ b/verify.c
@@ -1022,11 +1022,27 @@ int get_next_verify(struct thread_data *td, struct io_u *io_u)
 		struct rb_node *n = rb_first(&td->io_hist_tree);
 
 		ipo = rb_entry(n, struct io_piece, rb_node);
+
+		/*
+		 * Ensure that the associated IO has completed
+		 */
+		read_barrier();
+		if (ipo->flags & IP_F_IN_FLIGHT)
+			goto nothing;
+
 		rb_erase(n, &td->io_hist_tree);
 		assert(ipo->flags & IP_F_ONRB);
 		ipo->flags &= ~IP_F_ONRB;
 	} else if (!flist_empty(&td->io_hist_list)) {
 		ipo = flist_entry(td->io_hist_list.next, struct io_piece, list);
+
+		/*
+		 * Ensure that the associated IO has completed
+		 */
+		read_barrier();
+		if (ipo->flags & IP_F_IN_FLIGHT)
+			goto nothing;
+
 		flist_del(&ipo->list);
 		assert(ipo->flags & IP_F_ONLIST);
 		ipo->flags &= ~IP_F_ONLIST;
@@ -1072,6 +1088,7 @@ int get_next_verify(struct thread_data *td, struct io_u *io_u)
 		return 0;
 	}
 
+nothing:
 	dprint(FD_VERIFY, "get_next_verify: empty\n");
 	return 1;
 }

-- 
Jens Axboe

Re: Rip out verify_backlog support?

[Puthikorn Voravootivat]

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

That fix runs fine with my test script (attached), but I think you
might want to remove or reword this comment.

diff --git a/backend.c b/backend.c
index 9ececaa..99f9065 100644
--- a/backend.c
+++ b/backend.c
@@ -724,10 +724,6 @@ static uint64_t do_io(struct thread_data *td)
                else
                        td_set_runstate(td, TD_RUNNING);

-               /*
-                * Verify_backlog disabled: We need to log rand seed before the
-                * actual IO to be able to replay it correctly in the
verify phase.
-                */
                if (td_write(td) && io_u->ddir == DDIR_WRITE &&
                    td->o.do_verify &&
                    td->o.verify != VERIFY_NONE &&


On Wed, Feb 5, 2014 at 7:34 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On Wed, Feb 05 2014, Jens Axboe wrote:
>> On Wed, Feb 05 2014, Grant Grundler wrote:
>> > > It does - that's the log_io_piece() mechanism. The writer will generate
>> > > on, and verify will read those and verify. We just have to ensure that
>> > > it is correct in the way that it is logged. The alternative would be to
>> > > rely purely on the generator rollback, and for that you would then need
>> > > some specific notification on how far the reader could proceed, if async
>> > > verify_backlog is used.
>> >
>> > Yes - I was referring to the "rely purely on generator rollback".
>> >
>> > Once log_io_piece() is called, the verify code assumes the IO is
>> > complete...which isn't true if log_io_piece() is used to record "order
>> > issued".
>>
>> Right, we'd need to ensure the state is accurately known to the
>> verifier.
>
> Something like this should work, though I don't like the extra
> overhead... Totally untested.
>
> diff --git a/backend.c b/backend.c
> index 62fa17c3a209..9ececaa1d5af 100644
> --- a/backend.c
> +++ b/backend.c
> @@ -731,8 +731,7 @@ static uint64_t do_io(struct thread_data *td)
>                 if (td_write(td) && io_u->ddir == DDIR_WRITE &&
>                     td->o.do_verify &&
>                     td->o.verify != VERIFY_NONE &&
> -                   !td->o.experimental_verify &&
> -                   !(td->flags & TD_F_VER_BACKLOG))
> +                   !td->o.experimental_verify)
>                         log_io_piece(td, io_u);
>
>                 ret = td_io_queue(td, io_u);
> diff --git a/io_u.c b/io_u.c
> index 4264cd54115c..64ff73cd5555 100644
> --- a/io_u.c
> +++ b/io_u.c
> @@ -1285,6 +1285,7 @@ again:
>                 io_u->acct_ddir = -1;
>                 td->cur_depth++;
>                 io_u->flags |= IO_U_F_IN_CUR_DEPTH;
> +               io_u->ipo = NULL;
>         } else if (td->o.verify_async) {
>                 /*
>                  * We ran out, wait for async verify threads to finish and
> @@ -1568,6 +1569,15 @@ static void io_completed(struct thread_data *td, struct io_u *io_u,
>         td_io_u_lock(td);
>         assert(io_u->flags & IO_U_F_FLIGHT);
>         io_u->flags &= ~(IO_U_F_FLIGHT | IO_U_F_BUSY_OK);
> +
> +       /*
> +        * Mark IO ok to verify
> +        */
> +       if (io_u->ipo) {
> +               io_u->ipo->flags &= ~IP_F_IN_FLIGHT;
> +               write_barrier();
> +       }
> +
>         td_io_u_unlock(td);
>
>         if (ddir_sync(io_u->ddir)) {
> @@ -1623,17 +1633,6 @@ static void io_completed(struct thread_data *td, struct io_u *io_u,
>                                          utime_since_now(&td->start));
>                 }
>
> -               /*
> -                * Verify_backlog enable: We need to log the write job after
> -                * finishing it to prevent verifying before finish writing.
> -                */
> -               if (td_write(td) && idx == DDIR_WRITE &&
> -                   td->o.do_verify &&
> -                   td->o.verify != VERIFY_NONE &&
> -                   !td->o.experimental_verify &&
> -                   (td->flags & TD_F_VER_BACKLOG))
> -                       log_io_piece(td, io_u);
> -
>                 icd->bytes_done[idx] += bytes;
>
>                 if (io_u->end_io) {
> diff --git a/ioengine.h b/ioengine.h
> index 0756bc7e6c13..37627bb1dc76 100644
> --- a/ioengine.h
> +++ b/ioengine.h
> @@ -71,6 +71,8 @@ struct io_u {
>          */
>         unsigned long buf_filled_len;
>
> +       struct io_piece *ipo;
> +
>         union {
>  #ifdef CONFIG_LIBAIO
>                 struct iocb iocb;
> diff --git a/iolog.c b/iolog.c
> index 017b235c217a..5fd9416c036e 100644
> --- a/iolog.c
> +++ b/iolog.c
> @@ -189,6 +189,9 @@ void log_io_piece(struct thread_data *td, struct io_u *io_u)
>         ipo->offset = io_u->offset;
>         ipo->len = io_u->buflen;
>         ipo->numberio = io_u->numberio;
> +       ipo->flags = IP_F_IN_FLIGHT;
> +
> +       io_u->ipo = ipo;
>
>         if (io_u_should_trim(td, io_u)) {
>                 flist_add_tail(&ipo->trim_list, &td->trim_list);
> diff --git a/iolog.h b/iolog.h
> index 321576dbe611..3ec48f2100fe 100644
> --- a/iolog.h
> +++ b/iolog.h
> @@ -67,6 +67,7 @@ enum {
>         IP_F_ONRB       = 1,
>         IP_F_ONLIST     = 2,
>         IP_F_TRIMMED    = 4,
> +       IP_F_IN_FLIGHT  = 8,
>  };
>
>  /*
> diff --git a/verify.c b/verify.c
> index 90cd093add1f..93731228f1b6 100644
> --- a/verify.c
> +++ b/verify.c
> @@ -1022,11 +1022,27 @@ int get_next_verify(struct thread_data *td, struct io_u *io_u)
>                 struct rb_node *n = rb_first(&td->io_hist_tree);
>
>                 ipo = rb_entry(n, struct io_piece, rb_node);
> +
> +               /*
> +                * Ensure that the associated IO has completed
> +                */
> +               read_barrier();
> +               if (ipo->flags & IP_F_IN_FLIGHT)
> +                       goto nothing;
> +
>                 rb_erase(n, &td->io_hist_tree);
>                 assert(ipo->flags & IP_F_ONRB);
>                 ipo->flags &= ~IP_F_ONRB;
>         } else if (!flist_empty(&td->io_hist_list)) {
>                 ipo = flist_entry(td->io_hist_list.next, struct io_piece, list);
> +
> +               /*
> +                * Ensure that the associated IO has completed
> +                */
> +               read_barrier();
> +               if (ipo->flags & IP_F_IN_FLIGHT)
> +                       goto nothing;
> +
>                 flist_del(&ipo->list);
>                 assert(ipo->flags & IP_F_ONLIST);
>                 ipo->flags &= ~IP_F_ONLIST;
> @@ -1072,6 +1088,7 @@ int get_next_verify(struct thread_data *td, struct io_u *io_u)
>                 return 0;
>         }
>
> +nothing:
>         dprint(FD_VERIFY, "get_next_verify: empty\n");
>         return 1;
>  }
>
> --
> Jens Axboe
>

[-- Attachment #2: test.sh --]
[-- Type: application/x-sh, Size: 1683 bytes --]

Re: Rip out verify_backlog support?

[Jens Axboe]

On Thu, Feb 06 2014, Puthikorn Voravootivat wrote:
> That fix runs fine with my test script (attached), but I think you
> might want to remove or reword this comment.

I already did in my local one, you just got the first cut yesterday.
Reads like this now:

/*
 * Always log IO before it's issued, so we know the specific
 * order of it. The logged unit will track when the IO has
 * completed
 */

Thanks for testing it, will go in now...

-- 
Jens Axboe