| Message ID | 20201112195945.819915-3-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/nvme: dulbe and dsm support | expand |
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.
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.
On Thu, Nov 12, 2020 at 08:59:42PM +0100, Klaus Jensen wrote: > +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; > + } Just curious, is there potentially a more appropriate way to set an nvme status based on the value of 'ret'? What is 'ret' representing anyway? Are these errno values?
On Nov 16 09:57, Keith Busch wrote: > On Thu, Nov 12, 2020 at 08:59:42PM +0100, Klaus Jensen wrote: > > +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; > > + } > > Just curious, is there potentially a more appropriate way to set an nvme > status based on the value of 'ret'? What is 'ret' representing anyway? > Are these errno values? > Yes, it's errno values from down below. But looking at this more closely, it actually looks like this is where we should behave as dictated by the rerror and werror drive options. I'll do a follow up patch to fix that.
On Nov 16 19:18, Klaus Jensen wrote: > On Nov 16 09:57, Keith Busch wrote: > > On Thu, Nov 12, 2020 at 08:59:42PM +0100, Klaus Jensen wrote: > > > +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; > > > + } > > > > Just curious, is there potentially a more appropriate way to set an nvme > > status based on the value of 'ret'? What is 'ret' representing anyway? > > Are these errno values? > > > > Yes, it's errno values from down below. > > But looking at this more closely, it actually looks like this is where > we should behave as dictated by the rerror and werror drive options. > > I'll do a follow up patch to fix that. So, following up on this after looking more into it. Currently, the device is basically behaving as if werror and rerror were both set to "report" - that is, report the error to the guest. Since we currently do not support werror and rerror, I think it is fine to behave as if it was report and set a meaningful status code that fits the command that failed (if we can). But I'll start working on a patch to support rerror/werror, since it would be nice to support.
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; + } + + req->status = status; +} + static void nvme_rw_cb(void *opaque, int ret) { NvmeRequest *req = opaque; @@ -887,37 +922,13 @@ static void nvme_rw_cb(void *opaque, int ret) BlockAcctCookie *acct = &req->acct; BlockAcctStats *stats = blk_get_stats(blk); - Error *local_err = NULL; - trace_pci_nvme_rw_cb(nvme_cid(req), blk_name(blk)); if (!ret) { block_acct_done(stats, acct); } else { - uint16_t status; - block_acct_failed(stats, acct); - - 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); - - req->status = status; + nvme_aio_err(req, ret); } nvme_enqueue_req_completion(nvme_cq(req), req);