On 2021/12/02 18:42, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> For a thread that has zonemode == ZONE_MODE_ZBD set, the zbd code requires
> that each file (for that thread) has a valid f->zbd_info pointer.
>
> This intent was further clarified by commit 5ddf46d0b2df ("zbd: change some
> f->zbd_info conditionals to asserts").
>
> The zbd info pointer is set by zbd_init_files(), either by creating a new
> zbd_info struct, or by increasing the refcount of an existing zbd_info.
>
> A zbd_info struct contains the in memory state of the zones, including e.g.
> each zone's wp and zone capacity.
>
> Normally, zbd_init_files() is always called, even for read only workloads.
> However, in the case where a read iolog was supplied, setup_files()
> currently jumps to the done label before zbd_init_files() has been called.
>
> Even for a read only workload, zbd_adjust_block() will do things as
> checking if the read I/O is below the wp (unless td->o.read_beyond_wp is
> enabled). In order to be able to do this comparison, we need a valid
> zbd_info.
>
> There is no reason why the zbd code should treat a read only workload
> different from a read iolog workload. (E.g. the wp for the zones might
> have changed since the read iolog was recorded.)
>
> If the user for some reason wants to disregard the wp check during a read
> iolog workload, the td->o.read_beyond_wp option can be used, just like in
> the regular read only workload case.
>
> Move the read iolog check and the matching "goto done" after the call to
> zbd_init_files(). This way, we treat a read iolog workload simlar to a
> regular read only workload, while avoiding an assertion failure in
> zbd_setup_files() (which is called after the done label).
>
> Reported-by: Shane Moore <shane.moore@xxxxxxx>
> Suggested-by: Dmitry Fomichev <dmitry.fomichev@xxxxxxx>
> Tested-by: Shane Moore <shane.moore@xxxxxxx>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> filesetup.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/filesetup.c b/filesetup.c
> index 228e4fff..fb556d84 100644
> --- a/filesetup.c
> +++ b/filesetup.c
> @@ -1119,9 +1119,6 @@ int setup_files(struct thread_data *td)
> if (err)
> goto err_out;
>
> - if (o->read_iolog_file)
> - goto done;
> -
> if (td->o.zone_mode == ZONE_MODE_ZBD) {
> err = zbd_init_files(td);
> if (err)
> @@ -1129,6 +1126,9 @@ int setup_files(struct thread_data *td)
> }
> zbd_recalc_options_with_zone_granularity(td);
>
> + if (o->read_iolog_file)
> + goto done;
> +
> /*
> * check sizes. if the files/devices do not exist and the size
> * isn't passed to fio, abort.
>
Looks OK to me.
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
