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

Bart Van Assche
2025-02-18T18:10:17Z

On 2/18/25 4:13 AM, John Garry wrote:
> 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.

I'm not aware of any such drivers.

Driver-private data was introduced together with the scsi-mq code. I'm
not aware of a similar concept in the legacy SCSI core.

Commit d285203cf647 ("scsi: add support for a blk-mq based I/O path")
introduced the following code in kernel v3.17-rc1:

+static int scsi_mq_prep_fn(struct request *req)
+{
[ ... ]
+ memset(cmd, 0, sizeof(struct scsi_cmnd));
[ ... ]
+static int scsi_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
+{
[ ... ]
+ if (!(req->cmd_flags & REQ_DONTPREP)) {
+ ret = prep_to_mq(scsi_mq_prep_fn(req));
+ if (ret)
+ goto out_dec_host_busy;
+ req->cmd_flags |= REQ_DONTPREP;
+ }

I think the above memset() call was introduced because of the following
code in the legacy SCSI core (from kernel v3.16):

struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t
gfp_mask)
{
struct scsi_cmnd *cmd = scsi_host_alloc_command(shost, gfp_mask);

if (unlikely(!cmd)) {
unsigned long flags;

spin_lock_irqsave(&shost->free_list_lock, flags);
if (likely(!list_empty(&shost->free_list))) {
cmd = list_entry(shost->free_list.next,
struct scsi_cmnd, list);
list_del_init(&cmd->list);
}
spin_unlock_irqrestore(&shost->free_list_lock, flags);

if (cmd) {
void *buf, *prot;

buf = cmd->sense_buffer;
prot = cmd->prot_sdb;

memset(cmd, 0, sizeof(*cmd));

cmd->sense_buffer = buf;
cmd->prot_sdb = prot;
}
}

return cmd;
}
EXPORT_SYMBOL_GPL(__scsi_get_command);

If I'm reading the v3.16 block layer and SCSI code correctly,
__scsi_get_command() was called not only when a command was submitted
but also when it got resubmitted. See also the q->prep_rq_fn() call in
blk_peek_request().

Since the historic behavior involved clearing the entire struct
scsi_cmnd during requeuing, I'm fine with restoring this behavior.

Thanks,

Bart.