nvme wakes the request coroutine via qemu_coroutine_enter() from a BH
scheduled in the BDS AioContext. This may not be the same context as
the one in which the request originally ran, which would be wrong:
- It could mean we enter the coroutine before it yields,
- We would move the coroutine in to a different context.
(Can be reproduced with multiqueue by adding a usleep(100000) before the
`while (data.ret == -EINPROGRESS)` loop.)
To fix that, schedule nvme_rw_cb_bh() in the coroutine AioContext.
(Just like in the preceding iscsi and nfs patches, we could drop the
trivial nvme_rw_cb_bh() and just use aio_co_wake() directly, but don’t
do that to keep replay_bh_schedule_oneshot_event().)
With this, we can remove NVMeCoData.ctx.
Note the check of data->co == NULL to bypass the BH/yield combination in
case nvme_rw_cb() is called from nvme_submit_command(): We probably want
to keep this fast path for performance reasons, but we have to be quite
careful about it:
- We cannot overload .ret for this, but have to use a dedicated
.skip_yield field. Otherwise, if nvme_rw_cb() runs in a different
thread than the coroutine, it may see .ret set and skip the yield,
while nvme_rw_cb() will still schedule a BH for waking. Therefore,
the signal to skip the yield can only be set in nvme_rw_cb() if waking
too is skipped, which is independent from communicating the return
value.
- We can only skip the yield if nvme_rw_cb() actually runs in the
request coroutine. Otherwise (specifically if they run in different
AioContexts), the order between this function’s execution and the
coroutine yielding (or not yielding) is not reliable.
- There is no point to yielding in a loop; there are no spurious wakes,
so once we yield, we will only be re-entered once the command is done.
Replace `while` by `if`.
Cc: qemu-stable@nongnu.org
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
block/nvme.c | 56 +++++++++++++++++++++-------------------------------
1 file changed, 23 insertions(+), 33 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 7ed5f570bc..4b1f623e7d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1176,8 +1176,8 @@ fail:
typedef struct {
Coroutine *co;
+ bool skip_yield;
int ret;
- AioContext *ctx;
} NVMeCoData;
static void nvme_rw_cb_bh(void *opaque)
@@ -1189,12 +1189,22 @@ static void nvme_rw_cb_bh(void *opaque)
static void nvme_rw_cb(void *opaque, int ret)
{
NVMeCoData *data = opaque;
+ AioContext *ctx;
+
data->ret = ret;
- if (!data->co) {
- /* The rw coroutine hasn't yielded, don't try to enter. */
- return;
+
+ if (data->co == qemu_coroutine_self()) {
+ /*
+ * Fast path: We are inside of the request coroutine (through
+ * nvme_submit_command, nvme_deferred_fn, nvme_process_completion).
+ * We can set data->skip_yield here to keep the coroutine from
+ * yielding, and then we don't need to schedule a BH to wake it.
+ */
+ data->skip_yield = true;
+ } else {
+ ctx = qemu_coroutine_get_aio_context(data->co);
+ replay_bh_schedule_oneshot_event(ctx, nvme_rw_cb_bh, data);
}
- replay_bh_schedule_oneshot_event(data->ctx, nvme_rw_cb_bh, data);
}
static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
@@ -1217,10 +1227,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
.cdw11 = cpu_to_le32(((offset >> s->blkshift) >> 32) & 0xFFFFFFFF),
.cdw12 = cpu_to_le32(cdw12),
};
- NVMeCoData data = {
- .ctx = bdrv_get_aio_context(bs),
- .ret = -EINPROGRESS,
- };
+ NVMeCoData data = { .co = qemu_coroutine_self() };
trace_nvme_prw_aligned(s, is_write, offset, bytes, flags, qiov->niov);
assert(s->queue_count > 1);
@@ -1235,9 +1242,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
return r;
}
nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);
-
- data.co = qemu_coroutine_self();
- while (data.ret == -EINPROGRESS) {
+ if (!data.skip_yield) {
qemu_coroutine_yield();
}
@@ -1332,18 +1337,13 @@ static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
.opcode = NVME_CMD_FLUSH,
.nsid = cpu_to_le32(s->nsid),
};
- NVMeCoData data = {
- .ctx = bdrv_get_aio_context(bs),
- .ret = -EINPROGRESS,
- };
+ NVMeCoData data = { .co = qemu_coroutine_self() };
assert(s->queue_count > 1);
req = nvme_get_free_req(ioq);
assert(req);
nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);
-
- data.co = qemu_coroutine_self();
- if (data.ret == -EINPROGRESS) {
+ if (!data.skip_yield) {
qemu_coroutine_yield();
}
@@ -1383,10 +1383,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
.cdw11 = cpu_to_le32(((offset >> s->blkshift) >> 32) & 0xFFFFFFFF),
};
- NVMeCoData data = {
- .ctx = bdrv_get_aio_context(bs),
- .ret = -EINPROGRESS,
- };
+ NVMeCoData data = { .co = qemu_coroutine_self() };
if (flags & BDRV_REQ_MAY_UNMAP) {
cdw12 |= (1 << 25);
@@ -1404,9 +1401,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
assert(req);
nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);
-
- data.co = qemu_coroutine_self();
- while (data.ret == -EINPROGRESS) {
+ if (!data.skip_yield) {
qemu_coroutine_yield();
}
@@ -1433,10 +1428,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
.cdw11 = cpu_to_le32(1 << 2), /*deallocate bit*/
};
- NVMeCoData data = {
- .ctx = bdrv_get_aio_context(bs),
- .ret = -EINPROGRESS,
- };
+ NVMeCoData data = { .co = qemu_coroutine_self() };
if (!s->supports_discard) {
return -ENOTSUP;
@@ -1479,9 +1471,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
trace_nvme_dsm(s, offset, bytes);
nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);
-
- data.co = qemu_coroutine_self();
- while (data.ret == -EINPROGRESS) {
+ if (!data.skip_yield) {
qemu_coroutine_yield();
}
--
2.51.0