| Message ID | 1430743343-47174-12-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 04, 2015 at 02:42:17PM +0200, Hannes Reinecke wrote: > Most sense code is already handled in the generic > code, so we shouldn't be adding special cases here. > However, when doing so we need to check for > unit attention whenever we're sending an internal > command. Shouldn't we move handling of all these sense codes to common code? They are part of the generic SPC list of sense codes, so splitting them up into two functions is rather confusing. > @@ -474,6 +440,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w > } > > err = alua_check_sense(sdev, &sense_hdr); > + if (sense_hdr.sense_key == UNIT_ATTENTION) > + err = ADD_TO_MLQUEUE; And this really should be a separate patch. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/11/2015 08:58 AM, Christoph Hellwig wrote: > On Mon, May 04, 2015 at 02:42:17PM +0200, Hannes Reinecke wrote: >> Most sense code is already handled in the generic >> code, so we shouldn't be adding special cases here. >> However, when doing so we need to check for >> unit attention whenever we're sending an internal >> command. > > Shouldn't we move handling of all these sense codes to common code? They > are part of the generic SPC list of sense codes, so splitting them up > into two functions is rather confusing. > Sigh. This is one of the sore topics with the SCSI stack. The default sense code handling is correct only for filesystem I/O; BLOCK_PC callers are expected to handle all errors themselves. Which typically is a pain as one always forgets the one or the other issue. The device handlers have a callout into that generic function to handle and device handler specific sense codes. So with that I do agree that calling alua_check_sense() here is dubious as it should have been run from the generic path already. Will be checking and fixing it up. >> @@ -474,6 +440,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w >> } >> >> err = alua_check_sense(sdev, &sense_hdr); >> + if (sense_hdr.sense_key == UNIT_ATTENTION) >> + err = ADD_TO_MLQUEUE; > > And this really should be a separate patch. > Okay, will be doing so. Cheers, Hannes
On Mon, May 11, 2015 at 04:52:14PM +0200, Hannes Reinecke wrote: > Sigh. This is one of the sore topics with the SCSI stack. > > The default sense code handling is correct only for filesystem > I/O; BLOCK_PC callers are expected to handle all errors themselves. > Which typically is a pain as one always forgets the one or the > other issue. > > The device handlers have a callout into that generic function > to handle and device handler specific sense codes. > > So with that I do agree that calling alua_check_sense() here > is dubious as it should have been run from the generic path already. > > Will be checking and fixing it up. What I meant is that we really shouldn't handle the sense codes in the ALUA handler - they are generic SCS? sense codes and we'd better handle them in the core code, ditto for the other device handlers actually. Now the problem of BLOCK_PC ignoring the sense handling is a different one, but why don't we export scsi_check_sense and allow BLOCK_PC callers like the device handlers reuse the logic instead of duplicating it? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 492b721..98d4a22 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -349,28 +349,6 @@ static int alua_check_sense(struct scsi_device *sdev, * LUN Not Accessible - ALUA state transition */ return ADD_TO_MLQUEUE; - if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0b) - /* - * LUN Not Accessible -- Target port in standby state - */ - return SUCCESS; - if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0c) - /* - * LUN Not Accessible -- Target port in unavailable state - */ - return SUCCESS; - if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x12) - /* - * LUN Not Ready -- Offline - */ - return SUCCESS; - if (sdev->allow_restart && - sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x02) - /* - * if the device is not started, we need to wake - * the error handler to start the motor - */ - return FAILED; break; case UNIT_ATTENTION: if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00) @@ -385,7 +363,7 @@ static int alua_check_sense(struct scsi_device *sdev, return ADD_TO_MLQUEUE; if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x01) /* - * Mode Parameters Changed + * Mode parameter changed */ return ADD_TO_MLQUEUE; if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06) @@ -398,18 +376,6 @@ static int alua_check_sense(struct scsi_device *sdev, * Implicit ALUA state transition failed */ return ADD_TO_MLQUEUE; - if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03) - /* - * Inquiry data has changed - */ - return ADD_TO_MLQUEUE; - if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x0e) - /* - * REPORTED_LUNS_DATA_HAS_CHANGED is reported - * when switching controllers on targets like - * Intel Multi-Flex. We can just retry. - */ - return ADD_TO_MLQUEUE; break; } @@ -474,6 +440,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w } err = alua_check_sense(sdev, &sense_hdr); + if (sense_hdr.sense_key == UNIT_ATTENTION) + err = ADD_TO_MLQUEUE; if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) { sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n", ALUA_DH_NAME);
Most sense code is already handled in the generic code, so we shouldn't be adding special cases here. However, when doing so we need to check for unit attention whenever we're sending an internal command. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/device_handler/scsi_dh_alua.c | 38 +++--------------------------- 1 file changed, 3 insertions(+), 35 deletions(-)