Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On 05/07/19 12:31, Shinichiro Kawasaki wrote:
>> This way there's generally no need to shoehorn ASC codes into errno.  I
>> still have to test my changes, but I hope to send something within a
>> couple of days.
> 
> Thanks for sharing your thoughts. Now I understand your idea.
> 
> I'm awaiting your patch. In case you want me to create the patch based on your 
> idea, please let me know. I can afford some time next week to work on it.

I didn't have time to finish, but since I will be out for part of next
week, here is what I currently have (untested, somewhat uncompiled).
It's a bugfix so it can be included after hard freeze.

Thanks!

Paolo

[-- Attachment #2: scsi-sense.patch --]
[-- Type: text/x-patch, Size: 14051 bytes --]

From fdccbd77192ca4450f6e8900bef392fa36242200 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 5 Jul 2019 19:07:17 +0200
Subject: [PATCH 0/5] *** SUBJECT HERE ***

*** BLURB HERE ***

Paolo Bonzini (4):
  scsi: explicitly list guest-recoverable sense codes
  scsi: add guest-recoverable ZBC errors
  iscsi: fix busy/timeout/task set full
  iscsi: base all handling of check condition on scsi_sense_to_errno

Shinichiro Kawasaki (1):
  scsi-disk: pass sense correctly for guest-recoverable errors

 block/iscsi.c        | 28 +++++++++++------------
 hw/scsi/scsi-disk.c  | 15 ++++++++++---
 include/scsi/utils.h |  1 +
 scsi/utils.c         | 53 +++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 77 insertions(+), 20 deletions(-)

-- 
2.21.0

From f8a4b9cd0cd878aacbdf4ad4e7a2451c87339dab Mon Sep 17 00:00:00 2001
From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Date: Tue, 2 Jul 2019 10:08:00 +0200
Subject: [PATCH 1/5] scsi-disk: pass sense correctly for guest-recoverable
 errors

When an error was passed down to the guest because it was recoverable,
the sense length was not copied from the SG_IO data.  As a result,
the guest saw the CHECK CONDITION status but not the sense data.

Signed-off-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ed7295bfd7..5d3fb3c9d5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -62,6 +62,7 @@ typedef struct SCSIDiskClass {
     DMAIOFunc       *dma_readv;
     DMAIOFunc       *dma_writev;
     bool            (*need_fua_emulation)(SCSICommand *cmd);
+    void            (*update_sense)(SCSIRequest *r);
 } SCSIDiskClass;
 
 typedef struct SCSIDiskReq {
@@ -438,6 +439,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
 {
     bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
     BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
                                                    is_read, error);
 
@@ -456,6 +458,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
             if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
                 error == 0)  {
                 /* These errors are handled by guest. */
+                sdc->update_sense(&r->req);
                 scsi_req_complete(&r->req, *r->status);
                 return true;
             }
@@ -2894,6 +2897,12 @@ static int scsi_block_parse_cdb(SCSIDevice *d, SCSICommand *cmd,
     }
 }
 
+static void scsi_block_update_sense(SCSIRequest *req)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+    SCSIBlockReq *br = DO_UPCAST(SCSIBlockReq, req, r);
+    r->req.sense_len = MIN(br->io_header.sb_len_wr, sizeof(r->req.sense));
+}
 #endif
 
 static
@@ -3059,6 +3068,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     sc->parse_cdb    = scsi_block_parse_cdb;
     sdc->dma_readv   = scsi_block_dma_readv;
     sdc->dma_writev  = scsi_block_dma_writev;
+    sdc->update_sense = scsi_block_update_sense;
     sdc->need_fua_emulation = scsi_block_no_fua;
     dc->desc = "SCSI block device passthrough";
     dc->props = scsi_block_properties;
-- 
2.21.0


From ab60dfcd4c2bbad79d2bdfae37a88b031917ec99 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 10:23:20 +0200
Subject: [PATCH 2/5] scsi: explicitly list guest-recoverable sense codes

It's not really possible to fit all sense codes into errno codes, especially
in such a way that sense codes can be properly categorized as either.
guest-recoverable and host-handled.  Just create a new function that
checks for guest recoverable sense, then scsi_sense_buf_to_errno only
needs to be called for host handled sense codes.
---
 hw/scsi/scsi-disk.c  |  5 ++---
 include/scsi/utils.h |  1 +
 scsi/utils.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5d3fb3c9d5..8e95e3e38d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -454,14 +454,13 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
              * pause the host.
              */
             assert(r->status && *r->status);
