On 09/09/2020 19:07, Jens Axboe wrote: > On 9/9/20 9:48 AM, Pavel Begunkov wrote: >> On 09/09/2020 16:10, Jens Axboe wrote: >>> On 9/9/20 1:09 AM, Pavel Begunkov wrote: >>>> On 09/09/2020 01:54, Jens Axboe wrote: >>>>> On 9/8/20 3:22 PM, Jens Axboe wrote: >>>>>> On 9/8/20 2:58 PM, Pavel Begunkov wrote: >>>>>>> On 08/09/2020 20:48, Jens Axboe wrote: >>>>>>>> Fd instantiating commands like IORING_OP_ACCEPT now work with SQPOLL, but >>>>>>>> we have an error in grabbing that if IOSQE_ASYNC is set. Ensure we assign >>>>>>>> the ring fd/file appropriately so we can defer grab them. >>>>>>> >>>>>>> IIRC, for fcheck() in io_grab_files() to work it should be under fdget(), >>>>>>> that isn't the case with SQPOLL threads. Am I mistaken? >>>>>>> >>>>>>> And it looks strange that the following snippet will effectively disable >>>>>>> such requests. >>>>>>> >>>>>>> fd = dup(ring_fd) >>>>>>> close(ring_fd) >>>>>>> ring_fd = fd >>>>>> >>>>>> Not disagreeing with that, I think my initial posting made it clear >>>>>> it was a hack. Just piled it in there for easier testing in terms >>>>>> of functionality. >>>>>> >>>>>> But the next question is how to do this right...> >>>>> Looking at this a bit more, and I don't necessarily think there's a >>>>> better option. If you dup+close, then it just won't work. We have no >>>>> way of knowing if the 'fd' changed, but we can detect if it was closed >>>>> and then we'll end up just EBADF'ing the requests. >>>>> >>>>> So right now the answer is that we can support this just fine with >>>>> SQPOLL, but you better not dup and close the original fd. Which is not >>>>> ideal, but better than NOT being able to support it. >>>>> >>>>> Only other option I see is to to provide an io_uring_register() >>>>> command to update the fd/file associated with it. Which may be useful, >>>>> it allows a process to indeed to this, if it absolutely has to. >>>> >>>> Let's put aside such dirty hacks, at least until someone actually >>>> needs it. Ideally, for many reasons I'd prefer to get rid of >>> >>> BUt it is actually needed, otherwise we're even more in a limbo state of >>> "SQPOLL works for most things now, just not all". And this isn't that >>> hard to make right - on the flush() side, we just need to park/stall the >> >> I understand that it isn't hard, but I just don't want to expose it to >> the userspace, a) because it's a userspace API, so couldn't probably be >> killed in the future, b) works around kernel's problems, and so >> shouldn't really be exposed to the userspace in normal circumstances. >> >> And it's not generic enough because of a possible "many fds -> single >> file" mapping, and there will be a lot of questions and problems. >> >> e.g. if a process shares a io_uring with another process, then >> dup()+close() would require not only this hook but also additional >> inter-process synchronisation. And so on. > > I think you're blowing this out of proportion. Just to restate the I just think that if there is a potentially cleaner solution without involving userspace, we should try to look for it first, even if it would take more time. That was the point. > goal, but it's to have SQPOLL be as useful as the other modes. One of > those things is making non-registered files work. For some use cases, > registered files is fine, for others it's basically a non-starter.> With that out of the way, the included patch handles the "close ring > fd case". You're talking about the dup or receive case, or anything > that doesn't close an existing ring. And yes, that won't work as-is, > because we know have multiple fds for that particular ring. That boils > the case down to "we're now using this fd for the ring", and the only > requirement here would be to ensure you do a io_uring_enter() if you > decide to swap fds or use a new fd. Only caveat here is that we can't > make it automatic like we can for the "old fd gets closed" case, so > the app would absolutely have to ensure it enters the kernel if it > uses a new fd. > > Not really a huge deal imho in terms of API, especially since this > is into the realm of "nobody probably ever does this, or if they do, > then this requirement isn't really a problem". > >>> thread and clear the ring_fd/ring_file, then mark the ring as needing a >>> queue enter. On the queue enter, we re-set the fd/file if they're NULL, >>> unpark/unstall the thread. That's it. I'll write this up and test it. >>> >>>> fcheck(ctx->ring_fd) in favour of synchronisation in io_grab_files(), >>>> but I wish I knew how. >>> >>> That'd be nice, and apply equally to all cases as the SQPOLL case isn't >>> special at all anymore. >> >> I miss the whole story, have you asked fs guys about the problem? >> Or is it known that nothing would work? > > I haven't looked into it. Any chance you have someone in mind who can take a look? I don't think I have a chance to get to anyone in fs. -- Pavel Begunkov
