Re: BUG: Reads issued past end of size with libaio

On Wed, May 14 2008, Shawn Lewis wrote:
> Hi,
>
> This type of conf file is useful for testing disk cache speeds. But it
> exposes an fio bug. The workaround is to use rw=randread and norandommap=1
> (to avoid some cpu overhead).
>
> With this conf file:
>
> [hdc-cache_speed]
> filename=/dev/hdc
> rw=read
> bs=64k
> size=64k
> ioengine=libaio
> iodepth=8
> runtime=10
> bwavgtime=5000
> time_based=1
> direct=1
> thread=1
> ioscheduler=noop
> write_iolog=cache_speed.log
>
>
>
> we get this as an io_log (truncated):
>
> fio version 2 iolog
> /dev/hdc add
> /dev/hdc open
> /dev/hdc read 0 65536
> /dev/hdc read 65536 65536
> /dev/hdc read 131072 65536
> /dev/hdc read 196608 65536
> /dev/hdc read 262144 65536
> /dev/hdc read 327680 65536
> /dev/hdc read 393216 65536
> /dev/hdc read 458752 65536
> /dev/hdc close
> /dev/hdc open
> /dev/hdc read 0 65536
> /dev/hdc read 65536 65536
> /dev/hdc read 131072 65536
> /dev/hdc read 196608 65536
> /dev/hdc read 262144 65536
> /dev/hdc read 327680 65536
> /dev/hdc read 393216 65536
> /dev/hdc read 458752 65536
> /dev/hdc close
> ...
>
> fio shouldn't write past 65536 because size=64k.
>
>
> Sorry, I'm not working out the head fio tree right now, so this may already
> be fixed, but I wanted to let you know just in case.

HEAD is the same, it's still buggy. I've committed a fix for that,
attached below. I hope it's fully correct, it feels a little hackish for
my taste but I don't see a better way...

diff --git a/fio.h b/fio.h
index 5ee43e0..47b6d48 100644
--- a/fio.h
+++ b/fio.h
@@ -980,6 +980,12 @@ extern void close_ioengine(struct thread_data *);
         } \
 } while (0)
 
+static inline void fio_file_reset(struct fio_file *f)
+{
+ f->last_free_lookup = 0;
+ f->last_pos = f->file_offset;
+}
+
 static inline void clear_error(struct thread_data *td)
 {
         td->error = 0;
diff --git a/io_u.c b/io_u.c
index 7f52a24..55ac826 100644
--- a/io_u.c
+++ b/io_u.c
@@ -188,7 +188,14 @@ static int get_next_offset(struct thread_data *td, struct io_u *io_u)
                         b = (f->last_pos - f->file_offset) / td->o.min_bs[ddir];
         }
 
- io_u->offset = (b * td->o.min_bs[ddir]) + f->file_offset;
+ io_u->offset = b * td->o.min_bs[ddir];
+ if (io_u->offset >= f->io_size) {
+ dprint(FD_IO, "get_next_offset: offset %llu >= io_size %llu\n",
+ io_u->offset, f->io_size);
+ return 1;
+ }
+
+ io_u->offset += f->file_offset;
         if (io_u->offset >= f->real_file_size) {
                 dprint(FD_IO, "get_next_offset: offset %llu >= size %llu\n",
                                         io_u->offset, f->real_file_size);
@@ -651,6 +658,16 @@ set_file:
                         break;
 
                 /*
+ * optimization to prevent close/open of the same file. This
+ * way we preserve queueing etc.
+ */
+ if (td->o.nr_files == 1 && td->o.time_based) {
+ put_file(td, f);
+ fio_file_reset(f);
+ goto set_file;
+ }
+
+ /*
                  * td_io_close() does a put_file() as well, so no need to
                  * do that here.
                  */
diff --git a/ioengines.c b/ioengines.c
index d3ee4b9..9e1c556 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -327,8 +327,7 @@ int td_io_open_file(struct thread_data *td, struct fio_file *f)
                 }
         }
 
- f->last_free_lookup = 0;
- f->last_pos = f->file_offset;
+ fio_file_reset(f);
         f->flags |= FIO_FILE_OPEN;
         f->flags &= ~FIO_FILE_CLOSING;
 

-- 
Jens Axboe