On Wed, Feb 15, 2023 at 04:21:08PM -0700, Keith Busch wrote:
> On Fri, Feb 10, 2023 at 11:59:11AM +0530, Ankit Kumar wrote:
> > > +.. option:: fdp=bool : [io_uring_cmd]
> > > +
> > > + Enable Flexible Data Placement mode for write commands.
> > > +
> > > +.. option:: fdp_pli=int[,int][,int] : [io_uring_cmd]
> > I think it probably should just be "fdp_pli=str"
> > Kind of similar to the fio "option cpus_allowed=str"
> ...
> > > + Select which Placement ID Index/Indicies this job is allowed to use for
> > > + writes. By default, the job will cycle through all available Placement
> > > + IDs, so use this to isolate these identifiers to specific jobs..
> > Maybe we can also add one line description something like this:
> > If you want fio to use placement identifier only at indices 0,2 and 5 specify
> > ``fdp_pli=0,2,5``.
>
> Sure, definitely don't need to be sequential. Also, like cpus_allowed, perhaps
> ranges are desirable (ex: 0-2,5).
>
> > > +int fio_nvme_iomgmt_ruhs(struct thread_data *td, struct fio_file *f,
> > > + struct nvme_fdp_ruh_status *ruhs, __u32 bytes)
> > > +{
> > > + struct nvme_data *data = FILE_ENG_DATA(f);
> > > + int fd, ret;
> > > +
> > > + fd = open(f->file_name, O_RDONLY | O_LARGEFILE);
> > > + if (fd < 0)
> > > + return -errno;
> > > +
> > > + ret = nvme_fdp_reclaim_unit_handle_status(fd, data->nsid, bytes, ruhs);
> > > + if (ret) {
> > > + log_err("%s: nvme_fdp_reclaim_unit_handle_status failed, err=%d\n",
> > > + f->file_name, ret);
> > > + errno = ENOTSUP;
> > > + }
> > > +
> > > + close(fd);
> > > + return -errno;
> > should be "return ret;"
>
> The return needs to be a negative number, and 'ret' can sometimes be positive
> or negative.
ok, in that case errno should be set to 0, before you call
nvme_fdp_reclaim_unit_handle_status.
As I see from the caller function (io_uring.c)
+ ret = fio_nvme_iomgmt_ruhs(td, f, ruhs, bytes);
+ if (ret)
+ goto free;
Currently when I try I get:
/dev/ng0n1: fdp fetch ruhs failed (17).
fio: ruh info failed for /dev/ng0n1 (17).
fio: pid=0, err=17/file:fdp.c:35, func=fdp fetch ruhs failed, error=File exists
>
> > > +/*
> > > + * Maximum number of RUHS to fetch
> > > + * TODO: Revisit this so that we can work with more than 1024 RUH's
> > > + */
> > For me, I will send a fix once these changes get merged.
>
> Could you help clarify some spec confusion on my side? The Namespace Management
> command allows you to specify at most 128 placement handles when you create the
> namespace, so why would we need more than that? Under what conditions could the
> namespaces' IO Mgmt Recv RUH Status ever return more than 128 descriptors?
Frankly, I never thought of this. I have only tried with qemu setup which can
return more than 128 descriptors (I even got more than 1024).
In that case, should it just be limited to 128?
>
> And speaking of that, since 128 is the largest value, it seems odd the spec
> folks gave this field two bytes.
>
Certainly seems odd.
