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

[Stefan Hajnoczi]

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().

>  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?

signature.asc
Description: PGP signature