| Message ID | 20201116184041.60465-6-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: scsi-disk corrupts data | expand |
On 16/11/20 19:40, Hannes Reinecke wrote: > + case SCSI_HOST_TARGET_FAILURE: > + *sense = SENSE_CODE(TARGET_FAILURE); > + return CHECK_CONDITION; > + case SCSI_HOST_RESERVATION_ERROR: > + return RESERVATION_CONFLICT; > + case SCSI_HOST_ALLOCATION_FAILURE: > + *sense = SENSE_CODE(SPACE_ALLOC_FAILED); > + return CHECK_CONDITION; > + case SCSI_HOST_MEDIUM_ERROR: > + *sense = SENSE_CODE(READ_ERROR); > + return CHECK_CONDITION; Can these actually be visible to userspace? I'd rather avoid having them in QEMU if possible. Otherwise, the patches are completely sensible. Paolo
On 11/16/20 7:57 PM, Paolo Bonzini wrote: > On 16/11/20 19:40, Hannes Reinecke wrote: >> + case SCSI_HOST_TARGET_FAILURE: >> + *sense = SENSE_CODE(TARGET_FAILURE); >> + return CHECK_CONDITION; >> + case SCSI_HOST_RESERVATION_ERROR: >> + return RESERVATION_CONFLICT; >> + case SCSI_HOST_ALLOCATION_FAILURE: >> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED); >> + return CHECK_CONDITION; >> + case SCSI_HOST_MEDIUM_ERROR: >> + *sense = SENSE_CODE(READ_ERROR); >> + return CHECK_CONDITION; > > Can these actually be visible to userspace? I'd rather avoid having > them in QEMU if possible. > > Otherwise, the patches are completely sensible. > And I did it exactly for the opposite purpose: rather than painstakingly figuring out which codes _might_ be returned (and be utterly surprised if we missed some) add an interpretation for every _possible_ code, avoiding nasty surprises. Cheers, Hannes
On 16/11/20 20:03, Hannes Reinecke wrote: >> >>> + case SCSI_HOST_TARGET_FAILURE: >>> + *sense = SENSE_CODE(TARGET_FAILURE); >>> + return CHECK_CONDITION; >>> + case SCSI_HOST_RESERVATION_ERROR: >>> + return RESERVATION_CONFLICT; >>> + case SCSI_HOST_ALLOCATION_FAILURE: >>> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED); >>> + return CHECK_CONDITION; >>> + case SCSI_HOST_MEDIUM_ERROR: >>> + *sense = SENSE_CODE(READ_ERROR); >>> + return CHECK_CONDITION; >> >> Can these actually be visible to userspace? I'd rather avoid having >> them in QEMU if possible. >> >> Otherwise, the patches are completely sensible. >> > And I did it exactly for the opposite purpose: rather than painstakingly > figuring out which codes _might_ be returned (and be utterly surprised > if we missed some) add an interpretation for every _possible_ code, > avoiding nasty surprises. And that certainly makes sense too. On the other hand it'd be nice if Linux was clearer about which the SCSI_HOST values are part of the userspace API and which are just an (ugly) implementation detail. Paolo
On 11/16/20 9:05 PM, Paolo Bonzini wrote: > On 16/11/20 20:03, Hannes Reinecke wrote: >>> >>>> + case SCSI_HOST_TARGET_FAILURE: >>>> + *sense = SENSE_CODE(TARGET_FAILURE); >>>> + return CHECK_CONDITION; >>>> + case SCSI_HOST_RESERVATION_ERROR: >>>> + return RESERVATION_CONFLICT; >>>> + case SCSI_HOST_ALLOCATION_FAILURE: >>>> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED); >>>> + return CHECK_CONDITION; >>>> + case SCSI_HOST_MEDIUM_ERROR: >>>> + *sense = SENSE_CODE(READ_ERROR); >>>> + return CHECK_CONDITION; >>> >>> Can these actually be visible to userspace? I'd rather avoid having >>> them in QEMU if possible. >>> >>> Otherwise, the patches are completely sensible. >>> >> And I did it exactly for the opposite purpose: rather than >> painstakingly figuring out which codes _might_ be returned (and be >> utterly surprised if we missed some) add an interpretation for every >> _possible_ code, avoiding nasty surprises. > > And that certainly makes sense too. > > On the other hand it'd be nice if Linux was clearer about which the > SCSI_HOST values are part of the userspace API and which are just an > (ugly) implementation detail. > Oh, I certainly agree with that. But that is more of a long-term prospect; I do see some discussions ahead if one were to try it. Especially as (like DID_BAD_TARGET and DID_NO_CONNECT) have no clear distinction between them, and are used more-or-less interchangeably. But a clear definition of these values would inevitably lead to a change in various drivers, which then would lead to a change in behaviour, and a possible user-space regression. So not that easy, sadly. Cheers, Hannes
diff --git a/scsi/utils.c b/scsi/utils.c index 262ef1c3ea..ae68881184 100644 --- a/scsi/utils.c +++ b/scsi/utils.c @@ -252,6 +252,21 @@ const struct SCSISense sense_code_LUN_COMM_FAILURE = { .key = ABORTED_COMMAND, .asc = 0x08, .ascq = 0x00 }; +/* Command aborted, LUN does not respond to selection */ +const struct SCSISense sense_code_LUN_NOT_RESPONDING = { + .key = ABORTED_COMMAND, .asc = 0x05, .ascq = 0x00 +}; + +/* Command aborted, Command Timeout during processing */ +const struct SCSISense sense_code_COMMAND_TIMEOUT = { + .key = ABORTED_COMMAND, .asc = 0x2e, .ascq = 0x02 +}; + +/* Command aborted, Commands cleared by device server */ +const struct SCSISense sense_code_COMMAND_ABORTED = { + .key = ABORTED_COMMAND, .asc = 0x2f, .ascq = 0x02 +}; + /* Medium Error, Unrecovered read error */ const struct SCSISense sense_code_READ_ERROR = { .key = MEDIUM_ERROR, .asc = 0x11, .ascq = 0x00 @@ -568,6 +583,14 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr, switch (errno_value) { case EDOM: return TASK_SET_FULL; + case EBADE: + return RESERVATION_CONFLICT; + case ENODATA: + *sense = SENSE_CODE(READ_ERROR); + return CHECK_CONDITION; + case EREMOTEIO: + *sense = SENSE_CODE(LUN_COMM_FAILURE); + return CHECK_CONDITION; case ENOMEM: *sense = SENSE_CODE(TARGET_FAILURE); return CHECK_CONDITION; @@ -576,14 +599,41 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr, return CHECK_CONDITION; } } else { - if (io_hdr->host_status == SCSI_HOST_NO_LUN || - io_hdr->host_status == SCSI_HOST_BUSY || - io_hdr->host_status == SCSI_HOST_TIME_OUT || - (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) { + switch (io_hdr->host_status) { + case SCSI_HOST_NO_LUN: + *sense = SENSE_CODE(LUN_NOT_RESPONDING); + return CHECK_CONDITION; + case SCSI_HOST_BUSY: return BUSY; - } else if (io_hdr->host_status) { + case SCSI_HOST_TIME_OUT: + *sense = SENSE_CODE(COMMAND_TIMEOUT); + return CHECK_CONDITION; + case SCSI_HOST_BAD_RESPONSE: + *sense = SENSE_CODE(LUN_COMM_FAILURE); + return CHECK_CONDITION; + case SCSI_HOST_ABORTED: + *sense = SENSE_CODE(COMMAND_ABORTED); + return CHECK_CONDITION; + case SCSI_HOST_RESET: + *sense = SENSE_CODE(RESET); + return CHECK_CONDITION; + case SCSI_HOST_TRANSPORT_DISRUPTED: *sense = SENSE_CODE(I_T_NEXUS_LOSS); return CHECK_CONDITION; + case SCSI_HOST_TARGET_FAILURE: + *sense = SENSE_CODE(TARGET_FAILURE); + return CHECK_CONDITION; + case SCSI_HOST_RESERVATION_ERROR: + return RESERVATION_CONFLICT; + case SCSI_HOST_ALLOCATION_FAILURE: + *sense = SENSE_CODE(SPACE_ALLOC_FAILED); + return CHECK_CONDITION; + case SCSI_HOST_MEDIUM_ERROR: + *sense = SENSE_CODE(READ_ERROR); + return CHECK_CONDITION; + } + if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) { + return BUSY; } else if (io_hdr->status) { return io_hdr->status; } else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
As we don't have a driver-specific mapping (yet) we should provide for a detailed mapping from host_status to SCSI sense codes. Signed-off-by: Hannes Reinecke <hare@suse.de> --- scsi/utils.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 5 deletions(-)