On Apr 15, 2020 / 01:09, Damien Le Moal wrote:
> On 2020/04/15 10:03, Damien Le Moal wrote:
> > On 2020/04/14 19:00, Shin'ichiro Kawasaki wrote:
> >> Commit fb0259fb ("zbd: Ensure first I/O is write for random read/write to
> >> sequential zones") introduced a step to change direction of io_u from
> >> read to write when that is the first I/O of the random read/write
> >> workload to zoned block devices. However, such direction adjustment
> >> results in inconsistent I/O length when read block size and write block
> >> size are different.
> >>
> >> To avoid the inconsistency between I/O direction and I/O length,
> >> adjust the I/O direction before the I/O length is set. Move the step
> >> from zbd_adjust_block() to set_rw_ddir(). To minimize changes in
> >> set_rw_ddir(), introduce zbd_adjust_ddir() helper function.
> >>
> >> Fixes: fb0259fb ("zbd: Ensure first I/O is write for random read/write to sequential zones")
> >> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> >> ---
> >> io_u.c | 2 ++
> >> zbd.c | 34 ++++++++++++++++++++++++----------
> >> zbd.h | 2 ++
> >> 3 files changed, 28 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/io_u.c b/io_u.c
> >> index 5d62a76c..234dd268 100644
> >> --- a/io_u.c
> >> +++ b/io_u.c
> >> @@ -746,6 +746,8 @@ static void set_rw_ddir(struct thread_data *td, struct io_u *io_u)
> >> {
> >> enum fio_ddir ddir = get_rw_ddir(td);
> >>
> >> + zbd_adjust_ddir(td, io_u, &ddir);
> >> +
> >> if (td_trimwrite(td)) {
> >> struct fio_file *f = io_u->file;
> >> if (f->last_pos[DDIR_WRITE] == f->last_pos[DDIR_TRIM])
> >> diff --git a/zbd.c b/zbd.c
> >> index de0c5bf4..82810511 100644
> >> --- a/zbd.c
> >> +++ b/zbd.c
> >> @@ -1331,6 +1331,30 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
> >> }
> >> }
> >>
> >> +/**
> >> + * zbd_adjust_ddir - Adjust the I/O direction to zoned block devices.
> >
> > zbd_adjust_ddir - Adjust an I/O direction for zonemode=zbd.
Thanks. Will rephrase as commented.
> >
> >> + *
> >> + * @td: FIO thread data.
> >> + * @io_u: FIO I/O unit.
> >> + * @ddir: Data direction to adjust.
> >> + */
> >> +void zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
> >> + enum fio_ddir *ddir)
> >
> > Why use a pointer for ddir ? Simply have this function return an enum fio_ddir
> > similarly to what get_rw_ddir() does.
> >
> > enum fio_ddir zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
> > enum fio_ddir ddir)
Ok, it looks better to have simlairity with get_rw_ddir(). Will change as
suggested.
> >
> >> +{
> >> + /*
> >> + * In case read direction is chosen for the first random I/O, fio with
> >> + * zonemode=zbd stops because no data can be read from zoned block
> >> + * devices with all empty zones. Overwrite the first I/O direction as
> >> + * write to make sure data to read exists.
> >> + */
> >> + if (td->o.zone_mode == ZONE_MODE_ZBD &&
> >> + td_rw(td) &&
> >> + *ddir == DDIR_READ &&
> >> + !io_u->file->zbd_info->sectors_with_data &&
> >> + !td->o.read_beyond_wp)
> >> + *ddir = DDIR_WRITE;
> >
> > Aouch. Really long condition... This is bad for performance for cases that do
> > not care about zonemode=zbd. I think it is much better to split this and return
> > early for the cases that do not need change. So something like:
> >
> > enum fio_ddir zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
> > enum fio_ddir ddir)
> > {
> > if (td->o.zone_mode != ZONE_MODE_ZBD ||
> > ddir != DDIR_READ)
> > return ddir;
> >
> > if (io_u->file->zbd_info->sectors_with_data ||
> > td->o.read_beyond_wp)
> > return DDIR_READ;
> >
> > return DDIR_WRITE;
> > }
> >
> > That is a lot more readable in my opinion.
>
> Forgot to carry over your explanation comment. Better include it as that
> clarifies the code and why the change in direction is needed.
Thanks. Will modify the function as suggested keeping the comment. I think check
with td_rw() is required also to ensure that the workload mixes the read and
write. I'll include it in the first if group.
>
> >
> >> +}
> >> +
> >> /**
> >> * zbd_adjust_block - adjust the offset and length as necessary for ZBD drives
> >> * @td: FIO thread data.
> >> @@ -1364,16 +1388,6 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
> >> if (!zbd_zone_swr(zb))
> >> return io_u_accept;
> >>
> >> - /*
> >> - * In case read direction is chosen for the first random I/O, fio with
> >> - * zonemode=zbd stops because no data can be read from zoned block
> >> - * devices with all empty zones. Overwrite the first I/O direction as
> >> - * write to make sure data to read exists.
> >> - */
> >> - if (td_rw(td) && !f->zbd_info->sectors_with_data
> >> - && !td->o.read_beyond_wp)
> >> - io_u->ddir = DDIR_WRITE;
> >> -
> >> /*
> >> * Accept the I/O offset for reads if reading beyond the write pointer
> >> * is enabled.
> >> diff --git a/zbd.h b/zbd.h
> >> index 4eaf902e..196853ab 100644
> >> --- a/zbd.h
> >> +++ b/zbd.h
> >> @@ -82,6 +82,8 @@ int zbd_init(struct thread_data *td);
> >> void zbd_file_reset(struct thread_data *td, struct fio_file *f);
> >> bool zbd_unaligned_write(int error_code);
> >> void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u);
> >> +void zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
> >> + enum fio_ddir *ddir);
> >> enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u);
> >> char *zbd_write_status(const struct thread_stat *ts);
> >>
> >>
> >
> >
>
>
> --
> Damien Le Moal
> Western Digital Research
>
--
Best Regards,
Shin'ichiro Kawasaki
