On Feb 11 11:49, Minwoo Im wrote:
> On 21-01-27 14:15:05, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jen...@samsung.com>
> >
> > For most commands, when issuing an AIO, the BlockAIOCB is stored in the
> > NvmeRequest aiocb pointer when the AIO is issued. The purpose of storing
> > this is to allow the AIO to be cancelled when deleting submission
> > queues (it is currently not used for Abort).
> >
> > Since the addition of the Dataset Management command and Zoned
> > Namespaces, NvmeRequests may involve more than one AIO and 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.
> >
> > Fix this by instead of explicitly cancelling the requests, just allow
> > the AIOs to complete by draining the namespace blockdevs.
> >
> > Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
> > ---
> > hw/block/nvme.c | 18 +++++++++++++-----
> > 1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 316858fd8adf..91f6fb6da1e2 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -403,6 +403,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;
> > }
> > @@ -2396,6 +2397,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest
> > *req)
> > NvmeSQueue *sq;
> > NvmeCQueue *cq;
> > uint16_t qid = le16_to_cpu(c->qid);
> > + int i;
> >
> > if (unlikely(!qid || nvme_check_sqid(n, qid))) {
> > trace_pci_nvme_err_invalid_del_sq(qid);
> > @@ -2404,12 +2406,18 @@ static uint16_t nvme_del_sq(NvmeCtrl *n,
> > NvmeRequest *req)
> >
> > trace_pci_nvme_del_sq(qid);
> >
> > - 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);
> > + for (i = 1; i <= n->num_namespaces; i++) {
> > + NvmeNamespace *ns = nvme_ns(n, i);
> > + if (!ns) {
> > + continue;
> > + }
> > +
> > + nvme_ns_drain(ns);
>
> If we just drain the entire namespaces here, commands which has nothing
> to do with the target sq to be deleted will be drained. And this might
> be a burden for a single SQ deletion.
> That is true. But how often would you dynamically delete and create I/O submission queues in the fast path? > By the way, agree with the multiple AIOs references problem for newly added > commands. But, shouldn't we manage the inflight AIO request references for > the newlly added commands with some other way and kill them all > explicitly as it was? Maybe some of list for AIOCBs? I was hesitant to add more stuff to NvmeRequest (like a QTAILQ to track this). Getting a steady-state with draining was an easy fix.
signature.asc
Description: PGP signature
