| Message ID | 20220222140443.589882-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] bsg: don't include scsi_request.h in bsg-lib.h | expand |
On 22/02/2022 14:04, Christoph Hellwig wrote: Hi Christoph, > 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; I am just wondering did you consider using struct_group() for safety? I don't think the scsi_cmnd members have special ordering apart from co-locating related members. Here's how it could look (on top of yours): ---8<---- diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 0b5c4606f641..51d2afa2d4c9 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1549,18 +1549,8 @@ static blk_status_t scsi_prepare_cmd(struct request *req) scsi_init_command(sdev, cmd); - cmd->eh_eflags = 0; - cmd->allowed = 0; - cmd->prot_type = 0; - cmd->prot_flags = 0; - cmd->submitter = 0; - 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; + memset((char *)cmd + offsetof(struct scsi_cmnd, to_be_zeroed), 0, sizeof(cmd->to_be_zeroed)); + if (in_flight) __set_bit(SCMD_STATE_INFLIGHT, &cmd->state); diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 76c5eaeeb3b5..8d7a7186a3ab 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -73,10 +73,46 @@ struct scsi_cmnd { struct rcu_head rcu; - int eh_eflags; /* Used by error handlr */ - int budget_token; + struct_group(to_be_zeroed, + int eh_eflags; /* Used by error handlr */ + int allowed; + unsigned char prot_type; + unsigned char prot_flags; + enum scsi_cmnd_submitter submitter; ... ---->8--- Thanks, John
On Wed, Feb 23, 2022 at 12:21:10PM +0000, John Garry wrote: > I am just wondering did you consider using struct_group() for safety? > > I don't think the scsi_cmnd members have special ordering apart from > co-locating related members. > > Here's how it could look (on top of yours): Besides being ugly as hell I don't see the benefit. Quite contrary, a lot of these fields are properly initialized later and we can drop the zeroing as well, but I'd rather do that separately and with a proper audit.
On 23/02/2022 12:39, Christoph Hellwig wrote: > On Wed, Feb 23, 2022 at 12:21:10PM +0000, John Garry wrote: >> I am just wondering did you consider using struct_group() for safety? >> >> I don't think the scsi_cmnd members have special ordering apart from >> co-locating related members. >> >> Here's how it could look (on top of yours): > > Besides being ugly as hell I don't see the benefit. Quite contrary, > a lot of these fields are properly initialized later and we can drop > the zeroing as well, but I'd rather do that separately and with a proper > audit. > > . ok, fine - so the init may be removed later. For the record, advantages now could include: - prob more efficient - somewhat safer coding practice - more condense C code But, for sure, it's not so pretty to have this sub-struct. Anyway, I don't feel too strongly about it and it was just a suggestion. Thanks, John
On Wed, Feb 23, 2022 at 12:56:32PM +0000, John Garry wrote: > For the record, advantages now could include: > - prob more efficient > - somewhat safer coding practice > - more condense C code > > But, for sure, it's not so pretty to have this sub-struct. > > Anyway, I don't feel too strongly about it and it was just a suggestion. In general most of the zeroing here should go away entirely. Right now that code is a bit convolute due to the fake EH scsi command that also uses scsi_init_command. Once that is sorted out (I've just pinged Hannes for his series) scsi_init_command can be folded into scsi_prepare_cmd and a lot more of this can be cleaned up.
On 2/23/22 04:58, Christoph Hellwig wrote: > In general most of the zeroing here should go away entirely. Right > now that code is a bit convolute due to the fake EH scsi command that > also uses scsi_init_command. Once that is sorted out (I've just pinged > Hannes for his series) scsi_init_command can be folded into > scsi_prepare_cmd and a lot more of this can be cleaned up. Several SCSI LLDs rely on the SCSI core zeroing the driver-private data. I'd be more than happy if the code for zeroing of driver-private data would be pushed into the LLD drivers. This may require the introduction of a new per-command flag since that zeroing should only happen if scsi_queue_rq() decided to call scsi_prepare_cmd(). Bart.
On 2/22/22 06: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 ^ missing space? > alreadly uses kzalloc to allocate a pre-zeroed command. > > This is still conservative and can probably be optimizated further. ^^^^^^^^^^^ optimized? Anyway: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Wed, Feb 23, 2022 at 12:16:24PM -0800, Bart Van Assche wrote: > On 2/23/22 04:58, Christoph Hellwig wrote: >> In general most of the zeroing here should go away entirely. Right >> now that code is a bit convolute due to the fake EH scsi command that >> also uses scsi_init_command. Once that is sorted out (I've just pinged >> Hannes for his series) scsi_init_command can be folded into >> scsi_prepare_cmd and a lot more of this can be cleaned up. > > Several SCSI LLDs rely on the SCSI core zeroing the driver-private data. > I'd be more than happy if the code for zeroing of driver-private data would > be pushed into the LLD drivers. This may require the introduction of a new > per-command flag since that zeroing should only happen if scsi_queue_rq() > decided to call scsi_prepare_cmd(). If the driver provides a init_cmd_priv method it is not zeroed. So if a driver does not want it zeroed, it can always provide this callback, including an empty one.
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@lst.de> FWIW: Reviewed-by: John Garry <john.garry@huawei.com> 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);
On Thu, Feb 24, 2022 at 08:28:48AM +0000, John Garry wrote: >> + 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 * with below you mean the cmd->flags &= SCMD_PRESERVED_FLAGS; ? No, that doen't clear the flag, but all the others.
On 24/02/2022 16:27, Christoph Hellwig wrote: > On Thu, Feb 24, 2022 at 08:28:48AM +0000, John Garry wrote: >>> + 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 * > with below you mean the > > cmd->flags &= SCMD_PRESERVED_FLAGS; > > ? No, that doen't clear the flag, but all the others. Yeah, I was wrong as SCMD_INITIALIZED is included in (actually same as) SCMD_PRESERVED_FLAGS mask. Sorry for the noise. Cheers, John
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; 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);
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 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@lst.de> --- drivers/scsi/scsi_lib.c | 61 ++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 32 deletions(-)