sg: retrofit SG_FLAG_Q_AT_TAIL flag

In response to

    http://bugzilla.kernel.org/show_bug.cgi?id=15565

and the fact this capability has been present
in bsg for some time, add SG_FLAG_Q_AT_TAIL flag. It has the same binary value as the bsg flag and the define name is the same apart from the leading "B". The semantics are the same, namely to override the default queue at head action of the SCSI midlevel when a low level driver blocks (i.e. when a LLD returns non-zero to a queuecommand() ). Tested with scsi_debug.

Changelog
- add SG_FLAG_Q_AT_TAIL flag to override default queue at head semantics of the SCSI midlevel queue.

Signed-off-by Douglas Gilbert <[email protected]>

include/scsi/sg.h2633qat1 renamed from include/scsi/sg.h +2 -2

@@ -11,9 +11,9 @@
 Original driver (sg.h):
 *       Copyright (C) 1992 Lawrence Foard
 Version 2 and 3 extensions to driver:
-*       Copyright (C) 1998 - 2006 Douglas Gilbert
+*       Copyright (C) 1998 - 2010 Douglas Gilbert
 
-    Version: 3.5.34 (20060920)
+    Version: 3.5.35 (20100319)
     This version is for 2.6 series kernels.
 
     For a full changelog see http://www.torque.net/sg

drivers/scsi/sg.c2633qat1 renamed from drivers/scsi/sg.c +3 -0

 	int k, data_dir;
 	Sg_device *sdp = sfp->parentdp;
 	sg_io_hdr_t *hp = &srp->header;
+	int at_head = 1;
 
 	srp->data.cmd_opcode = cmnd[0];	/* hold opcode of command */
+	if ('\0' != hp->interface_id)	/* old interface misuses flags */
+		at_head = (SG_FLAG_Q_AT_TAIL & hp->flags) ? 0 : 1;
 	hp->status = 0;
 	hp->masked_status = 0;
 	hp->msg_status = 0;

Quoted 11 lines · Douglas Gilbert

In response to

    http://bugzilla.kernel.org/show_bug.cgi?id=15565

and the fact this capability has been present
in bsg for some time, add SG_FLAG_Q_AT_TAIL flag. It has the same binary value as the bsg flag and the define name is the same apart from the leading "B". The semantics are the same, namely to override the default queue at head action of the SCSI midlevel when a low level driver blocks (i.e. when a LLD returns non-zero to a queuecommand() ). Tested with scsi_debug.

Quoted 5 lines

Changelog
- add SG_FLAG_Q_AT_TAIL flag to override default queue at head semantics of the SCSI midlevel queue.

Signed-off-by Douglas Gilbert <[email protected]>

Review-by Boaz Harrosh <[email protected]>

Quoted 15 lines

include/scsi/sg.h2633qat1 renamed from include/scsi/sg.h +2 -2

@@ -11,9 +11,9 @@
 Original driver (sg.h):
 *       Copyright (C) 1992 Lawrence Foard
 Version 2 and 3 extensions to driver:
-*       Copyright (C) 1998 - 2006 Douglas Gilbert
+*       Copyright (C) 1998 - 2010 Douglas Gilbert
 
-    Version: 3.5.34 (20060920)
+    Version: 3.5.35 (20100319)
     This version is for 2.6 series kernels.
 
     For a full changelog see http://www.torque.net/sg

include/scsi/sg.h2633qat1 line 127

Quoted 4 lines

include/scsi/sg.h2633qat1 renamed from include/scsi/sg.h +1 -0

@@ -124,6 +124,7 @@
 #define SG_FLAG_UNUSED_LUN_INHIBIT 2   /* default is overwrite lun in SCSI */
 				/* command block (when <= SCSI_2) */
 #define SG_FLAG_MMAP_IO 4       /* request memory mapped IO */
+#define SG_FLAG_Q_AT_TAIL 0x10  /* default, without this flag, is Q_AT_HEAD */

I have chosen this value exactly so it can fit with SG as well.

drivers/scsi/sg.c2633qat1

Quoted 26 lines

include/scsi/sg.h2633qat1 renamed from include/scsi/sg.h +0 -0

 #define SG_FLAG_NO_DXFER 0x10000 /* no transfer of kernel buffers to/from */
 				/* user space (debug indirect IO) */
 

drivers/scsi/sg.c2633qat1 renamed from drivers/scsi/sg.c +3 -3

@@ -18,8 +18,8 @@
  *
  */
 
