Re: [PATCH v4 17/24] nvme: allow multiple aios per command

[Klaus Birkelund Jensen]

On Jan  9 11:40, Beata Michalska wrote:
> Hi Klaus,
> 
> On Thu, 19 Dec 2019 at 13:09, Klaus Jensen <address@hidden> wrote:
> > +static NvmeAIO *nvme_aio_new(BlockBackend *blk, int64_t offset, size_t len,
> > +    QEMUSGList *qsg, QEMUIOVector *iov, NvmeRequest *req,
> > +    NvmeAIOCompletionFunc *cb)
> 
> Minor: The indentation here (and in a few other places across the patchset)
> does not seem right . And maybe inline ?

I tried to follow the style in CODING_STYLE.rst for "Multiline Indent",
but how the style is for function definition is a bit underspecified.

I can change it to align with the opening paranthesis. I just found the
"one indent" more readable for these long function definitions.

> Also : seems that there are cases when some of the parameters are
> not required (NULL) , maybe having a simplified version for those cases
> might be useful ?
> 

True. Actually - at this point in the series there are no users of the
NvmeAIOCompletionFunc. It is preparatory for other patches I have in the
pipeline. But I'll clean it up.

> > +static void nvme_aio_cb(void *opaque, int ret)
> > +{
> > +    NvmeAIO *aio = opaque;
> > +    NvmeRequest *req = aio->req;
> > +
> > +    BlockBackend *blk = aio->blk;
> > +    BlockAcctCookie *acct = &aio->acct;
> > +    BlockAcctStats *stats = blk_get_stats(blk);
> > +
> > +    Error *local_err = NULL;
> > +
> > +    trace_nvme_dev_aio_cb(nvme_cid(req), aio, blk_name(blk), aio->offset,
> > +        nvme_aio_opc_str(aio), req);
> > +
> > +    if (req) {
> > +        QTAILQ_REMOVE(&req->aio_tailq, aio, tailq_entry);
> > +    }
> > +
> >      if (!ret) {
> > -        block_acct_done(blk_get_stats(n->conf.blk), &req->acct);
> > -        req->status = NVME_SUCCESS;
> > +        block_acct_done(stats, acct);
> > +
> > +        if (aio->cb) {
> > +            aio->cb(aio, aio->cb_arg);
> 
> We are dropping setting status to SUCCESS here,
> is that expected ?

Yes, that is on purpose. nvme_aio_cb is called for *each* issued AIO and
we do not want to overwrite a previously set error status with a success
(if one aio in the request fails even though others succeed, it should
not go unnoticed). Note that NVME_SUCCESS is the default setting in the
request, so if no one sets an error code we are still good.

> Also the aio callback will not get
> called case failure and it probably should ?
> 

I tried both but ended up with just not calling it on failure, but I
think that in the future some AIO callbacks might want to take a
different action if the request failed, so I'll add it back in an add
the aio return value (ret) to the callback function definition.


Thanks,
Klaus