| Message ID | 1443523658-87622-15-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 09/29/2015 03:47 AM, Hannes Reinecke wrote: > Seperate out SET TARGET PORT GROUP functionality into a separate ^ Separate ? > function alua_stpg(). > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/device_handler/scsi_dh_alua.c | 86 ++++++++++++++++++------------ > 1 file changed, 52 insertions(+), 34 deletions(-) > > diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c > index 63423b2..59fe3e9 100644 > --- a/drivers/scsi/device_handler/scsi_dh_alua.c > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c > @@ -614,6 +614,56 @@ 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. > + */ In SPC-4 I found the following description next to the STPG command: "SET TARGET PORT GROUPS". How about using the official SPC-4 description ? > +static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h, > + activate_complete fn, void *data) > +{ > + int err = SCSI_DH_OK; > + int stpg = 0; > + > + if (!(h->tpgs & TPGS_MODE_EXPLICIT)) { > + /* Only implicit ALUA supported */ > + return err; > + } > + > + 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) > + h->callback_fn = h->callback_data = NULL; > + } > + return err; > +} > + > +/* > * alua_initialize - Initialize ALUA state > * @sdev: the device to be initialized > * > @@ -690,7 +740,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) > @@ -699,41 +748,10 @@ 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, fn, data); > > out: > - if (fn) > + if (err != SCSI_DH_OK && fn) > fn(data, err); > return 0; > } > The old implementation calls the callback function 'fn' if the flag TPGS_MODE_EXPLICIT has not been set but the new implementation not. Please mention this in the patch description if this behavior change is intentional. 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 10/02/2015 02:07 AM, Bart Van Assche wrote: > On 09/29/2015 03:47 AM, Hannes Reinecke wrote: >> Seperate out SET TARGET PORT GROUP functionality into a separate > ^ Separate ? >> function alua_stpg(). >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/device_handler/scsi_dh_alua.c | 86 >> ++++++++++++++++++------------ >> 1 file changed, 52 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c >> b/drivers/scsi/device_handler/scsi_dh_alua.c >> index 63423b2..59fe3e9 100644 >> --- a/drivers/scsi/device_handler/scsi_dh_alua.c >> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c >> @@ -614,6 +614,56 @@ 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. >> + */ > > In SPC-4 I found the following description next to the STPG command: > "SET TARGET PORT GROUPS". How about using the official SPC-4 > description ? > >> +static unsigned alua_stpg(struct scsi_device *sdev, struct >> alua_dh_data *h, >> + activate_complete fn, void *data) >> +{ >> + int err = SCSI_DH_OK; >> + int stpg = 0; >> + >> + if (!(h->tpgs & TPGS_MODE_EXPLICIT)) { >> + /* Only implicit ALUA supported */ >> + return err; >> + } >> + >> + 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) >> + h->callback_fn = h->callback_data = NULL; >> + } >> + return err; >> +} >> + >> +/* >> * alua_initialize - Initialize ALUA state >> * @sdev: the device to be initialized >> * >> @@ -690,7 +740,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) >> @@ -699,41 +748,10 @@ 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, fn, data); >> >> out: >> - if (fn) >> + if (err != SCSI_DH_OK && fn) >> fn(data, err); >> return 0; >> } >> > > The old implementation calls the callback function 'fn' if the flag > TPGS_MODE_EXPLICIT has not been set but the new implementation not. > Please mention this in the patch description if this behavior change > is intentional. > Actually, this looks more like an error with the old implementation. 'fn' is the callback signalling that the failover command has been completed, and the caller (in most case dm-multipath) relies on 'fn' to be executed for further processing. So 'fn' should be executed, irrespective of the TPGS_MODE_EXPLICIT flag. Cheers, Hannes
On 10/01/2015 11:06 PM, Hannes Reinecke wrote: > On 10/02/2015 02:07 AM, Bart Van Assche wrote: >> The old implementation calls the callback function 'fn' if the flag >> TPGS_MODE_EXPLICIT has not been set but the new implementation not. >> Please mention this in the patch description if this behavior change >> is intentional. >> > Actually, this looks more like an error with the old implementation. > 'fn' is the callback signalling that the failover command has been > completed, and the caller (in most case dm-multipath) relies on 'fn' > to be executed for further processing. > So 'fn' should be executed, irrespective of the TPGS_MODE_EXPLICIT flag. Thank you for the feedback. I think reviewers would be grateful if this change would be documented in the patch description. 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
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 63423b2..59fe3e9 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -614,6 +614,56 @@ 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, + activate_complete fn, void *data) +{ + int err = SCSI_DH_OK; + int stpg = 0; + + if (!(h->tpgs & TPGS_MODE_EXPLICIT)) { + /* Only implicit ALUA supported */ + return err; + } + + 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) + h->callback_fn = h->callback_data = NULL; + } + return err; +} + +/* * alua_initialize - Initialize ALUA state * @sdev: the device to be initialized * @@ -690,7 +740,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) @@ -699,41 +748,10 @@ 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, fn, data); out: - if (fn) + if (err != SCSI_DH_OK && fn) fn(data, err); return 0; }
Seperate out SET TARGET PORT GROUP functionality into a separate function alua_stpg(). Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/device_handler/scsi_dh_alua.c | 86 ++++++++++++++++++------------ 1 file changed, 52 insertions(+), 34 deletions(-)