On 6/23/21 12:57 PM, John Garry wrote:

On 04/05/2021 07:12, Hannes Reinecke wrote:

quoted

On 5/4/21 4:21 AM, Bart Van Assche wrote:

quoted

On 5/3/21 8:03 AM, Hannes Reinecke wrote:

quoted

+/**
+ * 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