On Mar 8 16:37, Stefan Hajnoczi wrote: > On Tue, Mar 02, 2021 at 12:10:37PM +0100, Klaus Jensen wrote: > > +static void nvme_dsm_cancel(BlockAIOCB *aiocb) > > +{ > > + NvmeDSMAIOCB *iocb = container_of(aiocb, NvmeDSMAIOCB, common); > > + > > + /* break loop */ > > + iocb->curr.len = 0; > > + iocb->curr.idx = iocb->nr; > > + > > + iocb->ret = -ECANCELED; > > + > > + if (iocb->aiocb) { > > + blk_aio_cancel_async(iocb->aiocb); > > + iocb->aiocb = NULL; > > + } > > +} > > Is the case where iocb->aiocb == NULL just in case nvme_dsm_cancel() is > called after the last discard has completed but before the BH runs? I > want to make sure there are no other cases because nothing would call > iocb->common.cb(). >
Yes - that case *can* happen, right? I modeled this after the appoach in the ide trim code (hw/ide/core.c). > > static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req) > > { > > NvmeNamespace *ns = req->ns; > > NvmeDsmCmd *dsm = (NvmeDsmCmd *) &req->cmd; > > - > > uint32_t attr = le32_to_cpu(dsm->attributes); > > uint32_t nr = (le32_to_cpu(dsm->nr) & 0xff) + 1; > > - > > uint16_t status = NVME_SUCCESS; > > > > trace_pci_nvme_dsm(nvme_cid(req), nvme_nsid(ns), nr, attr); > > > > if (attr & NVME_DSMGMT_AD) { > > - int64_t offset; > > - size_t len; > > - NvmeDsmRange range[nr]; > > - uintptr_t *discards = (uintptr_t *)&req->opaque; > > + NvmeDSMAIOCB *iocb = blk_aio_get(&nvme_dsm_aiocb_info, > > ns->blkconf.blk, > > + nvme_misc_cb, req); > > > > - status = nvme_dma(n, (uint8_t *)range, sizeof(range), > > + iocb->req = req; > > + iocb->bh = qemu_bh_new(nvme_dsm_bh, iocb); > > + iocb->ret = 0; > > + iocb->range = g_new(NvmeDsmRange, nr); > > + iocb->nr = nr; > > + iocb->curr.len = 0; > > + iocb->curr.idx = 0; > > + > > + status = nvme_dma(n, (uint8_t *)iocb->range, sizeof(NvmeDsmRange) > > * nr, > > DMA_DIRECTION_TO_DEVICE, req); > > if (status) { > > return status; > > } > > > > - /* > > - * AIO callbacks may be called immediately, so initialize discards > > to 1 > > - * to make sure the the callback does not complete the request > > before > > - * all discards have been issued. > > - */ > > - *discards = 1; > > + nvme_dsm_aio_cb(iocb, 0); > > + req->aiocb = &iocb->common; > > Want to move this line up one just in case something in > nvme_dsm_aio_cb() accesses req->aiocb? Sounds reasonable! Thanks!
signature.asc
Description: PGP signature