[PATCH resend] bsg: Add support for submitting requests at tail of queue

[PATCH resend] bsg: Add support for submitting requests at tail of queue

[Boaz Harrosh]

Hi Tomo Jens

Tomo you never ack-by on this patch. I absolutely needs this for the
user-mode API of osd-initiator. Which is needed with up-coming exofs
utilities.

What do you want to do?

Thanks 

---
From: Boaz Harrosh <bharrosh@panasas.com>

Currently inherited from sg.c bsg will submit asynchronous request
at the head-of-the-queue, (using "at_head" set in the call to
blk_execute_rq_nowait()). This is bad in situation where the queues
are full, requests will execute out of order, and can cause
starvation of the first submitted requests.

The sg_io_v4->flags member is used and a bit is allocated to denote the
Q_AT_TAIL. Zero is to queue at_head as before, to be compatible with old
code at the write/read path. SG_IO code path behavior was changed so to
be the same as write/read behavior. SG_IO was very rarely used and breaking
compatibility with it is OK at this stage.

sg_io_hdr at sg.h also has a flags member and uses 3 bits from the first
nibble and one bit from the last nibble. Even though none of these bits
are supported by bsg, The second nibble is allocated for use by bsg. Just
in case.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Douglas Gilbert <dgilbert@interlog.com>
---
 block/bsg.c         |    9 +++++++--
 include/linux/bsg.h |    8 ++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 44a2a0f..206060e 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -350,6 +350,8 @@ static void bsg_rq_end_io(struct request *rq, int uptodate)
 static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
 			    struct bsg_command *bc, struct request *rq)
 {
+	int at_head = (0 == (bc->hdr.flags & BSG_FLAG_Q_AT_TAIL));
+
 	/*
 	 * add bc command to busy queue and submit rq for io
 	 */
@@ -365,7 +367,7 @@ static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
 	dprintk("%s: queueing rq %p, bc %p\n", bd->name, rq, bc);
 
 	rq->end_io_data = bc;
-	blk_execute_rq_nowait(q, NULL, rq, 1, bsg_rq_end_io);
+	blk_execute_rq_nowait(q, NULL, rq, at_head, bsg_rq_end_io);
 }
 
 static struct bsg_command *bsg_next_done_cmd(struct bsg_device *bd)
@@ -921,6 +923,7 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		struct request *rq;
 		struct bio *bio, *bidi_bio = NULL;
 		struct sg_io_v4 hdr;
+		int at_head;
 		u8 sense[SCSI_SENSE_BUFFERSIZE];
 
 		if (copy_from_user(&hdr, uarg, sizeof(hdr)))
@@ -933,7 +936,9 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		bio = rq->bio;
 		if (rq->next_rq)
 			bidi_bio = rq->next_rq->bio;
-		blk_execute_rq(bd->queue, NULL, rq, 0);
+
+		at_head = (0 == (hdr.flags & BSG_FLAG_Q_AT_TAIL));
+		blk_execute_rq(bd->queue, NULL, rq, at_head);
 		ret = blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
 
 		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
diff --git a/include/linux/bsg.h b/include/linux/bsg.h
index cf0303a..3f0c64a 100644
--- a/include/linux/bsg.h
+++ b/include/linux/bsg.h
@@ -7,6 +7,14 @@
 #define BSG_SUB_PROTOCOL_SCSI_TMF	1
 #define BSG_SUB_PROTOCOL_SCSI_TRANSPORT	2
 
