Re: [PATCH 1/2] engines/io_uring: support r/w with metadata

Jens Axboe
2025-07-23T19:23:31Z

On 7/23/25 12:28 PM, Vincent Fu wrote:
> On 7/23/25 1:37 PM, Jens Axboe wrote:
>>>> + if (!strcmp(td->io_ops->name, "io_uring") && o->md_per_io_size) {
>>>> + struct nvme_data *data = FILE_ENG_DATA(io_u->file);
>>>> + struct nvme_cmd_ext_io_opts ext_opts = {0};
>>>> +
>>>> + if (data->pi_type) {
>>>> + if (o->pi_act)
>>>> + ext_opts.io_flags |= NVME_IO_PRINFO_PRACT;
>>>> +
>>>> + ext_opts.io_flags |= o->prchk;
>>>> + ext_opts.apptag = o->apptag;
>>>> + ext_opts.apptag_mask = o->apptag_mask;
>>>> + }
>>>> + fio_nvme_generate_guard(io_u, &ext_opts);
>>>> + }
>>>
>>> Ehh a strcmp() in the hot path?! First of all, that's a big no-no.
>>> Secondly, if this really was required, you'd add something to put that
>>> strcmp() in the slow path and flag it. Lastly, thankfully this should be
>>> much better as:
>>>
>>> if (td->io_ops == &ioengine_uring ...)
>>>
>>> instead.
>>
>> Eh I guess dynamically loaded engines would need special treatment. I'll
>> take a look. In any case, strcmp() is just too ugly to live, actually
>> quite a few in there and the io_uring engine is the only one that thinks
>> this is necessary.
>>
>
> Thanks for the feedback. How about doing a strcmp at ioengine init
> time and setting a flag that we can check in the hot path?

You can do that if you want, that'd be fine too. I did io_ops->prep
compare, which is a bit annoying in that it's a double deref to get
there. But at least it's not a strcmp! If you check the repo now, some
cleanups done too, last couple of feature additions to the io_uring
engine have not really been up to snuff imho.

--
Jens Axboe