[PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[Elias Oltmanns]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

Hi all,

this patch adds support for REQ_TYPE_LINUX_BLOCK requests to the scsi
midlayer. Low level drivers have the option to register their own
handlers for these special requests if necessary.

This patch is required in order to implement disk shock protection in
libata eventually. Please see [1] for further details and the full patch
series. Please note that this patch applies to 2.6.25-rc6 whereas the
original patch series (see [1]) applies to 2.6.24.3.

Please let me know what you think about the patch and if could go into
mainline.

Regards,

Elias

[1] <http://permalink.gmane.org/gmane.linux.ide/29519>

Signed-off-by: Elias Oltmanns <eo@nebensachen.de>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: scsi-req_lb-support.patch --]
[-- Type: text/x-patch, Size: 6363 bytes --]

---

 drivers/scsi/scsi_lib.c  |   65 +++++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/sd.c        |    9 ++++++
 drivers/scsi/sr.c        |    9 ++++++
 include/linux/blkdev.h   |    1 +
 include/scsi/scsi_host.h |   22 ++++++++++++++++
 5 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba21d97..dd9bea2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1262,6 +1262,15 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
 
 	if (req->cmd_type == REQ_TYPE_BLOCK_PC)
 		ret = scsi_setup_blk_pc_cmnd(sdev, req);
+	else if (req->cmd_type == REQ_TYPE_LINUX_BLOCK) {
+		get_device(&sdev->sdev_gendev);
+		/*
+		 * Since these requests don't need preparation, we'll
+		 * basically just accept them unconditionally at this
+		 * point.
+		 */
+		ret = BLKPREP_OK;
+	}
 	return scsi_prep_return(q, req, ret);
 }
 
@@ -1371,12 +1380,30 @@ static void scsi_kill_request(struct request *req, struct request_queue *q)
 	__scsi_done(cmd);
 }
 
+static void scsi_finish_lb_req(struct request *req)
+{
+	struct request_queue *q = req->q;
+	struct scsi_device *sdev = q->queuedata;
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	end_that_request_last(req, 1);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+	put_device(&sdev->sdev_gendev);
+}
+
 static void scsi_softirq_done(struct request *rq)
 {
 	struct scsi_cmnd *cmd = rq->completion_data;
-	unsigned long wait_for = (cmd->allowed + 1) * cmd->timeout_per_command;
+	unsigned long wait_for;
 	int disposition;
 
+	if (blk_lb_request(rq)) {
+		scsi_finish_lb_req(rq);
+		return;
+	}
+
+	wait_for = (cmd->allowed + 1) * cmd->timeout_per_command;
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
 	disposition = scsi_decide_disposition(cmd);
@@ -1406,6 +1433,24 @@ static void scsi_softirq_done(struct request *rq)
 	}
 }
 
+static void scsi_exec_lb_req(struct request *req)
+{
+	struct scsi_device *sdev = req->q->queuedata;
+	struct scsi_host_template *shostt = sdev->host->hostt;
+	int rc;
+
+	if (shostt->lb_request_fn)
+		rc = shostt->lb_request_fn(req);
+	else
+		rc = FAILED;
+
+	if (rc == FAILED)
+		req->errors = -EIO;
+	else if (rc == QUEUED)
+		return;
+	blk_complete_request(req);
+}
+
 /*
  * Function:    scsi_request_fn()
  *
@@ -1448,7 +1493,23 @@ static void scsi_request_fn(struct request_queue *q)
 		 * accept it.
 		 */
 		req = elv_next_request(q);
