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

John Garry
2021-06-23T11:04:25Z

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