[Qemu-devel] [PATCH 0/2] Fix werror=enospc for qcow2 on iscsi

[Qemu-devel] [PATCH 0/2] Fix werror=enospc for qcow2 on iscsi

[Fam Zheng]

When qcow2 is created on iscsi target with a virtual size greater than physical
capacity of the LUN, over time it's possible that guest fills too much data and
at that point, new clusters in qcow2 will be allocated beyond the end of disk.

werror=enospc is useful for that purpose to allocate more data for the guest,
except in this case, unlike a host file system, iscsi returns -EIO instead of
-ENOSPC, which makes it hard to detect and report proper error.

Fix this by improving iscsi error handling code to return meaningful error
codes (-ERANGE here), then further translate it to -ENOSPC in qcow2.

Fam


Fam Zheng (2):
  iscsi: Translate scsi sense into error code
  qcow2: Translate -ERANGE to -ENOSPC

 block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qcow2.c |  6 ++++++
 2 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[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.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 93f1ee4..f3e20ae 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,58 @@ 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 = 0;
+
+    switch (sense->key) {
+    case SCSI_SENSE_NO_SENSE:
+        return 0;
+        break;
+    case SCSI_SENSE_NOT_READY:
+        return -EBUSY;
+        break;
+    case SCSI_SENSE_DATA_PROTECTION:
+        return -EACCES;
+        break;
+    case SCSI_SENSE_COMMAND_ABORTED:
+        return -ECANCELED;
+        break;
+    case SCSI_SENSE_ILLEGAL_REQUEST:
+        /* Parse ASCQ */
+        break;
+    default:
+        return -EIO;
+        break;
+    }
+    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 = -ERANGE;
+        break;
+    case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED:
+        ret = -ENOTSUP;
+        break;
+    case SCSI_SENSE_ASCQ_WRITE_PROTECTED:
+        ret = -EACCES;
+        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;
+    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 +279,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 +509,7 @@ retry:
     }
 
     if (iTask.status != SCSI_STATUS_GOOD) {
-        return -EIO;
+        return iTask.err_code;
     }
 
     iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[Peter Lieven]

Am 22.10.2015 um 10:17 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.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 93f1ee4..f3e20ae 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,58 @@ 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 = 0;
> +
> +    switch (sense->key) {
> +    case SCSI_SENSE_NO_SENSE:
> +        return 0;
> +        break;
> +    case SCSI_SENSE_NOT_READY:
> +        return -EBUSY;
> +        break;
> +    case SCSI_SENSE_DATA_PROTECTION:
> +        return -EACCES;
> +        break;
> +    case SCSI_SENSE_COMMAND_ABORTED:
> +        return -ECANCELED;
> +        break;
> +    case SCSI_SENSE_ILLEGAL_REQUEST:
> +        /* Parse ASCQ */
> +        break;
> +    default:
> +        return -EIO;
> +        break;
> +    }
> +    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 = -ERANGE;
> +        break;
> +    case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED:
> +        ret = -ENOTSUP;
> +        break;
> +    case SCSI_SENSE_ASCQ_WRITE_PROTECTED:
> +        ret = -EACCES;
> +        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;
> +    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 +279,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 +509,7 @@ retry:
>       }
>   
>       if (iTask.status != SCSI_STATUS_GOOD) {
> -        return -EIO;
> +        return iTask.err_code;
>       }

why do you only use that translated error code for writev? Other calls could benefit from it as well.

Peter

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[Paolo Bonzini]

On 22/10/2015 10:31, Peter Lieven wrote:
> 
> +    switch (sense->key) {
> +    case SCSI_SENSE_NO_SENSE:
> +        return 0;
> +        break;
> +    case SCSI_SENSE_NOT_READY:
> +        return -EBUSY;
> +        break;
> +    case SCSI_SENSE_DATA_PROTECTION:
> +        return -EACCES;

Probably EPERM, not EACCES.

> +        break;
> +    case SCSI_SENSE_COMMAND_ABORTED:
> +        return -ECANCELED;
> +        break;
> +    case SCSI_SENSE_ILLEGAL_REQUEST:
> +        /* Parse ASCQ */
> +        break;
> +    default:
> +        return -EIO;
> +        break;
> +    }
> +    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 = -ERANGE;
> +        break;
> +    case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED:
> +        ret = -ENOTSUP;
> +        break;
> +    case SCSI_SENSE_ASCQ_WRITE_PROTECTED:
> +        ret = -EACCES;

Same here.

Paolo

> +        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;
> +    default:
> +        ret = -EIO;
> +        break;
> +    }

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[Peter Lieven]

