Set io_u field(endpos) value during verify phase

Set io_u field(endpos) value during verify phase

[Radha Ramachandran]

Hi,
We ran into an issue where during the verify phase (sync engine),
because the io_u->endpos field was not set, it was using a stale
value.
If the sync engine is used then for every read(during the verify
phase) to avoid an extra seek, the code checks if we are already at
the position we expect to be at and if so bypasses the seek.
So in the case where the current file position field which is stale
(From the previous i/o completion) matches  where the read needs to be
done from, there will be no seek and the code lands up reading the
wrong data and goes down the error path because the data does not
match.
This case is only seen during the verify phase where the code uses
__get_io_u directly. In the regular read/write phase the code uses
get_io_u and that function correctly sets the value of the endpos
field.

Hopefully, I explained the problem coherently

Here is the fix:

diff -up verify.c.old verify.c
--- verify.c.old        2009-05-21 13:14:51.703698000 -0700
+++ verify.c    2009-05-21 13:18:48.052540000 -0700
@@ -711,6 +711,7 @@ int get_next_verify(struct thread_data *
                io_u->ddir = DDIR_READ;
                io_u->xfer_buf = io_u->buf;
                io_u->xfer_buflen = io_u->buflen;
+               io_u->endpos = io_u->offset + io_u->buflen;
                free(ipo);
                dprint(FD_VERIFY, "get_next_verify: ret io_u %p\n", io_u);
                return 0;


Thanks
-radha

Re: Set io_u field(endpos) value during verify phase

[Radha Ramachandran]

Sorry for the confusion, Please disregard this fix.
Looks like I was looking at the older version of the fio code that had this bug.
The current version of fio does not work the same way, so will not see
this error.
-radha


On Thu, May 21, 2009 at 2:40 PM, Radha Ramachandran <radha@google.com> wrote:
> Hi,
> We ran into an issue where during the verify phase (sync engine),
> because the io_u->endpos field was not set, it was using a stale
> value.
> If the sync engine is used then for every read(during the verify
> phase) to avoid an extra seek, the code checks if we are already at
> the position we expect to be at and if so bypasses the seek.
> So in the case where the current file position field which is stale
> (From the previous i/o completion) matches  where the read needs to be
> done from, there will be no seek and the code lands up reading the
> wrong data and goes down the error path because the data does not
> match.
> This case is only seen during the verify phase where the code uses
> __get_io_u directly. In the regular read/write phase the code uses
> get_io_u and that function correctly sets the value of the endpos
> field.
>
> Hopefully, I explained the problem coherently
>
> Here is the fix:
>
> diff -up verify.c.old verify.c
> --- verify.c.old        2009-05-21 13:14:51.703698000 -0700
> +++ verify.c    2009-05-21 13:18:48.052540000 -0700
> @@ -711,6 +711,7 @@ int get_next_verify(struct thread_data *
>                io_u->ddir = DDIR_READ;
>                io_u->xfer_buf = io_u->buf;
>                io_u->xfer_buflen = io_u->buflen;
> +               io_u->endpos = io_u->offset + io_u->buflen;
>                free(ipo);
>                dprint(FD_VERIFY, "get_next_verify: ret io_u %p\n", io_u);
>                return 0;
>
>
> Thanks
> -radha
>

Re: Set io_u field(endpos) value during verify phase

[Jens Axboe]

On Thu, May 21 2009, Radha Ramachandran wrote:
> Sorry for the confusion, Please disregard this fix.  Looks like I was
> looking at the older version of the fio code that had this bug.  The
> current version of fio does not work the same way, so will not see
> this error.

I think the endpos code got in through the google verify work. Looking
at the current base, it's only set once and never read. So at least some
good came of your posting, I killed the variable :-)

-- 
Jens Axboe