ioctl: get_log_page by nvme uring cmd

[996refuse]

hello libnvme master,

io_uring on linux is pretty mature now, for the system with liburing, do the batch nvme_get_log by nvme uring cmd.
This change could gain about 10% performance improvement for some large log pages on my lab.

i believe the performance can be better if implement producer and consumer in two separated threads, if it's worth and acceptable, i can spend some time on this later.

thanks

[igaw]

I am fine with using uring but would it be possible to have it fallback during runtime to the ioctl interface when uring is not available, like on older kernels?

[igaw]

Could we hook this up on library load time, so it only tests once if the io_uring is available?

Also it should not print an error at all, this would spam the logs when io_uring is not available. Internal function shouldn't use the POSIX style of error propagation (setting errno and return -1). The plan is to fix this once we work on the next major version.

[996refuse]

thanks for your clarification.

Could we hook this up on library load time, so it only tests once if the io_uring is available?

correct me if i'm wrong, i think it's not possible to catch the error on .so library loading time in an ordinary way.
nvme: error while loading shared libraries: liburing.so.2: cannot open shared object file: No such file or directory
ld.so looks for the .so from the ELF dynamic section then load the .so, then run the ELF .text code.
when the error is occurs, the program has not started yet.
we can hack to load the .so on program runtime, but it's not recommended.

what if add a parameter for get-log to give an explicit instruction to use io_uring? and by default, this feature is not enabled, what do you think?
nvme get-log /dev/nvme2 --log-id=0x00 --log-len=0x1000 --uring-cmd

[igaw]

Indeed, my idea doesn't work. We don't have a nvme_init_lib function yet. So the simplest thing is to introduce a global static variable which tracks if io_uring is available. That doesn't need to be race free, so no locks or so are necessary. Worst case if two threads are figuring out that io_uring is available or not at the same time and both should come to the same conclusion.

[996refuse]

what the two threads are, could help explain it please? two threads do same task is inefficient, increasing the libnvme runtime complexity.

maybe we too overcomplicated the thing. by default, compile time disabled the feature, remove liburing.so and io_uring syscall dependencies. waiting for liburing being accepted by main linux distributions widely, then switch it to auto.

[igaw]

Just leave the meson auto detection in place. This part is fine.

No, it's very simple. If a library is loaded into process context, the application could run several threads using libnvme. This is perfectly fine, e.g. if there are multiple devices. So we don't want to have the detection everytime for each single command send out. So this part just needs to be done once. The io_uring detection though can't be made 'thread safe' without using global locks which is not really a good idea. So my point, it doesn't matter. We just need eventually consistency.

With this in mind, something like this is good enough:

enum io_uring_state {
  io_uring_state_detect, 
  io_uring_state_available,
  io_uring_state_not_available,
};
enum io_uring_state io_uring_state = io_uring_state_detect;


if (io_uring_state == io_uring_detetct) {
  if (io_uring_opcode_supported(probe, IORING_OP_URING_CMD))
    io_uring_state =  io_uring_available;
  else
    io_uring_state = io_uring_not_available;
}

[996refuse]

brilliant idea, got it

[996refuse]

i prefer constructor, which is a gcc specific attribute, the function attached is called when libnvme.so is loaded.

i tested the fallback mechanism manually by io_uring_disabled sysctl, it works well
sysctl -w kernel.io_uring_disabled=2