Re: [PATCH v8 2/5] hw/block/nvme: pull aio error handling

[Klaus Jensen]

On Nov 16 20:36, Minwoo Im wrote:
> On 11/12 20:59, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add a new function, nvme_aio_err, to handle errors resulting from AIOs
> > and use this from the callbacks.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 61 +++++++++++++++++++++++++++++--------------------
> >  1 file changed, 36 insertions(+), 25 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 51f35e8cf18b..a96a996ddc0d 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -878,6 +878,41 @@ static inline uint16_t nvme_check_bounds(NvmeNamespace 
> > *ns, uint64_t slba,
> >      return NVME_SUCCESS;
> >  }
> >  
> > +static void nvme_aio_err(NvmeRequest *req, int ret)
> > +{
> > +    uint16_t status = NVME_SUCCESS;
> > +    Error *local_err = NULL;
> > +
> > +    switch (req->cmd.opcode) {
> > +    case NVME_CMD_READ:
> > +        status = NVME_UNRECOVERED_READ;
> > +        break;
> > +    case NVME_CMD_FLUSH:
> > +    case NVME_CMD_WRITE:
> > +    case NVME_CMD_WRITE_ZEROES:
> > +        status = NVME_WRITE_FAULT;
> > +        break;
> > +    default:
> > +        status = NVME_INTERNAL_DEV_ERROR;
> > +        break;
> > +    }
> > +
> > +    trace_pci_nvme_err_aio(nvme_cid(req), strerror(ret), status);
> > +
> > +    error_setg_errno(&local_err, -ret, "aio failed");
> > +    error_report_err(local_err);
> > +
> > +    /*
> > +     * Set the command status code to the first encountered error but 
> > allow a
> > +     * subsequent Internal Device Error to trump it.
> > +     */
> > +    if (req->status && status != NVME_INTERNAL_DEV_ERROR) {
> > +        return;
> > +    }
> 
> Are we okay with the trace print up there in case that this if is taken?
> I guess if this if is taken, the trace print may not that matter.
> 

Yeah, I was thinking we always print the error that corresponds to the
AIO that we are handling right now.

But maybe we should not include the "translated" nvme error in the trace
at all. That might make more sense.

signature.asc
Description: PGP signature