Re: [PATCH v3] scsi: core: clear driver private data when retry request

[John Garry <john.g.garry@oracle.com>]

On 18/02/2025 11:23, yebin wrote:
> 
> 
> On 2025/2/17 17:44, John Garry wrote:
>> On 17/02/2025 02:16, Ye Bin wrote:
>>> From: Ye Bin <yebin10@huawei.com>
>>>
>>> After commit 1bad6c4a57ef
>>> ("scsi: zero per-cmd private driver data for each MQ I/O"),
>>> xen-scsifront/virtio_scsi/snic driver remove code that zeroes
>>> driver-private command data. If request do retry will lead to
>>> driver-private command data remains. Before commit 464a00c9e0ad
>>> ("scsi: core: Kill DRIVER_SENSE") if virtio_scsi do capacity
>>> expansion, first request may return UA then request will do retry,
>>> as driver-private command data remains, request will return UA
>>> again.
>>
>> So are there any drivers which expect this sort of behavior, i.e. keep
>> private data between retries?
> 
> No driver that depends on the last state is found. If yes, the driver
> should provide the init_cmd_priv function to manage private data. In
> this way, the SCSI middle layer ignores the private data of the driver.
> 

TBH, I am not sure on the history here. Maybe Bart or Christoph knows, 
but my impression is still that the priv data is only cleared once in 
the lifetime of the request (from 1bad6c4a) - at prep time - and some 
drivers may rely on that (not be cleared again). Unlikely, though.

>>
>>> As a result, the request keeps retrying, and the request
>>> times out and fails.
>>> So zeroes driver-private command data when request do retry.
>>>
>>> Fixes: f7de50da1479 ("scsi: xen-scsifront: Remove code that zeroes
>>> driver-private command data")
>>> Fixes: c2bb87318baa ("scsi: virtio_scsi: Remove code that zeroes
>>> driver-private command data")
>>> Fixes: c3006a926468 ("scsi: snic: Remove code that zeroes
>>> driver-private command data")
>>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>>
>>> ---
>>
>> Ps: in future, please list the changes per version here
>>
> Thanks for the heads-up.
>>>   drivers/scsi/scsi_lib.c | 14 +++++++-------
>>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index be0890e4e706..f1cfe0bb89b2 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -1669,13 +1669,6 @@ static blk_status_t scsi_prepare_cmd(struct
>>> request *req)
>>>       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);
>>> @@ -1842,6 +1835,13 @@ static blk_status_t scsi_queue_rq(struct
>>> blk_mq_hw_ctx *hctx,
>>>       if (!scsi_host_queue_ready(q, shost, sdev, cmd))
>>>           goto out_dec_target_busy;
>>> +    /*
>>> +     * Only clear the driver-private command data if the LLD does not
>>> supply
>>> +     * a function to initialize that data.
>>> +     */
>>> +    if (shost->hostt->cmd_size && !shost->hostt->init_cmd_priv)
>>> +        memset(cmd + 1, 0, shost->hostt->cmd_size);
>>> +
>>>       if (!(req->rq_flags & RQF_DONTPREP)) {
>>>           ret = scsi_prepare_cmd(req);
>>>           if (ret != BLK_STS_OK)
>>
> 
>