[v2] iscsi: Translate scsi sense into error code

Previously we return -EIO blindly when anything goes wrong. Add a helper
function to parse sense fields and try to make the return code more
meaningful.

This also fixes the default werror configuration (enospc) when we're
using qcow2 on an iscsi lun. The old -EIO not being treated as out of
space error failed to trigger vm stop.

Signed-off-by: Fam Zheng <famz@redhat.com>

---
v2: Drop the qcow2 patch with ERANGE -> ENOSPC.
    Drop dead break after return.
    Return EIO for NO_SENSE.
    Translate error code for other I/O paths too.
---
 block/iscsi.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 8 deletions(-)

On 04/11/2015 02:36, Fam Zheng wrote:
> Previously we return -EIO blindly when anything goes wrong. Add a helper
> function to parse sense fields and try to make the return code more
> meaningful.
> 
> This also fixes the default werror configuration (enospc) when we're
> using qcow2 on an iscsi lun. The old -EIO not being treated as out of
> space error failed to trigger vm stop.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> v2: Drop the qcow2 patch with ERANGE -> ENOSPC.
>     Drop dead break after return.
>     Return EIO for NO_SENSE.
>     Translate error code for other I/O paths too.
> ---
>  block/iscsi.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 9a628b7..b1bc0ef 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -84,6 +84,7 @@ typedef struct IscsiTask {
>      IscsiLun *iscsilun;
>      QEMUTimer retry_timer;
>      bool force_next_flush;
> +    int err_code;
>  } IscsiTask;
>  
>  typedef struct IscsiAIOCB {
> @@ -182,6 +183,51 @@ static inline unsigned exp_random(double mean)
>  #define QEMU_SCSI_STATUS_TIMEOUT        SCSI_STATUS_TIMEOUT
>  #endif
>  
> +static int iscsi_translate_sense(struct scsi_sense *sense)
> +{
> +    int ret;
> +
> +    switch (sense->key) {
> +    case SCSI_SENSE_NOT_READY:
> +        return -EBUSY;
> +    case SCSI_SENSE_DATA_PROTECTION:
> +        return -EACCES;
> +    case SCSI_SENSE_COMMAND_ABORTED:
> +        return -ECANCELED;
> +    case SCSI_SENSE_ILLEGAL_REQUEST:
> +        /* Parse ASCQ */
> +        break;
> +    default:
> +        return -EIO;
> +    }
> +    switch (sense->ascq) {
> +    case SCSI_SENSE_ASCQ_PARAMETER_LIST_LENGTH_ERROR:
> +    case SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE:
> +    case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB:
> +    case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_PARAMETER_LIST:
> +        ret = -EINVAL;
> +        break;
> +    case SCSI_SENSE_ASCQ_LBA_OUT_OF_RANGE:
> +        ret = -ENOSPC;
> +        break;
> +    case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED:
> +        ret = -ENOTSUP;
> +        break;
> +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT:
> +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_CLOSED:
> +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_OPEN:
> +        ret = -ENOMEDIUM;
> +        break;
> +    case SCSI_SENSE_ASCQ_WRITE_PROTECTED:
> +        ret = -EACCES;
> +        break;
> +    default:
> +        ret = -EIO;
> +        break;
> +    }
> +    return ret;
> +}
> +
>  static void
>  iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
>                          void *command_data, void *opaque)
> @@ -226,6 +272,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
>                  return;
>              }
>          }
> +        iTask->err_code = iscsi_translate_sense(&task->sense);
>          error_report("iSCSI Failure: %s", iscsi_get_error(iscsi));
>      } else {
>          iTask->iscsilun->force_next_flush |= iTask->force_next_flush;
> @@ -455,7 +502,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
> @@ -536,14 +583,14 @@ retry:
>  
>      lbas = scsi_datain_unmarshall(iTask.task);
>      if (lbas == NULL) {
> -        ret = -EIO;
> +        ret = iTask.err_code;
>          goto out;
>      }
>  
>      lbasd = &lbas->descriptors[0];
>  
>      if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
> -        ret = -EIO;
> +        ret = iTask.err_code;
>          goto out;
>      }
>  
> @@ -644,7 +691,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      return 0;
> @@ -683,7 +730,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      return 0;
> @@ -703,7 +750,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 = -EIO;
> +        acb->status = iscsi_translate_sense(&acb->task->sense);
>      }
>  
>      acb->ioh->driver_status = 0;
> @@ -905,7 +952,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      iscsi_allocationmap_clear(iscsilun, sector_num, nb_sectors);
> @@ -999,7 +1046,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      if (flags & BDRV_REQ_MAY_UNMAP) {
> 

Queued, thanks.

Paolo

Am 04.11.2015 um 02:36 schrieb Fam Zheng:
> Previously we return -EIO blindly when anything goes wrong. Add a helper
> function to parse sense fields and try to make the return code more
> meaningful.
>
> This also fixes the default werror configuration (enospc) when we're
> using qcow2 on an iscsi lun. The old -EIO not being treated as out of
> space error failed to trigger vm stop.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
>
> ---
> v2: Drop the qcow2 patch with ERANGE -> ENOSPC.
>     Drop dead break after return.
>     Return EIO for NO_SENSE.
>     Translate error code for other I/O paths too.
> ---
>  block/iscsi.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 9a628b7..b1bc0ef 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -84,6 +84,7 @@ typedef struct IscsiTask {
>      IscsiLun *iscsilun;
>      QEMUTimer retry_timer;
>      bool force_next_flush;
> +    int err_code;
>  } IscsiTask;
>  
>  typedef struct IscsiAIOCB {
> @@ -182,6 +183,51 @@ static inline unsigned exp_random(double mean)
>  #define QEMU_SCSI_STATUS_TIMEOUT        SCSI_STATUS_TIMEOUT
>  #endif
>  
> +static int iscsi_translate_sense(struct scsi_sense *sense)
> +{
> +    int ret;
> +
> +    switch (sense->key) {
> +    case SCSI_SENSE_NOT_READY:
> +        return -EBUSY;
> +    case SCSI_SENSE_DATA_PROTECTION:
> +        return -EACCES;
> +    case SCSI_SENSE_COMMAND_ABORTED:
> +        return -ECANCELED;
> +    case SCSI_SENSE_ILLEGAL_REQUEST:
> +        /* Parse ASCQ */
> +        break;
> +    default:
> +        return -EIO;
> +    }
> +    switch (sense->ascq) {
> +    case SCSI_SENSE_ASCQ_PARAMETER_LIST_LENGTH_ERROR:
> +    case SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE:
> +    case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB:
> +    case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_PARAMETER_LIST:
> +        ret = -EINVAL;
> +        break;
> +    case SCSI_SENSE_ASCQ_LBA_OUT_OF_RANGE:
> +        ret = -ENOSPC;
> +        break;
> +    case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED:
> +        ret = -ENOTSUP;
> +        break;
> +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT:
> +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_CLOSED:
> +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_OPEN:
> +        ret = -ENOMEDIUM;
> +        break;
> +    case SCSI_SENSE_ASCQ_WRITE_PROTECTED:
> +        ret = -EACCES;
> +        break;
> +    default:
> +        ret = -EIO;
> +        break;
> +    }
> +    return ret;
> +}
> +
>  static void
>  iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
>                          void *command_data, void *opaque)
> @@ -226,6 +272,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
>                  return;
>              }
>          }
> +        iTask->err_code = iscsi_translate_sense(&task->sense);
>          error_report("iSCSI Failure: %s", iscsi_get_error(iscsi));
>      } else {
>          iTask->iscsilun->force_next_flush |= iTask->force_next_flush;
> @@ -455,7 +502,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
> @@ -536,14 +583,14 @@ retry:
>  
>      lbas = scsi_datain_unmarshall(iTask.task);
>      if (lbas == NULL) {
> -        ret = -EIO;
> +        ret = iTask.err_code;
>          goto out;
>      }

An unmarschalling error will not set iTask.err_code. It is likely
0 here. Please leave it at -EIO.

>  
>      lbasd = &lbas->descriptors[0];
>  
>      if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
> -        ret = -EIO;
> +        ret = iTask.err_code;
>          goto out;
>      }

