[Qemu-devel] [PATCH 0/2] iscsi fixes

[Qemu-devel] [PATCH 0/2] iscsi fixes

[Peter Lieven]

fix a bug leaving the allocmap in a wrong state if an UNMAP fails and
improve the logging of iscsi failures.

Peter Lieven (2):
  block/iscsi: dont leave allocmap in an invalid state on UNMAP failure
  block/iscsi: only report an iSCSI Failure if we don't handle it
    gracefully

 block/iscsi.c | 51 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 15 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure

[Peter Lieven]

we forgot to set the allocmap to invalid if an UNMAP call fails.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 4683f3b..c532ec7 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2,7 +2,7 @@
  * QEMU Block driver for iSCSI images
  *
  * Copyright (c) 2010-2011 Ronnie Sahlberg <ronniesahlberg@gmail.com>
- * Copyright (c) 2012-2016 Peter Lieven <pl@kamp.de>
+ * Copyright (c) 2012-2017 Peter Lieven <pl@kamp.de>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -1128,6 +1128,9 @@ retry:
         goto retry;
     }
 
+    iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
+                               bytes >> BDRV_SECTOR_BITS);
+
     if (iTask.status == SCSI_STATUS_CHECK_CONDITION) {
         /* the target might fail with a check condition if it
            is not happy with the alignment of the UNMAP request
@@ -1140,9 +1143,6 @@ retry:
         goto out_unlock;
     }
 
-    iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
-                               bytes >> BDRV_SECTOR_BITS);
-
 out_unlock:
     qemu_mutex_unlock(&iscsilun->mutex);
     return r;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure

[Eric Blake]

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

On 12/08/2017 05:51 AM, Peter Lieven wrote:
> we forgot to set the allocmap to invalid if an UNMAP call fails.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 

> @@ -1128,6 +1128,9 @@ retry:
>          goto retry;
>      }
>  
> +    iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
> +                               bytes >> BDRV_SECTOR_BITS);
> +

Semantic conflict with my pending patches to convert the allocmap to
byte-based:

https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg01253.html

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure

[Paolo Bonzini]

On 08/12/2017 12:51, Peter Lieven wrote:
> we forgot to set the allocmap to invalid if an UNMAP call fails.

This is only needed for "a power loss, a medium error, or hardware
error" according to the spec, but I guess this is the safe thing to do.

Paolo

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 4683f3b..c532ec7 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -2,7 +2,7 @@
>   * QEMU Block driver for iSCSI images
>   *
>   * Copyright (c) 2010-2011 Ronnie Sahlberg <ronniesahlberg@gmail.com>
> - * Copyright (c) 2012-2016 Peter Lieven <pl@kamp.de>
> + * Copyright (c) 2012-2017 Peter Lieven <pl@kamp.de>
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -1128,6 +1128,9 @@ retry:
>          goto retry;
>      }
>  
> +    iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
> +                               bytes >> BDRV_SECTOR_BITS);
> +
>      if (iTask.status == SCSI_STATUS_CHECK_CONDITION) {
>          /* the target might fail with a check condition if it
>             is not happy with the alignment of the UNMAP request
> @@ -1140,9 +1143,6 @@ retry:
>          goto out_unlock;
>      }
>  
> -    iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
> -                               bytes >> BDRV_SECTOR_BITS);
> -
>  out_unlock:
>      qemu_mutex_unlock(&iscsilun->mutex);
>      return r;
> 

[Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully

[Peter Lieven]

we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task
hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in
some cases and handle it gracefully. This is the case for misaligned UNMAPs
and WRITESAME10/16 calls without UNMAP. In this case a failure in the
logs can be quite misleading.

While we are at it improve the logging to reveal which operation failed
at what LBA.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index c532ec7..5c0a9e5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -104,6 +104,7 @@ typedef struct IscsiTask {
     IscsiLun *iscsilun;
     QEMUTimer retry_timer;
     int err_code;
+    char *err_str;
 } IscsiTask;
 
 typedef struct IscsiAIOCB {
@@ -265,7 +266,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
             }
         }
         iTask->err_code = iscsi_translate_sense(&task->sense);
-        error_report("iSCSI Failure: %s", iscsi_get_error(iscsi));
+        iTask->err_str = g_strdup(iscsi_get_error(iscsi));
     }
 
 out:
@@ -629,6 +630,8 @@ retry:
 
     if (iTask.status != SCSI_STATUS_GOOD) {
         iscsi_allocmap_set_invalid(iscsilun, sector_num, nb_sectors);
+        error_report("iSCSI WRITE10/16 failed at lba %" PRIu64 ": %s", lba,
+                     iTask.err_str);
         r = iTask.err_code;
         goto out_unlock;
     }
@@ -637,6 +640,7 @@ retry:
 
 out_unlock:
     qemu_mutex_unlock(&iscsilun->mutex);
+    g_free(iTask.err_str);
     return r;
 }
 
@@ -651,10 +655,9 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
     struct scsi_get_lba_status *lbas = NULL;
     struct scsi_lba_status_descriptor *lbasd = NULL;
     struct IscsiTask iTask;
+    uint64_t lba;
     int64_t ret;
 
-    iscsi_co_init_iscsitask(iscsilun, &iTask);
-
     if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
         ret = -EINVAL;
         goto out;
@@ -670,11 +673,13 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
         goto out;
     }
 
+    lba = sector_qemu2lun(sector_num, iscsilun);
+
+    iscsi_co_init_iscsitask(iscsilun, &iTask);
     qemu_mutex_lock(&iscsilun->mutex);
 retry:
     if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
-                                  sector_qemu2lun(sector_num, iscsilun),
-                                  8 + 16, iscsi_co_generic_cb,
+                                  lba, 8 + 16, iscsi_co_generic_cb,
                                   &iTask) == NULL) {
         ret = -ENOMEM;
         goto out_unlock;
@@ -701,6 +706,8 @@ retry:
          * because the device is busy or the cmd is not
          * supported) we pretend all blocks are allocated
          * for backwards compatibility */
+        error_report("iSCSI GET_LBA_STATUS failed at lba %" PRIu64 ": %s",
+                     lba, iTask.err_str);
         goto out_unlock;
     }
 
