Re: [PATCH RFC 1/4] hw/block/nvme: convert dsm to aiocb

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]