On 11/26/21 10:58 AM, John Garry wrote:
On 25/11/2021 15:10, Hannes Reinecke wrote:
+/**
+ * scsi_get_internal_cmd - allocate an internal SCSI command
+ * @sdev: SCSI device from which to allocate the command
+ * @data_direction: Data direction for the allocated command
+ * @nowait: do not wait for command allocation to succeed.
+ *
+ * Allocates a SCSI command for internal LLDD use.
+ */
+struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
+ int data_direction, bool nowait)
+{
+ struct request *rq;
+ struct scsi_cmnd *scmd;
+ blk_mq_req_flags_t flags = 0;
+ int op;
+
+ if (nowait)
+ flags |= BLK_MQ_REQ_NOWAIT;
+ op = (data_direction == DMA_TO_DEVICE) ?
+ REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
+ rq = blk_mq_alloc_request(sdev->request_queue, op, flags);
+ if (IS_ERR(rq))
+ return NULL;
+ scmd = blk_mq_rq_to_pdu(rq);
+ scmd->device = sdev;
+ return scmd;
+}
+EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);
So there are a couple of generally-accepted grievances about this approach:
a. we're being allocated a scsi_cmnd, but not using what is being
allocated as a scsi_cmnd, but rather just a holder as a reference to an
allocated tag
b. we're being allocated a request, which is not being sent through the
block layer*
And while being true in general, it does make some assumptions:
- Reserved commands will never being sent via the block layer
- None of the drivers will need to use the additional 'scsi_cmnd' payload.
Here I'm not sure if this is true in general.
While it doesn't seem to be necessary to send reserved commands via the
block layer (ie calling 'queue_rq' on them), we shouldn't exclude the
possibility.
Didn't we speak about that in the context of converting libata?
And I have some driver conversions queued (fnic in particular), which
encapsulate everything into a scsi command.
It just seems to me that what the block layer is providing is not suitable.
How about these:
a. allow block driver to specify size of reserved request PDU separately
to regular requests, so we can use something like this for rsvd commands:
struct scsi_rsvd_cmnd {
struct scsi_device *sdev;
}
And fix up SCSI iter functions and LLDs to deal with it.
That's what Bart suggested a while back, but then we have to problem
that the reserved tags are completed with the same interrupt routine,
and _that_ currently assumes that everything is a scsi command.
Trying to fix up that assumption would mean to audit the midlayer
(scmd->device is a particular common pattern), _and_ all drivers wanting
to make use of reserved commands.
For me that's too high an risk to introduce errors; audits are always
painful and error-prone.
b. provide block layer API to provide just same as is returned from
blk_mq_unique_tag(), but no request is provided. This just gives what we
need but would be disruptive in scsi layer and LLDs.
Having looked at the block layer and how tags are allocated I found it
too deeply interlinked with the request queue and requests in general.
Plus I've suggested that with a previous patchset, which got vetoed by
Bart as he couldn't use such an API for UFS.
c. as alternative to b., send all rsvd requests through the block layer,
but can be very difficult+disruptive for users
And, indeed, not possible when we will need to send these requests
during error handling, where the queue might be blocked/frozen/quiesced
precisely because we are in error handling ...
*For polling rsvd commands on a poll queue (which we will need for
hisi_sas driver and maybe others for poll mode support), we would need
to send the request through the block layer, but block layer polling
requires a request with a bio, which is a problem.
Allocating a bio is a relatively trivial task. But as soon as we ever
want to be able to implement polling support for reserved tags we
essentially _have_ to use requests, and that means we'll have to use the
provided interfaces from the block layer.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
