Skip to content

Commit

Permalink
hw/nvme: fix aio cancel in format
Browse files Browse the repository at this point in the history
There are several bugs in the async cancel code for the Format command.

Firstly, cancelling a format operation neglects to set iocb->ret as well
as clearing the iocb->aiocb after cancelling the underlying aiocb which
causes the aio callback to ignore the cancellation. Trivial fix.

Secondly, and worse, because the request is queued up for posting to the
CQ in a bottom half, if the cancellation is due to the submission queue
being deleted (which calls blk_aio_cancel), the req structure is
deallocated in nvme_del_sq prior to the bottom half being schedulued.

Fix this by simply removing the bottom half, there is no reason to defer
it anyway.

Fixes: 3bcf26d ("hw/nvme: reimplement format nvm to allow cancellation")
Reported-by: Jonathan Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
  • Loading branch information
birkelund committed Dec 1, 2022
1 parent c4ffd91 commit 433c71e
Showing 1 changed file with 12 additions and 16 deletions.
28 changes: 12 additions & 16 deletions hw/nvme/ctrl.c
Expand Up @@ -5741,7 +5741,6 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
typedef struct NvmeFormatAIOCB {
BlockAIOCB common;
BlockAIOCB *aiocb;
QEMUBH *bh;
NvmeRequest *req;
int ret;

Expand All @@ -5756,14 +5755,15 @@ typedef struct NvmeFormatAIOCB {
uint8_t pil;
} NvmeFormatAIOCB;

static void nvme_format_bh(void *opaque);

static void nvme_format_cancel(BlockAIOCB *aiocb)
{
NvmeFormatAIOCB *iocb = container_of(aiocb, NvmeFormatAIOCB, common);

iocb->ret = -ECANCELED;

if (iocb->aiocb) {
blk_aio_cancel_async(iocb->aiocb);
iocb->aiocb = NULL;
}
}

Expand All @@ -5787,13 +5787,17 @@ static void nvme_format_set(NvmeNamespace *ns, uint8_t lbaf, uint8_t mset,
nvme_ns_init_format(ns);
}

static void nvme_do_format(NvmeFormatAIOCB *iocb);

static void nvme_format_ns_cb(void *opaque, int ret)
{
NvmeFormatAIOCB *iocb = opaque;
NvmeNamespace *ns = iocb->ns;
int bytes;

if (ret < 0) {
if (iocb->ret < 0) {
goto done;
} else if (ret < 0) {
iocb->ret = ret;
goto done;
}
Expand All @@ -5817,8 +5821,7 @@ static void nvme_format_ns_cb(void *opaque, int ret)
iocb->offset = 0;

done:
iocb->aiocb = NULL;
qemu_bh_schedule(iocb->bh);
nvme_do_format(iocb);
}

static uint16_t nvme_format_check(NvmeNamespace *ns, uint8_t lbaf, uint8_t pi)
Expand All @@ -5842,9 +5845,8 @@ static uint16_t nvme_format_check(NvmeNamespace *ns, uint8_t lbaf, uint8_t pi)
return NVME_SUCCESS;
}

static void nvme_format_bh(void *opaque)
static void nvme_do_format(NvmeFormatAIOCB *iocb)
{
NvmeFormatAIOCB *iocb = opaque;
NvmeRequest *req = iocb->req;
NvmeCtrl *n = nvme_ctrl(req);
uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
Expand Down Expand Up @@ -5882,11 +5884,7 @@ static void nvme_format_bh(void *opaque)
return;

done:
qemu_bh_delete(iocb->bh);
iocb->bh = NULL;

iocb->common.cb(iocb->common.opaque, iocb->ret);

qemu_aio_unref(iocb);
}

Expand All @@ -5905,7 +5903,6 @@ static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
iocb = qemu_aio_get(&nvme_format_aiocb_info, NULL, nvme_misc_cb, req);

iocb->req = req;
iocb->bh = qemu_bh_new(nvme_format_bh, iocb);
iocb->ret = 0;
iocb->ns = NULL;
iocb->nsid = 0;
Expand Down Expand Up @@ -5934,14 +5931,13 @@ static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
}

req->aiocb = &iocb->common;
qemu_bh_schedule(iocb->bh);
nvme_do_format(iocb);

return NVME_NO_COMPLETE;

out:
qemu_bh_delete(iocb->bh);
iocb->bh = NULL;
qemu_aio_unref(iocb);

return status;
}

Expand Down

0 comments on commit 433c71e

Please sign in to comment.