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.
> + *
> + * @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)
> +{
> + /*
> + * 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.
> +}
> +
> /**
> * 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
