LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Nadav Amit <nadav.amit@gmail.com>,
	Pavel Begunkov <asml.silence@gmail.com>
Cc: Olivier Langlois <olivier@trillion01.com>,
	io-uring@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] io_uring: clear TIF_NOTIFY_SIGNAL when running task work
Date: Tue, 10 Aug 2021 20:51:40 -0600	[thread overview]
Message-ID: <1bf56100-e904-65b5-bbb8-fa313d85b01a@kernel.dk> (raw)
In-Reply-To: <FD8FD9BD-1E94-4A84-88EB-3A1531BCF556@gmail.com>

On 8/10/21 8:33 PM, Nadav Amit wrote:
> 
> 
>> On Aug 10, 2021, at 2:32 PM, Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 8/10/21 9:28 AM, Nadav Amit wrote:
>>>
>>> Unfortunately, there seems to be yet another issue (unless my code
>>> somehow caused it). It seems that when SQPOLL is used, there are cases
>>> in which we get stuck in io_uring_cancel_sqpoll() when tctx_inflight()
>>> never goes down to zero.
>>>
>>> Debugging... (while also trying to make some progress with my code)
>>
>> It's most likely because a request has been lost (mis-refcounted).
>> Let us know if you need any help. Would be great to solve it for 5.14.
>> quick tips: 
>>
>> 1) if not already, try out Jens' 5.14 branch
>> git://git.kernel.dk/linux-block io_uring-5.14
>>
>> 2) try to characterise the io_uring use pattern. Poll requests?
>> Read/write requests? Send/recv? Filesystem vs bdev vs sockets?
>>
>> If easily reproducible, you can match io_alloc_req() with it
>> getting into io_dismantle_req();
> 
> So actually the problem is more of a missing IO-uring functionality
> that I need. When an I/O is queued for async completion (i.e., after
> returning -EIOCBQUEUED), there should be a way for io-uring to cancel
> these I/Os if needed.

There's no way to cancel file/bdev related IO, and there likely never
will be. That's basically the only exception, everything else can get
canceled pretty easily. Many things can be written on why that is the
case, and they have (myself included), but it boils down to proper
hardware support which we'll likely never have as it's not a well tested
path. For other kind of async IO, we're either waiting in poll (which is
trivially cancellable) or in an async thread (which is also easily
cancellable). For DMA+irq driven block storage, we'd need to be able to
reliably cancel on the hardware side, to prevent errant DMA after the
fact.

None of this is really io_uring specific, io_uring just suffers from the
same limitations as others would (or are).

> Otherwise they might potentially never complete, as happens in my
> use-case.

If you see that, that is most certainly a bug. While bdev/reg file IO
can't really be canceled, they all have the property that they complete
in finite time. Either the IO completes normally in a "short" amount of
time, or a timeout will cancel it and complete it in error. There are no
unbounded execution times for uncancellable IO.

> AIO has ki_cancel() for this matter. So I presume the proper solution
> would be to move ki_cancel() from aio_kiocb to kiocb so it can be used
> by both io-uring and aio. And then - to use this infrastructure.

There is no infrastructure, I'm fraid. ki_cancel() is just a random hook
that nobody (outside of USB gadget??) ever implemented or used.

> But it is messy. There is already a bug in the (few) uses of
> kiocb_set_cancel_fn() that blindly assume AIO is used and not
> IO-uring. Then, I am not sure about some things in the AIO code. Oh
> boy. I’ll work on an RFC.

ki_cancel is a non-starter, it doesn't even work for the single case
that it's intended for, and I'm actually surprised it hasn't been
removed yet. It's one of those things that someone added a hook for, but
never really grew into something that is useful.

-- 
Jens Axboe


  reply	other threads:[~2021-08-11  2:51 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-08  0:13 [PATCH 0/2] io_uring: bug fixes Nadav Amit
2021-08-08  0:13 ` [PATCH 1/2] io_uring: clear TIF_NOTIFY_SIGNAL when running task work Nadav Amit
2021-08-08 12:55   ` Pavel Begunkov
2021-08-08 17:31     ` Nadav Amit
2021-08-09  4:07       ` Hao Xu
2021-08-09  4:50         ` Nadav Amit
2021-08-09 10:35           ` Pavel Begunkov
2021-08-09 10:18       ` Pavel Begunkov
2021-08-09 21:48   ` Olivier Langlois
2021-08-10  8:28     ` Nadav Amit
2021-08-10 13:33       ` Olivier Langlois
2021-08-10 21:32       ` Pavel Begunkov
2021-08-11  2:33         ` Nadav Amit
2021-08-11  2:51           ` Jens Axboe [this message]
2021-08-11  5:40             ` I/O cancellation in io-uring (was: io_uring: clear TIF_NOTIFY_SIGNAL ...) Nadav Amit
2021-08-08  0:13 ` [PATCH 2/2] io_uring: Use WRITE_ONCE() when writing to sq_flags Nadav Amit
2021-08-09 13:53 ` [PATCH 0/2] io_uring: bug fixes Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1bf56100-e904-65b5-bbb8-fa313d85b01a@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=asml.silence@gmail.com \
    --cc=io-uring@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nadav.amit@gmail.com \
    --cc=olivier@trillion01.com \
    --subject='Re: [PATCH 1/2] io_uring: clear TIF_NOTIFY_SIGNAL when running task work' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).