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().
> + * 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.
> 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
>
