| Message ID | 20210617190657.110823-12-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/nvme: reimplement all multi-aio commands with custom aiocbs | expand |
On Thu, Jun 17, 2021 at 09:06:57PM +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > This partially reverts commit 98f84f5a4eca5c03e32fff20f246d9b4b96d6422. > > Since all "multi aio" commands are now reimplemented to properly track > the nested aiocbs, we can revert the "hack" that was introduced to make > sure all requests we're properly drained upon sq deletion. > > The revert is partial since we keep the assert that no outstanding > requests remain on the submission queue after the explicit cancellation. > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > --- > hw/nvme/ctrl.c | 19 ++----------------- > 1 file changed, 2 insertions(+), 17 deletions(-) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index ec8ddb76cd39..5a1d25265a9d 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -3918,7 +3918,6 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req) > NvmeSQueue *sq; > NvmeCQueue *cq; > uint16_t qid = le16_to_cpu(c->qid); > - uint32_t nsid; > > if (unlikely(!qid || nvme_check_sqid(n, qid))) { > trace_pci_nvme_err_invalid_del_sq(qid); > @@ -3930,22 +3929,8 @@ 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); > - 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)) { Quick question: was this 'if' ever even possible to hit? The preceding 'while' loop doesn't break out until the queue is empty, so this should have been unreachable. > - for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) { > - NvmeNamespace *ns = nvme_ns(n, nsid); > - if (ns) { > - nvme_ns_drain(ns); > - } > - } > + assert(r->aiocb); > + blk_aio_cancel(r->aiocb); > } > > assert(QTAILQ_EMPTY(&sq->out_req_list)); > --
On Jun 28 09:00, Keith Busch wrote: >On Thu, Jun 17, 2021 at 09:06:57PM +0200, Klaus Jensen wrote: >> From: Klaus Jensen <k.jensen@samsung.com> >> >> This partially reverts commit 98f84f5a4eca5c03e32fff20f246d9b4b96d6422. >> >> Since all "multi aio" commands are now reimplemented to properly track >> the nested aiocbs, we can revert the "hack" that was introduced to make >> sure all requests we're properly drained upon sq deletion. >> >> The revert is partial since we keep the assert that no outstanding >> requests remain on the submission queue after the explicit cancellation. >> >> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> >> --- >> hw/nvme/ctrl.c | 19 ++----------------- >> 1 file changed, 2 insertions(+), 17 deletions(-) >> >> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c >> index ec8ddb76cd39..5a1d25265a9d 100644 >> --- a/hw/nvme/ctrl.c >> +++ b/hw/nvme/ctrl.c >> @@ -3918,7 +3918,6 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req) >> NvmeSQueue *sq; >> NvmeCQueue *cq; >> uint16_t qid = le16_to_cpu(c->qid); >> - uint32_t nsid; >> >> if (unlikely(!qid || nvme_check_sqid(n, qid))) { >> trace_pci_nvme_err_invalid_del_sq(qid); >> @@ -3930,22 +3929,8 @@ 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); >> - 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)) { > >Quick question: was this 'if' ever even possible to hit? The preceding >'while' loop doesn't break out until the queue is empty, so this should >have been unreachable. > Hi Keith, Good question ;) But yes. The requirement of that 'if' is the primary motivation for this series. The problem is that some commands (zone reset, copy, aka the commands reimplemented in this series) would not track the AIOCB in r->aiocb, so there would be no way to cancel/wait for them. See 98f84f5a4eca ("hw/block/nvme: drain namespaces on sq deletion") for a more elaborate description of the issue and the bandaid that the above fix was.
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index ec8ddb76cd39..5a1d25265a9d 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -3918,7 +3918,6 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req) NvmeSQueue *sq; NvmeCQueue *cq; uint16_t qid = le16_to_cpu(c->qid); - uint32_t nsid; if (unlikely(!qid || nvme_check_sqid(n, qid))) { trace_pci_nvme_err_invalid_del_sq(qid); @@ -3930,22 +3929,8 @@ 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); - 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(r->aiocb); + blk_aio_cancel(r->aiocb); } assert(QTAILQ_EMPTY(&sq->out_req_list));