+/*
+ * For flags member below
+ * sg.h sg_io_hdr also has bits defined for it's flags member. However
+ * none of these bits are implemented/used by bsg. The bits below are
+ * allocated to not conflict with sg.h ones anyway.
+ */
+#define BSG_FLAG_Q_AT_TAIL 0x10 /* default, == 0 at this bit, is Q_AT_HEAD */
+
 struct sg_io_v4 {
 	__s32 guard;		/* [i] 'Q' to differentiate from v3 */
 	__u32 protocol;		/* [i] 0 -> SCSI , .... */
-- 
1.6.2.1

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[Boaz Harrosh]

Boaz Harrosh wrote:
> Hi Tomo Jens
> 
> Tomo you never ack-by on this patch. I absolutely needs this for the
> user-mode API of osd-initiator. Which is needed with up-coming exofs
> utilities.
> 
> What do you want to do?
> 
> Thanks 
> 

Hi Jens.

I absolutely need this patch for 2.6.30 merge window. It is totally
un-dangerous since defaults are left unchanged.

I need it because the user-mode utilities for osd and exofs that
correspond to code in 2.6.30 will need this patch to compile,
because they use a flag constant defined in this patch.

I'm not getting any response from Tomo, some of the emails I send bounce,
do you know if he is on vacation or something?

Thank you
Boaz

> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> 
> Currently inherited from sg.c bsg will submit asynchronous request
> at the head-of-the-queue, (using "at_head" set in the call to
> blk_execute_rq_nowait()). This is bad in situation where the queues
> are full, requests will execute out of order, and can cause
> starvation of the first submitted requests.
> 
> The sg_io_v4->flags member is used and a bit is allocated to denote the
> Q_AT_TAIL. Zero is to queue at_head as before, to be compatible with old
> code at the write/read path. SG_IO code path behavior was changed so to
> be the same as write/read behavior. SG_IO was very rarely used and breaking
> compatibility with it is OK at this stage.
> 
> sg_io_hdr at sg.h also has a flags member and uses 3 bits from the first
> nibble and one bit from the last nibble. Even though none of these bits
> are supported by bsg, The second nibble is allocated for use by bsg. Just
> in case.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> CC: Douglas Gilbert <dgilbert@interlog.com>
> ---
>  block/bsg.c         |    9 +++++++--
>  include/linux/bsg.h |    8 ++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 44a2a0f..206060e 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -350,6 +350,8 @@ static void bsg_rq_end_io(struct request *rq, int uptodate)
>  static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
>  			    struct bsg_command *bc, struct request *rq)
>  {
> +	int at_head = (0 == (bc->hdr.flags & BSG_FLAG_Q_AT_TAIL));
> +
>  	/*
>  	 * add bc command to busy queue and submit rq for io
>  	 */
> @@ -365,7 +367,7 @@ static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
>  	dprintk("%s: queueing rq %p, bc %p\n", bd->name, rq, bc);
>  
>  	rq->end_io_data = bc;
> -	blk_execute_rq_nowait(q, NULL, rq, 1, bsg_rq_end_io);
> +	blk_execute_rq_nowait(q, NULL, rq, at_head, bsg_rq_end_io);
>  }
>  
>  static struct bsg_command *bsg_next_done_cmd(struct bsg_device *bd)
> @@ -921,6 +923,7 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		struct request *rq;
>  		struct bio *bio, *bidi_bio = NULL;
>  		struct sg_io_v4 hdr;
> +		int at_head;
>  		u8 sense[SCSI_SENSE_BUFFERSIZE];
>  
>  		if (copy_from_user(&hdr, uarg, sizeof(hdr)))
> @@ -933,7 +936,9 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		bio = rq->bio;
>  		if (rq->next_rq)
>  			bidi_bio = rq->next_rq->bio;
> -		blk_execute_rq(bd->queue, NULL, rq, 0);
> +
> +		at_head = (0 == (hdr.flags & BSG_FLAG_Q_AT_TAIL));
> +		blk_execute_rq(bd->queue, NULL, rq, at_head);
>  		ret = blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
>  
>  		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
> diff --git a/include/linux/bsg.h b/include/linux/bsg.h
> index cf0303a..3f0c64a 100644
> --- a/include/linux/bsg.h
> +++ b/include/linux/bsg.h
> @@ -7,6 +7,14 @@
>  #define BSG_SUB_PROTOCOL_SCSI_TMF	1
>  #define BSG_SUB_PROTOCOL_SCSI_TRANSPORT	2
>  
> +/*
> + * For flags member below
> + * sg.h sg_io_hdr also has bits defined for it's flags member. However
> + * none of these bits are implemented/used by bsg. The bits below are
> + * allocated to not conflict with sg.h ones anyway.
> + */
> +#define BSG_FLAG_Q_AT_TAIL 0x10 /* default, == 0 at this bit, is Q_AT_HEAD */
> +
>  struct sg_io_v4 {
>  	__s32 guard;		/* [i] 'Q' to differentiate from v3 */
>  	__u32 protocol;		/* [i] 0 -> SCSI , .... */

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[Jens Axboe]

On Tue, Mar 24 2009, Boaz Harrosh wrote:
> Boaz Harrosh wrote:
> > Hi Tomo Jens
> > 
> > Tomo you never ack-by on this patch. I absolutely needs this for the
> > user-mode API of osd-initiator. Which is needed with up-coming exofs
> > utilities.
> > 
> > What do you want to do?
> > 
> > Thanks 
> > 
> 
> Hi Jens.
> 
> I absolutely need this patch for 2.6.30 merge window. It is totally
> un-dangerous since defaults are left unchanged.
> 
> I need it because the user-mode utilities for osd and exofs that
> correspond to code in 2.6.30 will need this patch to compile,
> because they use a flag constant defined in this patch.
> 
> I'm not getting any response from Tomo, some of the emails I send bounce,
> do you know if he is on vacation or something?

I don't know if Tomo is away or not, but I've added your patch for
2.6.30 now at least.

-- 
Jens Axboe

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[FUJITA Tomonori]

On Tue, 24 Mar 2009 10:41:32 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> Boaz Harrosh wrote:
> > Hi Tomo Jens
> > 
> > Tomo you never ack-by on this patch. I absolutely needs this for the
> > user-mode API of osd-initiator. Which is needed with up-coming exofs
> > utilities.
> > 
> > What do you want to do?
> > 
> > Thanks 
> > 
> 
> Hi Jens.
> 
> I absolutely need this patch for 2.6.30 merge window. It is totally
> un-dangerous since defaults are left unchanged.

The question is we really need this feature or not. Though I guess
that we need to address this starvation issue.


> I need it because the user-mode utilities for osd and exofs that
> correspond to code in 2.6.30 will need this patch to compile,
> because they use a flag constant defined in this patch.

What do the user-mode utilities does for what?

You always say something like, "we need this for OSD, pNFS or
something". But I'm not sure you explain the detailed background.

A typical example is, why does the osd library export lots of
functions that exofs doesn't use with EXPORT_SYMBOL instead of
EXPORT_SYMBOL_GPL?

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[Jens Axboe]

On Tue, Mar 24 2009, FUJITA Tomonori wrote:
> On Tue, 24 Mar 2009 10:41:32 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > Boaz Harrosh wrote:
> > > Hi Tomo Jens
> > > 
> > > Tomo you never ack-by on this patch. I absolutely needs this for the
> > > user-mode API of osd-initiator. Which is needed with up-coming exofs
> > > utilities.
> > > 
> > > What do you want to do?
> > > 
> > > Thanks 
> > > 
> > 
> > Hi Jens.
> > 
> > I absolutely need this patch for 2.6.30 merge window. It is totally
> > un-dangerous since defaults are left unchanged.
> 
> The question is we really need this feature or not. Though I guess
> that we need to address this starvation issue.

I'd argue that tail insertion should be the most used way to queue
commands, head insertion should only be for the cases where you want
immediate access (like error handling).

-- 
Jens Axboe

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[FUJITA Tomonori]

On Tue, 24 Mar 2009 13:14:38 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Tue, Mar 24 2009, FUJITA Tomonori wrote:
> > On Tue, 24 Mar 2009 10:41:32 +0200
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> > > Boaz Harrosh wrote:
> > > > Hi Tomo Jens
> > > > 
> > > > Tomo you never ack-by on this patch. I absolutely needs this for the
> > > > user-mode API of osd-initiator. Which is needed with up-coming exofs
> > > > utilities.
> > > > 
> > > > What do you want to do?
> > > > 
> > > > Thanks 
> > > > 
> > > 
> > > Hi Jens.
> > > 
> > > I absolutely need this patch for 2.6.30 merge window. It is totally
> > > un-dangerous since defaults are left unchanged.
> > 
> > The question is we really need this feature or not. Though I guess
> > that we need to address this starvation issue.
> 
> I'd argue that tail insertion should be the most used way to queue
> commands, head insertion should only be for the cases where you want
> immediate access (like error handling).

As I wrote, I think that we need to address this starvation issue.

But nobody has complained about this (with sg and bsg) for years
because we have not seen applications sending scsi commands that are
many enough to make a queue full. So it's worth knowing what unknown
applications do for what, I think.

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[Jens Axboe]

On Tue, Mar 24 2009, FUJITA Tomonori wrote:
> On Tue, 24 Mar 2009 13:14:38 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Tue, Mar 24 2009, FUJITA Tomonori wrote:
> > > On Tue, 24 Mar 2009 10:41:32 +0200
> > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > 
> > > > Boaz Harrosh wrote:
> > > > > Hi Tomo Jens
> > > > > 
> > > > > Tomo you never ack-by on this patch. I absolutely needs this for the
> > > > > user-mode API of osd-initiator. Which is needed with up-coming exofs
> > > > > utilities.
> > > > > 
> > > > > What do you want to do?
> > > > > 
> > > > > Thanks 
> > > > > 
> > > > 
> > > > Hi Jens.
> > > > 
> > > > I absolutely need this patch for 2.6.30 merge window. It is totally
> > > > un-dangerous since defaults are left unchanged.
> > > 
> > > The question is we really need this feature or not. Though I guess
> > > that we need to address this starvation issue.
> > 
> > I'd argue that tail insertion should be the most used way to queue
> > commands, head insertion should only be for the cases where you want
> > immediate access (like error handling).
> 
> As I wrote, I think that we need to address this starvation issue.
> 
> But nobody has complained about this (with sg and bsg) for years
> because we have not seen applications sending scsi commands that are
> many enough to make a queue full. So it's worth knowing what unknown
> applications do for what, I think.

Not sure it exists, just saying that if I were to write an app that uses
bsg for doing "normal" IO, I would certainly use tail insertion.

-- 
Jens Axboe

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Tue, 24 Mar 2009 10:41:32 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> Boaz Harrosh wrote:
>>> Hi Tomo Jens
>>>
>>> Tomo you never ack-by on this patch. I absolutely needs this for the
>>> user-mode API of osd-initiator. Which is needed with up-coming exofs
>>> utilities.
>>>
>>> What do you want to do?
>>>
>>> Thanks 
>>>
>> Hi Jens.
>>
>> I absolutely need this patch for 2.6.30 merge window. It is totally
>> un-dangerous since defaults are left unchanged.
> 
> The question is we really need this feature or not. Though I guess
> that we need to address this starvation issue.
> 

Good, this patch solves exactly that problem. Thanks Tomo
 
> 
>> I need it because the user-mode utilities for osd and exofs that
>> correspond to code in 2.6.30 will need this patch to compile,
>> because they use a flag constant defined in this patch.
> 
> What do the user-mode utilities does for what?
> 

The immediate problem is that the code uses this API and will
not compile without it. This is because the user-mode library
exports the exact API and fixture set as the Kernel one. Submitting
commands in order is one important aspect.

This is a general purpose library for developers to use, with
published API. In the purpose of attracting users to develop for
OSD, so the argument of not yet used does not apply. I don't want
the users to go around inventing work arounds when the solution
is obvious and the blame is clear.

> You always say something like, "we need this for OSD, pNFS or
> something". But I'm not sure you explain the detailed background.
> 

I don't think this argument applies to this particular patch.
The shortcoming of bsg code is clear and should be fixed. And
it has a user. libosd in user-mode.

But if you want details then just calling osd_execute_request_async()
from user-mode in succession, will reveal this problem. Not fixing
it now will hide mines to any developer that wants to use libosd.

> A typical example is, why does the osd library export lots of
> functions that exofs doesn't use with EXPORT_SYMBOL 

This is unrelated to this patch of course, but...
They are all used by the osd_ktest.ko which you objected to
be included in the Kernel, and I removed.

> instead of
> EXPORT_SYMBOL_GPL?

What is that got to do with it?
The libosd.ko is a general purpose library for any kind of user.
It exports the API defined in osd_initiator.h. This API is globally
public, and has until now 3 implementations Linux-kernel, Linux-user-mode,
BSD-kernel. I hope it will have many more implementations on all
major OSs.
I do not designate this API to be private Linux API, hence
EXPORT_SYMBOL_GPL does not apply.

Boaz

Re: [PATCH resend] bsg: Add support for submitting requests at tail of queue

[FUJITA Tomonori]

On Tue, 24 Mar 2009 14:53:52 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> > instead of
> > EXPORT_SYMBOL_GPL?
> 
> What is that got to do with it?
> The libosd.ko is a general purpose library for any kind of user.
> It exports the API defined in osd_initiator.h. This API is globally
> public, and has until now 3 implementations Linux-kernel, Linux-user-mode,
> BSD-kernel. I hope it will have many more implementations on all
> major OSs.
> I do not designate this API to be private Linux API, hence
> EXPORT_SYMBOL_GPL does not apply.

I don't understand your logic.

Your osd code is released under GPL so of course anyone can use (and
modify) your code (design, or whatever) for whatever he wants as long
as he follows GPL. EXPORT_SYMBOL_GPL has nothing to do with it.

We are talking about EXPORT_SYMBOL_GPL, it's all about Linux kernel;
what kinds of Linux kernel modules can use your osd code in Linux
kernel. The your code will be newly added so EXPORT_SYMBOL_GPL should
be fine here unless you encourage non-GPL kernel modules.

Or I miss something?