| Message ID | 20250217021628.2929248-1-yebin@huaweicloud.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [v3] scsi: core: clear driver private data when retry request | expand |
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? > 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 > 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)
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. > >> 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) >
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) >> > >
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.
On 18/02/2025 18:10, Bart Van Assche wrote: > 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. ok, fine. Indeed, to me, it does not make much sense to keep the data in this structure persistent between retries anyway. Thanks, John
On 2/16/25 6:16 PM, Ye Bin wrote: > 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) Reviewed-by: Bart Van Assche <bvanassche@acm.org>
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)