| Message ID | 1430743343-47174-9-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/04/15 14:42, Hannes Reinecke wrote: > +static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h) > +{ > + int retval, err = SCSI_DH_RETRY; > + unsigned char sense[SCSI_SENSE_BUFFERSIZE]; > + 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, pg->state); > + return SCSI_DH_NOSYS; > + break; > + } > + /* Set state to transitioning */ > + h->state = TPGS_STATE_TRANSITIONING; > + retval = submit_stpg(sdev, h->group_id, sense); > + > + if (retval) { > + if (!(driver_byte(retval) & DRIVER_SENSE) || > + !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, > + &sense_hdr)) { > + sdev_printk(KERN_INFO, sdev, > + "%s: stpg failed, result %d", > + ALUA_DH_NAME, retval); > + /* Retry RTPG */ > + return err; > + } > + err = alua_check_sense(h->sdev, &sense_hdr); > + sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n", > + ALUA_DH_NAME); > + scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr); > + err = SCSI_DH_RETRY; > + } > + return err; > +} The value returned by alua_check_sense() is assigned to the variable 'err' but that value is not used. Otherwise in this function the variable 'err' always has the value SCSI_DH_RETRY. Does that mean that that variable can be removed ? Additionally, why is 'retval' only printed if normalizing the sense data succeeds and not if normalizing the sense data fails ? Has it been considered to print the ASC and ASCQ values upon STPG failure ? Having these values available can be a big help during debugging. Thanks, 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
On 05/07/2015 02:18 PM, Bart Van Assche wrote: > On 05/04/15 14:42, Hannes Reinecke wrote: >> +static unsigned alua_stpg(struct scsi_device *sdev, struct >> alua_dh_data *h) >> +{ >> + int retval, err = SCSI_DH_RETRY; >> + unsigned char sense[SCSI_SENSE_BUFFERSIZE]; >> + 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, pg->state); >> + return SCSI_DH_NOSYS; >> + break; >> + } >> + /* Set state to transitioning */ >> + h->state = TPGS_STATE_TRANSITIONING; >> + retval = submit_stpg(sdev, h->group_id, sense); >> + >> + if (retval) { >> + if (!(driver_byte(retval) & DRIVER_SENSE) || >> + !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, >> + &sense_hdr)) { >> + sdev_printk(KERN_INFO, sdev, >> + "%s: stpg failed, result %d", >> + ALUA_DH_NAME, retval); >> + /* Retry RTPG */ >> + return err; >> + } >> + err = alua_check_sense(h->sdev, &sense_hdr); >> + sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n", >> + ALUA_DH_NAME); >> + scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr); >> + err = SCSI_DH_RETRY; >> + } >> + return err; >> +} > > The value returned by alua_check_sense() is assigned to the variable > 'err' but that value is not used. Otherwise in this function the > variable 'err' always has the value SCSI_DH_RETRY. Does that mean > that that variable can be removed ? Hmm. Yes, we could. > Additionally, why is 'retval' > only printed if normalizing the sense data succeeds and not if > normalizing the sense data fails ? Has it been considered to print > the ASC and ASCQ values upon STPG failure ? Having these values > available can be a big help during debugging. > We do print the ASC / ASCQ values; that's what scsi_print_sense_hdr does. And the above line works on the assumption that iff DRIVER_SENSE is set in the retval we'll have a sense code, and this sense code will give more information than any other (internally generated) error status. So 'retval' isn't printed in that case, only if no sense code is provided or if we fail to decode it. Cheers, Hannes
On Mon, May 04, 2015 at 02:42:14PM +0200, Hannes Reinecke wrote: > We should be issuing STPG synchronously as we need to > evaluate the return code on failure. How does this fit into the calling convention? It seems like generally dm-mpath expects async execution of ->activate. If we don't actually need asynchronous activation we should remove it from all the drivers to have a consistent calling convention. -- 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:55 AM, Christoph Hellwig wrote: > On Mon, May 04, 2015 at 02:42:14PM +0200, Hannes Reinecke wrote: >> We should be issuing STPG synchronously as we need to >> evaluate the return code on failure. > > How does this fit into the calling convention? It seems like generally > dm-mpath expects async execution of ->activate. If we don't actually need > asynchronous activation we should remove it from all the drivers to have > a consistent calling convention. > This is just an intermediate step, before the entire thing moves off to a workqueue and becomes asynchronous again. It's not meant to be as a standalone patch; I've just split it off for easier review. Cheers, Hannes
On Mon, May 11, 2015 at 11:59:16AM +0200, Hannes Reinecke wrote: > This is just an intermediate step, before the entire thing moves off > to a workqueue and becomes asynchronous again. > It's not meant to be as a standalone patch; I've just split it off > for easier review. It actually made the review a bit harder. Also I don't really undertand what the benfit of running a synchronous STPG in a workqueue is over running it asynchronously. -- 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 03:50 PM, Christoph Hellwig wrote: > On Mon, May 11, 2015 at 11:59:16AM +0200, Hannes Reinecke wrote: >> This is just an intermediate step, before the entire thing moves off >> to a workqueue and becomes asynchronous again. >> It's not meant to be as a standalone patch; I've just split it off >> for easier review. > > It actually made the review a bit harder. Also I don't really undertand > what the benfit of running a synchronous STPG in a workqueue is over > running it asynchronously. > Because STPG might fail, at which point we need to re-run RTPG to figure out the current status. But that might clash with a scheduled RTPG from another task. _And_ issuing RTPG while STPG is active is pointless, too, as the information is pretty much guaranteed to be incorrect. Hence the use of a workqueue to serialize RTPG and STPG against each other. Cheers, Hannes
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 1bfc7a4..0b8e111 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -22,6 +22,7 @@ #include <linux/slab.h> #include <linux/delay.h> #include <linux/module.h> +#include <asm/unaligned.h> #include <scsi/scsi.h> #include <scsi/scsi_dbg.h> #include <scsi/scsi_eh.h> @@ -179,98 +180,51 @@ 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 (req->sense_len > 0) { - if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE, - &sense_hdr)) { - err = SCSI_DH_IO; - goto done; - } - 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; /* Prepare the data buffer */ - memset(h->buff, 0, stpg_len); - h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f; - h->buff[6] = (h->group_id >> 8) & 0xff; - h->buff[7] = h->group_id & 0xff; + 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; /* Prepare the command. */ rq->cmd[0] = MAINTENANCE_OUT; rq->cmd[1] = MO_SET_TARGET_PGS; - rq->cmd[6] = (stpg_len >> 24) & 0xff; - rq->cmd[7] = (stpg_len >> 16) & 0xff; - rq->cmd[8] = (stpg_len >> 8) & 0xff; - rq->cmd[9] = stpg_len & 0xff; + 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; + + err = blk_execute_rq(rq->q, NULL, rq, 1); + if (err < 0) { + if (!rq->errors) + err = DID_ERROR << 16; + else + err = rq->errors; + if (rq->sense_len) + err |= (DRIVER_SENSE << 24); + } + blk_put_request(rq); - blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio); - return SCSI_DH_OK; + return err; } /* @@ -641,6 +595,70 @@ 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, err = SCSI_DH_RETRY; + unsigned char sense[SCSI_SENSE_BUFFERSIZE]; + 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, pg->state); + return SCSI_DH_NOSYS; + break; + } + /* Set state to transitioning */ + h->state = TPGS_STATE_TRANSITIONING; + retval = submit_stpg(sdev, h->group_id, sense); + + if (retval) { + if (!(driver_byte(retval) & DRIVER_SENSE) || + !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, + &sense_hdr)) { + sdev_printk(KERN_INFO, sdev, + "%s: stpg failed, result %d", + ALUA_DH_NAME, retval); + /* Retry RTPG */ + return err; + } + err = alua_check_sense(h->sdev, &sense_hdr); + sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n", + ALUA_DH_NAME); + scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr); + err = SCSI_DH_RETRY; + } + return err; +} + +/* * alua_initialize - Initialize ALUA state * @sdev: the device to be initialized * @@ -717,7 +735,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) @@ -726,39 +743,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);
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 | 195 ++++++++++++++--------------- 1 file changed, 91 insertions(+), 104 deletions(-)