Re: [PATCH 19/33] scsi_debug: support sg chaining

Re: [PATCH 19/33] scsi_debug: support sg chaining

[Boaz Harrosh]

Jens Axboe wrote ...
> Cc: dgilbert@interlog.com
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> ---
>  drivers/scsi/scsi_debug.c |   30 ++++++++++++++++--------------
>  1 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 4cd9c58..46a3e07 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c

(Majordumo has unsubscribed me from list since yesterday)
(Don't know why. So this will appear out of order       )

Jens Hi!

I have queued for submission a: 
"convert to use the data buffer accessors and !use_sg cleanup"
for this driver Just the other day. Should we not push the 
cleanups first before this. That is what we did with other
drivers.

I have also sent patches for ide-scsi

I Have in my Q cleanup patches for:
drivers/scsi/qla1280.c
drivers/scsi/qlogicpti.c
drivers/scsi/aha1542.c
drivers/scsi/advansys.c
And more ...

And also for this one
drivers/scsi/gdth.c
But that one has a patch in Q and I should rebase.

If you want I can rebase all your driver patches above
to my patches. Than send cleanup patches to the list and
rebasing of sg-chaining if needed to you. I can send all
cleanup patches tonight, and your rebases tomorrow.

Boaz

Re: [PATCH 19/33] scsi_debug: support sg chaining

[Jens Axboe]

On Tue, Jul 17 2007, Boaz Harrosh wrote:
> Jens Axboe wrote ...
> > Cc: dgilbert@interlog.com
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > ---
> >  drivers/scsi/scsi_debug.c |   30 ++++++++++++++++--------------
> >  1 files changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index 4cd9c58..46a3e07 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> 
> (Majordumo has unsubscribed me from list since yesterday)
> (Don't know why. So this will appear out of order       )
> 
> Jens Hi!
> 
> I have queued for submission a: 
> "convert to use the data buffer accessors and !use_sg cleanup"
> for this driver Just the other day. Should we not push the 
> cleanups first before this. That is what we did with other
> drivers.
> 
> I have also sent patches for ide-scsi
> 
> I Have in my Q cleanup patches for:
> drivers/scsi/qla1280.c
> drivers/scsi/qlogicpti.c
> drivers/scsi/aha1542.c
> drivers/scsi/advansys.c
> And more ...
> 
> And also for this one
> drivers/scsi/gdth.c
> But that one has a patch in Q and I should rebase.
> 
> If you want I can rebase all your driver patches above
> to my patches. Than send cleanup patches to the list and
> rebasing of sg-chaining if needed to you. I can send all
> cleanup patches tonight, and your rebases tomorrow.

Go ahead and submit whatever driver cleanups, I'll just rebase and drop
patches from my branch as necessary! Less work for me :-)

-- 
Jens Axboe

[PATCH 00/33] SG table chaining support

[Jens Axboe]

Hi,

A repost of this patchset, which adds support forchaining of sg tables.
This enables much larger IO commands, since we don't have to allocate
large consecutive pieces of memory to represent the sgtable of a
huge command. Right now Linux is limited to somewhere between 128 and 256
segments, depending on the architecture. This translates into at most
512k-1mb request sizes. With this patchset, I've successfully pushed
10MiB commands through the IO stack.

This will potentially increase performance a lot on hardware that
requires larger IO commands to perform at their maximum.

Also see http://marc.info/?l=linux-kernel&m=117869783524152

To enable large IO commands for device sda, you would do:

# cd /sys/block/sda/queue
# echo 4096 > max_segments
# cat max_hw_sectors_kb  > max_sectors_kb

cat max_hw_sectors_kb to see what your largest IO size would now be.

Changes since last post:

- Rebase to current -git. Lots of SCSI drivers have been converted
  to use the sg accessor helpers, which nicely shrinks this patchset
  from 70 to 33 patches. Great!

-- 
Jens Axboe

[PATCH 19/33] scsi_debug: support sg chaining

[Jens Axboe]

Cc: dgilbert@interlog.com
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 drivers/scsi/scsi_debug.c |   30 ++++++++++++++++--------------
 1 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 4cd9c58..46a3e07 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -38,6 +38,7 @@
 #include <linux/proc_fs.h>
 #include <linux/vmalloc.h>
 #include <linux/moduleparam.h>
+#include <linux/scatterlist.h>
 
 #include <linux/blkdev.h>
 #include "scsi.h"
@@ -600,7 +601,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 	int k, req_len, act_len, len, active;
 	void * kaddr;
 	void * kaddr_off;
-	struct scatterlist * sgpnt;
+	struct scatterlist * sg;
 
 	if (0 == scp->request_bufflen)
 		return 0;
@@ -619,16 +620,16 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 			scp->resid = req_len - act_len;
 		return 0;
 	}
