axboe / liburing

Library providing helpers for the Linux kernel io_uring support
MIT License
2.86k stars 402 forks source link

Confused about `IOSQE_FIXED_FILE` #1253

Closed aconz2 closed 3 weeks ago

aconz2 commented 3 weeks ago

In the man pages for io_uring_prep_openat_direct it says

... To do so, IOSQE_FIXED_FILE must be set in the SQE flags member, and the SQE fd field should use the direct descriptor value rather than the regular file descriptor.

But the source for (v6.11) __io_openat_prep shows that if REQ_F_FIXED_FILE is set, then we get EBADF. (from my reading, req gets its flags initialized from the sqe and REQ_F_FIXED_FILE corresponds to checking if IOSQE_FIXED_FILE was set). Indeed, if I make this change:

diff --git a/test/fixed-reuse.c b/test/fixed-reuse.c
index 401251a..1d6d8d0 100644
--- a/test/fixed-reuse.c
+++ b/test/fixed-reuse.c
@@ -33,6 +33,7 @@ static int test(struct io_uring *ring)
        sqe = io_uring_get_sqe(ring);
        io_uring_prep_openat_direct(sqe, AT_FDCWD, FNAME1, O_RDONLY, 0, 0);
        sqe->user_data = 1;
+       sqe->flags |= IOSQE_FIXED_FILE;

        ret = io_uring_submit(ring);
        if (ret != 1) {
@@ -46,7 +47,7 @@ static int test(struct io_uring *ring)
                goto err;
        }
        if (cqe->res != 0) {
-               fprintf(stderr, "open res %d\n", ret);
+               fprintf(stderr, "open res %d\n", cqe->res);
                goto err;
        }
        io_uring_cqe_seen(ring, cqe);

then it prints open res -9 test failed.

I've gone round and round in the kernel but think I understand now that sqe->file_index is set for ops (when you want fixed file) that create an fd but shouldn't have IOSQE_FIXED_FILE set, while ops that use an fd (their opdef has needs_file) like read/write should have sqe->fd set with IOSQE_FIXED_FILE set. And close can use either fd xor file_index but never uses IOSQE_FIXED_FILE. Does that sound about right?

I had a followup about whether io_uring_read_direct could be added, but I'm not sure anymore. On the one hand it is consistent in that if you use a _direct the fd you pass is a fixed fd, but on the other, it is inconsistent if you understand that a _direct sets file_index (with handling for asking for the kernel to allocate in your table) and without _direct sets fd.

Can send a PR if you agree with my assessment on the openat man page if you'd like. Thanks!

isilence commented 3 weeks ago

I'd say, IOSQE_FIXED_FILE is for requests that take a file as an argument and using it to do something, so the man page from above lies. "direct" is for requests manipulating io_uring fixed file table, like creating a file and installing it there or removing a file. There can be some inconsistencies, but that's because it was gradually developed and we can't change the ABI.

io_uring_read_direct would be confusing, exactly because reads just use the file and don't alter the file table.

axboe commented 3 weeks ago

It would indeed be nice to have a bit of consistency in terms of how requests are prepared, but at least for the open case, as Pavel says, IOSQE_FIXED_FILE is set when you're passing in a fixed file descriptor. When creating one, it doesn't really make sense.

Would love a PR with a man page update! Thanks.

aconz2 commented 3 weeks ago

Awesome makes sense, I understand io_uring a bit better now, thanks very much! To be honest after re-reading, the quote I pulled isn't directly wrong, but was easy to get confused that it was referring to subsequent operations not the current one. I opened https://github.com/axboe/liburing/pull/1254 with something that seems a bit clearer to me.