Re: [PATCH 2/6] io_uring: add IORING_OP_PROVIDE_BUFFERS

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

On 2/29/2020 7:50 AM, Jens Axboe wrote:
> On 2/28/20 5:43 PM, Pavel Begunkov wrote:
>>> +static int io_provide_buffers(struct io_kiocb *req, struct io_kiocb **nxt,
>>> +			      bool force_nonblock)
>>> +{
>>> +	struct io_provide_buf *p = &req->pbuf;
>>> +	struct io_ring_ctx *ctx = req->ctx;
>>> +	struct list_head *list;
>>> +	int ret = 0;
>>> +
>>> +	/*
>>> +	 * "Normal" inline submissions always hold the uring_lock, since we
>>> +	 * grab it from the system call. Same is true for the SQPOLL offload.
>>> +	 * The only exception is when we've detached the request and issue it
>>> +	 * from an async worker thread, grab the lock for that case.
>>> +	 */
>>> +	if (!force_nonblock)
>>> +		mutex_lock(&ctx->uring_lock);
>>
>> io_poll_task_handler() calls it with force_nonblock==true, but it
>> doesn't hold the mutex AFAIK.
> 
> True, that's the only exception. And that command doesn't transfer data
> so would never need a buffer, but I agree that's perhaps not fully
> clear. The async task handler grabs the mutex.

Hmm, I meant io_poll_task_func(), which do __io_queue_sqe() for @nxt
request, which in its turn calls io_issue_sqe(force_nonblock=true).

Does io_poll_task_func() hold @uring_mutex? Otherwise, if @nxt happened
to be io_provide_buffers(), we get there without holding the mutex and
with force_nonblock=true.


>>> +	lockdep_assert_held(&ctx->uring_lock);
>>> +
>>> +	list = idr_find(&ctx->io_buffer_idr, p->gid);
>>> +	if (!list) {
>>> +		list = kmalloc(sizeof(*list), GFP_KERNEL);
>>> +		if (!list) {
>>> +			ret = -ENOMEM;
>>> +			goto out;
>>> +		}
>>> +		INIT_LIST_HEAD(list);
>>> +		ret = idr_alloc(&ctx->io_buffer_idr, list, p->gid, p->gid + 1,
>>> +					GFP_KERNEL);
>>> +		if (ret < 0) {
>>> +			kfree(list);
>>> +			goto out;
>>> +		}
>>> +	}
>>> +
>>> +	ret = io_add_buffers(p, list);
>>
>> Isn't it better to not do partial registration?
>> i.e. it may return ret < pbuf->nbufs
> 
> Most things work like that, though. If you ask for an 8k read, you can't
> unwind if you just get 4k. We return 4k for that. I think in general, if
> it fails, you're probably somewhat screwed in either case. At least with
> the partial return, you know which buffers got registered and how many
> you can use. If you return 0 and undo it all, then the application
> really has no way to continue except abort.
> 

-- 
Pavel Begunkov