Re: [PATCH 3/8] scsi: don't memset the entire scsi_cmnd in scsi_init_command

[John Garry <john.garry@xxxxxxxxxx>]

On 22/02/2022 14:04, Christoph Hellwig wrote:
> Replace the big fat memset that requires saving and restoring various
> fields with just initializing those fields that need initialization.
>
> All the clearing to 0 ismoved to scsi_prepare_cmd as scsi_ioctl_reset

nit: "is moved"?

> alreadly uses kzalloc to allocate a pre-zeroed command.
>
> This is still conservative and can probably be optimizated further.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

FWIW:

Reviewed-by: John Garry <john.garry@xxxxxxxxxx>

But just a comment below.

> ---
>   drivers/scsi/scsi_lib.c | 61 ++++++++++++++++++++---------------------
>   1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a1c18ba5e8d38..960795d469d8c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1163,45 +1163,17 @@ static void scsi_cleanup_rq(struct request *rq)
>   /* Called before a request is prepared. See also scsi_mq_prep_fn(). */
>   void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
>   {
> -	void *buf = cmd->sense_buffer;
> -	void *prot = cmd->prot_sdb;
>   	struct request *rq = scsi_cmd_to_rq(cmd);
> -	unsigned int flags = cmd->flags & SCMD_PRESERVED_FLAGS;
> -	unsigned long jiffies_at_alloc;
> -	int retries, to_clear;
> -	bool in_flight;
> -	int budget_token = cmd->budget_token;
> -
> -	if (!blk_rq_is_passthrough(rq) && !(flags & SCMD_INITIALIZED)) {
> -		flags |= SCMD_INITIALIZED;
> +
> +	if (!blk_rq_is_passthrough(rq) && !(cmd->flags & SCMD_INITIALIZED)) {
> +		cmd->flags |= SCMD_INITIALIZED;

Maybe I'm being dozy, but isn't this being cleared below *

>   		scsi_initialize_rq(rq);
>   	}
>   
> -	jiffies_at_alloc = cmd->jiffies_at_alloc;
> -	retries = cmd->retries;
> -	in_flight = test_bit(SCMD_STATE_INFLIGHT, &cmd->state);
> -	/*
> -	 * Zero out the cmd, except for the embedded scsi_request. Only clear
> -	 * the driver-private command data if the LLD does not supply a
> -	 * function to initialize that data.
> -	 */
> -	to_clear = sizeof(*cmd) - sizeof(cmd->req);
> -	if (!dev->host->hostt->init_cmd_priv)
> -		to_clear += dev->host->hostt->cmd_size;
> -	memset((char *)cmd + sizeof(cmd->req), 0, to_clear);
> -
>   	cmd->device = dev;
> -	cmd->sense_buffer = buf;
> -	cmd->prot_sdb = prot;
> -	cmd->flags = flags;
> +	cmd->flags &= SCMD_PRESERVED_FLAGS;

*

>   	INIT_LIST_HEAD(&cmd->eh_entry);
>   	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
> -	cmd->jiffies_at_alloc = jiffies_at_alloc;
> -	cmd->retries = retries;
> -	if (in_flight)
> -		__set_bit(SCMD_STATE_INFLIGHT, &cmd->state);
> -	cmd->budget_token = budget_token;
> -
>   }
>   
>   static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
> @@ -1586,10 +1558,35 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
>   	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>   	struct scsi_device *sdev = req->q->queuedata;
>   	struct Scsi_Host *shost = sdev->host;
> +	bool in_flight = test_bit(SCMD_STATE_INFLIGHT, &cmd->state);
>   	struct scatterlist *sg;
>   
>   	scsi_init_command(sdev, cmd);
>   
> +	cmd->eh_eflags = 0;
> +	cmd->allowed = 0;
> +	cmd->prot_type = 0;
> +	cmd->prot_flags = 0;
> +	cmd->submitter = 0;
> +	cmd->cmd_len = 0;
> +	cmd->cmnd = NULL;
> +	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
> +	cmd->underflow = 0;
> +	cmd->transfersize = 0;
> +	cmd->host_scribble = NULL;
> +	cmd->result = 0;
> +	cmd->extra_len = 0;
> +	cmd->state = 0;
> +	if (in_flight)
> +		__set_bit(SCMD_STATE_INFLIGHT, &cmd->state);
> +
> +	/*
> +	 * Only clear the driver-private command data if the LLD does not supply
> +	 * a function to initialize that data.
> +	 */
> +	if (!shost->hostt->init_cmd_priv)
> +		memset(cmd + 1, 0, shost->hostt->cmd_size);
> +
>   	cmd->prot_op = SCSI_PROT_NORMAL;
>   	if (blk_rq_bytes(req))
>   		cmd->sc_data_direction = rq_dma_dir(req);