This is an internal sanity check, iTask.err_code will also be 0 here.

>  
> @@ -644,7 +691,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      return 0;
> @@ -683,7 +730,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      return 0;
> @@ -703,7 +750,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 = -EIO;
> +        acb->status = iscsi_translate_sense(&acb->task->sense);
>      }
>  
>      acb->ioh->driver_status = 0;
> @@ -905,7 +952,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      iscsi_allocationmap_clear(iscsilun, sector_num, nb_sectors);
> @@ -999,7 +1046,7 @@ retry:
>      }
>  
>      if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>      }
>  
>      if (flags & BDRV_REQ_MAY_UNMAP) {

Peter

On Wed, 11/04 21:12, Peter Lieven wrote:
> Am 04.11.2015 um 02:36 schrieb Fam Zheng:
> > Previously we return -EIO blindly when anything goes wrong. Add a helper
> > function to parse sense fields and try to make the return code more
> > meaningful.
> >
> > This also fixes the default werror configuration (enospc) when we're
> > using qcow2 on an iscsi lun. The old -EIO not being treated as out of
> > space error failed to trigger vm stop.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> >
> > ---
> > v2: Drop the qcow2 patch with ERANGE -> ENOSPC.
> >     Drop dead break after return.
> >     Return EIO for NO_SENSE.
> >     Translate error code for other I/O paths too.
> > ---
> >  block/iscsi.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 55 insertions(+), 8 deletions(-)
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 9a628b7..b1bc0ef 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -84,6 +84,7 @@ typedef struct IscsiTask {
> >      IscsiLun *iscsilun;
> >      QEMUTimer retry_timer;
> >      bool force_next_flush;
> > +    int err_code;
> >  } IscsiTask;
> >  
> >  typedef struct IscsiAIOCB {
> > @@ -182,6 +183,51 @@ static inline unsigned exp_random(double mean)
> >  #define QEMU_SCSI_STATUS_TIMEOUT        SCSI_STATUS_TIMEOUT
> >  #endif
> >  
> > +static int iscsi_translate_sense(struct scsi_sense *sense)
> > +{
> > +    int ret;
> > +
> > +    switch (sense->key) {
> > +    case SCSI_SENSE_NOT_READY:
> > +        return -EBUSY;
> > +    case SCSI_SENSE_DATA_PROTECTION:
> > +        return -EACCES;
> > +    case SCSI_SENSE_COMMAND_ABORTED:
> > +        return -ECANCELED;
> > +    case SCSI_SENSE_ILLEGAL_REQUEST:
> > +        /* Parse ASCQ */
> > +        break;
> > +    default:
> > +        return -EIO;
> > +    }
> > +    switch (sense->ascq) {
> > +    case SCSI_SENSE_ASCQ_PARAMETER_LIST_LENGTH_ERROR:
> > +    case SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE:
> > +    case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB:
> > +    case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_PARAMETER_LIST:
> > +        ret = -EINVAL;
> > +        break;
> > +    case SCSI_SENSE_ASCQ_LBA_OUT_OF_RANGE:
> > +        ret = -ENOSPC;
> > +        break;
> > +    case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED:
> > +        ret = -ENOTSUP;
> > +        break;
> > +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT:
> > +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_CLOSED:
> > +    case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_OPEN:
> > +        ret = -ENOMEDIUM;
> > +        break;
> > +    case SCSI_SENSE_ASCQ_WRITE_PROTECTED:
> > +        ret = -EACCES;
> > +        break;
> > +    default:
> > +        ret = -EIO;
> > +        break;
> > +    }
> > +    return ret;
> > +}
> > +
> >  static void
> >  iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
> >                          void *command_data, void *opaque)
> > @@ -226,6 +272,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
> >                  return;
> >              }
> >          }
> > +        iTask->err_code = iscsi_translate_sense(&task->sense);
> >          error_report("iSCSI Failure: %s", iscsi_get_error(iscsi));
> >      } else {
> >          iTask->iscsilun->force_next_flush |= iTask->force_next_flush;
> > @@ -455,7 +502,7 @@ retry:
> >      }
> >  
> >      if (iTask.status != SCSI_STATUS_GOOD) {
> > -        return -EIO;
> > +        return iTask.err_code;
> >      }
> >  
> >      iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
> > @@ -536,14 +583,14 @@ retry:
> >  
> >      lbas = scsi_datain_unmarshall(iTask.task);
> >      if (lbas == NULL) {
> > -        ret = -EIO;
> > +        ret = iTask.err_code;
> >          goto out;
> >      }
> 
> An unmarschalling error will not set iTask.err_code. It is likely
> 0 here. Please leave it at -EIO.
> 
> >  
> >      lbasd = &lbas->descriptors[0];
> >  
> >      if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
> > -        ret = -EIO;
> > +        ret = iTask.err_code;
> >          goto out;
> >      }
> 
> This is an internal sanity check, iTask.err_code will also be 0 here.

You're right in both cases, will fix. Thanks!

Fam