-            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
-            if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
-                error == 0)  {
+            if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
                 /* These errors are handled by guest. */
                 sdc->update_sense(&r->req);
                 scsi_req_complete(&r->req, *r->status);
                 return true;
             }
+            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
             break;
         case ENOMEDIUM:
             scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 9351b21ead..fee6212e85 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -106,6 +106,7 @@ extern const struct SCSISense sense_code_SPACE_ALLOC_FAILED;
 
 int scsi_sense_to_errno(int key, int asc, int ascq);
 int scsi_sense_buf_to_errno(const uint8_t *sense, size_t sense_size);
+int scsi_sense_buf_is_guest_recoverable(const uint8_t *sense, size_t sense_size);
 
 int scsi_convert_sense(uint8_t *in_buf, int in_len,
                        uint8_t *buf, int len, bool fixed);
diff --git a/scsi/utils.c b/scsi/utils.c
index 8738522955..3ad28face1 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -336,6 +336,38 @@ int scsi_convert_sense(uint8_t *in_buf, int in_len,
     }
 }
 
+static bool scsi_sense_is_guest_recoverable(int key, int asc, int ascq)
+{
+    switch (key) {
+    case NO_SENSE:
+    case RECOVERED_ERROR:
+    case UNIT_ATTENTION:
+    case ABORTED_COMMAND:
+        return true;
+    case NOT_READY:
+    case ILLEGAL_REQUEST:
+    case DATA_PROTECT:
+        /* Parse ASCQ */
+        break;
+    default:
+        return false;
+    }
+
+    switch ((asc << 8) | ascq) {
+    case 0x1a00: /* PARAMETER LIST LENGTH ERROR */
+    case 0x2000: /* INVALID OPERATION CODE */
+    case 0x2400: /* INVALID FIELD IN CDB */
+    case 0x2500: /* LOGICAL UNIT NOT SUPPORTED */
+    case 0x2600: /* INVALID FIELD IN PARAMETER LIST */
+
+    case 0x0401: /* NOT READY, IN PROGRESS OF BECOMING READY */
+    case 0x0402: /* NOT READY, INITIALIZING COMMAND REQUIRED */
+        return true;
+    default:
+        return false;
+    }
+}
+
 int scsi_sense_to_errno(int key, int asc, int ascq)
 {
     switch (key) {
@@ -391,6 +423,17 @@ int scsi_sense_buf_to_errno(const uint8_t *in_buf, size_t in_len)
     return scsi_sense_to_errno(sense.key, sense.asc, sense.ascq);
 }
 
+bool scsi_sense_buf_is_guest_recoverable(const uint8_t *in_buf, size_t in_len)
+{
+    SCSISense sense;
+    if (in_len < 1) {
+        return false;
+    }
+
+    sense = scsi_parse_sense_buf(in_buf, in_len);
+    return scsi_sense_to_errno(sense.key, sense.asc, sense.ascq);
+}
+
 const char *scsi_command_name(uint8_t cmd)
 {
     static const char *names[] = {
-- 
2.21.0


From eca5917fcdf3ea690ee80d6f073d3b915691a499 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 10:01:03 +0200
Subject: [PATCH 3/5] scsi: add guest-recoverable ZBC errors

When I ran basic operations to the zoned storage from the guest via
scsi-block the following ASCs are reported for write or read commands
due to unexpected zone status or write pointer status:

     21h 04h: UNALIGNED WRITE COMMAND
     21h 05h: WRITE BOUNDARY VIOLATION
     21h 06h: ATTEMPT TO READ INVALID DATA
     55h 0Eh: INSUFFICIENT ZONE RESOURCES

Reporting these ASCs to the guest, the user applications can handle
them to manage zone/write pointer status, or help the user application
developers to understand the failure reason and fix bugs.

Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scsi/utils.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scsi/utils.c b/scsi/utils.c
index 3ad28face1..53274f62d7 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -360,6 +360,11 @@ static bool scsi_sense_is_guest_recoverable(int key, int asc, int ascq)
     case 0x2500: /* LOGICAL UNIT NOT SUPPORTED */
     case 0x2600: /* INVALID FIELD IN PARAMETER LIST */
 
+    case 0x2104: /* UNALIGNED WRITE COMMAND */
+    case 0x2105: /* WRITE BOUNDARY VIOLATION */
+    case 0x2106: /* ATTEMPT TO READ INVALID DATA */
+    case 0x550e: /* INSUFFICIENT ZONE RESOURCES */
+
     case 0x0401: /* NOT READY, IN PROGRESS OF BECOMING READY */
     case 0x0402: /* NOT READY, INITIALIZING COMMAND REQUIRED */
         return true;
-- 
2.21.0


From 594c17e8f6d617d5f234e1d3701cfc54e6804d1a Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 10:45:54 +0200
Subject: [PATCH 4/5] iscsi: fix busy/timeout/task set full

In this case, do_retry was set without calling aio_co_wake, thus never
waking up the coroutine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 267f160bf6..6e238bf0ad 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -272,7 +272,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                 timer_mod(&iTask->retry_timer,
                           qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + retry_time);
                 iTask->do_retry = 1;
-                return;
+                goto out;
             }
         }
         iTask->err_code = iscsi_translate_sense(&task->sense);
-- 
2.21.0


From fdccbd77192ca4450f6e8900bef392fa36242200 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 11:40:41 +0200
Subject: [PATCH 5/5] iscsi: base all handling of check condition on
 scsi_sense_to_errno

Now that scsi-disk is not using scsi_sense_to_errno to separate guest-recoverable
sense codes, we can modify it to simplify iscsi's own sense handling.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 28 ++++++++++++++--------------
 scsi/utils.c  |  5 ++---
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6e238bf0ad..5817967205 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -225,9 +225,9 @@ static inline unsigned exp_random(double mean)
 
 static int iscsi_translate_sense(struct scsi_sense *sense)
 {
-    return - scsi_sense_to_errno(sense->key,
-                                 (sense->ascq & 0xFF00) >> 8,
-                                 sense->ascq & 0xFF);
+    return scsi_sense_to_errno(sense->key,
+                               (sense->ascq & 0xFF00) >> 8,
+                               sense->ascq & 0xFF);
 }
 
 /* Called (via iscsi_service) with QemuMutex held.  */
@@ -244,13 +244,6 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
 
     if (status != SCSI_STATUS_GOOD) {
         if (iTask->retries++ < ISCSI_CMD_RETRIES) {
-            if (status == SCSI_STATUS_CHECK_CONDITION
-                && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
-                error_report("iSCSI CheckCondition: %s",
-                             iscsi_get_error(iscsi));
-                iTask->do_retry = 1;
-                goto out;
-            }
             if (status == SCSI_STATUS_BUSY ||
                 status == SCSI_STATUS_TIMEOUT ||
                 status == SCSI_STATUS_TASK_SET_FULL) {
@@ -272,11 +265,18 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                 timer_mod(&iTask->retry_timer,
                           qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + retry_time);
                 iTask->do_retry = 1;
-                goto out;
+            }
+        } else if (status == SCSI_STATUS_CHECK_CONDITION) {
+            int error = iscsi_translate_sense(&task->sense);
+            if (error == EAGAIN) {
+                error_report("iSCSI CheckCondition: %s",
+                             iscsi_get_error(iscsi));
+                iTask->do_retry = 1;
+            } else {
+                iTask->err_code = -error;
+                iTask->err_str = g_strdup(iscsi_get_error(iscsi));
             }
         }
-        iTask->err_code = iscsi_translate_sense(&task->sense);
-        iTask->err_str = g_strdup(iscsi_get_error(iscsi));
     }
 
 out:
@@ -974,7 +974,7 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     if (status < 0) {
         error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
                      iscsi_get_error(iscsi));
-        acb->status = iscsi_translate_sense(&acb->task->sense);
+        acb->status = -iscsi_translate_sense(&acb->task->sense);
     }
 
     acb->ioh->driver_status = 0;
diff --git a/scsi/utils.c b/scsi/utils.c
index 53274f62d7..7f332ebf1f 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -379,8 +379,7 @@ int scsi_sense_to_errno(int key, int asc, int ascq)
     case NO_SENSE:
     case RECOVERED_ERROR:
     case UNIT_ATTENTION:
-        /* These sense keys are not errors */
-        return 0;
+        return EAGAIN;
     case ABORTED_COMMAND: /* COMMAND ABORTED */
         return ECANCELED;
     case NOT_READY:
@@ -409,7 +408,7 @@ int scsi_sense_to_errno(int key, int asc, int ascq)
     case 0x2700: /* WRITE PROTECTED */
         return EACCES;
     case 0x0401: /* NOT READY, IN PROGRESS OF BECOMING READY */
-        return EAGAIN;
+        return EINPROGRESS;
     case 0x0402: /* NOT READY, INITIALIZING COMMAND REQUIRED */
         return ENOTCONN;
     default:
-- 
2.21.0