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.
Kind regards,
Niklas
