* [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion
2021-04-08 19:37 ` [PATCH 1/2] hw/block/nvme: store aiocb in compare Klaus Jensen
@ 2021-04-08 19:37 ` Klaus Jensen
2021-04-09 11:09 ` Minwoo Im
2021-04-09 11:52 ` Gollu Appalanaidu
2021-04-09 8:48 ` [PATCH 1/2] hw/block/nvme: store aiocb in compare Gollu Appalanaidu
2021-04-09 11:08 ` Minwoo Im
2 siblings, 2 replies; 7+ messages in thread
From: Klaus Jensen @ 2021-04-08 19:37 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Klaus Jensen, Keith Busch, Minwoo Im
From: Klaus Jensen <k.jensen@samsung.com>
For most commands, when issuing an AIO, the BlockAIOCB is stored in the
NvmeRequest aiocb pointer when the AIO is issued. The main use of this
is cancelling AIOs when deleting submission queues (it is currently not
used for Abort).
However, some commands like Dataset Management Zone Management Send
(zone reset) may involve more than one AIO and here the AIOs are issued
without saving a reference to the BlockAIOCB. This is a problem since
nvme_del_sq() will attempt to cancel outstanding AIOs, potentially with
an invalid BlockAIOCB since the aiocb pointer is not NULL'ed when the
request structure is recycled.
Fix this by
1. making sure the aiocb pointer is NULL'ed when requests are recycled
2. only attempt to cancel the AIO if the aiocb is non-NULL
3. if any AIOs could not be cancelled, drain all aio as a last resort.
Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
Fixes: c94973288cd9 ("hw/block/nvme: add broadcast nsid support flush command")
Fixes: e4e430b3d6ba ("hw/block/nvme: add simple copy command")
Fixes: 5f5dc4c6a942 ("hw/block/nvme: zero out zones on reset")
Fixes: 2605257a26b8 ("hw/block/nvme: add the dataset management command")
Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Cc: Minwoo Im <minwoo.im@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 94bc373260be..3c4297e38a52 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -470,6 +470,7 @@ static void nvme_req_clear(NvmeRequest *req)
{
req->ns = NULL;
req->opaque = NULL;
+ req->aiocb = NULL;
memset(&req->cqe, 0x0, sizeof(req->cqe));
req->status = NVME_SUCCESS;
}
@@ -3681,6 +3682,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
NvmeSQueue *sq;
NvmeCQueue *cq;
uint16_t qid = le16_to_cpu(c->qid);
+ int nsid;
if (unlikely(!qid || nvme_check_sqid(n, qid))) {
trace_pci_nvme_err_invalid_del_sq(qid);
@@ -3692,9 +3694,26 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
sq = n->sq[qid];
while (!QTAILQ_EMPTY(&sq->out_req_list)) {
r = QTAILQ_FIRST(&sq->out_req_list);
- assert(r->aiocb);
- blk_aio_cancel(r->aiocb);
+ if (r->aiocb) {
+ blk_aio_cancel(r->aiocb);
+ }
}
+
+ /*
+ * Drain all namespaces if there are still outstanding requests that we
+ * could not cancel explicitly.
+ */
+ if (!QTAILQ_EMPTY(&sq->out_req_list)) {
+ for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
+ NvmeNamespace *ns = nvme_ns(n, nsid);
+ if (ns) {
+ nvme_ns_drain(ns);
+ }
+ }
+ }
+
+ assert(QTAILQ_EMPTY(&sq->out_req_list));
+
if (!nvme_check_cqid(n, sq->cqid)) {
cq = n->cq[sq->cqid];
QTAILQ_REMOVE(&cq->sq_list, sq, entry);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion
2021-04-08 19:37 ` [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion Klaus Jensen
@ 2021-04-09 11:09 ` Minwoo Im
2021-04-09 11:45 ` Klaus Jensen
2021-04-09 11:52 ` Gollu Appalanaidu
1 sibling, 1 reply; 7+ messages in thread
From: Minwoo Im @ 2021-04-09 11:09 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Klaus Jensen, Gollu Appalanaidu,
qemu-devel, Max Reitz, Keith Busch, Minwoo Im
On 21-04-08 21:37:09, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> For most commands, when issuing an AIO, the BlockAIOCB is stored in the
> NvmeRequest aiocb pointer when the AIO is issued. The main use of this
> is cancelling AIOs when deleting submission queues (it is currently not
> used for Abort).
>
> However, some commands like Dataset Management Zone Management Send
> (zone reset) may involve more than one AIO and here the AIOs are issued
> without saving a reference to the BlockAIOCB. This is a problem since
> nvme_del_sq() will attempt to cancel outstanding AIOs, potentially with
> an invalid BlockAIOCB since the aiocb pointer is not NULL'ed when the
> request structure is recycled.
>
> Fix this by
>
> 1. making sure the aiocb pointer is NULL'ed when requests are recycled
> 2. only attempt to cancel the AIO if the aiocb is non-NULL
> 3. if any AIOs could not be cancelled, drain all aio as a last resort.
>
> Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
> Fixes: c94973288cd9 ("hw/block/nvme: add broadcast nsid support flush command")
> Fixes: e4e430b3d6ba ("hw/block/nvme: add simple copy command")
> Fixes: 5f5dc4c6a942 ("hw/block/nvme: zero out zones on reset")
> Fixes: 2605257a26b8 ("hw/block/nvme: add the dataset management command")
> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Cc: Minwoo Im <minwoo.im@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/block/nvme.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 94bc373260be..3c4297e38a52 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -470,6 +470,7 @@ static void nvme_req_clear(NvmeRequest *req)
> {
> req->ns = NULL;
> req->opaque = NULL;
> + req->aiocb = NULL;
> memset(&req->cqe, 0x0, sizeof(req->cqe));
> req->status = NVME_SUCCESS;
> }
> @@ -3681,6 +3682,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
> NvmeSQueue *sq;
> NvmeCQueue *cq;
> uint16_t qid = le16_to_cpu(c->qid);
> + int nsid;
Even we don't have fully supported number of namespaces in this device
(0xFFFFFFFF), can we have this one with `uint32_t` ?
Otherwise, looks good to me.
Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion
2021-04-09 11:09 ` Minwoo Im
@ 2021-04-09 11:45 ` Klaus Jensen
0 siblings, 0 replies; 7+ messages in thread
From: Klaus Jensen @ 2021-04-09 11:45 UTC (permalink / raw)
To: Minwoo Im
Cc: Kevin Wolf, qemu-block, Klaus Jensen, Gollu Appalanaidu,
qemu-devel, Max Reitz, Keith Busch, Minwoo Im
[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]
On Apr 9 20:09, Minwoo Im wrote:
>On 21-04-08 21:37:09, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> For most commands, when issuing an AIO, the BlockAIOCB is stored in the
>> NvmeRequest aiocb pointer when the AIO is issued. The main use of this
>> is cancelling AIOs when deleting submission queues (it is currently not
>> used for Abort).
>>
>> However, some commands like Dataset Management Zone Management Send
>> (zone reset) may involve more than one AIO and here the AIOs are issued
>> without saving a reference to the BlockAIOCB. This is a problem since
>> nvme_del_sq() will attempt to cancel outstanding AIOs, potentially with
>> an invalid BlockAIOCB since the aiocb pointer is not NULL'ed when the
>> request structure is recycled.
>>
>> Fix this by
>>
>> 1. making sure the aiocb pointer is NULL'ed when requests are recycled
>> 2. only attempt to cancel the AIO if the aiocb is non-NULL
>> 3. if any AIOs could not be cancelled, drain all aio as a last resort.
>>
>> Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
>> Fixes: c94973288cd9 ("hw/block/nvme: add broadcast nsid support flush command")
>> Fixes: e4e430b3d6ba ("hw/block/nvme: add simple copy command")
>> Fixes: 5f5dc4c6a942 ("hw/block/nvme: zero out zones on reset")
>> Fixes: 2605257a26b8 ("hw/block/nvme: add the dataset management command")
>> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>> Cc: Minwoo Im <minwoo.im@samsung.com>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>> hw/block/nvme.c | 23 +++++++++++++++++++++--
>> 1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 94bc373260be..3c4297e38a52 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -470,6 +470,7 @@ static void nvme_req_clear(NvmeRequest *req)
>> {
>> req->ns = NULL;
>> req->opaque = NULL;
>> + req->aiocb = NULL;
>> memset(&req->cqe, 0x0, sizeof(req->cqe));
>> req->status = NVME_SUCCESS;
>> }
>> @@ -3681,6 +3682,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
>> NvmeSQueue *sq;
>> NvmeCQueue *cq;
>> uint16_t qid = le16_to_cpu(c->qid);
>> + int nsid;
>
>Even we don't have fully supported number of namespaces in this device
>(0xFFFFFFFF), can we have this one with `uint32_t` ?
>
Sure!
>Otherwise, looks good to me.
>
>Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion
2021-04-08 19:37 ` [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion Klaus Jensen
2021-04-09 11:09 ` Minwoo Im
@ 2021-04-09 11:52 ` Gollu Appalanaidu
1 sibling, 0 replies; 7+ messages in thread
From: Gollu Appalanaidu @ 2021-04-09 11:52 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Minwoo Im
[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]
On Thu, Apr 08, 2021 at 09:37:09PM +0200, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>For most commands, when issuing an AIO, the BlockAIOCB is stored in the
>NvmeRequest aiocb pointer when the AIO is issued. The main use of this
>is cancelling AIOs when deleting submission queues (it is currently not
>used for Abort).
>
>However, some commands like Dataset Management Zone Management Send
>(zone reset) may involve more than one AIO and here the AIOs are issued
>without saving a reference to the BlockAIOCB. This is a problem since
>nvme_del_sq() will attempt to cancel outstanding AIOs, potentially with
>an invalid BlockAIOCB since the aiocb pointer is not NULL'ed when the
>request structure is recycled.
>
>Fix this by
>
> 1. making sure the aiocb pointer is NULL'ed when requests are recycled
> 2. only attempt to cancel the AIO if the aiocb is non-NULL
> 3. if any AIOs could not be cancelled, drain all aio as a last resort.
>
>Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
>Fixes: c94973288cd9 ("hw/block/nvme: add broadcast nsid support flush command")
>Fixes: e4e430b3d6ba ("hw/block/nvme: add simple copy command")
>Fixes: 5f5dc4c6a942 ("hw/block/nvme: zero out zones on reset")
>Fixes: 2605257a26b8 ("hw/block/nvme: add the dataset management command")
>Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>Cc: Minwoo Im <minwoo.im@samsung.com>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/block/nvme.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 94bc373260be..3c4297e38a52 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -470,6 +470,7 @@ static void nvme_req_clear(NvmeRequest *req)
> {
> req->ns = NULL;
> req->opaque = NULL;
>+ req->aiocb = NULL;
> memset(&req->cqe, 0x0, sizeof(req->cqe));
> req->status = NVME_SUCCESS;
> }
>@@ -3681,6 +3682,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
> NvmeSQueue *sq;
> NvmeCQueue *cq;
> uint16_t qid = le16_to_cpu(c->qid);
>+ int nsid;
>
> if (unlikely(!qid || nvme_check_sqid(n, qid))) {
> trace_pci_nvme_err_invalid_del_sq(qid);
>@@ -3692,9 +3694,26 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
> sq = n->sq[qid];
> while (!QTAILQ_EMPTY(&sq->out_req_list)) {
> r = QTAILQ_FIRST(&sq->out_req_list);
>- assert(r->aiocb);
>- blk_aio_cancel(r->aiocb);
>+ if (r->aiocb) {
>+ blk_aio_cancel(r->aiocb);
>+ }
> }
>+
>+ /*
>+ * Drain all namespaces if there are still outstanding requests that we
>+ * could not cancel explicitly.
>+ */
>+ if (!QTAILQ_EMPTY(&sq->out_req_list)) {
>+ for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
>+ NvmeNamespace *ns = nvme_ns(n, nsid);
>+ if (ns) {
>+ nvme_ns_drain(ns);
>+ }
>+ }
>+ }
>+
>+ assert(QTAILQ_EMPTY(&sq->out_req_list));
>+
> if (!nvme_check_cqid(n, sq->cqid)) {
> cq = n->cq[sq->cqid];
> QTAILQ_REMOVE(&cq->sq_list, sq, entry);
>--
LTM.
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>2.31.1
>
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw/block/nvme: store aiocb in compare
2021-04-08 19:37 ` [PATCH 1/2] hw/block/nvme: store aiocb in compare Klaus Jensen
2021-04-08 19:37 ` [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion Klaus Jensen
@ 2021-04-09 8:48 ` Gollu Appalanaidu
2021-04-09 11:08 ` Minwoo Im
2 siblings, 0 replies; 7+ messages in thread
From: Gollu Appalanaidu @ 2021-04-09 8:48 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch
[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]
On Thu, Apr 08, 2021 at 09:37:08PM +0200, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>nvme_compare() fails to store the aiocb from the blk_aio_preadv() call.
>Fix this.
>
>Fixes: 0a384f923f51 ("hw/block/nvme: add compare command")
>Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/block/nvme.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 6b1f056a0ebc..94bc373260be 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -2837,7 +2837,8 @@ static uint16_t nvme_compare(NvmeCtrl *n, NvmeRequest *req)
>
> block_acct_start(blk_get_stats(blk), &req->acct, data_len,
> BLOCK_ACCT_READ);
>- blk_aio_preadv(blk, offset, &ctx->data.iov, 0, nvme_compare_data_cb, req);
>+ req->aiocb = blk_aio_preadv(blk, offset, &ctx->data.iov, 0,
>+ nvme_compare_data_cb, req);
>
> return NVME_NO_COMPLETE;
> }
>--
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>2.31.1
>
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] hw/block/nvme: store aiocb in compare
2021-04-08 19:37 ` [PATCH 1/2] hw/block/nvme: store aiocb in compare Klaus Jensen
2021-04-08 19:37 ` [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion Klaus Jensen
2021-04-09 8:48 ` [PATCH 1/2] hw/block/nvme: store aiocb in compare Gollu Appalanaidu
@ 2021-04-09 11:08 ` Minwoo Im
2 siblings, 0 replies; 7+ messages in thread
From: Minwoo Im @ 2021-04-09 11:08 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Klaus Jensen, Gollu Appalanaidu,
qemu-devel, Max Reitz, Keith Busch
On 21-04-08 21:37:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> nvme_compare() fails to store the aiocb from the blk_aio_preadv() call.
> Fix this.
>
> Fixes: 0a384f923f51 ("hw/block/nvme: add compare command")
> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread