Re: [PATCH] scsi: core: Micro-optimize the hot path

[John Garry <john.garry@linux.dev>]

On 9/25/26 19:09, Bart Van Assche wrote:

nit: I think that the subject can be more specific, like "don't 
unnecessarily init scmd->eh_entry"

> Every code path that inserts scmd->eh_entry into a list uses
> list_add_tail() (see also scsi_abort_command() and scsi_eh_scmd_add()).
> list_add_tail() overwrites the next and prev pointers. Hence, the only
> code that depends on the initialization of scmd->eh_entry is the
> BUG_ON(!list_empty(&scmd->eh_entry)) statement in scsi_abort_command().
> Remove this statement because scsi_timeout() already prevents
> scsi_abort_command() from being called twice for the same command by
> setting SCMD_STATE_COMPLETE. Remove the scmd->eh_entry initialization
> because the only statement that relied on this initialization has been
> removed.
> 
> Cc: John Garry <john.garry@linux.dev>
> Cc: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Seems fine, so:
Reviewed-by: John Garry <john.garry@linux.dev>

> ---
>   drivers/scsi/scsi_error.c | 1 -
>   drivers/scsi/scsi_lib.c   | 3 ---
>   2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 2e0b1909dc69..75442d60c30e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -270,7 +270,6 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>   	spin_lock_irqsave(&shost->host_lock, flags);
>   	if (shost->eh_deadline != -1 && !shost->last_reset)
>   		shost->last_reset = jiffies;
> -	BUG_ON(!list_empty(&scmd->eh_entry));
>   	list_add_tail(&scmd->eh_entry, &shost->eh_abort_list);
>   	spin_unlock_irqrestore(&shost->host_lock, flags);
>   
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index db64470f5229..f695a6605815 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1322,7 +1322,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
>   	}
>   
>   	cmd->device = dev;
> -	INIT_LIST_HEAD(&cmd->eh_entry);
>   	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
>   }
>   
> @@ -1590,8 +1589,6 @@ static void scsi_complete(struct request *rq)
>   		return;
>   	}
>   
> -	INIT_LIST_HEAD(&cmd->eh_entry);
> -
>   	atomic_inc(&cmd->device->iodone_cnt);
>   	if (cmd->result)
>   		atomic_inc(&cmd->device->ioerr_cnt);