| Message ID | 20210503150333.130310-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enabled reserved commands for LLDDs | expand |
On 5/3/21 8:03 AM, Hannes Reinecke wrote: > +/** > + * scsi_get_internal_cmd - allocate an internal SCSI command > + * @sdev: SCSI device from which to allocate the command > + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT) > + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT. > + * > + * Allocates a SCSI command for internal LLDD use. > + */ > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + unsigned int op, blk_mq_req_flags_t flags) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && > + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); > + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + scmd->device = sdev; > + return scmd; > +} > +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); Multiple fields that are initialized by the regular command submission path are not initialized by the above function, e.g. scmd->tag. Has it been considered to call scsi_init_command() instead of adding yet another code path that initializes scmd->request? Thanks, Bart.
On 5/4/21 4:21 AM, Bart Van Assche wrote: > On 5/3/21 8:03 AM, Hannes Reinecke wrote: >> +/** >> + * scsi_get_internal_cmd - allocate an internal SCSI command >> + * @sdev: SCSI device from which to allocate the command >> + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT) >> + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT. >> + * >> + * Allocates a SCSI command for internal LLDD use. >> + */ >> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >> + unsigned int op, blk_mq_req_flags_t flags) >> +{ >> + struct request *rq; >> + struct scsi_cmnd *scmd; >> + >> + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && >> + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); >> + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); >> + if (IS_ERR(rq)) >> + return NULL; >> + scmd = blk_mq_rq_to_pdu(rq); >> + scmd->request = rq; >> + scmd->device = sdev; >> + return scmd; >> +} >> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); > > Multiple fields that are initialized by the regular command submission > path are not initialized by the above function, e.g. scmd->tag. Has it > been considered to call scsi_init_command() instead of adding yet > another code path that initializes scmd->request? > Hmm. No, I don't think it's a good idea. Basic idea is that the SCSI request serves as a container for (non-SCSI) management commands, _and_ that they are submitted directly from within the driver, ie never ever enter ->queue_rq(). As such we don't need an initialisation vie scsi_init_request(), and it would actually be counter-productive as we would be setting up fields which we'll never need anyway, and might need to tear down afterwards. Cheers, Hannes
On Mon, May 03, 2021 at 05:03:18PM +0200, Hannes Reinecke wrote: > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + unsigned int op, blk_mq_req_flags_t flags) Weird indentation - prototype continuations either use two tabs or are aligned after the opening brace (I generally prefer the former). > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && > + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); Woudn't a simple bool write command make more sense than passing the actual op here? > + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + scmd->device = sdev; Maybe a comment that explains what part of the scmd are initialized and which not would be useful.
On 5/4/21 11:53 AM, Christoph Hellwig wrote: > On Mon, May 03, 2021 at 05:03:18PM +0200, Hannes Reinecke wrote: >> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >> + unsigned int op, blk_mq_req_flags_t flags) > > Weird indentation - prototype continuations either use two tabs or > are aligned after the opening brace (I generally prefer the former). > Yeah, I'm never sure how one should be indenting the second line for a function declaration. But I'll fix it. >> +{ >> + struct request *rq; >> + struct scsi_cmnd *scmd; >> + >> + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && >> + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); > > Woudn't a simple bool write command make more sense than passing the > actual op here? > Yep, can do. >> + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); >> + if (IS_ERR(rq)) >> + return NULL; >> + scmd = blk_mq_rq_to_pdu(rq); >> + scmd->request = rq; >> + scmd->device = sdev; > > Maybe a comment that explains what part of the scmd are initialized > and which not would be useful. > Okay. Cheers, Hannes
On 04/05/2021 07:12, Hannes Reinecke wrote: > On 5/4/21 4:21 AM, Bart Van Assche wrote: >> On 5/3/21 8:03 AM, Hannes Reinecke wrote: >>> +/** >>> + * scsi_get_internal_cmd - allocate an internal SCSI command >>> + * @sdev: SCSI device from which to allocate the command >>> + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT) >>> + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT. >>> + * >>> + * Allocates a SCSI command for internal LLDD use. >>> + */ >>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >>> + unsigned int op, blk_mq_req_flags_t flags) >>> +{ >>> + struct request *rq; >>> + struct scsi_cmnd *scmd; >>> + >>> + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && >>> + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); >>> + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); >>> + if (IS_ERR(rq)) >>> + return NULL; >>> + scmd = blk_mq_rq_to_pdu(rq); >>> + scmd->request = rq; >>> + scmd->device = sdev; >>> + return scmd; >>> +} >>> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); >> >> Multiple fields that are initialized by the regular command submission >> path are not initialized by the above function, e.g. scmd->tag. Has it >> been considered to call scsi_init_command() instead of adding yet >> another code path that initializes scmd->request? >> > Hmm. No, I don't think it's a good idea. > Basic idea is that the SCSI request serves as a container for (non-SCSI) > management commands, _and_ that they are submitted directly from within > the driver, ie never ever enter ->queue_rq(). > As such we don't need an initialisation vie scsi_init_request(), and it > would actually be counter-productive as we would be setting up fields > which we'll never need anyway, and might need to tear down afterwards. I will note that we also bypass the queue budgeting in blk_mq_ops.{get,put}_budget. I figure that is not an issue... BTW, any chance of a new version? Thanks, John
On 6/23/21 12:57 PM, John Garry wrote: > On 04/05/2021 07:12, Hannes Reinecke wrote: >> On 5/4/21 4:21 AM, Bart Van Assche wrote: >>> On 5/3/21 8:03 AM, Hannes Reinecke wrote: >>>> +/** >>>> + * scsi_get_internal_cmd - allocate an internal SCSI command >>>> + * @sdev: SCSI device from which to allocate the command >>>> + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT) >>>> + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT. >>>> + * >>>> + * Allocates a SCSI command for internal LLDD use. >>>> + */ >>>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >>>> + unsigned int op, blk_mq_req_flags_t flags) >>>> +{ >>>> + struct request *rq; >>>> + struct scsi_cmnd *scmd; >>>> + >>>> + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && >>>> + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); >>>> + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); >>>> + if (IS_ERR(rq)) >>>> + return NULL; >>>> + scmd = blk_mq_rq_to_pdu(rq); >>>> + scmd->request = rq; >>>> + scmd->device = sdev; >>>> + return scmd; >>>> +} >>>> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); >>> >>> Multiple fields that are initialized by the regular command submission >>> path are not initialized by the above function, e.g. scmd->tag. Has it >>> been considered to call scsi_init_command() instead of adding yet >>> another code path that initializes scmd->request? >>> >> Hmm. No, I don't think it's a good idea. >> Basic idea is that the SCSI request serves as a container for >> (non-SCSI) management commands, _and_ that they are submitted directly >> from within the driver, ie never ever enter ->queue_rq(). >> As such we don't need an initialisation vie scsi_init_request(), and >> it would actually be counter-productive as we would be setting up >> fields which we'll never need anyway, and might need to tear down >> afterwards. > > I will note that we also bypass the queue budgeting in > blk_mq_ops.{get,put}_budget. I figure that is not an issue... > > BTW, any chance of a new version? > I have _so_ no idea. The review from Christoph to patch 07/18 he (apparently) changed his mind for the current implementation of using scsi_get_host_dev(), citing an approach I had been implemented for v2 (and which got changed due to his reviews for v2). So no I'm not sure if he retracted on his earlier review, or if he just had forgotten about it. And before I get clarification from him I can't really move forward, as both reviews contradict each other. Christoph? Cheers, Hannes
On 5/3/21 8:03 AM, Hannes Reinecke wrote: > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + unsigned int op, blk_mq_req_flags_t flags) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && > + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); Consider using blk_op_is_scsi() instead of open-coding it. Thanks, Bart.
On 23/06/2021 14:48, Hannes Reinecke wrote: >> >> I will note that we also bypass the queue budgeting in >> blk_mq_ops.{get,put}_budget. I figure that is not an issue... >> >> BTW, any chance of a new version? >> > I have _so_ no idea. > > The review from Christoph to patch 07/18 he (apparently) changed his > mind for the current implementation of using scsi_get_host_dev(), citing > an approach I had been implemented for v2 (and which got changed due to > his reviews for v2). > So no I'm not sure if he retracted on his earlier review, or if he just > had forgotten about it. > And before I get clarification from him I can't really move forward, as > both reviews contradict each other. > > Christoph? I see. FWIW, not using scsi_get_host_dev() means that internal command scsi_cmnd.device cannot be set. But then many fields are not set there. I suppose that if we delete scsi_get_host_dev() now [as it has no user], then that will narrow the choice... BTW, I was just checking the v7, and we have no queue sysfs folder for that host scsi_device. Regardless of how the queue is created - v2 vs v7 method - I think that we should have that somehow. Thanks, John
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index d7c0d5a5f263..f83b04e49bae 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1989,6 +1989,44 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) blk_mq_free_tag_set(&shost->tag_set); } +/** + * scsi_get_internal_cmd - allocate an internal SCSI command + * @sdev: SCSI device from which to allocate the command + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT) + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT. + * + * Allocates a SCSI command for internal LLDD use. + */ +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, + unsigned int op, blk_mq_req_flags_t flags) +{ + struct request *rq; + struct scsi_cmnd *scmd; + + WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) && + ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT)); + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); + if (IS_ERR(rq)) + return NULL; + scmd = blk_mq_rq_to_pdu(rq); + scmd->request = rq; + scmd->device = sdev; + return scmd; +} +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); + +/** + * scsi_put_internal_cmd - free an internal SCSI command + * @scmd: SCSI command to be freed + */ +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) +{ + struct request *rq = blk_mq_rq_from_pdu(scmd); + + blk_mq_free_request(rq); +} +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); + /** * scsi_device_from_queue - return sdev associated with a request_queue * @q: The request queue to return the sdev from diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index ac6ab16abee7..eaa5414ff220 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -462,6 +462,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev, return scsi_execute(sdev, cmd, data_direction, buffer, bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); } +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, + unsigned int op, blk_mq_req_flags_t flags); +void scsi_put_internal_cmd(struct scsi_cmnd *scmd); extern void sdev_disable_disk_events(struct scsi_device *sdev); extern void sdev_enable_disk_events(struct scsi_device *sdev); extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);