Am 22.10.2015 um 10:45 schrieb Paolo Bonzini:
>
> On 22/10/2015 10:31, Peter Lieven wrote:
>> +    switch (sense->key) {
>> +    case SCSI_SENSE_NO_SENSE:
>> +        return 0;
>> +        break;

Isn't it dangerous to return 0 (no error) if the status is != SCSI_STATUS_GOOD?

Peter

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[Fam Zheng]

On Thu, 10/22 10:45, Paolo Bonzini wrote:
> 
> 
> On 22/10/2015 10:31, Peter Lieven wrote:
> > 
> > +    switch (sense->key) {
> > +    case SCSI_SENSE_NO_SENSE:
> > +        return 0;
> > +        break;
> > +    case SCSI_SENSE_NOT_READY:
> > +        return -EBUSY;
> > +        break;
> > +    case SCSI_SENSE_DATA_PROTECTION:
> > +        return -EACCES;
> 
> Probably EPERM, not EACCES.

The comment of bdrv_write says -EACCES for read-only device, shoudn't this be
the same?

Fam

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[Paolo Bonzini]

On 22/10/2015 11:51, Fam Zheng wrote:
> > > 
> > > +    switch (sense->key) {
> > > +    case SCSI_SENSE_NO_SENSE:
> > > +        return 0;
> > > +        break;
> > > +    case SCSI_SENSE_NOT_READY:
> > > +        return -EBUSY;
> > > +        break;
> > > +    case SCSI_SENSE_DATA_PROTECTION:
> > > +        return -EACCES;
> > 
> > Probably EPERM, not EACCES.
> 
> The comment of bdrv_write says -EACCES for read-only device, shoudn't this be
> the same?

Oh, then that's fine.  I was looking at the write(2) man page.

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

[Kevin Wolf]

Am 22.10.2015 um 10:17 hat Fam Zheng geschrieben:
> 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.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 93f1ee4..f3e20ae 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,58 @@ 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 = 0;
> +
> +    switch (sense->key) {
> +    case SCSI_SENSE_NO_SENSE:
> +        return 0;
> +        break;
> +    case SCSI_SENSE_NOT_READY:
> +        return -EBUSY;
> +        break;
> +    case SCSI_SENSE_DATA_PROTECTION:
> +        return -EACCES;
> +        break;
> +    case SCSI_SENSE_COMMAND_ABORTED:
> +        return -ECANCELED;
> +        break;
> +    case SCSI_SENSE_ILLEGAL_REQUEST:
> +        /* Parse ASCQ */
> +        break;
> +    default:
> +        return -EIO;
> +        break;
> +    }

break after return is dead code.

Kevin

[Qemu-devel] [PATCH 2/2] qcow2: Translate -ERANGE to -ENOSPC

[Fam Zheng]

This will make the default werror (=enospc) work better when qcow2 is
created on top of iscsi or other block devices.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qcow2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index bacc4f2..8edf0fe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1586,6 +1586,12 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
                              cur_nr_sectors, &hd_qiov);
         qemu_co_mutex_lock(&s->lock);
         if (ret < 0) {
+            if (ret == -ERANGE) {
+                /* Out of range access means we're already allocating clusters
+                 * beyond end of disk, fix the error code to support
+                 * werror=enospc. */
+                ret = -ENOSPC;
+            }
             goto fail;
         }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/2] Fix werror=enospc for qcow2 on iscsi

[Peter Lieven]

Am 22.10.2015 um 10:17 schrieb Fam Zheng:
> When qcow2 is created on iscsi target with a virtual size greater than physical
> capacity of the LUN, over time it's possible that guest fills too much data and
> at that point, new clusters in qcow2 will be allocated beyond the end of disk.

Why would you want to put a QCOW2 on a fixed size block device?

Anyway, I like the error code translation.

Peter

Re: [Qemu-devel] [PATCH 0/2] Fix werror=enospc for qcow2 on iscsi

[Fam Zheng]

On Thu, 10/22 10:32, Peter Lieven wrote:
> Am 22.10.2015 um 10:17 schrieb Fam Zheng:
> >When qcow2 is created on iscsi target with a virtual size greater than physical
> >capacity of the LUN, over time it's possible that guest fills too much data and
> >at that point, new clusters in qcow2 will be allocated beyond the end of disk.
> 
> Why would you want to put a QCOW2 on a fixed size block device?
> 

It can have a backing file.

Fam

Re: [Qemu-devel] [PATCH 0/2] Fix werror=enospc for qcow2 on iscsi

[Paolo Bonzini]

On 22/10/2015 10:17, Fam Zheng wrote:
> When qcow2 is created on iscsi target with a virtual size greater than physical
> capacity of the LUN, over time it's possible that guest fills too much data and
> at that point, new clusters in qcow2 will be allocated beyond the end of disk.
> 
> werror=enospc is useful for that purpose to allocate more data for the guest,
> except in this case, unlike a host file system, iscsi returns -EIO instead of
> -ENOSPC, which makes it hard to detect and report proper error.
> 
> Fix this by improving iscsi error handling code to return meaningful error
> codes (-ERANGE here), then further translate it to -ENOSPC in qcow2.

FWIW, Linux uses ENOSPC if it detects out of range LBAs:

        if (iocb->ki_pos >= size)
                return -ENOSPC;

so I think it's okay to convert LBA_OUT_OF_RANGE to ENOSPC directly and
avoid patch 2.

Paolo

Re: [Qemu-devel] [PATCH 0/2] Fix werror=enospc for qcow2 on iscsi

[Kevin Wolf]

Am 22.10.2015 um 10:45 hat Paolo Bonzini geschrieben:
> On 22/10/2015 10:17, Fam Zheng wrote:
> > When qcow2 is created on iscsi target with a virtual size greater than physical
> > capacity of the LUN, over time it's possible that guest fills too much data and
> > at that point, new clusters in qcow2 will be allocated beyond the end of disk.
> > 
> > werror=enospc is useful for that purpose to allocate more data for the guest,
> > except in this case, unlike a host file system, iscsi returns -EIO instead of
> > -ENOSPC, which makes it hard to detect and report proper error.
> > 
> > Fix this by improving iscsi error handling code to return meaningful error
> > codes (-ERANGE here), then further translate it to -ENOSPC in qcow2.
> 
> FWIW, Linux uses ENOSPC if it detects out of range LBAs:
> 
>         if (iocb->ki_pos >= size)
>                 return -ENOSPC;
> 
> so I think it's okay to convert LBA_OUT_OF_RANGE to ENOSPC directly and
> avoid patch 2.

Yes, definitely. Fixing this up in qcow2 would be wrong, all other image
formats are affected as well. The iscsi block driver already needs to
return the correct errno value, which is ENOSPC.

Kevin