On 2021/06/07 18:08, Niklas Cassel wrote:
> On Sat, Jun 05, 2021 at 12:48:31AM +0000, Damien Le Moal wrote:
>> On 2021/06/04 21:18, Niklas Cassel wrote:
>>> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>>>
>>> For a job with zonemode=zbd, we do not want any file to be ignored.
>>> Each file's file type in that job should be supported by either zbd.c
>>> or the ioengine. If not, we should return an error.
>>> This way, ZBD_IGNORE becomes redundant and can be removed.
>>>
>>> By removing ZBD_IGNORE, we know that all files belonging to a job that
>>> has zonemode=zbd set, will either be a zoned block device, or emulate
>>> a zoned block device.
>>>
>>> This means that for jobs that have zonemode=zbd, f->zbd_info will always
>>> be non-NULL. This will make the zbd code slightly easier to reason about
>>> and to maintain.
>>>
>>> When removing zbd_zoned_model ZBD_IGNORE, define the new first enum value
>>> as 0x1, so that we avoid potential ABI problems with existing binaries.
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
>>> ---
>>> engines/libzbc.c | 3 +--
>>> engines/skeleton_external.c | 1 -
>>> oslib/linux-blkzoned.c | 3 +--
>>> zbd.c | 3 +--
>>> zbd_types.h | 7 +++----
>>> 5 files changed, 6 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/engines/libzbc.c b/engines/libzbc.c
>>> index 3dde93db..c3551bec 100644
>>> --- a/engines/libzbc.c
>>> +++ b/engines/libzbc.c
>>> @@ -181,8 +181,7 @@ static int libzbc_get_zoned_model(struct thread_data *td, struct fio_file *f,
>>> int ret;
>>>
>>> if (f->filetype != FIO_TYPE_BLOCK && f->filetype != FIO_TYPE_CHAR) {
>>> - *model = ZBD_IGNORE;
>>> - return 0;
>>> + return -EINVAL;
>>> }
>>
>> Please drop the curly brackets.
>>
>>>
>>> ret = libzbc_open_dev(td, f, &ld);
>>> diff --git a/engines/skeleton_external.c b/engines/skeleton_external.c
>>> index c79b6f11..cff83a10 100644
>>> --- a/engines/skeleton_external.c
>>> +++ b/engines/skeleton_external.c
>>> @@ -156,7 +156,6 @@ static int fio_skeleton_close(struct thread_data *td, struct fio_file *f)
>>> /*
>>> * Hook for getting the zoned model of a zoned block device for zonemode=zbd.
>>> * The zoned model can be one of (see zbd_types.h):
>>> - * - ZBD_IGNORE: skip regular files
>>> * - ZBD_NONE: regular block device (zone emulation will be used)
>>> * - ZBD_HOST_AWARE: host aware zoned block device
>>> * - ZBD_HOST_MANAGED: host managed zoned block device
>>> diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
>>> index 6f89ec6f..8cb9c53a 100644
>>> --- a/oslib/linux-blkzoned.c
>>> +++ b/oslib/linux-blkzoned.c
>>> @@ -141,8 +141,7 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>>> char *model_str = NULL;
>>>
>>> if (f->filetype != FIO_TYPE_BLOCK) {
>>> - *model = ZBD_IGNORE;
>>> - return 0;
>>> + return -EINVAL;
>>> }
>>
>> Please drop the curly brackets.
>>
>> Also, missing in this patch: zbd_verify_sizes() has a "if (!f->zbd_info)" check
>> which I think should be changed into an assert.
>
> Hello Damien,
>
> zbd_verify_sizes()
> does a:
>
> for_each_td() {
> for_each_file() {
> if (!f->zbd_info)
> continue;
> }
> }
>
> Considering that this is a for_each_td(), which will iterate over all
> threads in the job file, I disagree with this comment.
>
> A job file can have one job with
> zonemode=zbd
>
> and in the same job file, another job without zonemode=zbd, which targets
> a completely different device, so I do think that we need to keep the
> continue here.
Indeed. You are right.
>
>
> Kind regards,
> Niklas
>
--
Damien Le Moal
Western Digital Research
