io_uring: Add 'readfua' and 'writefua' options

[minwooim]

io_uring: Add 'readfua' and 'writefua' options

Provide options to set the FUA flag in CDW12 in the NVMe command. FUA
affects the internal operation of the NVMe controller and is used for
testing. In this patchset we expand readfua and writefua options to
directly control FUA flag in io_uring_cmd engine.

Signed-off-by: Minwoo Im [email protected]

[axboe]

I seem to be missing something here, because who actually acts on the FUA being set? I just see the option being checked and it added to the io_u, but nothing apart from that.

[vincentkfu]

Yes the code setting the FUA flag in the NVMe command is missing.

[minwooim]

Sorry for the noise, I missed that one while cleaning up the patch. Will update soon.

[axboe]

While making it actually work, please also consider setting CDW12 flags at setup time. Then the command side setup can be simply

cmd->cdw12 |= o->cdw12_flags;

or something like that, rather than having pretty ugly:

if ((read && readfua) || (write && writefua))
...

branches in the hot path. May obviously want to split o->cdw12_flags into a read and write side, but could be indexed by ddir or something like that. Anyway, above is obviously just pseudo code, but let's not be lazy and just add branches when it can be done much better.

[minwooim]

While making it actually work, please also consider setting CDW12 flags at setup time. Then the command side setup can be simply

cmd->cdw12 |= o->cdw12_flags;

I'm not sure that I'm properly understand what you meant, but do you mean to add 'cdw12_flags' as a option to make user give CDW12 actual value? If so, existing dtype and dspec might be overwritten to user-given value.

If I miss something here, please let me know :)

[vincentkfu]

If I understand correctly, Jens is suggesting a way to avoid having if ((read && readfua) || (write && writefua)) in the hot path.

Declare cdw12_flags[DDIR_RWDIR_CNT] in struct ioring_data and then set the appropriate bits when the readfua and writefua options are enabled. Then just have cmd->cdw12 |= o->cdw12_flags[ddir]; to set the appropriate bits in the NVMe command. Since the FUA bit is either always set or always unset we can avoid checking whether the option is enabled for every single I/O.

[minwooim]

I updated the patch applying review comments, but I have a doubt here that It's a good way to add an additional argument to fio_nvme_uring_cmd_prep() for cdw12_flags, or do we have to export ioring_data to a header file and make engines/nvme.c and engines/io_uring.c share the structure?

[vincentkfu]

Please change this to uint32_t just in case this is ever compiled one day on a platform where int is not 4 bytes.

[minwooim]

Updated, Thanks !

[vincentkfu]

I updated the patch applying review comments, but I have a doubt here that It's a good way to add an additional argument to fio_nvme_uring_cmd_prep() for cdw12_flags, or do we have to export ioring_data to a header file and make engines/nvme.c and engines/io_uring.c share the structure?

We don't want to export ioring_data to a header file. So adding another parameter is fine.