On Wed, 2021-08-04 at 06:39 +0000, Shinichiro Kawasaki wrote:
> On Aug 03, 2021 / 19:36, Dmitry Fomichev wrote:
> > On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> > > The trim workload to zoned block devices is supported as zone
> > > reset,
> > > and
> > > this feature is available for I/O engines which support both
> > > zoned
> > > devices and trim workload. Libzbc I/O engine supports zoned but
> > > lacks
> > > trim workload support. To enable trim support with libzbc I/O
> > > engine,
> > > remove the check which inhibited trim from requests to libzbc I/O
> > > engine. Also set file open flags for trim same as write, and call
> > > zbd_do_io_u_trim() for trim I/O.
> > >
> > > Of note is that libzbc I/O engine now can support trim to
> > > sequential
> > > write required zones, but still can not support os_trim() call
> > > and
> > > BLKDISCARD ioctl for the conventional zones. The trim I/Os to
> > > conventional zones are reported as an error.
> > >
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > > ---
> > > engines/libzbc.c | 16 ++++++++++------
> > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/engines/libzbc.c b/engines/libzbc.c
> > > index 7f2bc431..5b4c5e8e 100644
> > > --- a/engines/libzbc.c
> > > +++ b/engines/libzbc.c
> > > @@ -14,6 +14,7 @@
> > > #include "fio.h"
> > > #include "err.h"
> > > #include "zbd_types.h"
> > > +#include "zbd.h"
> > >
> > > struct libzbc_data {
> > > struct zbc_device *zdev;
> > > @@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data
> > > *td,
> > > struct fio_file *f,
> > > return -EINVAL;
> > > }
> > >
> > > - if (td_write(td)) {
> > > + if (td_write(td) || td_trim(td)) {
> > > if (!read_only)
> > > flags |= O_RDWR;
> > > } else if (td_read(td)) {
> > > @@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data
> > > *td,
> > > struct fio_file *f,
> > > flags |= O_RDWR;
> > > else
> > > flags |= O_RDONLY;
> > > - } else if (td_trim(td)) {
> > > - td_verror(td, EINVAL, "libzbc does not support
> > > trim");
> > > - log_err("%s: libzbc does not support trim\n", f-
> > > > file_name);
> > > - return -EINVAL;
> > > }
> > >
> > > if (td->o.oatomic) {
> > > @@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct
> > > thread_data *td, struct io_u *io_u)
> > > ret = zbc_flush(ld->zdev);
> > > if (ret)
> > > log_err("zbc_flush error %zd\n", ret);
> > > - } else if (io_u->ddir != DDIR_TRIM) {
> > > + } else if (io_u->ddir == DDIR_TRIM) {
> > > + ret = zbd_do_io_u_trim(td, io_u);
> > > + if (!ret) {
> > > + log_err("libzbc does not support trim to
> > > "
> > > + "conventional zones\n");
> > > + ret = EINVAL;
> >
> > Wouldn't that be more appropriate to just call os_trim() here and
> > avoid
> > this error? This way, any ZBD trim workload that succeeds without
> > using
> > libzbc would also succeed with using this engine... not the case
> > with
> > the code above.
>
> os_trim() for Linux calls BLKDISCARD ioctl, which does not work for
> SG node.
> Then, os_trim() call is not appropriate here.
>
> One point to improve is the log_err() message I added. "Trim" and
> "Conventional zone" does not have relation, then the message is
> confusing.
> The error message is too much, probably. I will remove it and just
> return
> EINVAL.
I see... since we don't have the block FD readily available, a
WRITE_SAME with UNMAP bit over SG_IO could do the trick. But doing this
would add a lot of complexity just to cover this relatively narrow use
case. Erroring out with no message should be fine here.
>
> >
> > > + }
> > > + } else {
> > > log_err("Unsupported operation %u\n", io_u-
> > > >ddir);
> > > ret = -EINVAL;
> > > }
> >
>
