[libvirt] [PATCH] storage: Ignore block devices that fail format detection
https://bugzilla.redhat.com/show_bug.cgi?id=1276198 Prior to commit id '98322052' failure to saferead the block device would cause an error to be logged and the device to be skipped while attempting to discover/create a stable target path for a new LUN (NPIV). This was because virStorageBackendSCSIFindLUs ignored errors from processLU and virStorageBackendSCSINewLun. Ignoring the failure allowed a multipath device with an "active" and "ghost" to be present on the host with the "ghost" block device being ignored. This patch will return a -2 to the caller indicating the desire to ignore the block device since it cannot be used directly rather than fail the pool startup. Additionally, it was found during some debugging that it was possible for the virStorageBackendDetectBlockVolFormatFD to not detect a format, which while not a probably - we probably should at least add some sort of warning message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_scsi.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..2de606f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1393,6 +1393,10 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } } + if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN) + VIR_WARN("cannot determine the target format for '%s'", + target->path); + return 0; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a593a2b..d60473d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -224,9 +224,14 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto cleanup; } + /* Failing to process the format is not fatal - we'll just skip + * this volume. + */ if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { + retval = -2; goto cleanup; + } if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) goto cleanup; -- 2.1.0
ping Thanks - John On 10/30/2015 11:13 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1276198
Prior to commit id '98322052' failure to saferead the block device would cause an error to be logged and the device to be skipped while attempting to discover/create a stable target path for a new LUN (NPIV).
This was because virStorageBackendSCSIFindLUs ignored errors from processLU and virStorageBackendSCSINewLun.
Ignoring the failure allowed a multipath device with an "active" and "ghost" to be present on the host with the "ghost" block device being ignored. This patch will return a -2 to the caller indicating the desire to ignore the block device since it cannot be used directly rather than fail the pool startup.
Additionally, it was found during some debugging that it was possible for the virStorageBackendDetectBlockVolFormatFD to not detect a format, which while not a probably - we probably should at least add some sort of warning message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_scsi.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..2de606f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1393,6 +1393,10 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } }
+ if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN) + VIR_WARN("cannot determine the target format for '%s'", + target->path); + return 0; }
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a593a2b..d60473d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -224,9 +224,14 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto cleanup; }
+ /* Failing to process the format is not fatal - we'll just skip + * this volume. + */ if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { + retval = -2; goto cleanup; + }
if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) goto cleanup;
ping*2? Tks - John On 10/30/2015 11:13 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1276198
Prior to commit id '98322052' failure to saferead the block device would cause an error to be logged and the device to be skipped while attempting to discover/create a stable target path for a new LUN (NPIV).
This was because virStorageBackendSCSIFindLUs ignored errors from processLU and virStorageBackendSCSINewLun.
Ignoring the failure allowed a multipath device with an "active" and "ghost" to be present on the host with the "ghost" block device being ignored. This patch will return a -2 to the caller indicating the desire to ignore the block device since it cannot be used directly rather than fail the pool startup.
Additionally, it was found during some debugging that it was possible for the virStorageBackendDetectBlockVolFormatFD to not detect a format, which while not a probably - we probably should at least add some sort of warning message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_scsi.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..2de606f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1393,6 +1393,10 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } }
+ if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN) + VIR_WARN("cannot determine the target format for '%s'", + target->path); + return 0; }
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a593a2b..d60473d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -224,9 +224,14 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto cleanup; }
+ /* Failing to process the format is not fatal - we'll just skip + * this volume. + */ if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { + retval = -2; goto cleanup; + }
if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) goto cleanup;
On Fri, Oct 30, 2015 at 11:13:21AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1276198
Prior to commit id '98322052' failure to saferead the block device would cause an error to be logged and the device to be skipped while attempting to discover/create a stable target path for a new LUN (NPIV).
This was because virStorageBackendSCSIFindLUs ignored errors from processLU and virStorageBackendSCSINewLun.
Ignoring the failure allowed a multipath device with an "active" and "ghost" to be present on the host with the "ghost" block device being ignored. This patch will return a -2 to the caller indicating the desire to ignore the block device since it cannot be used directly rather than fail the pool startup.
Additionally, it was found during some debugging that it was possible for the virStorageBackendDetectBlockVolFormatFD to not detect a format, which while not a probably - we probably should at least add some sort of warning message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_scsi.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..2de606f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1393,6 +1393,10 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } }
+ if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN) + VIR_WARN("cannot determine the target format for '%s'", + target->path); + return 0; }
This change is unrelated to the other one. Also, is this warning really needed? I think leaving the format as 'unknown' is visible enough.
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a593a2b..d60473d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -224,9 +224,14 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto cleanup; }
+ /* Failing to process the format is not fatal - we'll just skip + * this volume. + */ if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { + retval = -2; goto cleanup;
Adding the VIR_STORAGE_VOL_OPEN_NOERROR flag and propagating it down to virStorageBackendDetectBlockVolFormatFD would be nicer. That way it can return -2 on EIO if the flag is present without spamming the logs. The other functions called by virStorageBackendUpdateVolInfo can also return -1 on a fatal error that way (although there do not seem to be any functions that could fail with NOERROR at the moment). Jan
On 11/23/2015 09:13 AM, Ján Tomko wrote:
On Fri, Oct 30, 2015 at 11:13:21AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1276198
Prior to commit id '98322052' failure to saferead the block device would cause an error to be logged and the device to be skipped while attempting to discover/create a stable target path for a new LUN (NPIV).
This was because virStorageBackendSCSIFindLUs ignored errors from processLU and virStorageBackendSCSINewLun.
Ignoring the failure allowed a multipath device with an "active" and "ghost" to be present on the host with the "ghost" block device being ignored. This patch will return a -2 to the caller indicating the desire to ignore the block device since it cannot be used directly rather than fail the pool startup.
Additionally, it was found during some debugging that it was possible for the virStorageBackendDetectBlockVolFormatFD to not detect a format, which while not a probably - we probably should at least add some sort of warning message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_scsi.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..2de606f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1393,6 +1393,10 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } }
+ if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN) + VIR_WARN("cannot determine the target format for '%s'", + target->path); + return 0; }
This change is unrelated to the other one. Also, is this warning really needed? I think leaving the format as 'unknown' is visible enough.
No not needed - I can remove it or change to VIR_DEBUG - it was certainly useful when debugging.
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a593a2b..d60473d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -224,9 +224,14 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto cleanup; }
+ /* Failing to process the format is not fatal - we'll just skip + * this volume. + */ if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { + retval = -2; goto cleanup;
Adding the VIR_STORAGE_VOL_OPEN_NOERROR flag and propagating it down to virStorageBackendDetectBlockVolFormatFD would be nicer.
So it's not an OPEN failure rather this a READ failure. The volume could be opened and passing OPEN_NOERROR along in the "more general" is probably not what should be done as that opens up a new classification of errors that could get by. I'll work on adding a similar read noerror setting John
That way it can return -2 on EIO if the flag is present without spamming the logs. The other functions called by virStorageBackendUpdateVolInfo can also return -1 on a fatal error that way (although there do not seem to be any functions that could fail with NOERROR at the moment).
Jan
participants (2)
-
John Ferlan -
Ján Tomko