-		if (!req || !scsi_dev_queue_ready(q, sdev))
+		if (!req)
+			break;
+
+		/*
+		 * We do not account for linux blk req in the device
+		 * or host busy accounting because it is not necessarily
+		 * a scsi command that is sent to some object. The lower
+		 * level can translate it into a request/scsi_cmnd, if
+		 * necessary, and then queue that up using REQ_TYPE_BLOCK_PC.
+		 */
+		if (blk_lb_request(req)) {
+			blkdev_dequeue_request(req);
+			scsi_exec_lb_req(req);
+			continue;
+		}
+
+		if (!scsi_dev_queue_ready(q, sdev))
 			break;
 
 		if (unlikely(!scsi_device_online(sdev))) {
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7aee64d..fff8b49 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -357,6 +357,15 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
 		goto out;
+	} else if (rq->cmd_type == REQ_TYPE_LINUX_BLOCK) {
+		get_device(&sdp->sdev_gendev);
+		/*
+		 * Since these requests don't need preparation, we'll
+		 * basically just accept them unconditionally at this
+		 * point.
+		 */
+		ret = BLKPREP_OK;
+		goto out;
 	} else if (rq->cmd_type != REQ_TYPE_FS) {
 		ret = BLKPREP_KILL;
 		goto out;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 208565b..b478b3f 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -337,6 +337,15 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
 		goto out;
+	} else if (rq->cmd_type == REQ_TYPE_LINUX_BLOCK) {
+		get_device(&sdp->sdev_gendev);
+		/*
+		 * Since these requests don't need preparation, we'll
+		 * basically just accept them unconditionally at this
+		 * point.
+		 */
+		ret = BLKPREP_OK;
+		goto out;
 	} else if (rq->cmd_type != REQ_TYPE_FS) {
 		ret = BLKPREP_KILL;
 		goto out;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6f79d40..f4ed034 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -453,6 +453,7 @@ enum {
 
 #define blk_fs_request(rq)	((rq)->cmd_type == REQ_TYPE_FS)
 #define blk_pc_request(rq)	((rq)->cmd_type == REQ_TYPE_BLOCK_PC)
+#define blk_lb_request(rq)	((rq)->cmd_type == REQ_TYPE_LINUX_BLOCK)
 #define blk_special_request(rq)	((rq)->cmd_type == REQ_TYPE_SPECIAL)
 #define blk_sense_request(rq)	((rq)->cmd_type == REQ_TYPE_SENSE)
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 530ff4c..1765855 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -9,6 +9,7 @@
 #include <scsi/scsi.h>
 
 struct request_queue;
+struct request;
 struct block_device;
 struct completion;
 struct module;
@@ -153,6 +154,27 @@ struct scsi_host_template {
 				  void (*done)(struct scsi_cmnd *));
 
 	/*
+	 * The lb_request_fn function is used to pass
+	 * REQ_TYPE_LINUX_BLOCK requests to the LLDD. The return value
+	 * FAILED indicates that the command opcode has not been known
+	 * by lb_request_fn. In contrast, the return value SUCCESS
+	 * means that the opcode has been recognised and the request
+	 * has been processed accordingly. Note, however, that SUCCESS
+	 * does not necessarily mean that all actions have been
+	 * performed successfully; errors are recorded in req->errors.
+	 * lb_request_fn can also return QUEUED in order to prevent
+	 * midlayer from enqueueng the request for completion.
+	 * Obviously, the LLDD must take care that the request will be
+	 * completed by means of blk_complete_request eventually.
+	 *
+	 * Status: OPTIONAL
+	 */
+	/* TODO: We might need to accept a return value NEEDS_RETRY some
+	 * time.
+	 */
+	int (* lb_request_fn)(struct request *req);
+
+	/*
 	 * This is an error handling strategy routine.  You don't need to
 	 * define one of these if you don't want to - there is a default
 	 * routine that is present that should work in most cases.  For those

Re: [PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[Elias Oltmanns]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

Elias Oltmanns <eo@nebensachen.de> wrote:
> Hi all,
>
> this patch adds support for REQ_TYPE_LINUX_BLOCK requests to the scsi
> midlayer. Low level drivers have the option to register their own
> handlers for these special requests if necessary.
>
[...]
> +static void scsi_finish_lb_req(struct request *req)
> +{
> +	struct request_queue *q = req->q;
> +	struct scsi_device *sdev = q->queuedata;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	end_that_request_last(req, 1);

That's obsolete, of course. Sorry for missing that. See the correct
patch for 2.6.25-rc7 below.

Regards,

Elias


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: scsi-req_lb-support --]
[-- Type: text/x-patch, Size: 6349 bytes --]

---

 drivers/scsi/scsi_lib.c  |   66 +++++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/sd.c        |    9 ++++++
 drivers/scsi/sr.c        |    9 ++++++
 include/linux/blkdev.h   |    1 +
 include/scsi/scsi_host.h |   22 +++++++++++++++
 5 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f40898d..5e72640 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1262,6 +1262,15 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
 
 	if (req->cmd_type == REQ_TYPE_BLOCK_PC)
 		ret = scsi_setup_blk_pc_cmnd(sdev, req);
+	else if (req->cmd_type == REQ_TYPE_LINUX_BLOCK) {
+		get_device(&sdev->sdev_gendev);
+		/*
+		 * Since these requests don't need preparation, we'll
+		 * basically just accept them unconditionally at this
+		 * point.
+		 */
+		ret = BLKPREP_OK;
+	}
 	return scsi_prep_return(q, req, ret);
 }
 
@@ -1371,12 +1380,31 @@ static void scsi_kill_request(struct request *req, struct request_queue *q)
 	__scsi_done(cmd);
 }
 
+static void scsi_finish_lb_req(struct request *req)
+{
+	struct scsi_device *sdev = req->q->queuedata;
+
+	/*
+	 * TODO: Perhaps the data_len or extra_len of
+	 * REQ_TYPE_LINUX_BLOCK requests are allowed to be different
+	 * from 0?
+	 */
+	blk_end_request(req, 0, 0);
+	put_device(&sdev->sdev_gendev);
+}
+
 static void scsi_softirq_done(struct request *rq)
 {
 	struct scsi_cmnd *cmd = rq->completion_data;
-	unsigned long wait_for = (cmd->allowed + 1) * cmd->timeout_per_command;
+	unsigned long wait_for;
 	int disposition;
 
+	if (blk_lb_request(rq)) {
+		scsi_finish_lb_req(rq);
+		return;
+	}
+
+	wait_for = (cmd->allowed + 1) * cmd->timeout_per_command;
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
 	disposition = scsi_decide_disposition(cmd);
@@ -1406,6 +1434,24 @@ static void scsi_softirq_done(struct request *rq)
 	}
 }
 
+static void scsi_exec_lb_req(struct request *req)
+{
+	struct scsi_device *sdev = req->q->queuedata;
+	struct scsi_host_template *shostt = sdev->host->hostt;
+	int rc;
+
+	if (shostt->lb_request_fn)
+		rc = shostt->lb_request_fn(req);
+	else
+		rc = FAILED;
+
+	if (rc == FAILED)
+		req->errors = -EIO;
+	else if (rc == QUEUED)
+		return;
+	blk_complete_request(req);
+}
+
 /*
  * Function:    scsi_request_fn()
  *
@@ -1448,7 +1494,23 @@ static void scsi_request_fn(struct request_queue *q)
 		 * accept it.
 		 */
 		req = elv_next_request(q);
-		if (!req || !scsi_dev_queue_ready(q, sdev))
+		if (!req)
+			break;
+
+		/*
+		 * We do not account for linux blk req in the device
+		 * or host busy accounting because it is not necessarily
+		 * a scsi command that is sent to some object. The lower
+		 * level can translate it into a request/scsi_cmnd, if
+		 * necessary, and then queue that up using REQ_TYPE_BLOCK_PC.
+		 */
+		if (blk_lb_request(req)) {
+			blkdev_dequeue_request(req);
+			scsi_exec_lb_req(req);
+			continue;
+		}
+
+		if (!scsi_dev_queue_ready(q, sdev))
 			break;
 
 		if (unlikely(!scsi_device_online(sdev))) {
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5fe7aae..95eba9f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -357,6 +357,15 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
 		goto out;
+	} else if (rq->cmd_type == REQ_TYPE_LINUX_BLOCK) {
+		get_device(&sdp->sdev_gendev);
+		/*
+		 * Since these requests don't need preparation, we'll
+		 * basically just accept them unconditionally at this
+		 * point.
+		 */
+		ret = BLKPREP_OK;
+		goto out;
 	} else if (rq->cmd_type != REQ_TYPE_FS) {
 		ret = BLKPREP_KILL;
 		goto out;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7ee86d4..24bdfae 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -337,6 +337,15 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
 		goto out;
+	} else if (rq->cmd_type == REQ_TYPE_LINUX_BLOCK) {
+		get_device(&sdp->sdev_gendev);
+		/*
+		 * Since these requests don't need preparation, we'll
+		 * basically just accept them unconditionally at this
+		 * point.
+		 */
+		ret = BLKPREP_OK;
+		goto out;
 	} else if (rq->cmd_type != REQ_TYPE_FS) {
 		ret = BLKPREP_KILL;
 		goto out;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6f79d40..f4ed034 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -453,6 +453,7 @@ enum {
 
 #define blk_fs_request(rq)	((rq)->cmd_type == REQ_TYPE_FS)
 #define blk_pc_request(rq)	((rq)->cmd_type == REQ_TYPE_BLOCK_PC)
+#define blk_lb_request(rq)	((rq)->cmd_type == REQ_TYPE_LINUX_BLOCK)
 #define blk_special_request(rq)	((rq)->cmd_type == REQ_TYPE_SPECIAL)
 #define blk_sense_request(rq)	((rq)->cmd_type == REQ_TYPE_SENSE)
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 530ff4c..1765855 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -9,6 +9,7 @@
 #include <scsi/scsi.h>
 
 struct request_queue;
+struct request;
 struct block_device;
 struct completion;
 struct module;
@@ -153,6 +154,27 @@ struct scsi_host_template {
 				  void (*done)(struct scsi_cmnd *));
 
 	/*
+	 * The lb_request_fn function is used to pass
+	 * REQ_TYPE_LINUX_BLOCK requests to the LLDD. The return value
+	 * FAILED indicates that the command opcode has not been known
+	 * by lb_request_fn. In contrast, the return value SUCCESS
+	 * means that the opcode has been recognised and the request
+	 * has been processed accordingly. Note, however, that SUCCESS
+	 * does not necessarily mean that all actions have been
+	 * performed successfully; errors are recorded in req->errors.
+	 * lb_request_fn can also return QUEUED in order to prevent
+	 * midlayer from enqueueng the request for completion.
+	 * Obviously, the LLDD must take care that the request will be
+	 * completed by means of blk_complete_request eventually.
+	 *
+	 * Status: OPTIONAL
+	 */
+	/* TODO: We might need to accept a return value NEEDS_RETRY some
+	 * time.
+	 */
+	int (* lb_request_fn)(struct request *req);
+
+	/*
 	 * This is an error handling strategy routine.  You don't need to
 	 * define one of these if you don't want to - there is a default
 	 * routine that is present that should work in most cases.  For those

Re: [PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[Boaz Harrosh]

On Wed, Mar 26 2008 at 16:23 +0200, Elias Oltmanns <eo@nebensachen.de> wrote:
> Elias Oltmanns <eo@nebensachen.de> wrote:
>> Hi all,
>>
>> this patch adds support for REQ_TYPE_LINUX_BLOCK requests to the scsi
>> midlayer. Low level drivers have the option to register their own
>> handlers for these special requests if necessary.
>>
> [...]
>> +static void scsi_finish_lb_req(struct request *req)
>> +{
>> +	struct request_queue *q = req->q;
>> +	struct scsi_device *sdev = q->queuedata;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(q->queue_lock, flags);
>> +	end_that_request_last(req, 1);
> 
> That's obsolete, of course. Sorry for missing that. See the correct
> patch for 2.6.25-rc7 below.
> 
> Regards,
> 
> Elias
> 
> 

It looks to me like you can accomplish any of that (and more) with
regular BLOCK_PC commands plus the varlen facility I have sent:
(http://www.spinics.net/lists/linux-scsi/msg25202.html)

If you want a request with no BIO's and any arbitrary data this can
be just a varlen command. In the presence of varlen commands block
or scsi layers will not assume anything about the passed CDB and will
just channel it all the way to the LLD.

But if you also need mapped buffers you allocate a BIO with right 
direction (or bidi) and the mid-layers will also take care of that in 
the regular way.

BLOCK_PC commands with-or-without data, are always completed at once.

The LLD in question will only need to filter for those special commands
at the .queuecommand entry and act accordingly.

The only problem you might have is with a dumb initiator that might issue
commands to devices that do not know what to do with the new none-standard
commands. There is 3 things you can do.
1. Make the Initiator smarter to only send these commands to good devices
   In a manner of a special flag or a registration process.
2. use commands that are bigger than 16 so .max_cmd_len of legacy drivers
   will not allow these commands through.
3. Do nothing and let the setup process only setup the compatible devices
   to be issued the new commands.

The bsg driver can already be used to issue such commands from user space.
Tell me if you need example code to easily issue such commands from kernel.

Boaz

Re: [PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[Elias Oltmanns]

Boaz Harrosh <bharrosh@panasas.com> wrote:
> On Wed, Mar 26 2008 at 16:23 +0200, Elias Oltmanns <eo@nebensachen.de> wrote:
>> Elias Oltmanns <eo@nebensachen.de> wrote:
>>> Hi all,
>>>
>>> this patch adds support for REQ_TYPE_LINUX_BLOCK requests to the scsi
>>> midlayer. Low level drivers have the option to register their own
>>> handlers for these special requests if necessary.
>>>
>> [...]
>>> +static void scsi_finish_lb_req(struct request *req)
>>> +{
>>> +	struct request_queue *q = req->q;
>>> +	struct scsi_device *sdev = q->queuedata;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(q->queue_lock, flags);
>>> +	end_that_request_last(req, 1);
>> 
>> That's obsolete, of course. Sorry for missing that. See the correct
>> patch for 2.6.25-rc7 below.
>> 
>> Regards,
>> 
>> Elias
>> 
>> 
>
> It looks to me like you can accomplish any of that (and more) with
> regular BLOCK_PC commands plus the varlen facility I have sent:
> (http://www.spinics.net/lists/linux-scsi/msg25202.html)

Thanks for the hint, the varlen facility certainly is a nice
enhancement. However, I wonder whether this really is a replacement for
REQ_TYPE_LINUX_BLOCK requests. This is a linux specific type of requests
intended to be used as generic block layer messages. In my patch set,
for instance, a REQ_TYPE_LINUX_BLOCK request is enqueued to notify LLDs
that the queue has just been frozen / thawed. The block layer doesn't
really care (or know, for that matter) whether the device is an SCSI,
IDE, or an loop device. Thus, only the LLD itself can decide what kind
of commands (if any) have to be executed in response to the generic
block layer message.

Of course, the same can be achieved by means of the varlen facility if
we could, for instance, reserve a certain range of codes in the service
action of a variable length CDB for this kind of requests. Is that what
you had in mind?

[...]
> BLOCK_PC commands with-or-without data, are always completed at once.

??? At once, as opposed to what? For all I know, they are enqueued for
asynchronous completion.

>
> The LLD in question will only need to filter for those special commands
> at the .queuecommand entry and act accordingly.
>
> The only problem you might have is with a dumb initiator that might issue
> commands to devices that do not know what to do with the new none-standard
> commands. There is 3 things you can do.

This will be a common situation and we are not just talking about scsi
devices either.

> 1. Make the Initiator smarter to only send these commands to good
>    devices In a manner of a special flag or a registration process.

We wanted to avoid just that.

> 2. use commands that are bigger than 16 so .max_cmd_len of legacy
>    drivers will not allow these commands through.
> 3. Do nothing and let the setup process only setup the compatible
>    devices to be issued the new commands.
>
> The bsg driver can already be used to issue such commands from user space.
> Tell me if you need example code to easily issue such commands from kernel.

If you still think that all of the above can (and should) be
accomplished using BLOCK_PC commands and the varlen facility, I'd be
interested in some example code.

Regards,

Elias

Re: [PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[Boaz Harrosh]

On Fri, Mar 28 2008 at 14:43 +0300, Elias Oltmanns <eo@nebensachen.de> wrote:
> Boaz Harrosh <bharrosh@panasas.com> wrote:
>> On Wed, Mar 26 2008 at 16:23 +0200, Elias Oltmanns <eo@nebensachen.de> wrote:
>>> Elias Oltmanns <eo@nebensachen.de> wrote:
>>>> Hi all,
>>>>
>>>> this patch adds support for REQ_TYPE_LINUX_BLOCK requests to the scsi
>>>> midlayer. Low level drivers have the option to register their own
>>>> handlers for these special requests if necessary.
>>>>
>>> [...]
>>>> +static void scsi_finish_lb_req(struct request *req)
>>>> +{
>>>> +	struct request_queue *q = req->q;
>>>> +	struct scsi_device *sdev = q->queuedata;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(q->queue_lock, flags);
>>>> +	end_that_request_last(req, 1);
>>> That's obsolete, of course. Sorry for missing that. See the correct
>>> patch for 2.6.25-rc7 below.
>>>
>>> Regards,
>>>
>>> Elias
>>>
>>>
>> It looks to me like you can accomplish any of that (and more) with
>> regular BLOCK_PC commands plus the varlen facility I have sent:
>> (http://www.spinics.net/lists/linux-scsi/msg25202.html)
> 
> Thanks for the hint, the varlen facility certainly is a nice
> enhancement. However, I wonder whether this really is a replacement for
> REQ_TYPE_LINUX_BLOCK requests. This is a linux specific type of requests
> intended to be used as generic block layer messages. In my patch set,
> for instance, a REQ_TYPE_LINUX_BLOCK request is enqueued to notify LLDs
> that the queue has just been frozen / thawed. The block layer doesn't
> really care (or know, for that matter) whether the device is an SCSI,
> IDE, or an loop device. Thus, only the LLD itself can decide what kind
> of commands (if any) have to be executed in response to the generic
> block layer message.

You do have the same problem with you approach, and from what it looks
you have audited all users and changed them. Wherever a block device
did "if (REQ_TYPE_BLOCK_PC)" and so on, you added another else-if case 
and took care of the new type. You can just do the same but the 
if (linux_special_somthing()) will look inside the cdb now.
So you have not gained but have not lost on any particular block LLD.

In scsi however you can save a lot. By doing almost nothing, save the
special flag at host template. (See below)

> 
> Of course, the same can be achieved by means of the varlen facility if
> we could, for instance, reserve a certain range of codes in the service
> action of a variable length CDB for this kind of requests. Is that what
> you had in mind?
> 

You can easily brute force it. But you could also use the special format
and range the scsi standard reserves for vendor specific commands - Linux
vendor specific commands - You might even want to publish this command set
and then they can have a special status of: publicly available vendor specific
set.

If you decide to go the standard scsi way I'll point you to the right place in
the documentation.

> [...]
>> BLOCK_PC commands with-or-without data, are always completed at once.
> 
> ??? At once, as opposed to what? For all I know, they are enqueued for
> asynchronous completion.
> 

I mean, as opposed to FS_PC where a long request can be split into few
scsi-commands. BLOCK_PC is executed as one opaque piece.

>> The LLD in question will only need to filter for those special commands
>> at the .queuecommand entry and act accordingly.
>>
>> The only problem you might have is with a dumb initiator that might issue
>> commands to devices that do not know what to do with the new none-standard
>> commands. There is 3 things you can do.
> 
> This will be a common situation and we are not just talking about scsi
> devices either.
> 
>> 1. Make the Initiator smarter to only send these commands to good
>>    devices In a manner of a special flag or a registration process.
> 
> We wanted to avoid just that.

I did not see the complete patchset. But in the scsi patchset you did
just that. You have added a new call vector - lb_request_fn - at host
template. What I suggest is a bit-flag at host template of lets say,
.support_linux_specific_cmnd the transport is exactly the same as 
BLOCK_PC but a simple if will not send these commands to scsi LLDS that 
do not support them.

Something similar could be done for other block devices look at the the
QUEUE_FLAG_BIDI of how bsg driver checks if to allow bidi commands to
devices. However there is a safety mechanism to my block layer varlen
commands, that if a legacy driver is untouched, the varlen command will 
look like a zero length command, most drivers already just ignore these.
(OK with spiting some messages at times).

> 
>> 2. use commands that are bigger than 16 so .max_cmd_len of legacy
>>    drivers will not allow these commands through.
>> 3. Do nothing and let the setup process only setup the compatible
>>    devices to be issued the new commands.
>>
>> The bsg driver can already be used to issue such commands from user space.
>> Tell me if you need example code to easily issue such commands from kernel.
> 
> If you still think that all of the above can (and should) be
> accomplished using BLOCK_PC commands and the varlen facility, I'd be
> interested in some example code.
> 

It's easy, see below. In general scsi_libs's *_execute_* are good examples
they just issue commands through the block layer they have no special scsi stuff.
(almost) (also bsg driver is a good example)

> Regards,
> 
> Elias
> --

I think that in overall your code will be much smaller. And it is not hard to
audit all users to make sure they comply. 
You just need to define the right API at block level that says. what are the 
special commands and their format. The blk_lb_request(req) will inspects the ->cmd[] 
as well as the BLOCK_PC type.

If it was me I would define the above to also adhere to the scsi standard by 
defining it as a true scsi varlen / vendor specific command. To avoid any conflicts
with future command-sets. And again if it was me I would make high level helpers that
would encode/decode the different commands for me so if things change, they only 
change in one place.

Tell me if you need any help, and also point me to where you have the complete
body of work, including the initiators that use the code. I will have a look to
make sure every thing is safe.

Boaz

---
/* synchronuse execution of a dataless command */

int execute_nodata_command(struct request_queue *q, u8 *cmnd, ushort cmnd_len,
				int rw, gfp_t flags)
{
	struct request *req;
	u8 sense_buffer[SCSI_SENSE_BUFFERSIZE];
	int ret;

	req = blk_get_request(q, READ, flags); /* No data is a READ */
	if (!req) {
		ERROR("blk_get_request faild\n");
		return -ENOMEM;
	}

	req->cmd_type = REQ_TYPE_BLOCK_PC;
	req->timeout = SOME_TIMEOUT;
	req->retries = SOME_RETRIES;
	req->sense = &sense_buffer;
	req->sense_len = 0;

	/* send a special command that should be safe for
         * drivers that know nothing about these commands
         */
	rq_set_varlen_cdb(req, cmnd, cmnd_len);

	ret = blk_execute_rq(q, NULL, req, 0);
	blk_put_request(req);

	return ret;
}

Re: [PATCH] SCSI support for REQ_TYPE_LINUX_BLOCK requests

[Elias Oltmanns]

Boaz Harrosh <bharrosh@panasas.com> wrote:
> On Fri, Mar 28 2008 at 14:43 +0300, Elias Oltmanns <eo@nebensachen.de> wrote:
>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>> On Wed, Mar 26 2008 at 16:23 +0200, Elias Oltmanns <eo@nebensachen.de> wrote:
>>>> Elias Oltmanns <eo@nebensachen.de> wrote:
>>>>> Hi all,
>>>>>
>>>>> this patch adds support for REQ_TYPE_LINUX_BLOCK requests to the scsi
>>>>> midlayer. Low level drivers have the option to register their own
>>>>> handlers for these special requests if necessary.
>>>>>
>>>> [...]
>>>>> +static void scsi_finish_lb_req(struct request *req)
>>>>> +{
>>>>> +	struct request_queue *q = req->q;
>>>>> +	struct scsi_device *sdev = q->queuedata;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	spin_lock_irqsave(q->queue_lock, flags);
>>>>> +	end_that_request_last(req, 1);
>>>> That's obsolete, of course. Sorry for missing that. See the correct
>>>> patch for 2.6.25-rc7 below.
>>>>
>>>> Regards,
>>>>
>>>> Elias
>>>>
>>>>
>>> It looks to me like you can accomplish any of that (and more) with
>>> regular BLOCK_PC commands plus the varlen facility I have sent:
>>> (http://www.spinics.net/lists/linux-scsi/msg25202.html)
>> 
>> Thanks for the hint, the varlen facility certainly is a nice
>> enhancement. However, I wonder whether this really is a replacement for
>> REQ_TYPE_LINUX_BLOCK requests. This is a linux specific type of requests
>> intended to be used as generic block layer messages. In my patch set,
>> for instance, a REQ_TYPE_LINUX_BLOCK request is enqueued to notify LLDs
>> that the queue has just been frozen / thawed. The block layer doesn't
>> really care (or know, for that matter) whether the device is an SCSI,
>> IDE, or an loop device. Thus, only the LLD itself can decide what kind
>> of commands (if any) have to be executed in response to the generic
>> block layer message.
>
> You do have the same problem with you approach, and from what it looks
> you have audited all users and changed them. Wherever a block device
> did "if (REQ_TYPE_BLOCK_PC)" and so on, you added another else-if case 
> and took care of the new type. You can just do the same but the 
> if (linux_special_somthing()) will look inside the cdb now.
> So you have not gained but have not lost on any particular block LLD.

Jens, would you agree that variable length block pc commands could take
the place of REQ_TYPE_LINUX_BLOCK requests? Please see below.

>
> In scsi however you can save a lot. By doing almost nothing, save the
> special flag at host template. (See below)
>
>> 
>> Of course, the same can be achieved by means of the varlen facility if
>> we could, for instance, reserve a certain range of codes in the service
>> action of a variable length CDB for this kind of requests. Is that what
>> you had in mind?
>> 
>
> You can easily brute force it. But you could also use the special format
> and range the scsi standard reserves for vendor specific commands - Linux
> vendor specific commands - You might even want to publish this command set
> and then they can have a special status of: publicly available vendor specific
> set.

Well, this sounds all very appealing as it would certainly simplify
matters as far as scsi is concerned. There are just two things that
worry me a little:

1. Currently, the only LLDDs I care about are libata and the ide
   subsystem. I wonder whether an *scsi* variable length command, even
   if it is vendor specific, really is the right way to implement an ATA
   specific feature. Then again, a similar feature could pop up in a
   later version of the scsi standards famili, I suppose.
2. If we were going to use vendor specific variable length cdbs as a
   means to issue generic block layer messages, where would be the right
   place to define linux specific variable length cdbs? I don't suppose
   we'd want to load scsi.h in all LLDs of block devices.

>
> If you decide to go the standard scsi way I'll point you to the right
>place in
> the documentation.

It's in the latest scsi primary commands document, isn't it?

>
>> [...]
>>> BLOCK_PC commands with-or-without data, are always completed at once.
>> 
>> ??? At once, as opposed to what? For all I know, they are enqueued for
>> asynchronous completion.
>> 
>
> I mean, as opposed to FS_PC where a long request can be split into few
> scsi-commands. BLOCK_PC is executed as one opaque piece.

Right, I see.

[...]
> I think that in overall your code will be much smaller. And it is not hard to
> audit all users to make sure they comply. 
> You just need to define the right API at block level that says. what are the 
> special commands and their format. The blk_lb_request(req) will inspects the ->cmd[] 
> as well as the BLOCK_PC type.
>
> If it was me I would define the above to also adhere to the scsi standard by 
> defining it as a true scsi varlen / vendor specific command. To avoid any conflicts
> with future command-sets. And again if it was me I would make high level helpers that
> would encode/decode the different commands for me so if things change, they only 
> change in one place.
>
> Tell me if you need any help, and also point me to where you have the complete
> body of work, including the initiators that use the code. I will have a look to
> make sure every thing is safe.

I'm certainly grateful for your offer. At present the most pressing
question on my part is whether Jens will accept variable length cdbs
instead of REQ_TYPE_LINUX_BLOCK and whether this idea doesn't mix block
layer and scsi midlayer too much. James?

Thanks,

Elias