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 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. -- Jens Axboe
