Re: [PATCH 04/24] scsi: core: Implement reserved command handling

[John Garry <john.g.garry@oracle.com>]

On 04/04/2025 17:34, Bart Van Assche wrote:
>>> Quite some drivers are using management commands internally. These 
>>> commands
>>> typically use the same tag pool as regular SCSI commands. Tags for these
>>> management commands are set aside before allocating the block-mq tag 
>>> bitmap
>>> for regular SCSI commands. The block layer already supports this via the
>>> reserved tag mechanism. Add a new field 'nr_reserved_cmds' to the 
>>> SCSI host
>>> template to instruct the block layer to set aside a tag space for these
>>> management commands by using reserved tags. Exclude reserved commands 
>>> from
>>> .can_queue because .can_queue is visible in sysfs.
>>>
>>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>>> [ bvanassche: modified patch description ]
>>> Cc: John Garry<john.g.garry@oracle.com>
>>> Signed-off-by: Bart Van Assche<bvanassche@acm.org>
>>
>> How is this related to the rest of the series?
>>
>> To allocate a reserved-tag request we need to use BLK_MQ_REQ_RESERVED, 
>> right? I don't see that used...
> 
> Hi John,
> 
> Calling blk_mq_alloc_request(..., BLK_MQ_REQ_RESERVED) is not the only 
> way to allocated a reserved tag. Another possibility is to let the
> driver manage reserved tags. The UFS driver only needs a single reserved
> tag and already serializes use of that tag. See also the following
> change in patch 21/24:
> 
> -    hba->reserved_slot = hba->nutrs - 1;
> +    hba->reserved_slot = 0;
> 
> Use of hba->reserved_slot is serialized by calling ufshcd_dev_man_lock()
> and ufshcd_dev_man_unlock(). More code is serialized by these calls than
> only the region in which hba->reserved_slot is used so I don't think
> that the UFS driver code would become shorter by using block layer
> functions for allocating / freeing reserved tags.

Now I see this in 23/24:

+/*
+ * Convert a block layer tag into a SCSI command pointer. This function is
+ * called once per I/O completion path and is also called from error paths.
+ */
+static inline struct scsi_cmnd *ufshcd_tag_to_cmd(struct ufs_hba *hba, 
u32 tag)
+{
+	struct blk_mq_tags *tags = hba->host->tag_set.tags[0];
+	struct request *rq;
+
+	/*
+	 * Use .static_rqs[] for reserved commands because blk_mq_get_tag()
+	 * is not called for reserved commands by the UFS driver.
+	 */
+	rq = tag < UFSHCD_NUM_RESERVED ? tags->static_rqs[tag] :
+					 blk_mq_tag_to_rq(tags, tag);
+
+	if (WARN_ON_ONCE(!rq))
+		return NULL;
+
+	return blk_mq_rq_to_pdu(rq);
+}
+

Do you really think that it is ok that anything outside the block layer 
should be referencing tags->static_rqs[] directly?

Even using blk_mq_alloc_request() would seem better than that.