On 10/30/19 8:02 AM, Bijan Mottahedeh wrote: >> OK, so I still don't quite see where the issue is. Your setup has more >> than one CPU per poll queue, and I can reproduce the issue quite easily >> here with a similar setup. > > That's probably why I couldn't reproduce this in a vm. This time I set > up one poll queue in a 8 cpu vm and reproduced it. You definitely do need poll queue sharing to hit this. >> Below are some things that are given: >> >> 1) If we fail to submit the IO, io_complete_rw_iopoll() is ultimately >> invoked _from_ the submission path. This means that the result is >> readily available by the time we go and check: >> >> if (req->result == -EAGAIN) >> >> in __io_submit_sqe(). > > Is that always true? > > Let's say the operation was __io_submit_sqe()->io_read() > > By "failing to submit the io", do you mean that > io_read()->call_read_iter() returned success or failure? Are you saying > that req->result was set from kiocb_done() or later in the block layer? By "failed to submit" I mean that req->result == -EAGAIN. We set that in io_complete_rw_iopoll(), which is called off bio_wouldblock_error() in the block layer. This is different from an ret != 0 return, which would have been some sort of other failure we encountered, failing to submit the request. For that error, we just end the request. For the bio_wouldblock_error(), we need to retry submission. > Anyway I assume that io_read() would have to return success since > otherwise __io_submit_sqe() would immediately return and not check > req->result: > > if (ret) > return ret; Right, if ret != 0, then we have a fatal error for that request. ->result will not have been set at all for that case. > So if io_read() did return success, are we guaranteed that setting > req->result = -EAGAIN would always happen before the check? Always, since it happens inline from when you attempt to issue the read. > Also, is it possible that we can be preempted in __io_submit_sqe() after > the call to io_read() but before the -EAGAIN check? Sure, we're not disabling preemption. But that shouldn't have any bearing on this case. > >> This is a submission time failure, not >> something that should be happening from a completion path after the >> IO has been submitted successfully. >> >> 2) If the succeed in submitting the request, given that we have other >> tasks polling, the request can complete any time. It can very well be >> complete by the time we call io_iopoll_req_issued(), and this is >> perfectly fine. We know the request isn't going away, as we're >> holding a reference to it. kiocb has the same lifetime, as it's >> embedded in the io_kiocb request. Note that this isn't the same >> io_ring_ctx at all, some other task with its own io_ring_ctx just >> happens to find our request when doing polling on the same queue >> itself. > > Ah yes, it's a different io_ring_ctx, different poll list etc. For my Exactly > own clarity, I assume all contexts are mapping the same actual sq/cq > rings? The ring/context isn't mapped to a particular ring, a specific IO is mapped to a specific queue at the time it's being submitted. That depends on the IO type and where that task happens to be running at the time the IO is being submitted. -- Jens Axboe