@@ -738,6 +745,7 @@ retry:
     }
 out_unlock:
     qemu_mutex_unlock(&iscsilun->mutex);
+    g_free(iTask.err_str);
 out:
     if (iTask.task != NULL) {
         scsi_free_scsi_task(iTask.task);
@@ -756,6 +764,7 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
     struct IscsiTask iTask;
     uint64_t lba;
     uint32_t num_sectors;
+    int r = 0;
 
     if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
         return -EINVAL;
@@ -853,19 +862,23 @@ retry:
         iTask.complete = 0;
         goto retry;
     }
-    qemu_mutex_unlock(&iscsilun->mutex);
 
     if (iTask.status != SCSI_STATUS_GOOD) {
-        return iTask.err_code;
+        error_report("iSCSI READ10/16 failed at lba %" PRIu64 ": %s",
+                     lba, iTask.err_str);
+        r = iTask.err_code;
     }
 
-    return 0;
+    qemu_mutex_unlock(&iscsilun->mutex);
+    g_free(iTask.err_str);
+    return r;
 }
 
 static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
+    int r = 0;
 
     iscsi_co_init_iscsitask(iscsilun, &iTask);
     qemu_mutex_lock(&iscsilun->mutex);
@@ -892,13 +905,15 @@ retry:
         iTask.complete = 0;
         goto retry;
     }
-    qemu_mutex_unlock(&iscsilun->mutex);
 
     if (iTask.status != SCSI_STATUS_GOOD) {
-        return iTask.err_code;
+        error_report("iSCSI SYNCHRONIZECACHE10 failed: %s", iTask.err_str);
+        r = iTask.err_code;
     }
 
