bsg: Add support for submitting requests at tail of queue

PATCH RESEND In review Boaz Harrosh 3 participants 9 messages Mar 19, 2009 – Mar 25, 2009

Subsystems (3)

SCSI Block layer BSG (block layer generic sg v4 driver)

lore

Share & tools

lore

Boaz Harrosh

author

2009-03-19 09:16 UTC #0 lore raw

PATCH RESEND 1/1 bsg: Add support for submitting requests at tail of queue

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 <[email protected]>

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 <[email protected]>

CC Douglas Gilbert <[email protected]>

---
 block/bsg.c         |    9 +++++++--
 include/linux/bsg.h |    8 ++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

block/bsg.c +7 -2

index 44a2a0f..206060e 100644
@@ -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)))

include/linux/bsg.h +8 -0

index cf0303a..3f0c64a 100644
@@ -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 , .... */

Signature · 1 line

1.6.2.1

Boaz Harrosh

author

2009-03-24 08:44 UTC #1 lore raw

Quoted 10 lines · 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

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

Quoted 87 lines

---
From: Boaz Harrosh <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

CC Douglas Gilbert <[email protected]>

---
 block/bsg.c         |    9 +++++++--
 include/linux/bsg.h |    8 ++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

block/bsg.c +7 -2

index 44a2a0f..206060e 100644
@@ -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)))

include/linux/bsg.h +8 -0

index cf0303a..3f0c64a 100644
@@ -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 , .... */

Jens Axboe

2009-03-24 11:23 UTC #2 lore raw

Quoted 17 lines · Boaz Harrosh

Quoted 10 lines · 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
Hi Jens.
I absolutely need this patch for 2.6.30 merge window. It is totally un-dangerous since defaults are left unchanged.

Quoted 6 lines

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.

Signature · 1 line

Jens Axboe

FUJITA Tomonori

maintainer

2009-03-24 12:05 UTC #3 lore raw

In reply to Boaz Harrosh

Quoted 9 lines · On Tue, 24 Mar 2009 10:41:32 +0200 Boaz Harrosh <[email protected]> wrote:

Quoted 8 lines · 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?

Quoted 7 lines

Quoted 2 lines
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.

Quoted 3 lines

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?

Jens Axboe

2009-03-24 12:14 UTC #4 lore raw

Quoted 15 lines · FUJITA Tomonori

Quoted 12 lines · On Tue, 24 Mar 2009 10:41:32 +0200 Boaz Harrosh <[email protected]> wrote:
Quoted 10 lines · 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

Quoted 7 lines

Quoted 4 lines
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).

Signature · 1 line

Jens Axboe

FUJITA Tomonori

maintainer

2009-03-24 12:23 UTC #5 lore raw

Quoted 21 lines · On Tue, 24 Mar 2009 13:14:38 +0100 Jens Axboe <[email protected]> wrote:

Quoted 20 lines · FUJITA Tomonori
Quoted 16 lines · On Tue, 24 Mar 2009 10:41:32 +0200 Boaz Harrosh <[email protected]> wrote:
Quoted 10 lines · 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
Hi Jens.
I absolutely need this patch for 2.6.30 merge window. It is totally un-dangerous since defaults are left unchanged.

Quoted 6 lines

Quoted 2 lines
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.

Jens Axboe

2009-03-24 12:26 UTC #6 lore raw

Quoted 31 lines · FUJITA Tomonori

Quoted 21 lines · On Tue, 24 Mar 2009 13:14:38 +0100 Jens Axboe <[email protected]> wrote:
Quoted 20 lines · FUJITA Tomonori
Quoted 16 lines · On Tue, 24 Mar 2009 10:41:32 +0200 Boaz Harrosh <[email protected]> wrote:
Quoted 10 lines · 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
Hi Jens.
I absolutely need this patch for 2.6.30 merge window. It is totally un-dangerous since defaults are left unchanged.
Quoted 6 lines
Quoted 2 lines
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).

Quoted 6 lines

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.

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-03-24 12:55 UTC #7 lore raw

In reply to FUJITA Tomonori

Quoted 14 lines · FUJITA Tomonori

Quoted 11 lines · On Tue, 24 Mar 2009 10:41:32 +0200 Boaz Harrosh <[email protected]> wrote:
Quoted 10 lines · 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

Quoted 8 lines

Quoted 4 lines
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

Quoted 7 lines

Quoted 4 lines
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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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

FUJITA Tomonori

maintainer

2009-03-25 04:07 UTC #8 lore raw

Quoted 11 lines · On Tue, 24 Mar 2009 14:53:52 +0200 Boaz Harrosh <[email protected]> wrote:

Quoted 2 lines
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?