On Thu, 21 Nov 2019 at 11:57, Klaus Birkelund <i...@irrelevant.dk> wrote:
>
> On Tue, Nov 12, 2019 at 03:25:06PM +0000, Beata Michalska wrote:
> > Hi Klaus,
> >
> > On Tue, 15 Oct 2019 at 11:55, Klaus Jensen <i...@irrelevant.dk> wrote:
> > > @@ -341,19 +344,18 @@ static uint16_t nvme_dma_write_prp(NvmeCtrl *n,
> > > uint8_t *ptr, uint32_t len,
> > Any reason why the nvme_dma_write_prp is missing the changes applied
> > to nvme_dma_read_prp ?
> >
>
> This was adressed by proxy through changes to the previous patch
> (by combining the read/write functions).
>
> > > + case NVME_AIO_OPC_WRITE_ZEROES:
> > > + block_acct_start(stats, acct, aio->iov.size, BLOCK_ACCT_WRITE);
> > > + aio->aiocb = blk_aio_pwrite_zeroes(aio->blk, aio->offset,
> > > + aio->iov.size, BDRV_REQ_MAY_UNMAP, nvme_aio_cb, aio);
> > Minor: aio->blk => blk
> >
>
> Thanks. Fixed this in a couple of other places as well.
>
> > > @@ -621,8 +880,11 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd
> > > *cmd)
> > > sq = n->sq[qid];
> > > while (!QTAILQ_EMPTY(&sq->out_req_list)) {
> > > req = QTAILQ_FIRST(&sq->out_req_list);
> > > - assert(req->aiocb);
> > > - blk_aio_cancel(req->aiocb);
> > > + while (!QTAILQ_EMPTY(&req->aio_tailq)) {
> > > + aio = QTAILQ_FIRST(&req->aio_tailq);
> > > + assert(aio->aiocb);
> > > + blk_aio_cancel(aio->aiocb);
> > What about releasing memory associated with given aio ?
>
> I believe the callback is still called when cancelled? That should take
> care of it. Or have I misunderstood that? At least for the DMAAIOCBs it
> is.
>
It seems that the completion callback is supposed to be called.
My bad.
BR
Beata
> > > +struct NvmeAIO {
> > > + NvmeRequest *req;
> > > +
> > > + NvmeAIOOp opc;
> > > + int64_t offset;
> > > + BlockBackend *blk;
> > > + BlockAIOCB *aiocb;
> > > + BlockAcctCookie acct;
> > > +
> > > + NvmeAIOCompletionFunc *cb;
> > > + void *cb_arg;
> > > +
> > > + QEMUSGList *qsg;
> > > + QEMUIOVector iov;
> >
> > There is a bit of inconsistency on the ownership of IOVs and SGLs.
> > SGLs now seem to be owned by request whereas IOVs by the aio.
> > WOuld be good to have that unified or documented at least.
> >
>
> Fixed this. The NvmeAIO only holds pointers now.
>
> > > +#define NVME_REQ_TRANSFER_DMA 0x1
> > This one does not seem to be used ....
> >
>
> I have dropped the flags and reverted to a simple req->is_cmb as that is
> all that is really needed.
>