-	sgpnt = (struct scatterlist *)scp->request_buffer;
 	active = 1;
-	for (k = 0, req_len = 0, act_len = 0; k < scp->use_sg; ++k, ++sgpnt) {
+	req_len = act_len = 0;
+	scsi_for_each_sg(scp, sg, scp->use_sg, k) {
 		if (active) {
 			kaddr = (unsigned char *)
-				kmap_atomic(sgpnt->page, KM_USER0);
+				kmap_atomic(sg->page, KM_USER0);
 			if (NULL == kaddr)
 				return (DID_ERROR << 16);
-			kaddr_off = (unsigned char *)kaddr + sgpnt->offset;
-			len = sgpnt->length;
+			kaddr_off = (unsigned char *)kaddr + sg->offset;
+			len = sg->length;
 			if ((req_len + len) > arr_len) {
 				active = 0;
 				len = arr_len - req_len;
@@ -637,7 +638,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 			kunmap_atomic(kaddr, KM_USER0);
 			act_len += len;
 		}
-		req_len += sgpnt->length;
+		req_len += sg->length;
 	}
 	if (scp->resid)
 		scp->resid -= act_len;
@@ -653,7 +654,7 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 	int k, req_len, len, fin;
 	void * kaddr;
 	void * kaddr_off;
-	struct scatterlist * sgpnt;
+	struct scatterlist * sg;
 
 	if (0 == scp->request_bufflen)
 		return 0;
@@ -668,13 +669,14 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 		memcpy(arr, scp->request_buffer, len);
 		return len;
 	}
-	sgpnt = (struct scatterlist *)scp->request_buffer;
-	for (k = 0, req_len = 0, fin = 0; k < scp->use_sg; ++k, ++sgpnt) {
-		kaddr = (unsigned char *)kmap_atomic(sgpnt->page, KM_USER0);
+	sg = scsi_sglist(scp);
+	req_len = fin = 0;
+	for (k = 0; k < scp->use_sg; ++k, sg = sg_next(sg)) {
+		kaddr = (unsigned char *)kmap_atomic(sg->page, KM_USER0);
 		if (NULL == kaddr)
 			return -1;
-		kaddr_off = (unsigned char *)kaddr + sgpnt->offset;
-		len = sgpnt->length;
+		kaddr_off = (unsigned char *)kaddr + sg->offset;
+		len = sg->length;
 		if ((req_len + len) > max_arr_len) {
 			len = max_arr_len - req_len;
 			fin = 1;
@@ -683,7 +685,7 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 		kunmap_atomic(kaddr, KM_USER0);
 		if (fin)
 			return req_len + len;
-		req_len += sgpnt->length;
+		req_len += sg->length;
 	}
 	return req_len;
 }
-- 
1.5.3.rc0.90.gbaa79

Re: [PATCH 19/33] scsi_debug: support sg chaining

[Douglas Gilbert]

Jens Axboe wrote:
> Cc: dgilbert@interlog.com
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> ---
>  drivers/scsi/scsi_debug.c |   30 ++++++++++++++++--------------
>  1 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 4cd9c58..46a3e07 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -38,6 +38,7 @@
>  #include <linux/proc_fs.h>
>  #include <linux/vmalloc.h>
>  #include <linux/moduleparam.h>
> +#include <linux/scatterlist.h>
>  
>  #include <linux/blkdev.h>
>  #include "scsi.h"
> @@ -600,7 +601,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
>  	int k, req_len, act_len, len, active;
>  	void * kaddr;
>  	void * kaddr_off;
> -	struct scatterlist * sgpnt;
> +	struct scatterlist * sg;
>  
>  	if (0 == scp->request_bufflen)
>  		return 0;
> @@ -619,16 +620,16 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
>  			scp->resid = req_len - act_len;
>  		return 0;
>  	}
> -	sgpnt = (struct scatterlist *)scp->request_buffer;
>  	active = 1;
> -	for (k = 0, req_len = 0, act_len = 0; k < scp->use_sg; ++k, ++sgpnt) {
> +	req_len = act_len = 0;
> +	scsi_for_each_sg(scp, sg, scp->use_sg, k) {
>  		if (active) {
>  			kaddr = (unsigned char *)
> -				kmap_atomic(sgpnt->page, KM_USER0);
> +				kmap_atomic(sg->page, KM_USER0);
>  			if (NULL == kaddr)
>  				return (DID_ERROR << 16);
> -			kaddr_off = (unsigned char *)kaddr + sgpnt->offset;
> -			len = sgpnt->length;
> +			kaddr_off = (unsigned char *)kaddr + sg->offset;
> +			len = sg->length;
>  			if ((req_len + len) > arr_len) {
>  				active = 0;
>  				len = arr_len - req_len;
> @@ -637,7 +638,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
>  			kunmap_atomic(kaddr, KM_USER0);
>  			act_len += len;
>  		}
> -		req_len += sgpnt->length;
> +		req_len += sg->length;
>  	}
>  	if (scp->resid)
>  		scp->resid -= act_len;
> @@ -653,7 +654,7 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
>  	int k, req_len, len, fin;
>  	void * kaddr;
>  	void * kaddr_off;
> -	struct scatterlist * sgpnt;
> +	struct scatterlist * sg;
>  
>  	if (0 == scp->request_bufflen)
>  		return 0;
> @@ -668,13 +669,14 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
>  		memcpy(arr, scp->request_buffer, len);
>  		return len;
>  	}
> -	sgpnt = (struct scatterlist *)scp->request_buffer;
> -	for (k = 0, req_len = 0, fin = 0; k < scp->use_sg; ++k, ++sgpnt) {
> -		kaddr = (unsigned char *)kmap_atomic(sgpnt->page, KM_USER0);
> +	sg = scsi_sglist(scp);
> +	req_len = fin = 0;
> +	for (k = 0; k < scp->use_sg; ++k, sg = sg_next(sg)) {
> +		kaddr = (unsigned char *)kmap_atomic(sg->page, KM_USER0);
>  		if (NULL == kaddr)
>  			return -1;
> -		kaddr_off = (unsigned char *)kaddr + sgpnt->offset;
> -		len = sgpnt->length;
> +		kaddr_off = (unsigned char *)kaddr + sg->offset;
> +		len = sg->length;
>  		if ((req_len + len) > max_arr_len) {
>  			len = max_arr_len - req_len;
>  			fin = 1;
> @@ -683,7 +685,7 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
>  		kunmap_atomic(kaddr, KM_USER0);
>  		if (fin)
>  			return req_len + len;
> -		req_len += sgpnt->length;
> +		req_len += sg->length;
>  	}
>  	return req_len;
>  }

Signed-off-by: Douglas Gilbert <dougg@torque.net>


In a post titled: "Re: [PATCH 4/9] scsi_debug: convert to
                   use the data buffer accessors"
FUJITA Tomonori wrote:
> On Fri, 07 Sep 2007 15:12:06 -0400
> Douglas Gilbert <dougg@torque.net> wrote:
>
>> FUJITA Tomonori wrote:
>>> From: Boaz Harrosh <bharrosh@panasas.com>
>>>
>>> - remove the unnecessary map_single path.
>>>
>>> - convert to use the new accessors for the sg lists and the
>>> parameters.
>>>
>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> Signed-off-by: Douglas Gilbert <dougg@torque.net>
>
> This patch is on the top of the following patch:
>
> http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git;a=commitdiff;h=63499aaafa6ec0c107a925574c2af84a2cd86703
>
> Can we get your ACK on this too?

Done.

> Probably, it would be fine to integrate these patches.

Yes that is fine.

When the dust settles, I can send a patch of my various mode
and log pages additions to scsi_debug in my working copy.

Doug Gilbert