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
