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

Hannes Reinecke
2021-06-23T13:48:39Z

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