* [Qemu-devel] [PATCH 0/2] iscsi fixes
@ 2017-12-08 11:51 Peter Lieven
2017-12-08 11:51 ` [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure Peter Lieven
2017-12-08 11:51 ` [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully Peter Lieven
0 siblings, 2 replies; 9+ messages in thread
From: Peter Lieven @ 2017-12-08 11:51 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: mreitz, kwolf, ronniesahlberg, pbonzini, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure
2017-12-08 11:51 [Qemu-devel] [PATCH 0/2] iscsi fixes Peter Lieven
@ 2017-12-08 11:51 ` Peter Lieven
2017-12-08 15:07 ` Eric Blake
2017-12-13 21:35 ` Paolo Bonzini
2017-12-08 11:51 ` [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully Peter Lieven
1 sibling, 2 replies; 9+ messages in thread
From: Peter Lieven @ 2017-12-08 11:51 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: mreitz, kwolf, ronniesahlberg, pbonzini, Peter Lieven, qemu-stable
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
2017-12-08 11:51 [Qemu-devel] [PATCH 0/2] iscsi fixes Peter Lieven
2017-12-08 11:51 ` [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure Peter Lieven
@ 2017-12-08 11:51 ` Peter Lieven
2017-12-08 15:11 ` Eric Blake
1 sibling, 1 reply; 9+ messages in thread
From: Peter Lieven @ 2017-12-08 11:51 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: mreitz, kwolf, ronniesahlberg, pbonzini, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure
2017-12-08 11:51 ` [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure Peter Lieven
@ 2017-12-08 15:07 ` Eric Blake
2017-12-13 21:35 ` Paolo Bonzini
1 sibling, 0 replies; 9+ messages in thread
From: Eric Blake @ 2017-12-08 15:07 UTC (permalink / raw)
To: Peter Lieven, qemu-devel, qemu-block
Cc: kwolf, qemu-stable, mreitz, ronniesahlberg, pbonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
2017-12-08 11:51 ` [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully Peter Lieven
@ 2017-12-08 15:11 ` Eric Blake
2017-12-08 16:14 ` Peter Lieven
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-12-08 15:11 UTC (permalink / raw)
To: Peter Lieven, qemu-devel, qemu-block
Cc: kwolf, pbonzini, ronniesahlberg, mreitz
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
2017-12-08 15:11 ` Eric Blake
@ 2017-12-08 16:14 ` Peter Lieven
2017-12-08 17:03 ` Eric Blake
0 siblings, 1 reply; 9+ messages in thread
From: Peter Lieven @ 2017-12-08 16:14 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block
Cc: kwolf, pbonzini, ronniesahlberg, mreitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
2017-12-08 16:14 ` Peter Lieven
@ 2017-12-08 17:03 ` Eric Blake
2017-12-13 16:28 ` Peter Lieven
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-12-08 17:03 UTC (permalink / raw)
To: Peter Lieven, qemu-devel, qemu-block
Cc: kwolf, pbonzini, ronniesahlberg, mreitz
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
2017-12-08 17:03 ` Eric Blake
@ 2017-12-13 16:28 ` Peter Lieven
0 siblings, 0 replies; 9+ messages in thread
From: Peter Lieven @ 2017-12-13 16:28 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block
Cc: kwolf, pbonzini, ronniesahlberg, mreitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure
2017-12-08 11:51 ` [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure Peter Lieven
2017-12-08 15:07 ` Eric Blake
@ 2017-12-13 21:35 ` Paolo Bonzini
1 sibling, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2017-12-13 21:35 UTC (permalink / raw)
To: Peter Lieven, qemu-devel, qemu-block
Cc: kwolf, qemu-stable, mreitz, ronniesahlberg
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;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-13 21:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-08 11:51 [Qemu-devel] [PATCH 0/2] iscsi fixes Peter Lieven
2017-12-08 11:51 ` [Qemu-devel] [PATCH 1/2] block/iscsi: dont leave allocmap in an invalid state on UNMAP failure Peter Lieven
2017-12-08 15:07 ` Eric Blake
2017-12-13 21:35 ` Paolo Bonzini
2017-12-08 11:51 ` [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully Peter Lieven
2017-12-08 15:11 ` Eric Blake
2017-12-08 16:14 ` Peter Lieven
2017-12-08 17:03 ` Eric Blake
2017-12-13 16:28 ` Peter Lieven
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.