diff mbox series

scsi_dh_alua: Do not run STPG for implicit ALUA

Message ID 20191018135537.69462-1-hare@suse.de (mailing list archive)
State Changes Requested
Headers show
Series scsi_dh_alua: Do not run STPG for implicit ALUA | expand

Commit Message

Hannes Reinecke Oct. 18, 2019, 1:55 p.m. UTC 
If a target only supports implicit ALUA sending a SET TARGET PORT GROUPS
command is not only pointless, but might actually cause issues.
So don't.

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

Comments

Martin K. Petersen Oct. 18, 2019, 9:44 p.m. UTC | #1 
Hannes,

> If a target only supports implicit ALUA sending a SET TARGET PORT
> GROUPS command is not only pointless, but might actually cause issues.

We already have a conditional in alua_stpg():

        if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
                /* Only implicit ALUA supported, retry */
                return SCSI_DH_RETRY;
        }

> @@ -832,6 +832,10 @@ static void alua_rtpg_work(struct work_struct *work)
>  		if (err != SCSI_DH_OK)
>  			pg->flags &= ~ALUA_PG_RUN_STPG;
>  	}
> +	/* Do not run STPG if only implicit ALUA is supported */
> +	if (scsi_device_tpgs(sdev) == TPGS_MODE_IMPLICIT)
> +		pg->flags &= ~ALUA_PG_RUN_STPG;
> +
>  	if (pg->flags & ALUA_PG_RUN_STPG) {
>  		pg->flags &= ~ALUA_PG_RUN_STPG;
>  		spin_unlock_irqrestore(&pg->lock, flags);

Instead of checking for EXPLICIT one place and checking for !IMPLICIT
another, can we consolidate the two and maybe do:

  	if (pg->flags & ALUA_PG_RUN_STPG &&
            scsi_device_tpgs(sdev) == TPGS_MODE_EXPLICIT) {
        	[...]

and then remove the redundant check in alua_stpg()?
Hannes Reinecke Oct. 19, 2019, 3:54 p.m. UTC | #2 
On 10/18/19 11:44 PM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> If a target only supports implicit ALUA sending a SET TARGET PORT
>> GROUPS command is not only pointless, but might actually cause issues.
> 
> We already have a conditional in alua_stpg():
> 
>          if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
>                  /* Only implicit ALUA supported, retry */
>                  return SCSI_DH_RETRY;
>          }
> 
>> @@ -832,6 +832,10 @@ static void alua_rtpg_work(struct work_struct *work)
>>   		if (err != SCSI_DH_OK)
>>   			pg->flags &= ~ALUA_PG_RUN_STPG;
>>   	}
>> +	/* Do not run STPG if only implicit ALUA is supported */
>> +	if (scsi_device_tpgs(sdev) == TPGS_MODE_IMPLICIT)
>> +		pg->flags &= ~ALUA_PG_RUN_STPG;
>> +
>>   	if (pg->flags & ALUA_PG_RUN_STPG) {
>>   		pg->flags &= ~ALUA_PG_RUN_STPG;
>>   		spin_unlock_irqrestore(&pg->lock, flags);
> 
> Instead of checking for EXPLICIT one place and checking for !IMPLICIT
> another, can we consolidate the two and maybe do:
> 
>    	if (pg->flags & ALUA_PG_RUN_STPG &&
>              scsi_device_tpgs(sdev) == TPGS_MODE_EXPLICIT) {
>          	[...]
> 
> and then remove the redundant check in alua_stpg()?
> 
Good point.
Will be resending the patch.

Cheers,

Hannes
diff mbox series

Patch

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 4971104b1817..0053277721d0 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -832,6 +832,10 @@  static void alua_rtpg_work(struct work_struct *work)
 		if (err != SCSI_DH_OK)
 			pg->flags &= ~ALUA_PG_RUN_STPG;
 	}
+	/* Do not run STPG if only implicit ALUA is supported */
+	if (scsi_device_tpgs(sdev) == TPGS_MODE_IMPLICIT)
+		pg->flags &= ~ALUA_PG_RUN_STPG;
+
 	if (pg->flags & ALUA_PG_RUN_STPG) {
 		pg->flags &= ~ALUA_PG_RUN_STPG;
 		spin_unlock_irqrestore(&pg->lock, flags);