On 2/24/23 03:55, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> Add support for NVMe TP4146 Flexible Data Placemen, allowing placement
> identifiers in write commands. The user can enabled this with the new
> "fdp=1" parameter for fio's io_uring_cmd ioengine. By default, the fio
> jobs will cycle through all the namespace's available placement
> identifiers for write commands. The user can limit which placement
> identifiers can be used with additional parameter, "fdp_pli=<list,>",
> which can be used to separate write intensive jobs from less intensive
> ones.
>
> Setting up your namespace for FDP is outside the scope of 'fio', so this
> assumes the namespace is already properly configured for the mode.
>
> Based-on-a-patch-by: Ankit Kumar <ankit.kumar@xxxxxxxxxxx>
> Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
[...]
> +static int fdp_ruh_info(struct thread_data *td, struct fio_file *f,
> + struct fio_ruhs_info *ruhs)
> +{
> + int ret = -EINVAL;
> +
> + if (td->io_ops && td->io_ops->fdp_fetch_ruhs)
> + ret = td->io_ops->fdp_fetch_ruhs(td, f, ruhs);
> + else
> + log_err("%s: engine (%s) lacks fetch ruhs.\n",
> + f->file_name, td->io_ops->name);
> + if (ret < 0) {
> + td_verror(td, errno, "fdp fetch ruhs failed");
> + log_err("%s: fdp fetch ruhs failed (%d).\n",
> + f->file_name, errno);
Nit: for the !td->io_ops->fdp_fetch_ruhs case, this error message will also be
displayed. You could reverse the earlier if and return earlier to avoid that and
make the error messages clearer. Not a big deal though :)
> + }
> +
> + return ret;
> +}
> +
> +static int init_ruh_info(struct thread_data *td, struct fio_file *f)
> +{
> + struct fio_ruhs_info *ruhs, *tmp;
> + int i, ret;
> +
> + ruhs = calloc(1, sizeof(*ruhs) + 128 * sizeof(*ruhs->plis));
> + if (!ruhs)
> + return -ENOMEM;
> +
> + ret = fdp_ruh_info(td, f, ruhs);
> + if (ret) {
> + log_info("fio: ruh info failed for %s (%d).\n",
> + f->file_name, -ret);
> + goto out;
> + }
> +
> + if (ruhs->nr_ruhs > 128)
> + ruhs->nr_ruhs = 128;
> +
> + if (td->o.fdp_nrpli == 0) {
> + f->ruhs_info = ruhs;
> + return 0;
> + }
> +
> + for (i = 0; i < td->o.fdp_nrpli; i++) {
> + if (td->o.fdp_plis[i] > ruhs->nr_ruhs) {
> + ret = -EINVAL;
> + goto out;
> + }
> + }
> +
> + tmp = calloc(1, sizeof(*tmp) + ruhs->nr_ruhs * sizeof(*tmp->plis));
Nit: since you checked the calloc above, you could check this one too.
> + tmp->nr_ruhs = td->o.fdp_nrpli;
> + for (i = 0; i < td->o.fdp_nrpli; i++)
> + tmp->plis[i] = ruhs->plis[td->o.fdp_plis[i]];
> + f->ruhs_info = tmp;
> +out:
> + free(ruhs);
> + return ret;
> +}
Apart from these 2 nits, looks OK to me.
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
