* [Bug] io_uring_register_files_update broken @ 2021-06-30 21:14 Victor Stewart 2021-06-30 21:27 ` Pavel Begunkov 2021-07-01 14:50 ` Pavel Begunkov 0 siblings, 2 replies; 7+ messages in thread From: Victor Stewart @ 2021-06-30 21:14 UTC (permalink / raw) To: io-uring i'm fairly confident there is something broken with io_uring_register_files_update, especially the offset parameter. when trying to update a single fd, and getting a successful result of 1, proceeding operations with IOSQE_FIXED_FILE fail with -9. but if i update all of the fds with then my recv operations succeed, but close still fails with -9. on Clear LInux 5.12.13-1050.native here's a diff for liburing send_recv test, to demonstrate this. diff --git a/test/send_recv.c b/test/send_recv.c index 19adbdd..492b591 100644 --- a/test/send_recv.c +++ b/test/send_recv.c @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv over io_uring!"; # define io_uring_prep_recv io_uring_prep_read #endif +static int *fds; + static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock, int registerfiles) { @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock, goto err; } + fds = malloc(100 * sizeof(int)); + memset(fds, 0xff, sizeof(int) * 100); + if (registerfiles) { - ret = io_uring_register_files(ring, &sockfd, 1); + ret = io_uring_register_files(ring, fds, 100); if (ret) { fprintf(stderr, "file reg failed\n"); goto err; } - use_fd = 0; - } else { - use_fd = sockfd; + + fds[sockfd] = sockfd; + int result = io_uring_register_files_update(ring, sockfd, fds, 1); + + if (result != 1) + { + fprintf(stderr, "file update failed\n"); + goto err; + } } + use_fd = sockfd; + sqe = io_uring_get_sqe(ring); io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0); if (registerfiles) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Bug] io_uring_register_files_update broken 2021-06-30 21:14 [Bug] io_uring_register_files_update broken Victor Stewart @ 2021-06-30 21:27 ` Pavel Begunkov 2021-07-01 14:01 ` Daniele Salvatore Albano 2021-07-01 14:50 ` Pavel Begunkov 1 sibling, 1 reply; 7+ messages in thread From: Pavel Begunkov @ 2021-06-30 21:27 UTC (permalink / raw) To: Victor Stewart, io-uring On 6/30/21 10:14 PM, Victor Stewart wrote: > i'm fairly confident there is something broken with > io_uring_register_files_update, > especially the offset parameter. > > when trying to update a single fd, and getting a successful result of > 1, proceeding > operations with IOSQE_FIXED_FILE fail with -9. but if i update all of > the fds with > then my recv operations succeed, but close still fails with -9. > > on Clear LInux 5.12.13-1050.native Thanks for letting know, I'll take a look > here's a diff for liburing send_recv test, to demonstrate this. > > diff --git a/test/send_recv.c b/test/send_recv.c > index 19adbdd..492b591 100644 > --- a/test/send_recv.c > +++ b/test/send_recv.c > @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv > over io_uring!"; > # define io_uring_prep_recv io_uring_prep_read > #endif > > +static int *fds; > + > static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock, > int registerfiles) > { > @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct > iovec *iov, int *sock, > goto err; > } > > + fds = malloc(100 * sizeof(int)); > + memset(fds, 0xff, sizeof(int) * 100); > + > if (registerfiles) { > - ret = io_uring_register_files(ring, &sockfd, 1); > + ret = io_uring_register_files(ring, fds, 100); > if (ret) { > fprintf(stderr, "file reg failed\n"); > goto err; > } > - use_fd = 0; > - } else { > - use_fd = sockfd; > + > + fds[sockfd] = sockfd; > + int result = io_uring_register_files_update(ring, > sockfd, fds, 1); > + > + if (result != 1) > + { > + fprintf(stderr, "file update failed\n"); > + goto err; > + } > } > > + use_fd = sockfd; > + > sqe = io_uring_get_sqe(ring); > io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0); > if (registerfiles) > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] io_uring_register_files_update broken 2021-06-30 21:27 ` Pavel Begunkov @ 2021-07-01 14:01 ` Daniele Salvatore Albano 2021-07-01 14:31 ` Pavel Begunkov 0 siblings, 1 reply; 7+ messages in thread From: Daniele Salvatore Albano @ 2021-07-01 14:01 UTC (permalink / raw) To: Pavel Begunkov; +Cc: Victor Stewart, io-uring On Thu, 1 Jul 2021 at 00:28, Pavel Begunkov <[email protected]> wrote: > > On 6/30/21 10:14 PM, Victor Stewart wrote: > > i'm fairly confident there is something broken with > > io_uring_register_files_update, > > especially the offset parameter. > > > > when trying to update a single fd, and getting a successful result of > > 1, proceeding > > operations with IOSQE_FIXED_FILE fail with -9. but if i update all of > > the fds with > > then my recv operations succeed, but close still fails with -9. > > > > on Clear LInux 5.12.13-1050.native > > Thanks for letting know, I'll take a look > > > > here's a diff for liburing send_recv test, to demonstrate this. > > > > diff --git a/test/send_recv.c b/test/send_recv.c > > index 19adbdd..492b591 100644 > > --- a/test/send_recv.c > > +++ b/test/send_recv.c > > @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv > > over io_uring!"; > > # define io_uring_prep_recv io_uring_prep_read > > #endif > > > > +static int *fds; > > + > > static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock, > > int registerfiles) > > { > > @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct > > iovec *iov, int *sock, > > goto err; > > } > > > > + fds = malloc(100 * sizeof(int)); > > + memset(fds, 0xff, sizeof(int) * 100); > > + > > if (registerfiles) { > > - ret = io_uring_register_files(ring, &sockfd, 1); > > + ret = io_uring_register_files(ring, fds, 100); > > if (ret) { > > fprintf(stderr, "file reg failed\n"); > > goto err; > > } > > - use_fd = 0; > > - } else { > > - use_fd = sockfd; > > + > > + fds[sockfd] = sockfd; > > + int result = io_uring_register_files_update(ring, > > sockfd, fds, 1); > > + > > + if (result != 1) > > + { > > + fprintf(stderr, "file update failed\n"); > > + goto err; > > + } > > } > > > > + use_fd = sockfd; > > + > > sqe = io_uring_get_sqe(ring); > > io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0); > > if (registerfiles) > > > > -- > Pavel Begunkov I sent an email a while ago to raise a question about a potential bug related to close. Looks like the close doesn't support registered files (although I saw some code within a patch from Jens to fix it while I was investigating). Attached below On Fri, 21 May 2021 at 21:28, Daniele Salvatore Albano <[email protected]> wrote: > > Hi, > > Is there any specific reason for which io_close_prep returns EBADF if > using REQ_F_FIXED_FILE? > > I discovered my software was failing to close sockets when using fixed > files a while ago but I put it to the side, initially thinking it was > a bug I introduced in my code. > In recent days I picked it up again and after investigating it, it > looks like that, instead, that's the expected behaviour. > > From what I see, although the behaviour was slightly changed with a > couple of commits (ie. with > https://github.com/torvalds/linux/commit/cf3040ca55f2085b0a372a620ee2cb93ae19b686 > ) the io_close_prep have had this behaviour from the very beginning > https://github.com/torvalds/linux/commit/b5dba59e0cf7e2cc4d3b3b1ac5fe81ddf21959eb > . > > @Jens during my researches I have also found > https://lkml.org/lkml/2020/5/7/1575 where there is a patch that > allows, at least from what it looks like at a first glance, fixed > files with io_close_prep but seems that the email thread died there. > > Shouldn't the close op match the behaviour of the other I/O related > ops when it comes to fds? > > If there aren't specific reasons, happy to look into it and write a patch. > > > Thanks, > Daniele ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] io_uring_register_files_update broken 2021-07-01 14:01 ` Daniele Salvatore Albano @ 2021-07-01 14:31 ` Pavel Begunkov 0 siblings, 0 replies; 7+ messages in thread From: Pavel Begunkov @ 2021-07-01 14:31 UTC (permalink / raw) To: Daniele Salvatore Albano; +Cc: Victor Stewart, io-uring On 7/1/21 3:01 PM, Daniele Salvatore Albano wrote: > On Thu, 1 Jul 2021 at 00:28, Pavel Begunkov <[email protected]> wrote: >> >> On 6/30/21 10:14 PM, Victor Stewart wrote: >>> i'm fairly confident there is something broken with >>> io_uring_register_files_update, >>> especially the offset parameter. >>> >>> when trying to update a single fd, and getting a successful result of >>> 1, proceeding >>> operations with IOSQE_FIXED_FILE fail with -9. but if i update all of >>> the fds with >>> then my recv operations succeed, but close still fails with -9. >>> >>> on Clear LInux 5.12.13-1050.native >> >> Thanks for letting know, I'll take a look >> [...] > I sent an email a while ago to raise a question about a potential bug > related to close. > > Looks like the close doesn't support registered files (although I saw > some code within a patch from Jens to fix it while I was > investigating). It have never been supported, don't remember it being discussed to be implemented at any point, and the links below are about unrelated things. If you want to close a fixed file, you can use IORING_OP_FILES_UPDATE with fd=-1 instead. Easy enough to add it to close, but not sure I see the reason for that considering existence of IORING_OP_FILES_UPDATE. fwiw, it doesn't sound related to Victor's report, but still need to look at it. > Attached below > > On Fri, 21 May 2021 at 21:28, Daniele Salvatore Albano > <[email protected]> wrote: >> >> Hi, >> >> Is there any specific reason for which io_close_prep returns EBADF if >> using REQ_F_FIXED_FILE? >> >> I discovered my software was failing to close sockets when using fixed >> files a while ago but I put it to the side, initially thinking it was >> a bug I introduced in my code. >> In recent days I picked it up again and after investigating it, it >> looks like that, instead, that's the expected behaviour. >> >> From what I see, although the behaviour was slightly changed with a >> couple of commits (ie. with >> https://github.com/torvalds/linux/commit/cf3040ca55f2085b0a372a620ee2cb93ae19b686 >> ) the io_close_prep have had this behaviour from the very beginning >> https://github.com/torvalds/linux/commit/b5dba59e0cf7e2cc4d3b3b1ac5fe81ddf21959eb >> . >> >> @Jens during my researches I have also found >> https://lkml.org/lkml/2020/5/7/1575 where there is a patch that >> allows, at least from what it looks like at a first glance, fixed >> files with io_close_prep but seems that the email thread died there. >> >> Shouldn't the close op match the behaviour of the other I/O related >> ops when it comes to fds? >> >> If there aren't specific reasons, happy to look into it and write a patch. >> >> >> Thanks, >> Daniele -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] io_uring_register_files_update broken 2021-06-30 21:14 [Bug] io_uring_register_files_update broken Victor Stewart 2021-06-30 21:27 ` Pavel Begunkov @ 2021-07-01 14:50 ` Pavel Begunkov 2021-07-01 15:46 ` Victor Stewart 1 sibling, 1 reply; 7+ messages in thread From: Pavel Begunkov @ 2021-07-01 14:50 UTC (permalink / raw) To: Victor Stewart, io-uring On 6/30/21 10:14 PM, Victor Stewart wrote: > i'm fairly confident there is something broken with > io_uring_register_files_update, > especially the offset parameter. > > when trying to update a single fd, and getting a successful result of > 1, proceeding > operations with IOSQE_FIXED_FILE fail with -9. but if i update all of > the fds with > then my recv operations succeed, but close still fails with -9. > > on Clear LInux 5.12.13-1050.native > > here's a diff for liburing send_recv test, to demonstrate this. > > diff --git a/test/send_recv.c b/test/send_recv.c > index 19adbdd..492b591 100644 > --- a/test/send_recv.c > +++ b/test/send_recv.c > @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv > over io_uring!"; > # define io_uring_prep_recv io_uring_prep_read > #endif > > +static int *fds; > + > static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock, > int registerfiles) > { > @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct > iovec *iov, int *sock, > goto err; > } > > + fds = malloc(100 * sizeof(int)); > + memset(fds, 0xff, sizeof(int) * 100); > + > if (registerfiles) { > - ret = io_uring_register_files(ring, &sockfd, 1); > + ret = io_uring_register_files(ring, fds, 100); > if (ret) { > fprintf(stderr, "file reg failed\n"); > goto err; > } > - use_fd = 0; > - } else { > - use_fd = sockfd; > + > + fds[sockfd] = sockfd; > + int result = io_uring_register_files_update(ring, > sockfd, fds, 1); s/fds/&fds[sockfd]/ Does it help? io_uring_register_files_update() doesn't apply offset parameter to the array, it's used only as an internal index. > + > + if (result != 1) > + { > + fprintf(stderr, "file update failed\n"); > + goto err; > + } > } > > + use_fd = sockfd; > + > sqe = io_uring_get_sqe(ring); > io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0); > if (registerfiles) > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] io_uring_register_files_update broken 2021-07-01 14:50 ` Pavel Begunkov @ 2021-07-01 15:46 ` Victor Stewart 2021-07-02 0:20 ` Pavel Begunkov 0 siblings, 1 reply; 7+ messages in thread From: Victor Stewart @ 2021-07-01 15:46 UTC (permalink / raw) To: Pavel Begunkov; +Cc: io-uring On Thu, Jul 1, 2021 at 3:51 PM Pavel Begunkov <[email protected]> wrote: > > On 6/30/21 10:14 PM, Victor Stewart wrote: > > i'm fairly confident there is something broken with > > io_uring_register_files_update, > > especially the offset parameter. > > > > when trying to update a single fd, and getting a successful result of > > 1, proceeding > > operations with IOSQE_FIXED_FILE fail with -9. but if i update all of > > the fds with > > then my recv operations succeed, but close still fails with -9. > > > > on Clear LInux 5.12.13-1050.native > > > > here's a diff for liburing send_recv test, to demonstrate this. > > > > diff --git a/test/send_recv.c b/test/send_recv.c > > index 19adbdd..492b591 100644 > > --- a/test/send_recv.c > > +++ b/test/send_recv.c > > @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv > > over io_uring!"; > > # define io_uring_prep_recv io_uring_prep_read > > #endif > > > > +static int *fds; > > + > > static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock, > > int registerfiles) > > { > > @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct > > iovec *iov, int *sock, > > goto err; > > } > > > > + fds = malloc(100 * sizeof(int)); > > + memset(fds, 0xff, sizeof(int) * 100); > > + > > if (registerfiles) { > > - ret = io_uring_register_files(ring, &sockfd, 1); > > + ret = io_uring_register_files(ring, fds, 100); > > if (ret) { > > fprintf(stderr, "file reg failed\n"); > > goto err; > > } > > - use_fd = 0; > > - } else { > > - use_fd = sockfd; > > + > > + fds[sockfd] = sockfd; > > + int result = io_uring_register_files_update(ring, > > sockfd, fds, 1); > > s/fds/&fds[sockfd]/ > > Does it help? io_uring_register_files_update() doesn't > apply offset parameter to the array, it's used only as > an internal index. i see yes, it works it like this! io_uring_register_files_update(&ring, fd, &(socketfds[fd]), 1); io_uring_register_files_update(&ring, fd, &(socketfds[fd] = -1), 1); and this behavior is clear upon a closer reading of... https://github.com/axboe/liburing/blob/11f6d56302c177a96d7eb1df86995939a4feb736/test/file-register.c#L80 i guess it's sometimes ambiguous whether int* is requesting an array or an actual pointer to a single int. all good now. > > > + > > + if (result != 1) > > + { > > + fprintf(stderr, "file update failed\n"); > > + goto err; > > + } > > } > > > > + use_fd = sockfd; > > + > > sqe = io_uring_get_sqe(ring); > > io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0); > > if (registerfiles) > > > > -- > Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] io_uring_register_files_update broken 2021-07-01 15:46 ` Victor Stewart @ 2021-07-02 0:20 ` Pavel Begunkov 0 siblings, 0 replies; 7+ messages in thread From: Pavel Begunkov @ 2021-07-02 0:20 UTC (permalink / raw) To: Victor Stewart; +Cc: io-uring On 7/1/21 4:46 PM, Victor Stewart wrote: > On Thu, Jul 1, 2021 at 3:51 PM Pavel Begunkov <[email protected]> wrote: [...] >>> sockfd, fds, 1); >> >> s/fds/&fds[sockfd]/ >> >> Does it help? io_uring_register_files_update() doesn't >> apply offset parameter to the array, it's used only as >> an internal index. > > i see yes, it works it like this! > > io_uring_register_files_update(&ring, fd, &(socketfds[fd]), 1); > io_uring_register_files_update(&ring, fd, &(socketfds[fd] = -1), 1); > > and this behavior is clear upon a closer reading of... > https://github.com/axboe/liburing/blob/11f6d56302c177a96d7eb1df86995939a4feb736/test/file-register.c#L80 > > i guess it's sometimes ambiguous whether int* is requesting an array > or an actual pointer to a single int. It's an array, just element 0 is registered as a registered file with index @off, element 1 as a reg-file with index @(off+1) and so on. > all good now. > >> >>> + >>> + if (result != 1) >>> + { >>> + fprintf(stderr, "file update failed\n"); >>> + goto err; >>> + } >>> } >>> >>> + use_fd = sockfd; >>> + >>> sqe = io_uring_get_sqe(ring); >>> io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0); >>> if (registerfiles) >>> >> >> -- >> Pavel Begunkov -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-02 0:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-30 21:14 [Bug] io_uring_register_files_update broken Victor Stewart 2021-06-30 21:27 ` Pavel Begunkov 2021-07-01 14:01 ` Daniele Salvatore Albano 2021-07-01 14:31 ` Pavel Begunkov 2021-07-01 14:50 ` Pavel Begunkov 2021-07-01 15:46 ` Victor Stewart 2021-07-02 0:20 ` Pavel Begunkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox