bsg: Add support for submitting requests at tail of queue

PATCH In review Boaz Harrosh 4 participants 13 messages Jan 21, 2009 – Jan 25, 2009

Subsystems (3)

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

lore

Share & tools

lore

Boaz Harrosh

author

2009-01-21 09:52 UTC #0 lore raw

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

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 we want to keep the queues full but need the requests to execute in order.

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.

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         |    3 ++-
 include/linux/bsg.h |    8 ++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

block/bsg.c +2 -1

index 44a2a0f..43e4344 100644
@@ -350,6 +350,7 @@ 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 +366,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)

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.0.1

FUJITA Tomonori

maintainer

2009-01-21 23:24 UTC #1 lore raw

Quoted 5 lines · On Wed, 21 Jan 2009 11:52:39 +0200 Boaz Harrosh <[email protected]> wrote:

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 we want to keep the queues full but need the requests to execute in order.

As I wrote, I think that blk_execute_rq_nowait inserts a request and plugs a queue. So how can you keep the queue full? On the completion of blk_execute_rq_nowait, the queue is empty.

Boaz Harrosh

author

2009-01-22 08:57 UTC #2 lore raw

Quoted 11 lines · FUJITA Tomonori

Quoted 4 lines · On Wed, 21 Jan 2009 11:52:39 +0200 Boaz Harrosh <[email protected]> wrote:
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 we want to keep the queues full but need the requests to execute in order.
As I wrote, I think that blk_execute_rq_nowait inserts a request and plugs a queue. So how can you keep the queue full? On the completion of blk_execute_rq_nowait, the queue is empty.

If it's so then why does the "at_head" matters why does it exist? Let me say it this way, if it does not matter then could we just reverse it? you just said it does not matter, lets reverse it to be 0 and not 1.

I don't understand what you are saying. bsg::write can accept an array of commands lets say I have this sequence in the array:

command1:command2:command3

And for the sake of the argument lets say the hosts queue is full, has can_queue commands waiting response. Let say from other clients.

if I use at_head==0, the target will surly see this sequence:
command1:command2:command3

But if I use at_head==1, the way I read the code, the target will see" command3:command2:command1 and some times command1:command3:command2 or command2:command1:command3
It all depends on the random completions of the other commands in flight.

This is the way I read the code, If I read it wrong, please explain what will be the actual behavior and what is the effect of at_head?

Thanks
Boaz

Jens Axboe

2009-01-22 11:14 UTC #3 lore raw

In reply to FUJITA Tomonori

Quoted 4 lines · FUJITA Tomonori

Quoted 1 line · On Wed, 21 Jan 2009 11:52:39 +0200 Boaz Harrosh <[email protected]> wrote:

Quoted 8 lines

Quoted 4 lines
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 we want to keep the queues full but need the requests to execute in order.
As I wrote, I think that blk_execute_rq_nowait inserts a request and plugs a queue. So how can you keep the queue full? On the completion of blk_execute_rq_nowait, the queue is empty.

That's not true at all. If you submit more than one request, request 2 and up would be queued according to the orientation given. It may even include request 1 as well, what if the queue is busy doing work for someone else already?

I think the patch makes sense, I also wish that the default would have been reversed so that at_back would be the default. at_back is more complex though, since it impacts existing requests for the device (it drains the scheduler queue).

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-01-22 12:44 UTC #4 lore raw

Quoted 17 lines · Jens Axboe

Quoted 10 lines · FUJITA Tomonori
Quoted 4 lines · On Wed, 21 Jan 2009 11:52:39 +0200 Boaz Harrosh <[email protected]> wrote:
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 we want to keep the queues full but need the requests to execute in order.
As I wrote, I think that blk_execute_rq_nowait inserts a request and plugs a queue. So how can you keep the queue full? On the completion of blk_execute_rq_nowait, the queue is empty.
That's not true at all. If you submit more than one request, request 2 and up would be queued according to the orientation given. It may even include request 1 as well, what if the queue is busy doing work for someone else already?

Quoted 5 lines

I think the patch makes sense, I also wish that the default would have been reversed so that at_back would be the default. at_back is more complex though, since it impacts existing requests for the device (it drains the scheduler queue).

bsg only sends BLOCK_PC commands, I think these are not held in the scheduler queue, right?

I think like you at_back is the default to use, but this is historic compatibility with SG that had problems with ABORT and such not. With my patch I give the user a choice and be done with it.

Thanks
Boaz

Jens Axboe

2009-01-22 12:48 UTC #5 lore raw

Quoted 18 lines · Boaz Harrosh

Quoted 17 lines · Jens Axboe
Quoted 10 lines · FUJITA Tomonori
Quoted 4 lines · On Wed, 21 Jan 2009 11:52:39 +0200 Boaz Harrosh <[email protected]> wrote:
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 we want to keep the queues full but need the requests to execute in order.
As I wrote, I think that blk_execute_rq_nowait inserts a request and plugs a queue. So how can you keep the queue full? On the completion of blk_execute_rq_nowait, the queue is empty.
That's not true at all. If you submit more than one request, request 2 and up would be queued according to the orientation given. It may even include request 1 as well, what if the queue is busy doing work for someone else already?

Quoted 8 lines

Quoted 5 lines
I think the patch makes sense, I also wish that the default would have been reversed so that at_back would be the default. at_back is more complex though, since it impacts existing requests for the device (it drains the scheduler queue).
bsg only sends BLOCK_PC commands, I think these are not held in the scheduler queue, right?

They are not, correct. But that queue may have other requests pending, which may be "normal" file system requests. Then an at_back bsg command would act almost like a barrier, draining the entire queue to dispatch.

Quoted 3 lines

I think like you at_back is the default to use, but this is historic compatibility with SG that had problems with ABORT and such not. With my patch I give the user a choice and be done with it.

at_head is definitely useful and required for some situations, but that doesn't mean it's a good default :-). But yes, we are stuck with it. I think the patch makes sense.

Signature · 1 line

Jens Axboe

FUJITA Tomonori

maintainer

2009-01-22 22:03 UTC #6 lore raw

Quoted 19 lines · On Thu, 22 Jan 2009 13:46:41 +0100 Jens Axboe <[email protected]> wrote:

Quoted 18 lines · Boaz Harrosh
Quoted 17 lines · Jens Axboe
Quoted 10 lines · FUJITA Tomonori
Quoted 4 lines · On Wed, 21 Jan 2009 11:52:39 +0200 Boaz Harrosh <[email protected]> wrote:
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 we want to keep the queues full but need the requests to execute in order.
As I wrote, I think that blk_execute_rq_nowait inserts a request and plugs a queue. So how can you keep the queue full? On the completion of blk_execute_rq_nowait, the queue is empty.
That's not true at all. If you submit more than one request, request 2 and up would be queued according to the orientation given. It may even include request 1 as well, what if the queue is busy doing work for someone else already?

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
I think the patch makes sense, I also wish that the default would have been reversed so that at_back would be the default. at_back is more complex though, since it impacts existing requests for the device (it drains the scheduler queue).
bsg only sends BLOCK_PC commands, I think these are not held in the scheduler queue, right?

Right, that's what I meant.

Quoted 3 lines

They are not, correct. But that queue may have other requests pending, which may be "normal" file system requests. Then an at_back bsg command would act almost like a barrier, draining the entire queue to dispatch.

Theoretically, yes. But for what do you want to mix pc and fs commands with a single device and send a pc command 'at the end of the queue'?

And as you know, you can assume that Boaz always cares about only OSD. ;) There is no way in mainline now to send fs commands to OSD.

So, Boaz, what do you want to do exactly? It should have in the patch description. I don't want to add something that nobody uses.

James Bottomley

maintainer

2009-01-22 22:28 UTC #7 lore raw

Quoted 2 lines · FUJITA Tomonori

So, Boaz, what do you want to do exactly? It should have in the patch description. I don't want to add something that nobody uses.

OK, can we step back a bit from this? Everyone seems to be talking past each other. The original complaint was that multiple commands against the same device issued by SG_IO could be executed "out of order". This is really irrelevant because we never guarantee execution order in the first place.

However, if you consider our current at head insertion policy coupled with a multi-threaded application issuing hundreds of SG_IO requests at once, you can see we have a potential starvation issue: Commands at the tail of the queue end up pushed further and further back as more commands are added to the head. This starvation issue is worth addressing, I think, and it can only be addressed by allowing tail insertion.

The original reason for at head is, as you surmise, inherited from sg and the rationale is largely for error handling: you need error handling commands to pre-empt everything in the current queue.

James

FUJITA Tomonori

maintainer

2009-01-23 06:15 UTC #8 lore raw

Quoted 10 lines · On Thu, 22 Jan 2009 16:27:59 -0600 James Bottomley…

Quoted 2 lines · FUJITA Tomonori
So, Boaz, what do you want to do exactly? It should have in the patch description. I don't want to add something that nobody uses.
OK, can we step back a bit from this? Everyone seems to be talking past each other. The original complaint was that multiple commands against the same device issued by SG_IO could be executed "out of order". This is really irrelevant because we never guarantee execution order in the first place.

Quoted 7 lines

However, if you consider our current at head insertion policy coupled with a multi-threaded application issuing hundreds of SG_IO requests at once, you can see we have a potential starvation issue: Commands at the tail of the queue end up pushed further and further back as more commands are added to the head. This starvation issue is worth addressing, I think, and it can only be addressed by allowing tail insertion.

Ah, I see. Thanks. We could see this with something busy.

BTW, bsg write interface enables you to send a command asynchronously so a single-thread-ed application could cause this.

Boaz Harrosh

author

2009-01-25 09:17 UTC #9 lore raw

Quoted 11 lines · FUJITA Tomonori

Quoted 8 lines · On Thu, 22 Jan 2009 16:27:59 -0600 James Bottomley…
Quoted 2 lines · FUJITA Tomonori
So, Boaz, what do you want to do exactly? It should have in the patch description. I don't want to add something that nobody uses.
OK, can we step back a bit from this? Everyone seems to be talking past each other. The original complaint was that multiple commands against the same device issued by SG_IO could be executed "out of order". This is really irrelevant because we never guarantee execution order in the first place.

out-of-order execution happens very rarely and I can live with that, as long as statistically, over an iscsi connection, they are submitted in order, then there are optimizations that can take advantage of this. (Actually I have never observed an out of order submission to a target)

Quoted 10 lines

Quoted 8 lines
However, if you consider our current at head insertion policy coupled with a multi-threaded application issuing hundreds of SG_IO requests at once, you can see we have a potential starvation issue: Commands at the tail of the queue end up pushed further and further back as more commands are added to the head. This starvation issue is worth addressing, I think, and it can only be addressed by allowing tail insertion.
Ah, I see. Thanks. We could see this with something busy.

That was my point. Submitting when queues are full.

Quoted 3 lines

BTW, bsg write interface enables you to send a command asynchronously so a single-thread-ed application could cause this.

However ...

The patch I submitted is not good enough. I have only added control for the write/read path, Not the SG_IO path. The last one was left at_tail, as before. But this is very bad, we should absolutely keep both interfaces the same, as much as we can.

Now here we have a problem. write/read was default "at_head" SG_IO was default "at_tail". If we want to absolutely keep backward compatibility with old applications we need two bits. One - BSG_FLAG_Q_AT_TAIL but another one BSG_FLAG_Q_FLAG_VALID.

In the light that I found a bad bug with SG_IO just recently, and for a fact I know that Pete and all his osd guys only used the write/read method. I would say that SG_IO applications are very rare, and could be disregarded.

TOMO, Jens, Please decide what you want to do. I'll post as reply to first patch the one-bit-clean version. If you decide I will be happy to send a two-bit-compatible version ASAP.

Thanks
Boaz

Boaz Harrosh

author

2009-01-25 09:41 UTC #10 lore raw

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

In reply to Boaz Harrosh

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 but requests need to execute in order. Failing to do that can cause the first submitted commands to never execute.

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..2e55655 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 == (bc->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.0.1

Boaz Harrosh

author

2009-01-25 09:44 UTC #11 lore raw

Quoted 17 lines · Boaz Harrosh

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 but requests need to execute in order. Failing to do that can cause the first submitted commands to never execute.
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.

Quoted 7 lines

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(-)

Sorrrrrrry bad patch, teach me to never send a patch in the morning before coffee.

Will send a version 3
Boaz

Boaz Harrosh

author

2009-01-25 10:07 UTC #12 lore raw

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

In reply to Boaz Harrosh

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.

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.0.1