[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC 1/4] hw/block/nvme: convert dsm to aiocb
From: |
Klaus Jensen |
Subject: |
Re: [PATCH RFC 1/4] hw/block/nvme: convert dsm to aiocb |
Date: |
Mon, 8 Mar 2021 19:05:40 +0100 |
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
[PATCH RFC 3/4] hw/block/nvme: convert flush to aiocb, Klaus Jensen, 2021/03/02
[PATCH RFC 2/4] hw/block/nvme: convert copy to aiocb, Klaus Jensen, 2021/03/02
[PATCH RFC 4/4] hw/block/nvme: convert zone reset to aiocb, Klaus Jensen, 2021/03/02
Re: [PATCH RFC 0/4] hw/block/nvme: convert ad-hoc aio tracking to aiocbs, Stefan Hajnoczi, 2021/03/08