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

[Jens Axboe <axboe@kernel.dk>]

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