Re: [PATCH] io_uring: use task_work for links if possible

[Pavel Begunkov <asml.silence@gmail.com>]

On 25/06/2020 21:27, Jens Axboe wrote:
> Currently links are always done in an async fashion, unless we
> catch them inline after we successfully complete a request without
> having to resort to blocking. This isn't necessarily the most efficient
> approach, it'd be more ideal if we could just use the task_work handling
> for this.
> 
> Outside of saving an async jump, we can also do less prep work for
> these kinds of requests.
> 
> Running dependent links from the task_work handler yields some nice
> performance benefits. As an example, examples/link-cp from the liburing
> repository uses read+write links to implement a copy operation. Without
> this patch, the a cache fold 4G file read from a VM runs in about
> 3 seconds:

A few comments below


> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 0bba12e4e559..389274a078c8 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -898,6 +898,7 @@ enum io_mem_account {
...
> +static void __io_req_task_submit(struct io_kiocb *req)
> +{
> +	struct io_ring_ctx *ctx = req->ctx;
> +
> +	__set_current_state(TASK_RUNNING);
> +	if (!io_sq_thread_acquire_mm(ctx, req)) {
> +		mutex_lock(&ctx->uring_lock);
> +		__io_queue_sqe(req, NULL, NULL);
> +		mutex_unlock(&ctx->uring_lock);
> +	} else {
> +		__io_req_task_cancel(req, -EFAULT);
> +	}
> +}
> +
> +static void io_req_task_submit(struct callback_head *cb)
> +{
> +	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
> +
> +	__io_req_task_submit(req);
> +}
> +
> +static void io_req_task_queue(struct io_kiocb *req)
> +{
> +	struct task_struct *tsk = req->task;
> +	int ret;
> +
> +	init_task_work(&req->task_work, io_req_task_submit);
> +
> +	ret = task_work_add(tsk, &req->task_work, true);
> +	if (unlikely(ret)) {
> +		init_task_work(&req->task_work, io_req_task_cancel);

Why not to kill it off here? It just was nxt, so shouldn't anything like
NOCANCEL

> +		tsk = io_wq_get_task(req->ctx->io_wq);
> +		task_work_add(tsk, &req->task_work, true);
> +	}
> +	wake_up_process(tsk);
> +}
> +
>  static void io_free_req(struct io_kiocb *req)
>  {
>  	struct io_kiocb *nxt = NULL;
> @@ -1671,8 +1758,12 @@ static void io_free_req(struct io_kiocb *req)
>  	io_req_find_next(req, &nxt);
>  	__io_free_req(req);
>  
> -	if (nxt)
> -		io_queue_async_work(nxt);
> +	if (nxt) {
> +		if (nxt->flags & REQ_F_WORK_INITIALIZED)
> +			io_queue_async_work(nxt);

Don't think it will work. E.g. io_close_prep() may have set
REQ_F_WORK_INITIALIZED but without io_req_work_grab_env().

> +		else
> +			io_req_task_queue(nxt);
> +	}
>  }
>  
>  static void io_wq_assign_next(struct io_wq_work **workptr, struct io_kiocb *nxt)
> @@ -2013,12 +2104,6 @@ static void kiocb_end_write(struct io_kiocb *req)
>  	file_end_write(req->file);
>  }
>  
> -static inline void req_set_fail_links(struct io_kiocb *req)
> -{
> -	if ((req->flags & (REQ_F_LINK | REQ_F_HARDLINK)) == REQ_F_LINK)
> -		req->flags |= REQ_F_FAIL_LINK;
> -}
> -

I think it'd be nicer in 2 patches, first moving io_sq_thread_drop_mm, etc. up.
And the second one doing actual work. 

>  static void io_complete_rw_common(struct kiocb *kiocb, long res,
>  				  struct io_comp_state *cs)
>  {
> @@ -2035,35 +2120,6 @@ static void io_complete_rw_common(struct kiocb *kiocb, long res,
>  	__io_req_complete(req, res, cflags, cs);
>  }
>  
...
>  	switch (req->opcode) {
>  	case IORING_OP_NOP:
> @@ -5347,7 +5382,7 @@ static int io_req_defer(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	if (!req->io) {
>  		if (io_alloc_async_ctx(req))
>  			return -EAGAIN;
> -		ret = io_req_defer_prep(req, sqe);
> +		ret = io_req_defer_prep(req, sqe, true);

Why head of a link is for_async?

>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -5966,7 +6001,7 @@ static void io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  			ret = -EAGAIN;
>  			if (io_alloc_async_ctx(req))
>  				goto fail_req;
> -			ret = io_req_defer_prep(req, sqe);
> +			ret = io_req_defer_prep(req, sqe, true);
>  			if (unlikely(ret < 0))
>  				goto fail_req;
>  		}
> @@ -6022,13 +6057,14 @@ static int io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  		if (io_alloc_async_ctx(req))
>  			return -EAGAIN;
>  
> -		ret = io_req_defer_prep(req, sqe);
> +		ret = io_req_defer_prep(req, sqe, false);
>  		if (ret) {
>  			/* fail even hard links since we don't submit */
>  			head->flags |= REQ_F_FAIL_LINK;
>  			return ret;
>  		}
>  		trace_io_uring_link(ctx, req, head);
> +		io_get_req_task(req);
>  		list_add_tail(&req->link_list, &head->link_list);
>  
>  		/* last request of a link, enqueue the link */
> @@ -6048,7 +6084,7 @@ static int io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  			if (io_alloc_async_ctx(req))
>  				return -EAGAIN;
>  
> -			ret = io_req_defer_prep(req, sqe);
> +			ret = io_req_defer_prep(req, sqe, true);
>  			if (ret)
>  				req->flags |= REQ_F_FAIL_LINK;
>  			*link = req;
> 

-- 
Pavel Begunkov