Re: [PATCH 11/23] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke <hare@suse.de>]

On 09/22/2015 08:50 PM, Ewan Milne wrote:
> On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote:
>> We should be issuing STPG synchronously as we need to
>> evaluate the return code on failure.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/device_handler/scsi_dh_alua.c | 179 +++++++++++++----------------
>>  1 file changed, 83 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index 9e2b3af..fd0385e 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -172,76 +172,28 @@ done:
>>  }
>>  
>>  /*
>> - * stpg_endio - Evaluate SET TARGET GROUP STATES
>> - * @sdev: the device to be evaluated
>> - * @state: the new target group state
>> - *
>> - * Evaluate a SET TARGET GROUP STATES command response.
>> - */
>> -static void stpg_endio(struct request *req, int error)
>> -{
>> -	struct alua_dh_data *h = req->end_io_data;
>> -	struct scsi_sense_hdr sense_hdr;
>> -	unsigned err = SCSI_DH_OK;
>> -
>> -	if (host_byte(req->errors) != DID_OK ||
>> -	    msg_byte(req->errors) != COMMAND_COMPLETE) {
>> -		err = SCSI_DH_IO;
>> -		goto done;
>> -	}
>> -
>> -	if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
>> -				 &sense_hdr)) {
>> -		err = alua_check_sense(h->sdev, &sense_hdr);
>> -		if (err == ADD_TO_MLQUEUE) {
>> -			err = SCSI_DH_RETRY;
>> -			goto done;
>> -		}
>> -		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;
>> -
>> -	if (err == SCSI_DH_OK) {
>> -		h->state = TPGS_STATE_OPTIMIZED;
>> -		sdev_printk(KERN_INFO, h->sdev,
>> -			    "%s: port group %02x switched to state %c\n",
>> -			    ALUA_DH_NAME, h->group_id,
>> -			    print_alua_state(h->state));
>> -	}
>> -done:
>> -	req->end_io_data = NULL;
>> -	__blk_put_request(req->q, req);
>> -	if (h->callback_fn) {
>> -		h->callback_fn(h->callback_data, err);
>> -		h->callback_fn = h->callback_data = NULL;
>> -	}
>> -	return;
>> -}
>> -
>> -/*
>>   * submit_stpg - Issue a SET TARGET GROUP STATES command
>>   *
>>   * Currently we're only setting the current target port group state
>>   * to 'active/optimized' and let the array firmware figure out
>>   * the states of the remaining groups.
>>   */
>> -static unsigned submit_stpg(struct alua_dh_data *h)
>> +static unsigned submit_stpg(struct scsi_device *sdev, int group_id,
>> +			    unsigned char *sense)
>>  {
>>  	struct request *rq;
>> +	unsigned char stpg_data[8];
>>  	int stpg_len = 8;
>> -	struct scsi_device *sdev = h->sdev;
>> +	int err = 0;
>>  
>>  	/* Prepare the data buffer */
>> -	memset(h->buff, 0, stpg_len);
>> -	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
>> -	put_unaligned_be16(h->group_id, &h->buff[6]);
>> +	memset(stpg_data, 0, stpg_len);
>> +	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
>> +	put_unaligned_be16(group_id, &stpg_data[6]);
>>  
>> -	rq = get_alua_req(sdev, h->buff, stpg_len, WRITE);
>> +	rq = get_alua_req(sdev, stpg_data, stpg_len, WRITE);
>>  	if (!rq)
>> -		return SCSI_DH_RES_TEMP_UNAVAIL;
>> +		return DRIVER_BUSY << 24;
>>  
>>  	/* Prepare the command. */
>>  	rq->cmd[0] = MAINTENANCE_OUT;
>> @@ -249,13 +201,17 @@ static unsigned submit_stpg(struct alua_dh_data *h)
>>  	put_unaligned_be32(stpg_len, &rq->cmd[6]);
>>  	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
>>  
>> -	rq->sense = h->sense;
>> +	rq->sense = sense;
>>  	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
>> -	rq->sense_len = h->senselen = 0;
>> -	rq->end_io_data = h;
>> +	rq->sense_len = 0;
>> +
>> +	blk_execute_rq(rq->q, NULL, rq, 1);
>> +	if (rq->errors)
>> +		err = rq->errors;
>> +
>> +	blk_put_request(rq);
>>  
>> -	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);
>> -	return SCSI_DH_OK;
>> +	return err;
>>  }
>>  
>>  /*
>> @@ -619,6 +575,68 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
>>  }
>>  
>>  /*
>> + * alua_stpg - Issue a SET TARGET GROUP STATES command
>> + *
>> + * Issue a SET TARGET GROUP STATES command and evaluate the
>> + * response. Returns SCSI_DH_RETRY per default to trigger
>> + * a re-evaluation of the target group state.
>> + */
>> +static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
>> +{
>> +	int retval;
>> +	struct scsi_sense_hdr sense_hdr;
>> +
>> +	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
>> +		/* Only implicit ALUA supported, retry */
>> +		return SCSI_DH_RETRY;
>> +	}
>> +	switch (h->state) {
>> +	case TPGS_STATE_OPTIMIZED:
>> +		return SCSI_DH_OK;
>> +	case TPGS_STATE_NONOPTIMIZED:
>> +		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
>> +		    !h->pref &&
>> +		    (h->tpgs & TPGS_MODE_IMPLICIT))
>> +			return SCSI_DH_OK;
>> +		break;
>> +	case TPGS_STATE_STANDBY:
>> +	case TPGS_STATE_UNAVAILABLE:
>> +		break;
>> +	case TPGS_STATE_OFFLINE:
>> +		return SCSI_DH_IO;
>> +		break;
>> +	case TPGS_STATE_TRANSITIONING:
>> +		break;
>> +	default:
>> +		sdev_printk(KERN_INFO, sdev,
>> +			    "%s: stpg failed, unhandled TPGS state %d",
>> +			    ALUA_DH_NAME, h->state);
>> +		return SCSI_DH_NOSYS;
>> +		break;
>> +	}
>> +	/* Set state to transitioning */
>> +	h->state = TPGS_STATE_TRANSITIONING;
>> +	retval = submit_stpg(sdev, h->group_id, h->sense);
>> +
>> +	if (retval) {
>> +		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
>> +					  &sense_hdr)) {
>> +			sdev_printk(KERN_INFO, sdev,
>> +				    "%s: stpg failed, result %d",
>> +				    ALUA_DH_NAME, retval);
>> +			if (driver_byte(retval) == DRIVER_BUSY)
>> +				return SCSI_DH_DEV_TEMP_BUSY;
>> +		} else {
>> +			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
>> +				    ALUA_DH_NAME);
>> +			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
>> +		}
>> +	}
>> +	/* Retry RTPG */
>> +	return SCSI_DH_RETRY;
>> +}
>> +
>> +/*
>>   * alua_initialize - Initialize ALUA state
>>   * @sdev: the device to be initialized
>>   *
>> @@ -695,7 +713,6 @@ static int alua_activate(struct scsi_device *sdev,
>>  {
>>  	struct alua_dh_data *h = sdev->handler_data;
>>  	int err = SCSI_DH_OK;
>> -	int stpg = 0;
>>  
>>  	err = alua_rtpg(sdev, h, 1);
>>  	if (err != SCSI_DH_OK)
>> @@ -704,39 +721,9 @@ static int alua_activate(struct scsi_device *sdev,
>>  	if (optimize_stpg)
>>  		h->flags |= ALUA_OPTIMIZE_STPG;
>>  
>> -	if (h->tpgs & TPGS_MODE_EXPLICIT) {
>> -		switch (h->state) {
>> -		case TPGS_STATE_NONOPTIMIZED:
>> -			stpg = 1;
>> -			if ((h->flags & ALUA_OPTIMIZE_STPG) &&
>> -			    (!h->pref) &&
>> -			    (h->tpgs & TPGS_MODE_IMPLICIT))
>> -				stpg = 0;
>> -			break;
>> -		case TPGS_STATE_STANDBY:
>> -		case TPGS_STATE_UNAVAILABLE:
>> -			stpg = 1;
>> -			break;
>> -		case TPGS_STATE_OFFLINE:
>> -			err = SCSI_DH_IO;
>> -			break;
>> -		case TPGS_STATE_TRANSITIONING:
>> -			err = SCSI_DH_RETRY;
>> -			break;
>> -		default:
>> -			break;
>> -		}
>> -	}
>> -
>> -	if (stpg) {
>> -		h->callback_fn = fn;
>> -		h->callback_data = data;
>> -		err = submit_stpg(h);
>> -		if (err == SCSI_DH_OK)
>> -			return 0;
>> -		h->callback_fn = h->callback_data = NULL;
>> -	}
>> -
>> +	err = alua_stpg(sdev, h);
>> +	if (err == SCSI_DH_RETRY)
>> +		err = alua_rtpg(sdev, h, 1);
>>  out:
>>  	if (fn)
>>  		fn(data, err);
> 
> submit_stpg() now returns DRIVER_BUSY << 24 instead of SCSI_DH_RES_TEMP_UNAVAIL,
> so you are changing the return code semantics here as in patch 5/23.  That's
> fine, but probably should be mentioned in the patch description.
> 
> The removed stpg_endio() code *did* evaluate the SCSI error code, and the code
> that has replaced it doesn't exactly do the same thing, so what difference in
> behavior are we going to get here?
> 
Correct. Is fixed up with the next version of the patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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