Alexander Krizhanovsky added a comment - 2021-06-10 23:29 I tested the Vlad's patch and it works. This is neither a kernel nor liburing bug. I had a look into Linux 5.4.105 (the one I use in the tests) and the latest 5.4.125 as well as the current Linus tree - in all the kernels the corresponding code is the same. While liburing changed the code involved in the problem a little bit, the main logic remains the same. The problem is caused by the __io_uring_peek_cqe() called from io_uring_wait_eqe(): do { unsigned tail = io_uring_smp_load_acquire(ring->cq.ktail); unsigned head = *ring->cq.khead;   cqe = NULL; available = tail - head; if (!available) break ;   cqe = &ring->cq.cqes[head & mask]; if (!(ring->features & IORING_FEAT_EXT_ARG) && cqe->user_data == LIBURING_UDATA_TIMEOUT) { if (cqe->res < 0) err = cqe->res; io_uring_cq_advance(ring, 1); if (!err) continue ; cqe = NULL; }   break ; } while (1); We firstly read the RB queue head, next we read the cqe->res and process an error if it's non-zero. The function in our case returns 0, so in this code we observer cqe->res = 0, but later, when we check it in io_uring_cqe_get_data() it's -EAGAIN. The kernel function io_cqring_fill_event() firstly moves the tail pointer and next writes the cqe->res: static struct io_uring_cqe *io_get_cqring( struct io_ring_ctx *ctx) { struct io_rings *rings = ctx->rings; unsigned tail;   tail = ctx->cached_cq_tail; /* * writes to the cq entry need to come after reading head; the * control dependency is enough as we're using WRITE_ONCE to * fill the cq entry */ if (tail - READ_ONCE(rings->cq.head) == rings->cq_ring_entries) return NULL;   ctx->cached_cq_tail++; return &rings->cqes[tail & ctx->cq_mask]; }   static void io_cqring_fill_event( struct io_ring_ctx *ctx, u64 ki_user_data, long res) { struct io_uring_cqe *cqe;   cqe = io_get_cqring(ctx); if (cqe) { WRITE_ONCE(cqe->res, res); ..... It's easy to see that the place is racy: we can read old value of cqe->res in liburing and after that read the new value. Actually, this isn't a bug and liburing/examples/io_uring-cp.c implements following: static void queue_prepped( struct io_uring *ring, struct io_data *data) { struct io_uring_sqe *sqe;   sqe = io_uring_get_sqe(ring); assert (sqe);   if (data->read) io_uring_prep_readv(sqe, infd, &data->iov, 1, data->offset); else io_uring_prep_writev(sqe, outfd, &data->iov, 1, data->offset);   io_uring_sqe_set_data(sqe, data); }   static int copy_file( struct io_uring *ring, off_t insize) { .... data = io_uring_cqe_get_data(cqe); if (cqe->res < 0) { if (cqe->res == -EAGAIN) { queue_prepped(ring, data); io_uring_cqe_seen(ring, cqe); continue ; } So I think the right patch should look like (the servers are still down after the recent outage, so I didn't test the patch yet): diff --git a/tpool/aio_liburing.cc b/tpool/aio_liburing.cc index bdc3601ae35..ba1138b2323 100644 --- a/tpool/aio_liburing.cc +++ b/tpool/aio_liburing.cc @@ -96,14 +96,7 @@ class aio_uring final : public tpool::aio // must be atomical. This is because liburing provides thread-unsafe calls. std::lock_guard<std::mutex> _(mutex_); - io_uring_sqe *sqe= io_uring_get_sqe(&uring_); - if (cb->m_opcode == tpool::aio_opcode::AIO_PREAD) - io_uring_prep_readv(sqe, cb->m_fh, static_cast<struct iovec *>(cb), 1, - cb->m_offset); - else - io_uring_prep_writev(sqe, cb->m_fh, static_cast<struct iovec *>(cb), 1, - cb->m_offset); - io_uring_sqe_set_data(sqe, cb); + queue_prepped(cb); return io_uring_submit(&uring_) == 1 ? 0 : -1; } @@ -129,6 +122,18 @@ class aio_uring final : public tpool::aio } private: + void queue_prepped(tpool::aiocb *cb) + { + io_uring_sqe *sqe= io_uring_get_sqe(&uring_); + if (cb->m_opcode == tpool::aio_opcode::AIO_PREAD) + io_uring_prep_readv(sqe, cb->m_fh, static_cast<struct iovec *>(cb), 1, + cb->m_offset); + else + io_uring_prep_writev(sqe, cb->m_fh, static_cast<struct iovec *>(cb), 1, + cb->m_offset); + io_uring_sqe_set_data(sqe, cb); + } + static void thread_routine(aio_uring *aio) { for (;;) @@ -151,6 +156,11 @@ class aio_uring final : public tpool::aio int res= cqe->res; if (res < 0) { + if (cqe->res == -EAGAIN) { + aio->queue_prepped(iocb); + io_uring_cqe_seen(&aio->uring_, cqe); + continue; + } iocb->m_err= -res; iocb->m_ret_len= 0; } This is very close to what Vlad proposed, but in io_uring way.

Marko Mäkelä added a comment - 2021-06-11 04:57 krizhanovsky , thank you for your analysis. I have not looked at liburing or the kernel code, but I fully trust your expertise. Please file a pull request, and I will merge it as soon as possible.

Alexander Krizhanovsky added a comment - 2021-06-14 01:48 Pull request is https://github.com/MariaDB/server/pull/1860

Alexander Krizhanovsky added a comment - 2021-06-14 13:45 - edited The patch handles the very rare case of the race in when -EAGAIN error code is written to cqe->res between checking the value in __io_uring_peek_cqe() and reading it again via io_uring_cqe_get_data() . In normal cases io_uring_wait_eqe() returns an error code aio_uring::thread_routine() aborts. If there are too many I/O requests and there is not space in the io_uring ring buffers, then the patch leads to the same error path as any other errors, i.e. aborting the server. If the underlying filesystem/block layers constantly return -EAGAIN and we constantly get into the race condition, then the code will be spinning until the race condition is not satisfied and io_uring_wait_eqe() returns with -EAGAIN .