Re: Regression in io_uring, leading to data corruption

[Jens Axboe <axboe@kernel.dk>]

On 11/15/23 4:03 AM, Timothy Pearson wrote:
> I haven't had much success in getting the IPI path to work properly,
> but by leaving task_work_add() in TWA_SIGNAL_NO_IPI mode I was at
> least able to narrow down one of the areas that's going wrong.  Bear
> in mind that as soon as I reenable IPI the corruption returns with a
> vengeance, so this is not the correct fix yet by any means -- I am
> currently soliciting feedback on what else might be going wrong at
> this point since I've already spent a couple of weeks on this and am
> not sure how much more time I can spend before we just have to shut
> io_uring down on ppc64 for the forseeable future.
> 
> Whatever the root cause actually is, something is *very* sensitive to
> timing in both the worker thread creation path and the io_queue_sqe()
> / io_queue_async() paths.  I can make the corruption disappear by
> adding a udelay(1000) before io_queue_async() in the io_queue_sqe()
> function, however no amount of memory barriers in the io_queue_async()
> path (including in the kbuf recycling code) will fully resolve the
> problem.
> 
> Jens, would a small delay like that in io_queue_sqe() reduce the
> amount of workers being created overall?  I know with some of the
> other delay locations worker allocation was changing, from what I see
> this one wouldn't seem to have much effect, but I'm still looking for
> a sanity check.  If we're needing to wait for a millisecond for some
> other thread to complete before moving on that might be valuable
> information -- would also potentially tie in to the IPI path still
> malfunctioning as the worker would immediately start executing.

If io_queue_sqe() ultimately ends up punting to io-wq for this request,
then yes doing a 1ms delay in there would ultimately then need to a 1ms
delay before we either pass to an existing worker or create a new one.

> On a related note, how is inter-thread safety of the io_kiocb buffer
> list guaranteed, especially on weak memory model systems?  As I
> understand it, different workers running on different cores could
> potentially be interacting with the same kiocb request and the same
> buffer list, and that does dovetail with the fact that punting to a
> different I/O worker (usually on another core) seems to provoke the
> problem.  I tried adding memory barriers to some of the basic recycle
> functions without too much success -- it seemed to help somewhat, but
> nowhere near complete resolution, and the buffers are used in a number
> of other places I didn't even try to poke at.  I wanted to get some
> feedback on this concept before going down yet another rabbit hole...

This relies on the fact that we grab the wq lock before inserting this
work, and the unlocking will be a barrier. It's important to note that
this isn't any different than from before io-wq was using native
workers, the only difference is that it used to be kthreads before, and
now it's native threads to the application. The kthreads did a bunch of
work to assume the necessary identity to do the read or write operation
(which is ultimately why that approach went away, as it was just
inherently unsafe), whereas the native threads do not as they already
have what they need.

I had a patch that just punted to a kthread and did the necessary
kthread_use_mm(), perform op, kthread_unuse_mm() and it works fine at
that point. Within the existing code...

-- 
Jens Axboe