-static int sg_version_num = 30534;	/* 2 digits for each component */
-#define SG_VERSION_STR "3.5.34"
+static int sg_version_num = 30535;	/* 2 digits for each component */
+#define SG_VERSION_STR "3.5.35"
 
 /*
  *  D. P. Gilbert ([email protected], [email protected]), notes:

@@ -61,7 +61,7 @@
 
 #ifdef CONFIG_SCSI_PROC_FS
 #include <linux/proc_fs.h>
-static char *sg_version_date = "20061027";
+static char *sg_version_date = "20100319";
 
 static int sg_proc_init(void);
 static void sg_proc_cleanup(void);

@@ -710,8 +710,11 @@

I wish you would have used diff "-p" option that shows us the function this hunk is at. (git diff does that by default)

Quoted 12 lines

drivers/scsi/sg.c2633qat1 renamed from drivers/scsi/sg.c +3 -0

 	int k, data_dir;
 	Sg_device *sdp = sfp->parentdp;
 	sg_io_hdr_t *hp = &srp->header;
+	int at_head = 1;
 
 	srp->data.cmd_opcode = cmnd[0];	/* hold opcode of command */
+	if ('\0' != hp->interface_id)	/* old interface misuses flags */
+		at_head = (SG_FLAG_Q_AT_TAIL & hp->flags) ? 0 : 1;
 	hp->status = 0;
 	hp->masked_status = 0;
 	hp->msg_status = 0;

drivers/scsi/sg.c2633qat1 line 759

Quoted 5 lines

drivers/scsi/sg.c2633qat1 renamed from drivers/scsi/sg.c +1 -1

@@ -753,7 +756,7 @@
 	srp->rq->timeout = timeout;
 	kref_get(&sfp->f_ref); /* sg_rq_end_io() does kref_put(). */
 	blk_execute_rq_nowait(sdp->device->request_queue, sdp->disk,
-			      srp->rq, 1, sg_rq_end_io);
+			      srp->rq, at_head, sg_rq_end_io);

Grate, this simple thing does wonders to performance.

drivers/scsi/sg.c2633qat1 line 762

Quoted 3 lines

drivers/scsi/sg.c2633qat1 renamed from drivers/scsi/sg.c +0 -0

 	return 0;
 }
 

Thanks for doing this
Boaz

Boaz,
Thanks for the review.

Now I decided to check the SG_IO ioctl used directly against block devices and it calls:

    blk_execute_rq(q, bd_disk, rq, 0);

The last argument is 'at_head' so it has been queuing at_tail for some time. How is that for compatibility??

That almost suggests there should be a
#define SG_FLAG_Q_AT_HEAD 0x20

added to sg.h to cover all the bases.

Doug Gilbert

Quoted 10 lines · Douglas Gilbert

Boaz,
Thanks for the review.

Now I decided to check the SG_IO ioctl used directly against block devices and it calls:

    blk_execute_rq(q, bd_disk, rq, 0);

The last argument is 'at_head' so it has been queuing at_tail for some time. How is that for compatibility??

Quoted 8 lines

That almost suggests there should be a
#define SG_FLAG_Q_AT_HEAD 0x20

added to sg.h to cover all the bases.

Doug Gilbert

OK I dug up the original patch and actually It was the same with bsg. See the commit log:

commit 05378940caf979a8655c18b18a17213dcfa52412

Author: Boaz Harrosh <[email protected]>
Date:   Tue Mar 24 12:23:40 2009 +0100

bsg: add support for tail queuing

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]>

Signed-off-by Jens Axboe <[email protected]>

(See second paragraph)
So bsg had the same compatibility problem between SG_IO and "write". What happened is that I found a nasty bug in bsg's SG_IO, just at the same time, which proves that SG_IO was never used, (since it would just crash), so I decided that it would be cost-less to just break compatibility with some thing that never work, for the sake of simple API sameness.

I do realize now that I have made it hard for sg and SG_IO to comply to new API but retain back compatibility. (Sorry)

Here is my suggestion:
- Allocate 2 bits at flags, [mask 0x30]
- Have an enum for these two flags with this meaning:
Q_AT_API_COMPATIBLE = 0,
This means for sg and SG_IO to keep their old behaviour.
sg.c - at_head
SG_IO to device - at_tail
bsg both cases - it is at_head.
Q_AT_TAIL = 1,
This means at_tail for all APIs
Q_AT_HEAD = 2,
This means at_head for all APIs
Q_AT_RESERVED = 3,
Should not be used

So this is effectively your idea as well but just elaborated on more.

If we do that I think we should maybe join headers, by moving common defines to sg.h and #include sg.h from bsg.h.

Now I decided to check the SG_IO ioctl used directly against block devices and it calls:

    blk_execute_rq(q, bd_disk, rq, 0);

The last argument is 'at_head' so it has been queuing at_tail for some time. How is that for compatibility??

That almost suggests there should be a
#define SG_FLAG_Q_AT_HEAD 0x20

added to sg.h to cover all the bases.