io_uring: Add .errdetails to parse CQ status #1804
Conversation
engines/io_uring.c
Outdated
| * Check the filename to figure out cmd_type since we don't have | ||
| * thread_data here. | ||
| */ | ||
| ret = sscanf(io_u->file->file_name, "/dev/ng%dn%d", &ctrlid, &nsid); |
There was a problem hiding this comment.
Can you just use strstr to check if the filename begins with /dev/ng? Then there is no need to have the ctrlid and nsid variables which are ultimately unused.
engines/io_uring.c
Outdated
| snprintf(msgchunk, MAXMSGCHUNK, "sc=0x%02x", sc); | ||
| strlcat(msg, msgchunk, MAXERRDETAIL); | ||
|
|
||
| strlcat(msg, ")", MAXERRDETAIL); |
There was a problem hiding this comment.
Can you remove this strlcat and just add the ) to the snprtinf call above this one?
|
This looks fine. Just two small changes requested. |
Applied all the changes, Thanks for the review. |
engines/io_uring.c
Outdated
| * Check the filename to figure out cmd_type since we don't have | ||
| * thread_data here. | ||
| */ | ||
| if (strstr(io_u->file->file_name, "/dev/ng")) { |
There was a problem hiding this comment.
This is pretty terrible, to be honest. You cannot impute type of device based on the name of the device special node. It's perfectly valid to have different names for these. This will need a rethink to be workable.
There was a problem hiding this comment.
I can think of two alternatives:
- Change
.errdetailsto also take astruct thread_dataargument.tdgives you access to the ioengine options which you can use to find thecmd_type. You would need to bumpFIO_IOOPS_VERSION. - Add a pointer to
tdtostruct io_u.
The first choice seems like the better approach to me.
There was a problem hiding this comment.
First choice definitely the better one. And yes, it's not a problem to change the prototype of the arguments. Just remember to change FIO_IOOPS_VERSION as well.
|
Please update the commit message for the patch adding Also you might as well squash the first two patches that change the function prototype and bump the version since they need to go together. |
No functional changes here, but added a 'struct thread_data *td' to the errdetails callback. This is a prep patch for the following commits to access 'td->eo' instance from .errdetails callback. Bump up FIO_IOOPS_VERSION to 36 since the previous commits updated .errdetails callback for ioengines by adding 'thread_data' argument. Signed-off-by: Minwoo Im <[email protected]>
Background - fio normally prints out the strerr and errno value when facing errors. In case of io_uring_cmd ioengine with --cmd_type=nvme, io_u->error represents the CQ entry status code type and status code which should be parsed as a NVMe error value rather than errno. In io_u error failure condition, it prints out parsed CQ entry error status values with SCT(Status Code Type) and SC(Status Code). The print will be like the following example: fio: io_uring_cmd: /dev/ng0n1: cq entry status (sct=0x00; sc=0x04) If --cmd_type!=nvme, it prints out generic status code like below: fio: io_uring_cmd: /dev/<devnode>: status=0x4 Signed-off-by: Minwoo Im <[email protected]>
Updated, Thanks for your review. |
|
This looks good to me now. |
Background
In io_u error failure condition, it prints out parsed CQ entry error status values with SCT(Status Code Type) and SC(Status Code). The print will be like the following example:
fio: io_uring_cmd: /dev/ng0n1: cq entry status (sct=0x00; sc=0x04)
If --cmd_type!=nvme, it prints out generic status code like below:
fio: io_uring_cmd: /dev/: status=0x4