On 2020/07/29 8:01, Dmitry Fomichev wrote:
>
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@xxxxxxx>
>> Sent: Monday, July 27, 2020 1:07 AM
>> To: Dmitry Fomichev <Dmitry.Fomichev@xxxxxxx>; Jens Axboe
>> <axboe@xxxxxxxxx>
>> Cc: fio@xxxxxxxxxxxxxxx; Shinichiro Kawasaki
>> <shinichiro.kawasaki@xxxxxxx>
>> Subject: Re: [PATCH 2/3] zbd: simplify zone reset code
>>
>> On 2020/07/27 12:16, Dmitry Fomichev wrote:
>>> zbd_reset_range() function is only called once from zbd_reset_zone()
>>> and it is always called for an individual zone, not a range.
>>>
>>> Make zone reset flow simpler by moving all the code needed
>>> to reset a single zone from zbd_reset_range() to zbd_reset_zone().
>>> Therefore, zbd_reset_range() is now dropped.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@xxxxxxx>
>>> ---
>>> zbd.c | 76 +++++++++++++++++++++--------------------------------------
>>> 1 file changed, 27 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/zbd.c b/zbd.c
>>> index 3eac5df3..f6ccf299 100644
>>> --- a/zbd.c
>>> +++ b/zbd.c
>>> @@ -670,54 +670,6 @@ int zbd_setup_files(struct thread_data *td)
>>> return 0;
>>> }
>>>
>>> -/**
>>> - * zbd_reset_range - reset zones for a range of sectors
>>> - * @td: FIO thread data.
>>> - * @f: Fio file for which to reset zones
>>> - * @sector: Starting sector in units of 512 bytes
>>> - * @nr_sectors: Number of sectors in units of 512 bytes
>>> - *
>>> - * Returns 0 upon success and a negative error code upon failure.
>>> - */
>>> -static int zbd_reset_range(struct thread_data *td, struct fio_file *f,
>>> - uint64_t offset, uint64_t length)
>>> -{
>>> - uint32_t zone_idx_b, zone_idx_e;
>>> - struct fio_zone_info *zb, *ze, *z;
>>> - int ret = 0;
>>> -
>>> - assert(is_valid_offset(f, offset + length - 1));
>>> -
>>> - switch (f->zbd_info->model) {
>>> - case ZBD_HOST_AWARE:
>>> - case ZBD_HOST_MANAGED:
>>> - ret = zbd_reset_wp(td, f, offset, length);
>>> - if (ret < 0)
>>> - return ret;
>>> - break;
>>> - default:
>>> - break;
>>> - }
>>> -
>>> - zone_idx_b = zbd_zone_idx(f, offset);
>>> - zb = &f->zbd_info->zone_info[zone_idx_b];
>>> - zone_idx_e = zbd_zone_idx(f, offset + length);
>>> - ze = &f->zbd_info->zone_info[zone_idx_e];
>>> - for (z = zb; z < ze; z++) {
>>> - pthread_mutex_lock(&z->mutex);
>>> - pthread_mutex_lock(&f->zbd_info->mutex);
>>> - f->zbd_info->sectors_with_data -= z->wp - z->start;
>>> - pthread_mutex_unlock(&f->zbd_info->mutex);
>>> - z->wp = z->start;
>>> - z->verify_block = 0;
>>> - pthread_mutex_unlock(&z->mutex);
>>> - }
>>> -
>>> - td->ts.nr_zone_resets += ze - zb;
>>> -
>>> - return ret;
>>> -}
>>> -
>>> static unsigned int zbd_zone_nr(struct zoned_block_device_info
>> *zbd_info,
>>> struct fio_zone_info *zone)
>>> {
>>> @@ -735,10 +687,36 @@ static unsigned int zbd_zone_nr(struct
>> zoned_block_device_info *zbd_info,
>>> 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;
>>> + int ret = 0;
>>> +
>>> + assert(is_valid_offset(f, offset + length - 1));
>>> +
>>> dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
>>> zbd_zone_nr(f->zbd_info, z));
>>> + switch (f->zbd_info->model) {
>>> + case ZBD_HOST_AWARE:
>>> + case ZBD_HOST_MANAGED:
>>> + ret = zbd_reset_wp(td, f, offset, length);
>>> + if (ret < 0)
>>> + return ret;
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>>
>>> - return zbd_reset_range(td, f, z->start, zbd_zone_end(z) - z->start);
>>> + pthread_mutex_lock(&z->mutex);
>>
>> Your change is not affecting the locking model, but I wonder if it would not
>> be
>> better to lock the zone before calling zbd_reset_wp() so that the device side
>> zone reset and the update "z->wp = z->start" are atomically executed...
>
> I am seeing that zbd_reset_zone() is always called with the zone already locked
> and we can remove that locking inside this function (need to add a note in the
> description that the caller must hold z->mutex). Overall, the additional change is
>
> --- a/zbd.c
> +++ b/zbd.c
> @@ -691,24 +691,26 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
>
> dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
> zbd_zone_nr(f->zbd_info, z));
> +
> + pthread_mutex_lock(&f->zbd_info->mutex);
I do not think this is needed.
> +
> switch (f->zbd_info->model) {
> case ZBD_HOST_AWARE:
> case ZBD_HOST_MANAGED:
> ret = zbd_reset_wp(td, f, offset, length);
> - if (ret < 0)
> + if (ret < 0) {
> + pthread_mutex_unlock(&f->zbd_info->mutex);
Neither is this.
> return ret;
> + }
> break;
> default:
> break;
> }
>
> - pthread_mutex_lock(&z->mutex);
Checking too, yes, the zone is already locked when zbd_reset_zone() is called.
So this can go away.
> - pthread_mutex_lock(&f->zbd_info->mutex);
But this one needs to stay to protect sectors_with_data.
> f->zbd_info->sectors_with_data -= z->wp - z->start;
> pthread_mutex_unlock(&f->zbd_info->mutex);
> z->wp = z->start;
> z->verify_block = 0;
> - pthread_mutex_unlock(&z->mutex);
>
> td->ts.nr_zone_resets++;
>
> I tried this now and I don't see any problems. This probably needs to be a separate
> patch. I can add it to this series. What do you suggest?
I would just squash the above changes in your initial patch as a v2. Since the
original patch is already moving the mutex calls around, you may as well change
them in the same patch with a mention about it in the commit message.
--
Damien Le Moal
Western Digital Research
