[03/18] scsi: add scsi_{get,put}_internal_cmd() helper

Message ID 20210503150333.130310-4-hare@suse.de
State New
Headers show
Series
  • scsi: enabled reserved commands for LLDDs
Related show

Commit Message

Hannes Reinecke May 3, 2021, 3:03 p.m.
Add helper functions to allow LLDDs to allocate and free
internal commands.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_lib.c    | 38 ++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  3 +++
 2 files changed, 41 insertions(+)

Comments

Bart Van Assche May 4, 2021, 2:21 a.m. | #1
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.
Hannes Reinecke May 4, 2021, 6:12 a.m. | #2
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
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
Christoph Hellwig May 4, 2021, 9:53 a.m. | #3
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.
Hannes Reinecke May 4, 2021, 12:54 p.m. | #4
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
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
John Garry June 23, 2021, 10:57 a.m. | #5
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
Hannes Reinecke June 23, 2021, 1:48 p.m. | #6
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
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
Bart Van Assche June 23, 2021, 4:09 p.m. | #7
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.
John Garry June 24, 2021, 9:55 a.m. | #8
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

Patch

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);