Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nvme/049: update fio_output msg check for io_uring_cmd support #152

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

yizhanglinux
Copy link
Contributor

Update the error info check string:
check: (g=0): rw=read, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1 fio-3.38-16-g6f3de
Starting 1 process
fio: pid=9638, err=1/file:engines/io_uring.c:1272, func=io_queue_init, error=Operation not permitted

Run status group 0 (all jobs):

@kawasaki
Copy link
Collaborator

The patch changes to the error string to check from "Permission Denied" (EACCES) to "Operation not permitted" (EPERM). As recorded in the past discussion, I suggested to add the check for EACCES to confirm that SELinux allows io_uring_cmd. So I think still we need the EACCES check.

Then I suggest to check both EACCES and EPERM as below.

diff --git a/tests/nvme/049 b/tests/nvme/049
index 599ab58..f25cba8 100755
--- a/tests/nvme/049
+++ b/tests/nvme/049
@@ -37,7 +37,8 @@ test_device() {
        # check security permission
        if ! fio_output=$(fio --name=check --size=4k --filename="$ngdev" \
                            --rw=read --ioengine=io_uring_cmd 2>&1) &&
-                       grep -qe "Permission denied" <<< "$fio_output"; then
+                       grep -q -e "Operation not permitted" \
+                             -e "Permission denied" <<< "$fio_output"; then
                SKIP_REASONS+=("IORING_OP_URING_CMD is not allowed for $ngdev")
                return
        fi

One point I wonder is why the EPERM is observed. Regardless of this question, I think still the check for both EACCES and EPERM will be a good change.

@yizhanglinux
Copy link
Contributor Author

Here is the 6.13 which I download from and reproduce environment, in case you want to have a try.

[1]
https://kojipkgs.fedoraproject.org//packages/kernel/6.13.0/
[2]

[root@storageqe-40 blktests]# uname -r
6.13.0-0.rc2.22.eln144.x86_64
[root@storageqe-40 blktests]# fio --version
fio-3.36
[root@storageqe-40 blktests]# cat /proc/sys/kernel/io_uring_disabled
2
[root@storageqe-40 blktests]# fio --name=check --size=4K --filename=/dev/ng0n1 --rw=rw --ioengine=io_uring_cmd
check: (g=0): rw=rw, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
fio-3.36
Starting 1 process
fio: pid=5601, err=1/file:engines/io_uring.c:1090, func=io_queue_init, error=Operation not permitted


Run status group 0 (all jobs):

@yizhanglinux
Copy link
Contributor Author

yizhanglinux commented Dec 17, 2024

Update the patch to include the two strings.

Update the error info check string with both "Permission Denied" (EACCES)
and "Operation not permitted" (EPERM)

check: (g=0): rw=read, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
fio-3.38-16-g6f3de
Starting 1 process
fio: pid=9638, err=1/file:engines/io_uring.c:1272, func=io_queue_init, error=Operation not permitted

Run status group 0 (all jobs):

Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
@kawasaki kawasaki merged commit 54d0175 into osandov:master Dec 18, 2024
5 checks passed
@kawasaki
Copy link
Collaborator

@yizhanglinux Thanks for the update. I've merged it.

@kawasaki
Copy link
Collaborator

[root@storageqe-40 blktests]# cat /proc/sys/kernel/io_uring_disabled
2

Now I see when fio reports "Operation not permitted". With the merged fix, the test case should be skipped when io uring is disabled. I guess introducing _io_uring_enable/restore will make the test case run when io uring is disabled. But this is beyond the scope of this PR.

@yizhanglinux yizhanglinux deleted the nvme-049-update branch December 20, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants