On Jun 07, 2023 / 13:15, Niklas Cassel wrote:
> On Wed, Jun 07, 2023 at 05:32:27PM +0900, Shin'ichiro Kawasaki wrote:
> > The commit e3be810bf0fd ("zbd: Support zone reset by trim") supported
> > trim for zonemode=zbd by introducing the function zbd_do_io_u_trim(),
> > which calls zbd_reset_zone(). However, it did not call
> > zbd_write_zone_put() to the trim target zone, then trim operation
> > resulted in wrong accounting of write zones.
> >
> > To fix the issue, call zbd_write_zone_put() from zbd_reset_zone(). To
> > cover the case to reset zones without a zbd_write_zone_put() call,
> > prepare another function __zbd_reset_zone(). While at it, simplify
> > zbd_reset_zones() by calling the modified zbd_reset_zone().
> >
> > Of note is that the constifier of the argument td of do_io_u_trim() is
> > removed since zbd_write_zone_put() requires changes in that argument.
> >
> > Fixes: e3be810bf0fd ("zbd: Support zone reset by trim")
> > Suggested-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > ---
> > engines/io_uring.c | 2 +-
> > io_u.c | 2 +-
> > io_u.h | 2 +-
> > zbd.c | 40 ++++++++++++++++++++++++++++++++--------
> > zbd.h | 2 +-
> > 5 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/engines/io_uring.c b/engines/io_uring.c
> > index ff64fc9f..73e4a27a 100644
> > --- a/engines/io_uring.c
> > +++ b/engines/io_uring.c
> > @@ -561,7 +561,7 @@ static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
> > ld->sqes[io_u->index].ioprio = io_u->ioprio;
> > }
> >
> > -static int fio_ioring_cmd_io_u_trim(const struct thread_data *td,
> > +static int fio_ioring_cmd_io_u_trim(struct thread_data *td,
> > struct io_u *io_u)
> > {
> > struct fio_file *f = io_u->file;
> > diff --git a/io_u.c b/io_u.c
> > index 6f5fc94d..faf512e5 100644
> > --- a/io_u.c
> > +++ b/io_u.c
> > @@ -2379,7 +2379,7 @@ int do_io_u_sync(const struct thread_data *td, struct io_u *io_u)
> > return ret;
> > }
> >
> > -int do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
> > +int do_io_u_trim(struct thread_data *td, struct io_u *io_u)
> > {
> > #ifndef FIO_HAVE_TRIM
> > io_u->error = EINVAL;
> > diff --git a/io_u.h b/io_u.h
> > index 55b4d083..b432a540 100644
> > --- a/io_u.h
> > +++ b/io_u.h
> > @@ -162,7 +162,7 @@ void io_u_mark_submit(struct thread_data *, unsigned int);
> > bool queue_full(const struct thread_data *);
> >
> > int do_io_u_sync(const struct thread_data *, struct io_u *);
> > -int do_io_u_trim(const struct thread_data *, struct io_u *);
> > +int do_io_u_trim(struct thread_data *, struct io_u *);
> >
> > #ifdef FIO_INC_DEBUG
> > static inline void dprint_io_u(struct io_u *io_u, const char *p)
> > diff --git a/zbd.c b/zbd.c
> > index 36b68d62..7eef49c6 100644
> > --- a/zbd.c
> > +++ b/zbd.c
> > @@ -254,7 +254,7 @@ static int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
> > }
> >
> > /**
> > - * zbd_reset_zone - reset the write pointer of a single zone
> > + * __zbd_reset_zone - reset the write pointer of a single zone
> > * @td: FIO thread data.
> > * @f: FIO file associated with the disk for which to reset a write pointer.
> > * @z: Zone to reset.
> > @@ -263,8 +263,8 @@ static int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
> > *
> > * The caller must hold z->mutex.
> > */
> > -static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
> > - struct fio_zone_info *z)
> > +static int __zbd_reset_zone(struct thread_data *td, struct fio_file *f,
> > + struct fio_zone_info *z)
> > {
> > uint64_t offset = z->start;
> > uint64_t length = (z+1)->start - offset;
> > @@ -339,6 +339,33 @@ static void zbd_write_zone_put(struct thread_data *td, const struct fio_file *f,
> > z->write = 0;
> > }
> >
> > +/**
> > + * zbd_reset_zone - reset the write pointer of a single zone and remove the zone
> > + * from the array of write zones.
> > + * @td: FIO thread data.
> > + * @f: FIO file associated with the disk for which to reset a write pointer.
> > + * @z: Zone to reset.
> > + *
> > + * Returns 0 upon success and a negative error code upon failure.
>
> Please add an empty line before the "The caller must hold z->mutex" line,
> such that we are consistent with other functions, e.g. __zbd_reset_zone().
Thanks, will add in v2.
>
> > + * The caller must hold z->mutex.
> > + */
> > +static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
> > + struct fio_zone_info *z)
> > +{
> > + int ret;
> > +
> > + ret = __zbd_reset_zone(td, f, z);
> > + if (ret)
> > + goto done;
> > +
> > + pthread_mutex_lock(&f->zbd_info->mutex);
> > + zbd_write_zone_put(td, f, z);
> > + pthread_mutex_unlock(&f->zbd_info->mutex);
> > +
> > +done:
> > + return ret;
> > +}
> > +
> > /**
> > * zbd_finish_zone - finish the specified zone
> > * @td: FIO thread data.
> > @@ -404,9 +431,6 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
> > continue;
> >
> > zone_lock(td, f, z);
> > - pthread_mutex_lock(&f->zbd_info->mutex);
> > - zbd_write_zone_put(td, f, z);
> > - pthread_mutex_unlock(&f->zbd_info->mutex);
> >
> > if (z->wp != z->start) {
> > dprint(FD_ZBD, "%s: resetting zone %u\n",
> > @@ -2048,7 +2072,7 @@ retry:
> > */
> > io_u_quiesce(td);
> > zb->reset_zone = 0;
> > - if (zbd_reset_zone(td, f, zb) < 0)
> > + if (__zbd_reset_zone(td, f, zb) < 0)
>
> Since you remove the zbd_write_zone_put() call, which I think is good,
> I think that you should continue to call zbd_reset_zone() here.
The two hunks above are difficult for review. The 1st hunk which removes the
zbd_write_zone_put() is a change for zbd_reset_zones(). zbd_reset_zones() no
longer needs to call zbd_write_zone_put(), since zbd_reset_zone() (no s) calls
zbd_write_zone_put().
The 2nd hunk is a change in zbd_adjust_block(). The zone reset here is done
just before the write command issue to the zone, so zbd_write_zone_put() should
not be called. Then zbd_reset_zone() should be replaced with __zbd_reset_zone().
>
> > goto eof;
> >
> > if (zb->capacity < min_bs) {
> > @@ -2167,7 +2191,7 @@ char *zbd_write_status(const struct thread_stat *ts)
> > * Return io_u_completed when reset zone succeeds. Return 0 when the target zone
> > * does not have write pointer. On error, return negative errno.
> > */
> > -int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
> > +int zbd_do_io_u_trim(struct thread_data *td, struct io_u *io_u)
> > {
> > struct fio_file *f = io_u->file;
> > struct fio_zone_info *z;
> > diff --git a/zbd.h b/zbd.h
> > index 25a3e0f1..f0ac9876 100644
> > --- a/zbd.h
> > +++ b/zbd.h
> > @@ -100,7 +100,7 @@ enum fio_ddir 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);
> > -int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u);
> > +int zbd_do_io_u_trim(struct thread_data *td, struct io_u *io_u);
> >
> > static inline void zbd_close_file(struct fio_file *f)
> > {
> > --
> > 2.40.1
> >
