diff mbox

[04/17] scsi_dh_alua: Improve error handling

Message ID 1430743343-47174-5-git-send-email-hare@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Hannes Reinecke May 4, 2015, 12:42 p.m. UTC 
Improve error handling and use standard logging functions
instead of hand-crafted ones.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 76 +++++++++++++++++-------------
 1 file changed, 42 insertions(+), 34 deletions(-)

Comments

Bart Van Assche May 7, 2015, 11:48 a.m. UTC | #1 
On 05/04/15 14:42, Hannes Reinecke wrote:
> @@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
>   	rq->sense_len = h->senselen = 0;
>
>   	err = blk_execute_rq(rq->q, NULL, rq, 1);
> -	if (err == -EIO) {
> -		sdev_printk(KERN_INFO, sdev,
> -			    "%s: rtpg failed with %x\n",
> -			    ALUA_DH_NAME, rq->errors);
> +	if (err < 0) {
> +		if (!rq->errors)
> +			err = DID_ERROR << 16;
> +		else
> +			err = rq->errors;
>   		h->senselen = rq->sense_len;
> -		err = SCSI_DH_IO;
>   	}
>   	blk_put_request(rq);
>   done:

Running the grep query "->errors = " over the Linux kernel source tree 
shows that sometimes a SCSI error code is written into that field and 
sometimes a negative error code. Does this mean that the test 
!rq->errors should be modified into !rq->errors && 
!IS_ERR_VALUE(rq->errors) ?

Bart.
--
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
Hannes Reinecke May 7, 2015, 11:52 a.m. UTC | #2 
On 05/07/2015 01:48 PM, Bart Van Assche wrote:
> On 05/04/15 14:42, Hannes Reinecke wrote:
>> @@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct
>> scsi_device *sdev, struct alua_dh_data *h,
>>       rq->sense_len = h->senselen = 0;
>>
>>       err = blk_execute_rq(rq->q, NULL, rq, 1);
>> -    if (err == -EIO) {
>> -        sdev_printk(KERN_INFO, sdev,
>> -                "%s: rtpg failed with %x\n",
>> -                ALUA_DH_NAME, rq->errors);
>> +    if (err < 0) {
>> +        if (!rq->errors)
>> +            err = DID_ERROR << 16;
>> +        else
>> +            err = rq->errors;
>>           h->senselen = rq->sense_len;
>> -        err = SCSI_DH_IO;
>>       }
>>       blk_put_request(rq);
>>   done:
> 
> Running the grep query "->errors = " over the Linux kernel source
> tree shows that sometimes a SCSI error code is written into that
> field and sometimes a negative error code. Does this mean that the
> test !rq->errors should be modified into !rq->errors &&
> !IS_ERR_VALUE(rq->errors) ?
> 
Hmm. I'm going to review this. 'rq->errors' should be used consistently.

Hannes
Hannes Reinecke May 11, 2015, 1:19 p.m. UTC | #3 
On 05/07/2015 01:52 PM, Hannes Reinecke wrote:
> On 05/07/2015 01:48 PM, Bart Van Assche wrote:
>> On 05/04/15 14:42, Hannes Reinecke wrote:
>>> @@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct
>>> scsi_device *sdev, struct alua_dh_data *h,
>>>       rq->sense_len = h->senselen = 0;
>>>
>>>       err = blk_execute_rq(rq->q, NULL, rq, 1);
>>> -    if (err == -EIO) {
>>> -        sdev_printk(KERN_INFO, sdev,
>>> -                "%s: rtpg failed with %x\n",
>>> -                ALUA_DH_NAME, rq->errors);
>>> +    if (err < 0) {
>>> +        if (!rq->errors)
>>> +            err = DID_ERROR << 16;
>>> +        else
>>> +            err = rq->errors;
>>>           h->senselen = rq->sense_len;
>>> -        err = SCSI_DH_IO;
>>>       }
>>>       blk_put_request(rq);
>>>   done:
>>
>> Running the grep query "->errors = " over the Linux kernel source
>> tree shows that sometimes a SCSI error code is written into that
>> field and sometimes a negative error code. Does this mean that the
>> test !rq->errors should be modified into !rq->errors &&
>> !IS_ERR_VALUE(rq->errors) ?
>>
> Hmm. I'm going to review this. 'rq->errors' should be used consistently.
> 
drivers/scsi/scsi_lib.c:scsi_execute() has this comment in the
header:

 * returns the req->errors value which is the scsi_cmnd result
 * field.

And later on 'req->errors' is used verbatim as the return value:

	if (resid)
		*resid = req->resid_len;
	ret = req->errors;
 out:
	blk_put_request(req);

	return ret;
}

As the above patch is modeled according to this, any issue here
would affect scsi_execute, too.

Cheers,

Hannes
diff mbox

Patch

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index b5903eb..f5d5859 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -23,6 +23,7 @@ 
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>
 
@@ -138,11 +139,13 @@  static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 			    bool rtpg_ext_hdr_req)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
 	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	if (!rq) {
+		err = DRIVER_BUSY << 24;
 		goto done;
+	}
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
@@ -161,12 +164,12 @@  static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 	rq->sense_len = h->senselen = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
+	if (err < 0) {
+		if (!rq->errors)
+			err = DID_ERROR << 16;
+		else
+			err = rq->errors;
 		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
 	}
 	blk_put_request(rq);
 done:
@@ -174,13 +177,11 @@  done:
 }
 
 /*
- * alua_stpg - Evaluate SET TARGET GROUP STATES
+ * stpg_endio - Evaluate SET TARGET GROUP STATES
  * @sdev: the device to be evaluated
  * @state: the new target group state
  *
- * Send a SET TARGET GROUP STATES command to the device.
- * We only have to test here if we should resubmit the command;
- * any other error is assumed as a failure.
+ * Evaluate a SET TARGET GROUP STATES command response.
  */
 static void stpg_endio(struct request *req, int error)
 {
@@ -195,9 +196,8 @@  static void stpg_endio(struct request *req, int error)
 	}
 
 	if (req->sense_len > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err) {
+		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
 			err = SCSI_DH_IO;
 			goto done;
 		}
@@ -206,10 +206,9 @@  static void stpg_endio(struct request *req, int error)
 			err = SCSI_DH_RETRY;
 			goto done;
 		}
-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: stpg sense code: %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(h->sdev, ALUA_DH_NAME, &sense_hdr);
 		err = SCSI_DH_IO;
 	} else if (error)
 		err = SCSI_DH_IO;
@@ -499,7 +498,7 @@  static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
-	unsigned err;
+	unsigned err, retval;
 	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
@@ -511,13 +510,21 @@  static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
-
-	if (err == SCSI_DH_IO && h->senselen > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err)
-			return SCSI_DH_IO;
+	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+
+	if (retval) {
+		if (h->senselen == 0 ||
+		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: rtpg failed, result %d\n",
+				    ALUA_DH_NAME, retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				err = SCSI_DH_DEV_TEMP_BUSY;
+			else
+				err = SCSI_DH_IO;
+			return err;
+		}
 
 		/*
 		 * submit_rtpg() has failed on existing arrays
@@ -535,16 +542,17 @@  static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
+				    ALUA_DH_NAME);
+			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 			goto retry;
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg sense code %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
-		err = SCSI_DH_IO;
+		}
+		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		return SCSI_DH_IO;
 	}
-	if (err != SCSI_DH_OK)
-		return err;
 
 	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
 		(h->buff[2] << 8) + h->buff[3] + 4;