Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Hannes Reinecke <hare@suse.de>]

On 11/22/21 6:46 PM, Bart Van Assche wrote:
> On 11/22/21 12:58 AM, John Garry wrote:
>> On 19/11/2021 19:57, Bart Van Assche wrote:
>>> +/**
>>> + * scsi_get_internal_cmd - Allocate an internal SCSI command
>>> + * @q: request queue from which to allocate the command. This
>>> request queue may
>>> + *    but does not have to be associated with a SCSI device. This
>>> request
>>> + *    queue must be associated with a SCSI tag set. See also
>>> + *    scsi_mq_setup_tags().
>>> + * @data_direction: Data direction for the allocated command.
>>> + * @flags: Zero or more BLK_MQ_REQ_* flags.
>>> + *
>>> + * Allocates a request for driver-internal use. The tag of the
>>> returned SCSI
>>> + * command is guaranteed to be unique.
>>> + */
>>> +struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
>>> +                    enum dma_data_direction data_direction,
>>> +                    blk_mq_req_flags_t flags)
>>
>> I'd pass the Scsi_Host or scsi_device rather than a request q, so maybe:
>>
>> struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, ..)
>> struct scsi_cmnd *scsi_host_get_internal_cmd(struct Scsi_Host *shost, ..)
> 
> Passing a request queue pointer as first argument instead of a struct
> scsi_device is a deliberate choice. In the UFS driver (and probably also
> in other SCSI LLDs) we want to allocate internal requests without these
> requests being visible in any existing SCSI device statistics. Creating
> a new SCSI device for the allocation of internal requests is not a good
> choice because that new SCSI device would have to be assigned a LUN
> number and would be visible in sysfs. Hence the choice to allocate
> internal requests from a request queue that is not associated with any
> SCSI device.
> 
It's actually a bit more involved.

The biggest issue is that the SCSI layer is littered with the assumption
that there _will_ be a ->device pointer in struct scsi_cmnd.
If we make up a scsi_cmnd structure _without_ that we'll have to audit
the entire stack to ensure we're not tripping over a NULL device pointer.
And to make matters worse, we also need to audit the completion path in
the driver, which typically have the same 'issue'.

Case in point:

# git grep -- '->device' drivers/scsi | wc --lines
2712

Which was the primary reason for adding a stub device to the SCSI Host;
simply to avoid all the pointless churn and have a valid device for all
commands.

The only way I can see how to avoid getting dragged down into that
rat-hole is to _not_ returning a scsi_cmnd, but rather something else
entirely; that's the avenue I've exploited with my last patchset which
would just return a tag number.
But as there are drivers which really need a scsi_cmnd I can't se how we
can get away with not having a stub scsi_device for the scsi host.

And that won't even show up in sysfs if we assign it a LUN number beyond
the addressable range; 'max_id':0 tends to be a safe choice here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer