[PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

The patch below implements the following:
* Defines a new internal function __scsi_execute_async(). The only difference
  with the existing function scsi_execute_async() is that this new function
  has an extra parameter, at_head, which allows to specify whether to insert
  the new request at the head or at the tail of the queue.
* Replaces the implementation of scsi_execute_async() by a call to the new
  function __scsi_execute_async(). The behavior of this function is not
  changed.
* Defines a new function scsi_execute_async_fifo() which inserts a request at
  the tail, in FIFO order.
* Adds #define SCSI_EXEC_REQ_FIFO_DEFINED such that out-of-tree kernel
  modules can easily find out whether or not the new function
  scsi_execute_async_fifo() is present.

Signed-off-by: bart.vanassche@gmail.com


diff -uprN -X linux-2.6.25.1/Documentation/dontdiff 
orig/linux-2.6.25.1/drivers/scsi/scsi_lib.c 
linux-2.6.25.1/drivers/scsi/scsi_lib.c
--- orig/linux-2.6.25.1/drivers/scsi/scsi_lib.c	2008-05-01 23:45:25.000000000 
+0200
+++ linux-2.6.25.1/drivers/scsi/scsi_lib.c	2008-05-02 15:26:46.000000000 +0200
@@ -363,7 +363,7 @@ free_bios:
 }
 
 /**
- * scsi_execute_async - insert request
+ * __scsi_execute_async - insert request
  * @sdev:	scsi device
  * @cmd:	scsi command
  * @cmd_len:	length of scsi cdb
@@ -376,11 +376,14 @@ free_bios:
  * @privdata:	data passed to done()
  * @done:	callback function when done
  * @gfp:	memory allocation flags
+ * @at_head:	insert request at head or tail of queue
  */
-int scsi_execute_async(struct scsi_device *sdev, const unsigned char *cmd,
+static inline int __scsi_execute_async(struct scsi_device *sdev,
+		       const unsigned char *cmd,
 		       int cmd_len, int data_direction, void *buffer, unsigned bufflen,
 		       int use_sg, int timeout, int retries, void *privdata,
-		       void (*done)(void *, char *, int, int), gfp_t gfp)
+		       void (*done)(void *, char *, int, int), gfp_t gfp,
+		       int at_head)
 {
 	struct request *req;
 	struct scsi_io_context *sioc;
@@ -417,7 +420,7 @@ int scsi_execute_async(struct scsi_devic
 	sioc->data = privdata;
 	sioc->done = done;
 
-	blk_execute_rq_nowait(req->q, NULL, req, 1, scsi_end_async);
+	blk_execute_rq_nowait(req->q, NULL, req, at_head, scsi_end_async);
 	return 0;
 
 free_req:
@@ -426,8 +429,57 @@ free_sense:
 	kmem_cache_free(scsi_io_context_cache, sioc);
 	return DRIVER_ERROR << 24;
 }
+
+/**
+ * scsi_execute_async - insert request
+ * @sdev:	scsi device
+ * @cmd:	scsi command
+ * @cmd_len:	length of scsi cdb
+ * @data_direction: data direction
+ * @buffer:	data buffer (this can be a kernel buffer or scatterlist)
+ * @bufflen:	len of buffer
+ * @use_sg:	if buffer is a scatterlist this is the number of elements
+ * @timeout:	request timeout in seconds
+ * @retries:	number of times to retry request
+ * @flags:	or into request flags
+ **/
+int scsi_execute_async(struct scsi_device *sdev, const unsigned char *cmd,
+		       int cmd_len, int data_direction, void *buffer,
+		       unsigned bufflen, int use_sg, int timeout, int retries,
+		       void *privdata, void (*done)(void *, char *, int, int),
+		       gfp_t gfp)
+{
+	return __scsi_execute_async(sdev, cmd, cmd_len, data_direction, buffer,
+				    bufflen, use_sg, timeout, retries, privdata,
+				    done, gfp, 1);
+}
 EXPORT_SYMBOL_GPL(scsi_execute_async);
 
+/**
+ * scsi_execute_async_fifo - insert request at tail, in FIFO order
+ * @sdev:	scsi device
+ * @cmd:	scsi command
+ * @cmd_len:	length of scsi cdb
+ * @data_direction: data direction
+ * @buffer:	data buffer (this can be a kernel buffer or scatterlist)
+ * @bufflen:	len of buffer
+ * @use_sg:	if buffer is a scatterlist this is the number of elements
+ * @timeout:	request timeout in seconds
+ * @retries:	number of times to retry request
+ * @flags:	or into request flags
+ **/
+int scsi_execute_async_fifo(struct scsi_device *sdev, const unsigned char 
*cmd,
+			    int cmd_len, int data_direction, void *buffer,
+			    unsigned bufflen, int use_sg, int timeout,
+			    int retries, void *privdata,
+			    void (*done)(void *, char *, int, int), gfp_t gfp)
+{
+	return __scsi_execute_async(sdev, cmd, cmd_len, data_direction, buffer,
+				    bufflen, use_sg, timeout, retries, privdata,
+				    done, gfp, 0);
+}
+EXPORT_SYMBOL_GPL(scsi_execute_async_fifo);
+
 /*
  * Function:    scsi_init_cmd_errh()
  *
diff -uprN -X linux-2.6.25.1/Documentation/dontdiff 
orig/linux-2.6.25.1/include/scsi/scsi_device.h 
linux-2.6.25.1/include/scsi/scsi_device.h
--- orig/linux-2.6.25.1/include/scsi/scsi_device.h	2008-05-01 
23:45:25.000000000 +0200
+++ linux-2.6.25.1/include/scsi/scsi_device.h	2008-05-02 15:28:29.000000000 
+0200
@@ -332,6 +332,14 @@ extern int scsi_execute_async(struct scs
 			      int timeout, int retries, void *privdata,
 			      void (*done)(void *, char *, int, int),
 			      gfp_t gfp);
+#define SCSI_EXEC_REQ_FIFO_DEFINED
+extern int scsi_execute_async_fifo(struct scsi_device *sdev,
+				   const unsigned char *cmd, int cmd_len,
+				   int data_direction, void *buffer,
+				   unsigned bufflen, int use_sg,
+				   int timeout, int retries, void *privdata,
+				   void (*done)(void *, char *, int, int),
+				   gfp_t gfp);
 
 static inline int __must_check scsi_device_reprobe(struct scsi_device *sdev)
 {

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Christoph Hellwig]

NACK.

First scsi_execute_async has a horrible API and thus it's on it's way
out, no way we'll add another variant of it.

Second we're not going to add things to the core just for out of tree
modules.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 5:33 PM, Christoph Hellwig <hch@infradead.org> wrote:
> NACK.
>
> First scsi_execute_async has a horrible API and thus it's on it's way
> out, no way we'll add another variant of it.
>
> Second we're not going to add things to the core just for out of tree
> modules.

Regarding out-of-tree modules: this is just a preparatory step before
submitting SCST for inclusion in the mainstream kernel.

Regarding scsi_execute_async(): I didn't know that this API is on its
way out. What will it be replaced by, and when ?

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Christoph Hellwig]

On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
> Regarding out-of-tree modules: this is just a preparatory step before
> submitting SCST for inclusion in the mainstream kernel.

And what crackpipe did you smoke to thing we'd put duplicated target
framework in?

> Regarding scsi_execute_async(): I didn't know that this API is on its
> way out. What will it be replaced by, and when ?

blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
ontop to build requests.

> 
> Bart.
---end quoted text---

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>> Regarding out-of-tree modules: this is just a preparatory step before
>> submitting SCST for inclusion in the mainstream kernel.
>
> And what crackpipe did you smoke to thing we'd put duplicated target
> framework in?

Why are you so aggressive ? I didn't insult you in any way.

Regarding inclusion of SCSI target code in the mainline, this subject
has already been discussed extensively in the past
(http://lkml.org/lkml/2008/1/23/134). The conclusion was clear: SCST
is faster than any other existing iSCSI target for Linux (IET, STGT,
LIO), stable, well maintained and the most standards compliant target.
Why do you want to reopen this discussion ?

James Bottomley clearly expressed in that thread that he doesn't want
to maintain two SCSI target frameworks. So what I propose is that SCST
is included in the mainline and afterwards that it is evaluated
whether or not it is desirable to keep other target code in the
mainline kernel.

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Matthew Wilcox]

On Fri, May 02, 2008 at 06:06:59PM +0200, Bart Van Assche wrote:
> James Bottomley clearly expressed in that thread that he doesn't want
> to maintain two SCSI target frameworks. So what I propose is that SCST
> is included in the mainline and afterwards that it is evaluated
> whether or not it is desirable to keep other target code in the
> mainline kernel.

That's not the way it works, sorry.

The way to get SCST in is to work with the people who care about target
frameworks (which doesn't include me, fwiw).  You come to a consensus
about the way to proceed.  Normally this will be a gradual migration of
the good bits from SCST into the kernel.  In *exceptional* circumstances,
we've replaced one piece of infrastructure with another (eg wireless
midlayers), but those are by no means the preferred ways to go.

Let me just re-emphasise this bit.  You HAVE to work with the existing
people.  If you can't come to a common understanding, your code won't
get in.  Even if it's better.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[James Bottomley]

On Fri, 2008-05-02 at 18:06 +0200, Bart Van Assche wrote:
> On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
> >> Regarding out-of-tree modules: this is just a preparatory step before
> >> submitting SCST for inclusion in the mainstream kernel.
> >
> > And what crackpipe did you smoke to thing we'd put duplicated target
> > framework in?
> 
> Why are you so aggressive ? I didn't insult you in any way.
> 
> Regarding inclusion of SCSI target code in the mainline, this subject
> has already been discussed extensively in the past
> (http://lkml.org/lkml/2008/1/23/134). The conclusion was clear: SCST
> is faster than any other existing iSCSI target for Linux (IET, STGT,
> LIO), stable, well maintained and the most standards compliant target.
> Why do you want to reopen this discussion ?

That's an interesting rewrite of history.  The evidence you presented
showed fairly identical results apart from on one contrived IB benchmark
that couldn't directly compare the two.

I'm also on record in the thread saying that was insufficient proof for
me to justify throwing STGT out and replacing it with SCST.

> James Bottomley clearly expressed in that thread that he doesn't want
> to maintain two SCSI target frameworks. So what I propose is that SCST
> is included in the mainline and afterwards that it is evaluated
> whether or not it is desirable to keep other target code in the
> mainline kernel.

That's hardly sufficient.  STGT is already in use.  Their either has to
be a migration path or, the preferred option, take the pieces of SCST
that are actual improvements and embed them in STGT.

James

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 6:16 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Fri, May 02, 2008 at 06:06:59PM +0200, Bart Van Assche wrote:
>> James Bottomley clearly expressed in that thread that he doesn't want
>> to maintain two SCSI target frameworks. So what I propose is that SCST
>> is included in the mainline and afterwards that it is evaluated
>> whether or not it is desirable to keep other target code in the
>> mainline kernel.
>
> That's not the way it works, sorry.
>
> The way to get SCST in is to work with the people who care about target
> frameworks (which doesn't include me, fwiw).  You come to a consensus
> about the way to proceed.  Normally this will be a gradual migration of
> the good bits from SCST into the kernel.  In *exceptional* circumstances,
> we've replaced one piece of infrastructure with another (eg wireless
> midlayers), but those are by no means the preferred ways to go.
>
> Let me just re-emphasise this bit.  You HAVE to work with the existing
> people.  If you can't come to a common understanding, your code won't
> get in.  Even if it's better.

Which target code is already in the mainline kernel, and who are the
maintainers for this code ? I checked the MAINTAINERS file in the
2.6.25 tarball, but in this file I could not find the information I
was looking for.

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[James Bottomley]

On Fri, 2008-05-02 at 18:23 +0200, Bart Van Assche wrote:
> On Fri, May 2, 2008 at 6:16 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > On Fri, May 02, 2008 at 06:06:59PM +0200, Bart Van Assche wrote:
> >> James Bottomley clearly expressed in that thread that he doesn't want
> >> to maintain two SCSI target frameworks. So what I propose is that SCST
> >> is included in the mainline and afterwards that it is evaluated
> >> whether or not it is desirable to keep other target code in the
> >> mainline kernel.
> >
> > That's not the way it works, sorry.
> >
> > The way to get SCST in is to work with the people who care about target
> > frameworks (which doesn't include me, fwiw).  You come to a consensus
> > about the way to proceed.  Normally this will be a gradual migration of
> > the good bits from SCST into the kernel.  In *exceptional* circumstances,
> > we've replaced one piece of infrastructure with another (eg wireless
> > midlayers), but those are by no means the preferred ways to go.
> >
> > Let me just re-emphasise this bit.  You HAVE to work with the existing
> > people.  If you can't come to a common understanding, your code won't
> > get in.  Even if it's better.
> 
> Which target code is already in the mainline kernel, and who are the
> maintainers for this code ? I checked the MAINTAINERS file in the
> 2.6.25 tarball, but in this file I could not find the information I
> was looking for.

Um, you claimed to be running benchmarks against it in the thread you
just quoted to push for SCST inclusion.  If you genuinely don't know
which code is actually in the kernel, what exactly were the benchmarks
against?

James

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 6:18 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2008-05-02 at 18:06 +0200, Bart Van Assche wrote:
>> Regarding inclusion of SCSI target code in the mainline, this subject
>> has already been discussed extensively in the past
>> (http://lkml.org/lkml/2008/1/23/134). The conclusion was clear: SCST
>> is faster than any other existing iSCSI target for Linux (IET, STGT,
>> LIO), stable, well maintained and the most standards compliant target.
>> Why do you want to reopen this discussion ?
>
> That's an interesting rewrite of history.  The evidence you presented
> showed fairly identical results apart from on one contrived IB benchmark
> that couldn't directly compare the two.
>
> I'm also on record in the thread saying that was insufficient proof for
> me to justify throwing STGT out and replacing it with SCST.

The trend of the measurements I did was very clear: SCST has a higher
bandwidth and a lower latency than the other three iSCSI
implementations I benchmarked. While this difference is neglectible
for 100 Mbit/s media, the difference is significant for 10 Gbit/s
media. One of the tests I did was with iSCSI via IPoIB. These results
allow direct comparison between STGT and SCST.

>> James Bottomley clearly expressed in that thread that he doesn't want
>> to maintain two SCSI target frameworks. So what I propose is that SCST
>> is included in the mainline and afterwards that it is evaluated
>> whether or not it is desirable to keep other target code in the
>> mainline kernel.
>
> That's hardly sufficient.  STGT is already in use.  Their either has to
> be a migration path or, the preferred option, take the pieces of SCST
> that are actual improvements and embed them in STGT.

In the thread I referred to it has been explained that for optimal
speed and minimum latency a kernelspace implementation is required.
Since most of STGT is implemented in userspace, embedding pieces of
SCST in STGT is not an option.

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 6:30 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> Um, you claimed to be running benchmarks against it in the thread you
> just quoted to push for SCST inclusion.  If you genuinely don't know
> which code is actually in the kernel, what exactly were the benchmarks
> against?

This is getting off-topic. To answer your question: for downloading,
installing and benchmarking an iSCSI target implementation, you don't
have to know the names of any kernel source files. Knowledge of the
name of the relevant kernel module is enough.

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Matthew Wilcox]

On Fri, May 02, 2008 at 06:23:08PM +0200, Bart Van Assche wrote:
> Which target code is already in the mainline kernel, and who are the
> maintainers for this code ? I checked the MAINTAINERS file in the
> 2.6.25 tarball, but in this file I could not find the information I
> was looking for.

Can you really not locate drivers/scsi/scsi_tgt_lib.c in the tree?  I
appreciate that there's a lot of files in the drivers/scsi/ directory,
but c'mon.  I had no idea what it was called and it took about 30
seconds to find them once I realised you wanted all the work to be done
for you.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 6:49 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> Can you really not locate drivers/scsi/scsi_tgt_lib.c in the tree?  I
> appreciate that there's a lot of files in the drivers/scsi/ directory,
> but c'mon.  I had no idea what it was called and it took about 30
> seconds to find them once I realised you wanted all the work to be done
> for you.

Thanks for looking this up. Would it be an acceptable approach if the
SCST code is modified such that it calls the code inside
drivers/scsi/scsi_tgt_lib.c whenever this is appropriate ?

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Matthew Wilcox]

On Fri, May 02, 2008 at 06:57:10PM +0200, Bart Van Assche wrote:
> Thanks for looking this up. Would it be an acceptable approach if the
> SCST code is modified such that it calls the code inside
> drivers/scsi/scsi_tgt_lib.c whenever this is appropriate ?

You need to talk to Mike and Tomo.

>From my point of view, the most important bit is the target layer <->
driver API.  ie the transfer_response() method in the host template.
I'm sure other people have their own preferences though.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

James Bottomley wrote:
> On Fri, 2008-05-02 at 18:06 +0200, Bart Van Assche wrote:
>> On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>> Regarding out-of-tree modules: this is just a preparatory step before
>>>> submitting SCST for inclusion in the mainstream kernel.
>>> And what crackpipe did you smoke to thing we'd put duplicated target
>>> framework in?
>> Why are you so aggressive ? I didn't insult you in any way.
>>
>> Regarding inclusion of SCSI target code in the mainline, this subject
>> has already been discussed extensively in the past
>> (http://lkml.org/lkml/2008/1/23/134). The conclusion was clear: SCST
>> is faster than any other existing iSCSI target for Linux (IET, STGT,
>> LIO), stable, well maintained and the most standards compliant target.
>> Why do you want to reopen this discussion ?
> 
> That's an interesting rewrite of history.  The evidence you presented
> showed fairly identical results apart from on one contrived IB benchmark
> that couldn't directly compare the two.
> 
> I'm also on record in the thread saying that was insufficient proof for
> me to justify throwing STGT out and replacing it with SCST.

James, why do you keep ignoring important points, written by me in that 
e-mail: http://lkml.org/lkml/2008/1/30/178?

Namely:

1. Solid architecture of SCST is inherently more simple than distributed 
user/kernel space processing, when kernel behaves under control of user 
space, used in STGT, and allows to get better results with less effort. 
Better in all aspects: simplicity (hence, maintainability), reliability 
and performance. Linux once made step away from microkernel based design 
and that was for really good reasons.

2. Zero-copy operations with page cache will halve processing latency on 
high speed links, like InfiniBand, and it is impossible to implement 
that in a sane way with STGT approach, while for SCST it can be 
implemented simply and naturally.

Vlad

SCSI target subsystem

[Vladislav Bolkhovitin]

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

James Bottomley wrote:
> On Fri, 2008-05-02 at 18:06 +0200, Bart Van Assche wrote:
>> James Bottomley clearly expressed in that thread that he doesn't want
>> to maintain two SCSI target frameworks. So what I propose is that SCST
>> is included in the mainline and afterwards that it is evaluated
>> whether or not it is desirable to keep other target code in the
>> mainline kernel.
> 
> That's hardly sufficient.  STGT is already in use.  Their either has to
> be a migration path or, the preferred option, take the pieces of SCST
> that are actual improvements and embed them in STGT.

Actually, between SCSI initiator and target subsystems there is almost 
*nothing* in common. This claim, at first glance, looks pretty wrong, 
because both serve SCSI, so they must have a lot common. But look deeper 
and you quickly find out that the majority of functionality as well as 
data they use are dedicated for each subsystem, not shared.

Just look at SCST/qla2x00t/(changes done in the initiator qla2xxx driver 
to support target mode, patch attached): 90% of changes is adding 
callbacks for external target add-on, the rest is support for older, 
than 2.6.17, kernels and sysfs magic. Note, no data are common between 
initiator and target parts in the meaning that they both use them.

Then look at SCST (http://scst.sf.net). It implements complete 
pass-through SCSI support and look how it interacts with initiator SCSI 
subsystem. It calls only 2 functions: FIFO version of 
scsi_execute_async() (original scsi_execute_async() provides 
unacceptable LIFO commands order) and scsi_reset_provider() for task 
management. And there is only one common variable: struct scsi_device. 
That's all! In other storage modes (FILEIO/BLOCKIO) there is nothing 
common with SCSI initiator subsystem at all.

Finally, try to find out in SCST any duplicated functionality.

Now, let's look how SCSI target/initiator integration currently done in 
the kernel. For me it looks pretty artificial. For example, if I make a 
general purpose kernel, for which 1% of users would run target mode, I 
would have to enable as module "SCSI target support" as well as SCSI 
target support for transport attributes. Now 99% of users of my kernel, 
who don't need SCSI target, but need SCSI initiator drivers, would have 
to have scsi_tgt loaded, because transport attribute drivers would 
depend on it:

# lsmod
Module                  Size  Used by
qla2xxx               130844  0
firmware_class          8064  1 qla2xxx
scsi_transport_fc      40900  1 qla2xxx
scsi_tgt               12196  1 scsi_transport_fc
brd                     6924  0
xfs                   511280  1
dm_mirror              24368  0
dm_mod                 51148  1 dm_mirror
uhci_hcd               21400  0
sg                     31784  0
e1000                 114536  0
pcspkr                  3328  0

No target functionality needed, but target mode subsystem is needed. Is 
it a good design?

I wrote all above to support my at first glance shocking conclusion that 
SCSI target subsystem is completely new subsystem of the kernel and it 
should live on its own with its own maintainer! This is the same as with 
the current interaction between SCSI and block subsystems in the kernel: 
SCSI uses block's functionality, but that doesn't mean that block and 
SCSI are the same subsystem.

Thus, how IMHO initiator and target drivers should be written:

  - All initiator drivers should live in the SCSI initiator subsystem 
(aka current SCSI subsystem) only, the same as today.

  - All target drivers should live in the SCSI target subsystem only and 
be either add-ons to initiator drivers, like, e.g., qla2x00t, or be a 
complete driver, like, e.g., iSCSI-SCST.

Vlad


[-- Attachment #2: qla2x00t.diff.gz --]
[-- Type: application/x-gzip, Size: 17294 bytes --]

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

Matthew Wilcox wrote:
> On Fri, May 02, 2008 at 06:57:10PM +0200, Bart Van Assche wrote:
>> Thanks for looking this up. Would it be an acceptable approach if the
>> SCST code is modified such that it calls the code inside
>> drivers/scsi/scsi_tgt_lib.c whenever this is appropriate ?
> 
> You need to talk to Mike and Tomo.
> 
>>From my point of view, the most important bit is the target layer <->
> driver API.  ie the transfer_response() method in the host template.
> I'm sure other people have their own preferences though.

SCST can't use anything from STGT or other SCSI targets support 
functionality of the kernel. But it can integrate inside something from 
transport attributes. See my e-mail with subject "SCSI target subsystem" 
why.

Vlad

Re: SCSI target subsystem

[Bart Van Assche]

On Fri, May 2, 2008 at 8:17 PM, Vladislav Bolkhovitin <vst@vlnb.net> wrote:
> I wrote all above to support my at first glance shocking conclusion that
> SCSI target subsystem is completely new subsystem of the kernel and it
> should live on its own with its own maintainer! This is the same as with the
> current interaction between SCSI and block subsystems in the kernel: SCSI
> uses block's functionality, but that doesn't mean that block and SCSI are
> the same subsystem.

Hello Tomo,

Due to the IET and STGT projects you have a lot of experience with
implementing SCSI target frameworks. What is your opinion about how a
kernel space SCSI target framework should fit in the Linux kernel ?

Bart.

Re: SCSI target subsystem

[Matthew Wilcox]

On Sat, May 03, 2008 at 11:41:41AM +0200, Bart Van Assche wrote:
> On Fri, May 2, 2008 at 8:17 PM, Vladislav Bolkhovitin <vst@vlnb.net> wrote:
> > I wrote all above to support my at first glance shocking conclusion that
> > SCSI target subsystem is completely new subsystem of the kernel and it
> > should live on its own with its own maintainer! This is the same as with the
> > current interaction between SCSI and block subsystems in the kernel: SCSI
> > uses block's functionality, but that doesn't mean that block and SCSI are
> > the same subsystem.
> 
> Hello Tomo,
> 
> Due to the IET and STGT projects you have a lot of experience with
> implementing SCSI target frameworks. What is your opinion about how a
> kernel space SCSI target framework should fit in the Linux kernel ?

Bart, what is your role in the SCST project?  You don't seem to have
contributed any code to it (going by the SVN logs on sourceforge), and
your questions and suggestions seem to be those of someone not familiar
with the code.  If you're not a developer, it might be more helpful for
you to step back and let Vladislav handle this.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: SCSI target subsystem

[Bart Van Assche]

On Sat, May 3, 2008 at 11:53 AM, Matthew Wilcox <matthew@wil.cx> wrote:
> Bart, what is your role in the SCST project?  You don't seem to have
> contributed any code to it (going by the SVN logs on sourceforge), and
> your questions and suggestions seem to be those of someone not familiar
> with the code.  If you're not a developer, it might be more helpful for
> you to step back and let Vladislav handle this.

I'd like to see this discussion focus on the technical issues. Why are
people constantly throwing up political arguments in this discussion ?

To answer your question: as you noticed, I did not yet contribute any
code to the SCST project. I'm an iSCSI/iSER/SRP user. I'd like to have
a SCSI target framework available on Linux that is as fast as
possible, as reliable as possible, as standards compliant as possible,
well maintained and for which all kernel code is integrated in the
mainstream Linux kernel. I'm probably sharing this desire with all
STGT, IET, SCST and LIO users. Do you consider the opinion of SCSI
target framework users irrelevant in this discussion ?

Bart.

Re: SCSI target subsystem

[Matthew Wilcox]

On Sat, May 03, 2008 at 12:39:19PM +0200, Bart Van Assche wrote:
> To answer your question: as you noticed, I did not yet contribute any
> code to the SCST project. I'm an iSCSI/iSER/SRP user. I'd like to have
> a SCSI target framework available on Linux that is as fast as
> possible, as reliable as possible, as standards compliant as possible,
> well maintained and for which all kernel code is integrated in the
> mainstream Linux kernel. I'm probably sharing this desire with all
> STGT, IET, SCST and LIO users. Do you consider the opinion of SCSI
> target framework users irrelevant in this discussion ?

Yes.

Once Vladislav is ready for his project to be merged, let him approach
us and ask.  I don't know if he considers his project ready for merging.
Did he ask you for his help?  It's generally considered rather rude to
merge someone else's code without their consent.  The way you tried to
get it merged was unhelpful and, if Vladislav were less skillful a
communicator, could have actively hindered getting it merged.

The correct way to have done this would have been to start a thread on
the scst mailing list asking whether the developers felt it ready to
merge.  Then *leave it to them*.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: SCSI target subsystem

[Bart Van Assche]

On Sat, May 3, 2008 at 3:28 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> Once Vladislav is ready for his project to be merged, let him approach
> us and ask.  I don't know if he considers his project ready for merging.
> Did he ask you for his help?  It's generally considered rather rude to
> merge someone else's code without their consent.  The way you tried to
> get it merged was unhelpful and, if Vladislav were less skillful a
> communicator, could have actively hindered getting it merged.

You'd better verify your assumptions before you take them for truth. I
asked Vladislav for permission by private e-mail before I posted the
scsi_execute_async_fifo() patch. Did try I to merge this code without
Vladislav's consent ? No. Anyone can see that I kept Vladislav and the
scst-devel mailing list in CC.

Let us return to the actual topic of this thread. Does the above mean
that you agree with inclusion of SCST in the mainline kernel ? It
still has to be decided who will maintain the mainline kernel SCSI
target code. In the past James wrote that he does not really have the
time for this. Until now the only one who volunteered to maintain such
a subsystem is Vladislav.

Bart.

Re: SCSI target subsystem

[Vladislav Bolkhovitin]

Matthew Wilcox wrote:
> On Sat, May 03, 2008 at 11:41:41AM +0200, Bart Van Assche wrote:
>> On Fri, May 2, 2008 at 8:17 PM, Vladislav Bolkhovitin <vst@vlnb.net> wrote:
>>> I wrote all above to support my at first glance shocking conclusion that
>>> SCSI target subsystem is completely new subsystem of the kernel and it
>>> should live on its own with its own maintainer! This is the same as with the
>>> current interaction between SCSI and block subsystems in the kernel: SCSI
>>> uses block's functionality, but that doesn't mean that block and SCSI are
>>> the same subsystem.
>> Hello Tomo,
>>
>> Due to the IET and STGT projects you have a lot of experience with
>> implementing SCSI target frameworks. What is your opinion about how a
>> kernel space SCSI target framework should fit in the Linux kernel ?
> 
> Bart, what is your role in the SCST project?  You don't seem to have
> contributed any code to it (going by the SVN logs on sourceforge), and
> your questions and suggestions seem to be those of someone not familiar
> with the code.  If you're not a developer, it might be more helpful for
> you to step back and let Vladislav handle this.

Consider Bart as a member of SCST team at the moment responsible for 
Linux kernel inclusion.

Vlad

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

Christoph Hellwig wrote:
> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>> Regarding scsi_execute_async(): I didn't know that this API is on its
>> way out. What will it be replaced by, and when ?
> 
> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
> ontop to build requests.

scsi_execute_async() is already a nice and simple helper function on top 
blk_execute_rq_nowait(). What's the point then to remove it? Do you 
consider that exposing scsi_execute_async() internals to its users is 
better?

>> Bart.
> ---end quoted text---
> 

Re: SCSI target subsystem

[Vladislav Bolkhovitin]

Vladislav Bolkhovitin wrote:
> James Bottomley wrote:
>> On Fri, 2008-05-02 at 18:06 +0200, Bart Van Assche wrote:
>>> James Bottomley clearly expressed in that thread that he doesn't want
>>> to maintain two SCSI target frameworks. So what I propose is that SCST
>>> is included in the mainline and afterwards that it is evaluated
>>> whether or not it is desirable to keep other target code in the
>>> mainline kernel.
>> That's hardly sufficient.  STGT is already in use.  Their either has to
>> be a migration path or, the preferred option, take the pieces of SCST
>> that are actual improvements and embed them in STGT.
> 
> Actually, between SCSI initiator and target subsystems there is almost 
> *nothing* in common. This claim, at first glance, looks pretty wrong, 
> because both serve SCSI, so they must have a lot common. But look deeper 
> and you quickly find out that the majority of functionality as well as 
> data they use are dedicated for each subsystem, not shared.
> 
> Just look at SCST/qla2x00t/(changes done in the initiator qla2xxx driver 
> to support target mode, patch attached): 90% of changes is adding 
> callbacks for external target add-on, the rest is support for older, 
> than 2.6.17, kernels and sysfs magic. Note, no data are common between 
> initiator and target parts in the meaning that they both use them.

Perhaps, I should elaborate more on this to eliminate possible 
misunderstanding. Of course, both main initiator driver and target 
add-on driver directly use the same hardware, so they share all internal 
hardware-related data, e.g. hardware_lock, but this doesn't matter for 
our topic, because this sharing is on the different level. All such data 
are hardware specific, hence different hardware have different sets of 
such data, hence it is impractical to find something common in them to 
expose as a common interface, which all initiator drivers should expose 
to its target add-ons: the interface would be more complicated than 
direct implementation in each particular case.

> Then look at SCST (http://scst.sf.net). It implements complete 
> pass-through SCSI support and look how it interacts with initiator SCSI 
> subsystem. It calls only 2 functions: FIFO version of 
> scsi_execute_async() (original scsi_execute_async() provides 
> unacceptable LIFO commands order) and scsi_reset_provider() for task 
> management. And there is only one common variable: struct scsi_device. 
> That's all! In other storage modes (FILEIO/BLOCKIO) there is nothing 
> common with SCSI initiator subsystem at all.
> 
> Finally, try to find out in SCST any duplicated functionality.
> 
> Now, let's look how SCSI target/initiator integration currently done in 
> the kernel. For me it looks pretty artificial. For example, if I make a 
> general purpose kernel, for which 1% of users would run target mode, I 
> would have to enable as module "SCSI target support" as well as SCSI 
> target support for transport attributes. Now 99% of users of my kernel, 
> who don't need SCSI target, but need SCSI initiator drivers, would have 
> to have scsi_tgt loaded, because transport attribute drivers would 
> depend on it:
> 
> # lsmod
> Module                  Size  Used by
> qla2xxx               130844  0
> firmware_class          8064  1 qla2xxx
> scsi_transport_fc      40900  1 qla2xxx
> scsi_tgt               12196  1 scsi_transport_fc
> brd                     6924  0
> xfs                   511280  1
> dm_mirror              24368  0
> dm_mod                 51148  1 dm_mirror
> uhci_hcd               21400  0
> sg                     31784  0
> e1000                 114536  0
> pcspkr                  3328  0
> 
> No target functionality needed, but target mode subsystem is needed. Is 
> it a good design?
> 
> I wrote all above to support my at first glance shocking conclusion that 
> SCSI target subsystem is completely new subsystem of the kernel and it 
> should live on its own with its own maintainer! This is the same as with 
> the current interaction between SCSI and block subsystems in the kernel: 
> SCSI uses block's functionality, but that doesn't mean that block and 
> SCSI are the same subsystem.
> 
> Thus, how IMHO initiator and target drivers should be written:
> 
>   - All initiator drivers should live in the SCSI initiator subsystem 
> (aka current SCSI subsystem) only, the same as today.
> 
>   - All target drivers should live in the SCSI target subsystem only and 
> be either add-ons to initiator drivers, like, e.g., qla2x00t, or be a 
> complete driver, like, e.g., iSCSI-SCST.
> 
> Vlad
> 

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>> Regarding scsi_execute_async(): I didn't know that this API is on its
>> way out. What will it be replaced by, and when ?
>
> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
> ontop to build requests.

Source reading learned me that the function scsi_execute_async() calls
blk_execute_rq_nowait() with 1 as fourth argument, which means that
the request is inserted at the head of the queue. This means that
requests queued with scsi_execute_async() are executed before other
queued requests, and that this function has LIFO (last in first out)
semantics.

All non-SCSI calls to blk_execute_rq() / blk_execute_rq_nowait() add
requests at the end of the queue (except those calls for terminating
I/O).

What is the background of this special behavior of the SCSI subsystem ?

Bart.

Re: SCSI target subsystem

[Bart Van Assche]

On Sat, May 3, 2008 at 12:39 PM, Bart Van Assche
<bart.vanassche@gmail.com> wrote:
> To answer your question: as you noticed, I did not yet contribute any
> code to the SCST project. I'm an iSCSI/iSER/SRP user. I'd like to have
> a SCSI target framework available on Linux that is as fast as
> possible, as reliable as possible, as standards compliant as possible,
> well maintained and for which all kernel code is integrated in the
> mainstream Linux kernel. I'm probably sharing this desire with all
> STGT, IET, SCST and LIO users.

Hello Matthew,

To be complete I should have mentioned that there already exists an
FCoE target implementation for SCST. I'm still learning about FCoE.
What I read about it is that FCoE has more to offer with regard to
storage management than iSCSI/iSER/SRP ?

See also: http://www.open-fcoe.org/openfc/wiki/index.php/Quickstart

Bart.

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Boaz Harrosh]

Vladislav Bolkhovitin wrote:
> Christoph Hellwig wrote:
>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>> way out. What will it be replaced by, and when ?
>>
>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>> ontop to build requests.
> 
> scsi_execute_async() is already a nice and simple helper function on top 
> blk_execute_rq_nowait(). What's the point then to remove it? Do you 
> consider that exposing scsi_execute_async() internals to its users is 
> better?
> 

The problem with it is the use of sg list to *hack* in bio's. Which totally 
ignores/duplicates block layer mechanisms. There is pending a large patchset
that removes the use of scsi_execute_async from sg/sr and friends to use blk_map_*
members and directly call blk_execute_*. The original patchset was written by
Mike Christie but is now brought up to date by Tomo. It should be submitted soon I think.
If you need good example of usage check out bsg.c it maps user-space data in all kind
of combinations. If you have kernel space memory it is even simpler.

>>> Bart.
>> ---end quoted text---
>>
> 

If you need any help, please feel free to ask.

Boaz

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Bart Van Assche]

On Sun, May 4, 2008 at 5:30 PM, Bart Van Assche
<bart.vanassche@gmail.com> wrote:
> On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>> way out. What will it be replaced by, and when ?
>>
>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>> ontop to build requests.
>
> Source reading learned me that the function scsi_execute_async() calls
> blk_execute_rq_nowait() with 1 as fourth argument, which means that
> the request is inserted at the head of the queue. This means that
> requests queued with scsi_execute_async() are executed before other
> queued requests, and that this function has LIFO (last in first out)
> semantics.
>
> All non-SCSI calls to blk_execute_rq() / blk_execute_rq_nowait() add
> requests at the end of the queue (except those calls for terminating
> I/O).
>
> What is the background of this special behavior of the SCSI subsystem ?

Is the linux-scsi mailing list the appropriate mailing list to post
questions like the above ?

Bart.

Re: [Scst-devel] [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Arne Redlich]

Am Donnerstag, den 08.05.2008, 17:02 +0200 schrieb Bart Van Assche:
> On Sun, May 4, 2008 at 5:30 PM, Bart Van Assche
> <bart.vanassche@gmail.com> wrote:
> > On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
> >>> Regarding scsi_execute_async(): I didn't know that this API is on its
> >>> way out. What will it be replaced by, and when ?
> >>
> >> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
> >> ontop to build requests.
> >
> > Source reading learned me that the function scsi_execute_async() calls
> > blk_execute_rq_nowait() with 1 as fourth argument, which means that
> > the request is inserted at the head of the queue. This means that
> > requests queued with scsi_execute_async() are executed before other
> > queued requests, and that this function has LIFO (last in first out)
> > semantics.
> >
> > All non-SCSI calls to blk_execute_rq() / blk_execute_rq_nowait() add
> > requests at the end of the queue (except those calls for terminating
> > I/O).
> >
> > What is the background of this special behavior of the SCSI subsystem ?
> 
> Is the linux-scsi mailing list the appropriate mailing list to post
> questions like the above ?

I guess this should
 http://marc.info/?l=linux-scsi&m=116668527804801&w=2
answer your question.

HTH,
Arne

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

Arne Redlich wrote:
> Am Donnerstag, den 08.05.2008, 17:02 +0200 schrieb Bart Van Assche:
>> On Sun, May 4, 2008 at 5:30 PM, Bart Van Assche
>> <bart.vanassche@gmail.com> wrote:
>>> On Fri, May 2, 2008 at 5:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
>>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>>>> way out. What will it be replaced by, and when ?
>>>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>>>> ontop to build requests.
>>> Source reading learned me that the function scsi_execute_async() calls
>>> blk_execute_rq_nowait() with 1 as fourth argument, which means that
>>> the request is inserted at the head of the queue. This means that
>>> requests queued with scsi_execute_async() are executed before other
>>> queued requests, and that this function has LIFO (last in first out)
>>> semantics.
>>>
>>> All non-SCSI calls to blk_execute_rq() / blk_execute_rq_nowait() add
>>> requests at the end of the queue (except those calls for terminating
>>> I/O).
>>>
>>> What is the background of this special behavior of the SCSI subsystem ?
>> Is the linux-scsi mailing list the appropriate mailing list to post
>> questions like the above ?
> 
> I guess this should
>  http://marc.info/?l=linux-scsi&m=116668527804801&w=2
> answer your question.

Thanks for pointing on it. There were another threads about that, e.g. 
http://thread.gmane.org/gmane.linux.scsi/16498/focus=16784. So, there 
was an agreement that the current LIFO requests submission behavior is 
inappropriate and a good solution will be to add to scsi_execute_async() 
one more argument to specify where to insert the request, in the tail or 
in the head of the queue. The patch Bart sent basically implements 
exactly that, with the only difference that the new argument passed 
explicitly via new function scsi_execute_async_fifo(). There only thing 
left is to update via another patch other users of scsi_execute_async() 
to use new function, where necessary. Correct?

I wouldn't mind if that will be done after the pending patchset removing 
scsi_execute_async() will be applied for new scsi_execute_async() 
substitute function. But it seems this patchset is pending for pretty 
long time (a half a year? Or even more?). So, what does prevent it from 
being applied? How long more are we going to wait for it?

Vlad

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

Boaz Harrosh wrote:
> Vladislav Bolkhovitin wrote:
>> Christoph Hellwig wrote:
>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>>> way out. What will it be replaced by, and when ?
>>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>>> ontop to build requests.
>> scsi_execute_async() is already a nice and simple helper function on top 
>> blk_execute_rq_nowait(). What's the point then to remove it? Do you 
>> consider that exposing scsi_execute_async() internals to its users is 
>> better?
>>
> 
> The problem with it is the use of sg list to *hack* in bio's. Which totally 
> ignores/duplicates block layer mechanisms. There is pending a large patchset
> that removes the use of scsi_execute_async from sg/sr and friends to use blk_map_*
> members and directly call blk_execute_*. The original patchset was written by
> Mike Christie but is now brought up to date by Tomo. It should be submitted soon I think.
> If you need good example of usage check out bsg.c it maps user-space data in all kind
> of combinations. If you have kernel space memory it is even simpler.

Thanks for pointing on it. But it still remained unclear for me what's 
the point in the scsi_execute_async() removal. Function 
scsi_req_map_sg() looks pretty simple and straightforward, so I don't 
see how the overall code can be simplified.

>>>> Bart.
>>> ---end quoted text---
>>>
> 
> If you need any help, please feel free to ask.
> 
> Boaz
> 
> 

Re: [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Boaz Harrosh]

Vladislav Bolkhovitin wrote:
> Boaz Harrosh wrote:
>> Vladislav Bolkhovitin wrote:
>>> Christoph Hellwig wrote:
>>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>>>> way out. What will it be replaced by, and when ?
>>>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>>>> ontop to build requests.
>>> scsi_execute_async() is already a nice and simple helper function on top 
>>> blk_execute_rq_nowait(). What's the point then to remove it? Do you 
>>> consider that exposing scsi_execute_async() internals to its users is 
>>> better?
>>>
>> The problem with it is the use of sg list to *hack* in bio's. Which totally 
>> ignores/duplicates block layer mechanisms. There is pending a large patchset
>> that removes the use of scsi_execute_async from sg/sr and friends to use blk_map_*
>> members and directly call blk_execute_*. The original patchset was written by
>> Mike Christie but is now brought up to date by Tomo. It should be submitted soon I think.
>> If you need good example of usage check out bsg.c it maps user-space data in all kind
>> of combinations. If you have kernel space memory it is even simpler.
> 
> Thanks for pointing on it. But it still remained unclear for me what's 
> the point in the scsi_execute_async() removal. Function 
> scsi_req_map_sg() looks pretty simple and straightforward, so I don't 
> see how the overall code can be simplified.
> 

Well No, scsi_req_map_sg() is a complete hack. If you have user memory
or kernel memory you better go through blk_map_* which will take care of
device masks, alignment and all, where here the ULD does that. So you have
2 places of waisted code both at ULD to build the SG right, and here to
translate SG to BIO. Where at block layer you have one function call.
Try it out you see that not using scsi_execute_async() is much more simple 
at ULD then using it.
If you do mmap then Tomo has code for block layer to support that.

>>>>> Bart.
>>>> ---end quoted text---
>>>>
>> If you need any help, please feel free to ask.
>>

Boaz

Re: [Scst-devel] [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

Boaz Harrosh wrote:
> Vladislav Bolkhovitin wrote:
>> Boaz Harrosh wrote:
>>> Vladislav Bolkhovitin wrote:
>>>> Christoph Hellwig wrote:
>>>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>>>>> way out. What will it be replaced by, and when ?
>>>>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>>>>> ontop to build requests.
>>>> scsi_execute_async() is already a nice and simple helper function on top 
>>>> blk_execute_rq_nowait(). What's the point then to remove it? Do you 
>>>> consider that exposing scsi_execute_async() internals to its users is 
>>>> better?
>>>>
>>> The problem with it is the use of sg list to *hack* in bio's. Which totally 
>>> ignores/duplicates block layer mechanisms. There is pending a large patchset
>>> that removes the use of scsi_execute_async from sg/sr and friends to use blk_map_*
>>> members and directly call blk_execute_*. The original patchset was written by
>>> Mike Christie but is now brought up to date by Tomo. It should be submitted soon I think.
>>> If you need good example of usage check out bsg.c it maps user-space data in all kind
>>> of combinations. If you have kernel space memory it is even simpler.
>> Thanks for pointing on it. But it still remained unclear for me what's 
>> the point in the scsi_execute_async() removal. Function 
>> scsi_req_map_sg() looks pretty simple and straightforward, so I don't 
>> see how the overall code can be simplified.
>>
> 
> Well No, scsi_req_map_sg() is a complete hack. If you have user memory
> or kernel memory you better go through blk_map_* which will take care of
> device masks, alignment and all, where here the ULD does that. So you have
> 2 places of waisted code both at ULD to build the SG right, and here to
> translate SG to BIO. Where at block layer you have one function call.
> Try it out you see that not using scsi_execute_async() is much more simple 
> at ULD then using it.
> If you do mmap then Tomo has code for block layer to support that.

Seems, I'm starting understanding you. You mean that all ULDs (User 
Level Devices, i.e. sg, st, etc.) deal with user supplied buffers, i.e. 
pointers to virtually continuous memory, which at the moment it has to 
convert to SG vector, which then will be translated to BIOs for the 
corresponding LDD by scsi_execute_async() (and then back to SG vector on 
the queuecommand() time). So, it will be simpler to supply that buffer 
pointer directly to block functions. Correct?

But the problem is that in SCST in each data transfer 2 LDDs 
participate: one target and one backstorage (initiator). And the target 
LDD deals with SG vectors only. So, SCST never deals with buffers, it 
always deals with SG vectors and pass them between target LDDs and 
backstorage as necessary.

Thus, looks like for SCST in the pass-through mode there is no 
alternative to scsi_execute_async().

Thanks,
Vlad

Re: [Scst-devel] [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Boaz Harrosh]

Vladislav Bolkhovitin wrote:
> Boaz Harrosh wrote:
>> Vladislav Bolkhovitin wrote:
>>> Boaz Harrosh wrote:
>>>> Vladislav Bolkhovitin wrote:
>>>>> Christoph Hellwig wrote:
>>>>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>>>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>>>>>> way out. What will it be replaced by, and when ?
>>>>>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>>>>>> ontop to build requests.
>>>>> scsi_execute_async() is already a nice and simple helper function on top 
>>>>> blk_execute_rq_nowait(). What's the point then to remove it? Do you 
>>>>> consider that exposing scsi_execute_async() internals to its users is 
>>>>> better?
>>>>>
>>>> The problem with it is the use of sg list to *hack* in bio's. Which totally 
>>>> ignores/duplicates block layer mechanisms. There is pending a large patchset
>>>> that removes the use of scsi_execute_async from sg/sr and friends to use blk_map_*
>>>> members and directly call blk_execute_*. The original patchset was written by
>>>> Mike Christie but is now brought up to date by Tomo. It should be submitted soon I think.
>>>> If you need good example of usage check out bsg.c it maps user-space data in all kind
>>>> of combinations. If you have kernel space memory it is even simpler.
>>> Thanks for pointing on it. But it still remained unclear for me what's 
>>> the point in the scsi_execute_async() removal. Function 
>>> scsi_req_map_sg() looks pretty simple and straightforward, so I don't 
>>> see how the overall code can be simplified.
>>>
>> Well No, scsi_req_map_sg() is a complete hack. If you have user memory
>> or kernel memory you better go through blk_map_* which will take care of
>> device masks, alignment and all, where here the ULD does that. So you have
>> 2 places of waisted code both at ULD to build the SG right, and here to
>> translate SG to BIO. Where at block layer you have one function call.
>> Try it out you see that not using scsi_execute_async() is much more simple 
>> at ULD then using it.
>> If you do mmap then Tomo has code for block layer to support that.
> 
> Seems, I'm starting understanding you. You mean that all ULDs (User 
> Level Devices, i.e. sg, st, etc.) deal with user supplied buffers, i.e. 
> pointers to virtually continuous memory, which at the moment it has to 
> convert to SG vector, which then will be translated to BIOs for the 
> corresponding LDD by scsi_execute_async() (and then back to SG vector on 
> the queuecommand() time). So, it will be simpler to supply that buffer 
> pointer directly to block functions. Correct?
> 
> But the problem is that in SCST in each data transfer 2 LDDs 
> participate: one target and one backstorage (initiator). And the target 
> LDD deals with SG vectors only. So, SCST never deals with buffers, it 
> always deals with SG vectors and pass them between target LDDs and 
> backstorage as necessary.

Where is that SG coming from? is it from Network stack? or is it an
SG prepared by scsi-ml with call to blk_rq_map_sg()?

> 
> Thus, looks like for SCST in the pass-through mode there is no 
> alternative to scsi_execute_async().
> 

Tomo what do you think is it logical to add a blk function that will
accept an SG list and map it to a request. Like, for example, an SG 
from Network? opposite of what blk_rq_map_sg does.

> Thanks,
> Vlad
> 

Boaz

Re: [Scst-devel] [PATCH 2.6.25.1] Add scsi_execute_async_fifo()

[Vladislav Bolkhovitin]

Boaz Harrosh wrote:
> Vladislav Bolkhovitin wrote:
>> Boaz Harrosh wrote:
>>> Vladislav Bolkhovitin wrote:
>>>> Boaz Harrosh wrote:
>>>>> Vladislav Bolkhovitin wrote:
>>>>>> Christoph Hellwig wrote:
>>>>>>> On Fri, May 02, 2008 at 05:53:22PM +0200, Bart Van Assche wrote:
>>>>>>>> Regarding scsi_execute_async(): I didn't know that this API is on its
>>>>>>>> way out. What will it be replaced by, and when ?
>>>>>>> blk_execute_rq/blk_execute_rq_nowait plus the block level helpers built
>>>>>>> ontop to build requests.
>>>>>> scsi_execute_async() is already a nice and simple helper function on top 
>>>>>> blk_execute_rq_nowait(). What's the point then to remove it? Do you 
>>>>>> consider that exposing scsi_execute_async() internals to its users is 
>>>>>> better?
>>>>>>
>>>>> The problem with it is the use of sg list to *hack* in bio's. Which totally 
>>>>> ignores/duplicates block layer mechanisms. There is pending a large patchset
>>>>> that removes the use of scsi_execute_async from sg/sr and friends to use blk_map_*
>>>>> members and directly call blk_execute_*. The original patchset was written by
>>>>> Mike Christie but is now brought up to date by Tomo. It should be submitted soon I think.
>>>>> If you need good example of usage check out bsg.c it maps user-space data in all kind
>>>>> of combinations. If you have kernel space memory it is even simpler.
>>>> Thanks for pointing on it. But it still remained unclear for me what's 
>>>> the point in the scsi_execute_async() removal. Function 
>>>> scsi_req_map_sg() looks pretty simple and straightforward, so I don't 
>>>> see how the overall code can be simplified.
>>>>
>>> Well No, scsi_req_map_sg() is a complete hack. If you have user memory
>>> or kernel memory you better go through blk_map_* which will take care of
>>> device masks, alignment and all, where here the ULD does that. So you have
>>> 2 places of waisted code both at ULD to build the SG right, and here to
>>> translate SG to BIO. Where at block layer you have one function call.
>>> Try it out you see that not using scsi_execute_async() is much more simple 
>>> at ULD then using it.
>>> If you do mmap then Tomo has code for block layer to support that.
>> Seems, I'm starting understanding you. You mean that all ULDs (User 
>> Level Devices, i.e. sg, st, etc.) deal with user supplied buffers, i.e. 
>> pointers to virtually continuous memory, which at the moment it has to 
>> convert to SG vector, which then will be translated to BIOs for the 
>> corresponding LDD by scsi_execute_async() (and then back to SG vector on 
>> the queuecommand() time). So, it will be simpler to supply that buffer 
>> pointer directly to block functions. Correct?
>>
>> But the problem is that in SCST in each data transfer 2 LDDs 
>> participate: one target and one backstorage (initiator). And the target 
>> LDD deals with SG vectors only. So, SCST never deals with buffers, it 
>> always deals with SG vectors and pass them between target LDDs and 
>> backstorage as necessary.
> 
> Where is that SG coming from? is it from Network stack? or is it an
> SG prepared by scsi-ml with call to blk_rq_map_sg()?

It's prepared by SCST core. A request comes from target LDD and reply 
finally sent there. Everything goes in the kernel mode only. It is 
simple pass-through, exactly as it sounds.

>> Thus, looks like for SCST in the pass-through mode there is no 
>> alternative to scsi_execute_async().
>>
> 
> Tomo what do you think is it logical to add a blk function that will
> accept an SG list and map it to a request. Like, for example, an SG 
> from Network? opposite of what blk_rq_map_sg does.
> 
>> Thanks,
>> Vlad
>>
> 
> Boaz
>