[PATCH] scsi: core: Remove ACTION_RETRY since it is redundant

[PATCH] scsi: core: Remove ACTION_RETRY since it is redundant

[Brian Bunker]

The case of ACTION_RETRY in scsi_io_completion_action does nothing
different than ACTION_DELAYED_RETRY, and by name gives the idea
that it does.

Following ACTION_RETRY:
__scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, false);

Following ACTION_DELAYED_RETRY:
__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, false);

Then __scsi_queue_insert calls scsi_set_blocked(cmd, reason) where
both of the reasons set by ACTION_RETRY and ACTION_DELAYED_RETRY
fall into the same case.

    case SCSI_MLQUEUE_DEVICE_BUSY:
    case SCSI_MLQUEUE_EH_RETRY:
        atomic_set(&device->device_blocked,
               device->max_device_blocked);
        break;

Acked-by: Krishna Kant <krishna.kant@purestorage.com>
Acked-by: Seamus Connor <sconnor@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_lib.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ef08029a0079..d85d72bc01f2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -687,7 +687,7 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
 	struct request *req = scsi_cmd_to_rq(cmd);
 	int level = 0;
 	enum {ACTION_FAIL, ACTION_REPREP, ACTION_DELAYED_REPREP,
-	      ACTION_RETRY, ACTION_DELAYED_RETRY} action;
+	      ACTION_DELAYED_RETRY} action;
 	struct scsi_sense_hdr sshdr;
 	bool sense_valid;
 	bool sense_current = true;      /* false implies "deferred sense" */
@@ -704,7 +704,7 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
 		 * reasons.  Just retry the command and see what
 		 * happens.
 		 */
-		action = ACTION_RETRY;
+		action = ACTION_DELAYED_RETRY;
 	} else if (sense_valid && sense_current) {
 		switch (sshdr.sense_key) {
 		case UNIT_ATTENTION:
@@ -720,7 +720,7 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
 				 * media change, so we just retry the
 				 * command and see what happens.
 				 */
-				action = ACTION_RETRY;
+				action = ACTION_DELAYED_RETRY;
 			}
 			break;
 		case ILLEGAL_REQUEST:
@@ -841,10 +841,6 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
 	case ACTION_DELAYED_REPREP:
 		scsi_mq_requeue_cmd(cmd, ALUA_TRANSITION_REPREP_DELAY);
 		break;
-	case ACTION_RETRY:
-		/* Retry the same command immediately */
-		__scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, false);
-		break;
 	case ACTION_DELAYED_RETRY:
 		/* Retry the same command after a delay */
 		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, false);
-- 
2.32.1 (Apple Git-133)

Re: [PATCH] scsi: core: Remove ACTION_RETRY since it is redundant

[Martin Wilck]

On Thu, 2022-09-01 at 14:47 -0700, Brian Bunker wrote:
> The case of ACTION_RETRY in scsi_io_completion_action does nothing
> different than ACTION_DELAYED_RETRY, and by name gives the idea
> that it does.
> 
> Following ACTION_RETRY:
> __scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, false);
> 
> Following ACTION_DELAYED_RETRY:
> __scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, false);
> 
> Then __scsi_queue_insert calls scsi_set_blocked(cmd, reason) where
> both of the reasons set by ACTION_RETRY and ACTION_DELAYED_RETRY
> fall into the same case.
> 
>     case SCSI_MLQUEUE_DEVICE_BUSY:
>     case SCSI_MLQUEUE_EH_RETRY:
>         atomic_set(&device->device_blocked,
>                device->max_device_blocked);
>         break;
> 
> Acked-by: Krishna Kant <krishna.kant@purestorage.com>
> Acked-by: Seamus Connor <sconnor@purestorage.com>
> Signed-off-by: Brian Bunker <brian@purestorage.com>

I like this, but I would suggest to keep ACTION_RETRY and ditch
ACTION_DELAYED_RETRY instead. The retry isn't delayed in the usual
sense of the word (which would imply some sort of time interval). We
just wait for the device_blocked counter to go to zero.

Thanks,
Martin

> ---
>  drivers/scsi/scsi_lib.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ef08029a0079..d85d72bc01f2 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -687,7 +687,7 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
>         struct request *req = scsi_cmd_to_rq(cmd);
>         int level = 0;
>         enum {ACTION_FAIL, ACTION_REPREP, ACTION_DELAYED_REPREP,
> -             ACTION_RETRY, ACTION_DELAYED_RETRY} action;
> +             ACTION_DELAYED_RETRY} action;
>         struct scsi_sense_hdr sshdr;
>         bool sense_valid;
>         bool sense_current = true;      /* false implies "deferred
> sense" */
> @@ -704,7 +704,7 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
>                  * reasons.  Just retry the command and see what
>                  * happens.
>                  */
> -               action = ACTION_RETRY;
> +               action = ACTION_DELAYED_RETRY;
>         } else if (sense_valid && sense_current) {
>                 switch (sshdr.sense_key) {
>                 case UNIT_ATTENTION:
> @@ -720,7 +720,7 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
>                                  * media change, so we just retry the
>                                  * command and see what happens.
>                                  */
> -                               action = ACTION_RETRY;
> +                               action = ACTION_DELAYED_RETRY;
>                         }
>                         break;
>                 case ILLEGAL_REQUEST:
> @@ -841,10 +841,6 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
>         case ACTION_DELAYED_REPREP:
>                 scsi_mq_requeue_cmd(cmd,
> ALUA_TRANSITION_REPREP_DELAY);
>                 break;
> -       case ACTION_RETRY:
> -               /* Retry the same command immediately */
> -               __scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY,
> false);
> -               break;
>         case ACTION_DELAYED_RETRY:
>                 /* Retry the same command after a delay */
>                 __scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY,
> false);