[PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[Douglas Gilbert]

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

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 <dgilbert@interlog.com>

[-- Attachment #2: sg2633qat1.patch --]
[-- Type: text/x-patch, Size: 2271 bytes --]

--- linux/include/scsi/sg.h	2008-10-10 17:04:54.000000000 -0400
+++ linux/include/scsi/sg.h2633qat1	2010-03-20 14:28:31.000000000 -0400
@@ -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
@@ -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 */
 #define SG_FLAG_NO_DXFER 0x10000 /* no transfer of kernel buffers to/from */
 				/* user space (debug indirect IO) */
 
--- linux/drivers/scsi/sg.c	2009-12-03 11:11:18.000000000 -0500
+++ linux/drivers/scsi/sg.c2633qat1	2010-03-19 19:42:10.000000000 -0400
@@ -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 (dgilbert@interlog.com, dougg@triode.net.au), 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 @@
 	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;
@@ -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);
 	return 0;
 }
 

Re: [PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[Boaz Harrosh]

On 03/20/2010 09:15 PM, Douglas Gilbert wrote:
> 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 <dgilbert@interlog.com>

Review-by: Boaz Harrosh <bharrosh@panasas.com>

> --- linux/include/scsi/sg.h	2008-10-10 17:04:54.000000000 -0400
> +++ linux/include/scsi/sg.h2633qat1	2010-03-20 14:28:31.000000000 -0400
> @@ -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
> @@ -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.

>  #define SG_FLAG_NO_DXFER 0x10000 /* no transfer of kernel buffers to/from */
>  				/* user space (debug indirect IO) */
>  
> --- linux/drivers/scsi/sg.c	2009-12-03 11:11:18.000000000 -0500
> +++ linux/drivers/scsi/sg.c2633qat1	2010-03-19 19:42:10.000000000 -0400
> @@ -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 (dgilbert@interlog.com, dougg@triode.net.au), 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)

>  	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;
> @@ -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.

>  	return 0;
>  }
>  

Thanks for doing this
Boaz

Re: [PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[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??

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


Boaz Harrosh wrote:
> On 03/20/2010 09:15 PM, Douglas Gilbert wrote:
>> 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 <dgilbert@interlog.com>
> 
> Review-by: Boaz Harrosh <bharrosh@panasas.com>
> 
>> --- linux/include/scsi/sg.h	2008-10-10 17:04:54.000000000 -0400
>> +++ linux/include/scsi/sg.h2633qat1	2010-03-20 14:28:31.000000000 -0400
>> @@ -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
>> @@ -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.
> 
>>  #define SG_FLAG_NO_DXFER 0x10000 /* no transfer of kernel buffers to/from */
>>  				/* user space (debug indirect IO) */
>>  
>> --- linux/drivers/scsi/sg.c	2009-12-03 11:11:18.000000000 -0500
>> +++ linux/drivers/scsi/sg.c2633qat1	2010-03-19 19:42:10.000000000 -0400
>> @@ -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 (dgilbert@interlog.com, dougg@triode.net.au), 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)
> 
>>  	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;
>> @@ -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.
> 
>>  	return 0;
>>  }
>>  
> 
> Thanks for doing this
> Boaz
> 

Re: [PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[Mike Hayward]

Hi Doug,

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

I would have to agree that default should be queue at tail and option
should be to queue at head since that presents risk of starvation and
is generally not what is desired, just like normal SCSI queue at head
in the target.

I'll give your patch a spin on live hardware soon.

- Mike

Re: [PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[Boaz Harrosh]

On 03/21/2010 07:56 PM, Douglas Gilbert wrote:
> 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
> 
> 

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

commit 05378940caf979a8655c18b18a17213dcfa52412
Author: Boaz Harrosh <bharrosh@panasas.com>
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 <bharrosh@panasas.com>
    CC: Douglas Gilbert <dgilbert@interlog.com>
    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>


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

Sigh!
Boaz

Re: [PATCH] sg: retrofit SG_FLAG_Q_AT_TAIL flag

[FUJITA Tomonori]

On Mon, 22 Mar 2010 10:07:02 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 03/21/2010 07:56 PM, Douglas Gilbert wrote:
> > 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
> > 
> > 
> 
> OK I dug up the original patch and actually It was the same
> with bsg. See the commit log:
> 
> commit 05378940caf979a8655c18b18a17213dcfa52412
> Author: Boaz Harrosh <bharrosh@panasas.com>
> 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 <bharrosh@panasas.com>
>     CC: Douglas Gilbert <dgilbert@interlog.com>
>     Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> 
> 
> (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.

I don't see any point of adding this feature to sg with such hacky
flags. Why can't we leave sg alone? Just use bsg for things that sg
aren't designed for.