On 3/2/20 1:45 PM, Pavel Begunkov wrote:
> Get next request when dropping the submission reference. However, if
> there is an asynchronous counterpart (i.e. read/write, timeout, etc),
> that would be dangerous to do, so ignore them using new
> REQ_F_DONT_STEAL_NEXT flag.
Hmm, not so sure I like this one. It's not quite clear to me where we
need REQ_F_DONT_STEAL_NEXT. If we have an async component, then we set
REQ_F_DONT_STEAL_NEXT. So this is generally the case where our
io_put_req() for submit is not the last drop. And for the other case,
the put is generally in the caller anyway. So I don't really see what
this extra flag buys us?
Few more comments below.
> +static void io_put_req_async_submission(struct io_kiocb *req,
> + struct io_wq_work **workptr)
> +{
> + static struct io_kiocb *nxt;
> +
> + nxt = io_put_req_submission(req);
> + if (nxt)
> + io_wq_assign_next(workptr, nxt);
> +}
This really should be called io_put_req_async_completion() since it's
called on completion. The naming is confusing.
> @@ -2581,14 +2598,11 @@ static void __io_fsync(struct io_kiocb *req)
> static void io_fsync_finish(struct io_wq_work **workptr)
> {
> struct io_kiocb *req = container_of(*workptr, struct io_kiocb, work);
> - struct io_kiocb *nxt = NULL;
>
> if (io_req_cancelled(req))
> return;
> __io_fsync(req);
> - io_put_req(req); /* drop submission reference */
> - if (nxt)
> - io_wq_assign_next(workptr, nxt);
> + io_put_req_async_submission(req, workptr);
> }
>
> static int io_fsync(struct io_kiocb *req, bool force_nonblock)
> @@ -2617,14 +2631,11 @@ static void __io_fallocate(struct io_kiocb *req)
> static void io_fallocate_finish(struct io_wq_work **workptr)
> {
> struct io_kiocb *req = container_of(*workptr, struct io_kiocb, work);
> - struct io_kiocb *nxt = NULL;
>
> if (io_req_cancelled(req))
> return;
> __io_fallocate(req);
> - io_put_req(req); /* drop submission reference */
> - if (nxt)
> - io_wq_assign_next(workptr, nxt);
> + io_put_req_async_submission(req, workptr);
> }
All of these cleanups are nice (except the naming, as mentioned).
> @@ -3943,7 +3947,10 @@ static int io_poll_add(struct io_kiocb *req)
> if (mask) {
> io_cqring_ev_posted(ctx);
> io_put_req(req);
> + } else {
> + req->flags |= REQ_F_DONT_STEAL_NEXT;
> }
> +
> return ipt.error;
> }
Is this racy? I guess it doesn't matter since we're still holding the
completion reference.
--
Jens Axboe
