On 2020/04/15 12:05, Shinichiro Kawasaki wrote:
> 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.
Ah, yes indeed, it is needed.
>
>>
>>>
>>>> +}
>>>> +
>>>> /**
>>>> * 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
>>
>
--
Damien Le Moal
Western Digital Research
