Re: [PATCHv3] fio: add fdp support for io_uring_cmd nvme engine

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2/24/23 03:55, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> 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@samsung.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

[...]

> +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@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research