lkml.org 
[lkml]   [2020]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 4/4] io_uring: get next req on subm ref drop
From
Date
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

\
 
 \ /
  Last update: 2020-03-03 05:26    [W:0.098 / U:7.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site