-    return 0;
+    qemu_mutex_unlock(&iscsilun->mutex);
+    g_free(iTask.err_str);
+    return r;
 }
 
 #ifdef __linux__
@@ -1139,12 +1154,15 @@ retry:
     }
 
     if (iTask.status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI UNMAP failed at lba %" PRIu64 ": %s",
+                     list.lba, iTask.err_str);
         r = iTask.err_code;
         goto out_unlock;
     }
 
 out_unlock:
     qemu_mutex_unlock(&iscsilun->mutex);
+    g_free(iTask.err_str);
     return r;
 }
 
@@ -1241,6 +1259,8 @@ retry:
     if (iTask.status != SCSI_STATUS_GOOD) {
         iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
                                    bytes >> BDRV_SECTOR_BITS);
+        error_report("iSCSI WRITESAME10/16 failed at lba %" PRIu64 ": %s",
+                     lba, iTask.err_str);
         r = iTask.err_code;
         goto out_unlock;
     }
@@ -1255,6 +1275,7 @@ retry:
 
 out_unlock:
     qemu_mutex_unlock(&iscsilun->mutex);
+    g_free(iTask.err_str);
     return r;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully

[Eric Blake]

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

On 12/08/2017 05:51 AM, Peter Lieven wrote:
> we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task
> hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in
> some cases and handle it gracefully. This is the case for misaligned UNMAPs

Is the block layer still capable of producing a misaligned UNMAP?  If
so, that's probably a bug in the block layer for not honoring the block
limit geometries.

> and WRITESAME10/16 calls without UNMAP. In this case a failure in the
> logs can be quite misleading.
> 
> While we are at it improve the logging to reveal which operation failed
> at what LBA.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c | 43 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 11 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully

[Peter Lieven]

Am 08.12.2017 um 16:11 schrieb Eric Blake:
> On 12/08/2017 05:51 AM, Peter Lieven wrote:
>> we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task
>> hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in
>> some cases and handle it gracefully. This is the case for misaligned UNMAPs
> Is the block layer still capable of producing a misaligned UNMAP?  If
> so, that's probably a bug in the block layer for not honoring the block
> limit geometries.

In theory there should be none. I think we can drop this code.

Peter

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully

[Eric Blake]

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

On 12/08/2017 10:14 AM, Peter Lieven wrote:
> Am 08.12.2017 um 16:11 schrieb Eric Blake:
>> On 12/08/2017 05:51 AM, Peter Lieven wrote:
>>> we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task
>>> hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in
>>> some cases and handle it gracefully. This is the case for misaligned UNMAPs
>> Is the block layer still capable of producing a misaligned UNMAP?  If
>> so, that's probably a bug in the block layer for not honoring the block
>> limit geometries.
> 
> In theory there should be none. I think we can drop this code.

Or, better yet, replace the check with an assert.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully

[Peter Lieven]

Am 08.12.2017 um 18:03 schrieb Eric Blake:
> On 12/08/2017 10:14 AM, Peter Lieven wrote:
>> Am 08.12.2017 um 16:11 schrieb Eric Blake:
>>> On 12/08/2017 05:51 AM, Peter Lieven wrote:
>>>> we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task
>>>> hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in
>>>> some cases and handle it gracefully. This is the case for misaligned UNMAPs
>>> Is the block layer still capable of producing a misaligned UNMAP?  If
>>> so, that's probably a bug in the block layer for not honoring the block
>>> limit geometries.
>> In theory there should be none. I think we can drop this code.
> Or, better yet, replace the check with an assert.

I would not add an assert if the device returns a CHECK CONDITION because
there might be other reasons for it. But I think its safe to remove the extra handling.
If for any reason there is a request that the target does not like it will pop up on
stderr.

Peter