On Feb 07, 2023 / 14:06, Niklas Cassel wrote:
> On Tue, Feb 07, 2023 at 03:37:38PM +0900, Shin'ichiro Kawasaki wrote:
> > The valid data bytes accounting field is initialized at file reset,
> > after each job started. Each job locks zones to check write pointer
> > positions of its write target zones. This can cause zone lock contention
> > with write by other jobs.
> >
> > To avoid the zone lock contention, move the initialization from file
> > reset to file set up before job start. It allows to access the write
> > pointers and the accounting field without locks. Remove the lock and
> > unlock codes which are no longer required. Ensure the locks are not
> > required by checking run-state in the struct thread_data.
> >
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > ---
> > zbd.c | 93 ++++++++++++++++++++++++++++-------------------------------
> > 1 file changed, 44 insertions(+), 49 deletions(-)
> >
> > diff --git a/zbd.c b/zbd.c
> > index ca6816b9..a464d6df 100644
> > --- a/zbd.c
> > +++ b/zbd.c
> > @@ -1074,6 +1074,44 @@ void zbd_recalc_options_with_zone_granularity(struct thread_data *td)
> > }
> > }
> >
> > +static uint64_t zbd_set_vdb(struct thread_data *td, const struct fio_file *f)
> > +{
> > + struct fio_zone_info *zb, *ze, *z;
> > + uint64_t wp_vdb = 0;
> > + struct zoned_block_device_info *zbdi = f->zbd_info;
> > +
> > + assert(td->runstate < TD_RUNNING);
> > + assert(zbdi);
> > +
> > + if (!accounting_vdb(td, f))
> > + return 0;
>
> zbd_set_vdb() is also called from zbd_file_reset().
Not really, please find my comment below.
>
> The code from here:
>
> > + if (zbdi->wp_write_min_zone != zbdi->wp_write_max_zone) {
> > + if (zbdi->wp_write_min_zone != f->min_zone ||
> > + zbdi->wp_write_max_zone != f->max_zone) {
> > + td_verror(td, EINVAL,
> > + "multi-jobs with different write ranges are "
> > + "not supported with zone_reset_threshold");
> > + log_err("multi-jobs with different write ranges are "
> > + "not supported with zone_reset_threshold\n");
> > + }
> > + return 0;
> > + }
> > +
> > + zbdi->wp_write_min_zone = f->min_zone;
> > + zbdi->wp_write_max_zone = f->max_zone;
>
> Up until here. (Basically the code added in patch 6/8.)
> Should probably only be called by zbd_setup_files().
>
> Or do we need to perform the verification for every single file reset?
>
> > +
> > + zb = zbd_get_zone(f, f->min_zone);
> > + ze = zbd_get_zone(f, f->max_zone);
> > + for (z = zb; z < ze; z++)
> > + if (z->has_wp)
> > + wp_vdb += z->wp - z->start;
> > +
> > + zbdi->wp_valid_data_bytes = wp_vdb;
> > +
> > + return wp_vdb;
> > +}
> > +
> > int zbd_setup_files(struct thread_data *td)
> > {
> > struct fio_file *f;
> > @@ -1099,6 +1137,7 @@ int zbd_setup_files(struct thread_data *td)
> > struct zoned_block_device_info *zbd = f->zbd_info;
> > struct fio_zone_info *z;
> > int zi;
> > + uint64_t vdb;
> >
> > assert(zbd);
> >
> > @@ -1106,6 +1145,11 @@ int zbd_setup_files(struct thread_data *td)
> > f->max_zone =
> > zbd_offset_to_zone_idx(f, f->file_offset + f->io_size);
> >
> > + vdb = zbd_set_vdb(td, f);
> > +
> > + dprint(FD_ZBD, "%s(%s): valid data bytes = %" PRIu64 "\n",
> > + __func__, f->file_name, vdb);
> > +
> > /*
> > * When all zones in the I/O range are conventional, io_size
> > * can be smaller than zone size, making min_zone the same
> > @@ -1197,54 +1241,9 @@ static bool zbd_dec_and_reset_write_cnt(const struct thread_data *td,
> > return write_cnt == 0;
> > }
> >
> > -static uint64_t zbd_set_vdb(struct thread_data *td, const struct fio_file *f)
> > -{
> > - struct fio_zone_info *zb, *ze, *z;
> > - uint64_t wp_vdb = 0;
> > - struct zoned_block_device_info *zbdi = f->zbd_info;
> > -
> > - if (!accounting_vdb(td, f))
> > - return 0;
> > -
> > - if (zbdi->wp_write_min_zone != zbdi->wp_write_max_zone) {
> > - if (zbdi->wp_write_min_zone != f->min_zone ||
> > - zbdi->wp_write_max_zone != f->max_zone) {
> > - td_verror(td, EINVAL,
> > - "multi-jobs with different write ranges are "
> > - "not supported with zone_reset_threshold");
> > - log_err("multi-jobs with different write ranges are "
> > - "not supported with zone_reset_threshold\n");
> > - }
> > - return 0;
> > - }
> > -
> > - zbdi->wp_write_min_zone = f->min_zone;
> > - zbdi->wp_write_max_zone = f->max_zone;
> > -
> > - zb = zbd_get_zone(f, f->min_zone);
> > - ze = zbd_get_zone(f, f->max_zone);
> > - for (z = zb; z < ze; z++) {
> > - if (z->has_wp) {
> > - zone_lock(td, f, z);
> > - wp_vdb += z->wp - z->start;
> > - }
> > - }
> > -
> > - pthread_mutex_lock(&zbdi->mutex);
> > - zbdi->wp_valid_data_bytes = wp_vdb;
> > - pthread_mutex_unlock(&zbdi->mutex);
> > -
> > - for (z = zb; z < ze; z++)
> > - if (z->has_wp)
> > - zone_unlock(z);
> > -
> > - return wp_vdb;
> > -}
> > -
> > void zbd_file_reset(struct thread_data *td, struct fio_file *f)
> > {
> > struct fio_zone_info *zb, *ze;
> > - uint64_t vdb;
> > bool verify_data_left = false;
> >
> > if (!f->zbd_info || !td_write(td))
> > @@ -1252,10 +1251,6 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
> >
> > zb = zbd_get_zone(f, f->min_zone);
> > ze = zbd_get_zone(f, f->max_zone);
> > - vdb = zbd_set_vdb(td, f);
The zbd_set_vbd() call in zbd_file_reset() is removed here. Then the check is
done only at zbd_setup_files() as you expect.
> > -
> > - dprint(FD_ZBD, "%s(%s): valid data bytes = %" PRIu64 "\n",
> > - __func__, f->file_name, vdb);
> >
> > /*
> > * If data verification is enabled reset the affected zones before
> > --
> > 2.38.1
> >
--
Shin'ichiro Kawasaki
