[PATCH] scsi_debug: disable clustering

[PATCH] scsi_debug: disable clustering

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] scsi_debug: disable clustering

scsi_debug does at several places:

for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
	kaddr = (unsigned char *)
		kmap_atomic(sg_page(sg), KM_USER0);


We cannot do something like that with the clustering enabled (or we
can use scsi_kmap_atomic_sg).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/scsi_debug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1541c17..d1777a9 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -222,7 +222,7 @@ static struct scsi_host_template sdebug_driver_template = {
 	.cmd_per_lun =		16,
 	.max_sectors =		0xffff,
 	.unchecked_isa_dma = 	0,
-	.use_clustering = 	ENABLE_CLUSTERING,
+	.use_clustering = 	DISABLE_CLUSTERING,
 	.module =		THIS_MODULE,
 };
 
-- 
1.5.3.7

Re: [PATCH] scsi_debug: disable clustering

[Douglas Gilbert]

FUJITA Tomonori wrote:
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] scsi_debug: disable clustering
> 
> scsi_debug does at several places:
> 
> for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
> 	kaddr = (unsigned char *)
> 		kmap_atomic(sg_page(sg), KM_USER0);
> 
> 
> We cannot do something like that with the clustering enabled (or we
> can use scsi_kmap_atomic_sg).
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Thanks, Tomo.

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


> ---
>  drivers/scsi/scsi_debug.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 1541c17..d1777a9 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -222,7 +222,7 @@ static struct scsi_host_template sdebug_driver_template = {
>  	.cmd_per_lun =		16,
>  	.max_sectors =		0xffff,
>  	.unchecked_isa_dma = 	0,
> -	.use_clustering = 	ENABLE_CLUSTERING,
> +	.use_clustering = 	DISABLE_CLUSTERING,
>  	.module =		THIS_MODULE,
>  };
>  

Re: [PATCH] scsi_debug: disable clustering

[Matthew Wilcox]

On Sat, Feb 16, 2008 at 11:57:15PM +0900, FUJITA Tomonori wrote:
> scsi_debug does at several places:
> 
> for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
> 	kaddr = (unsigned char *)
> 		kmap_atomic(sg_page(sg), KM_USER0);
> 
> 
> We cannot do something like that with the clustering enabled (or we
> can use scsi_kmap_atomic_sg).

Why not?  Is KM_USER0 used for something else with clstering enabled?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] scsi_debug: disable clustering

[James Bottomley]

On Sun, 2008-02-17 at 07:10 -0700, Matthew Wilcox wrote:
> On Sat, Feb 16, 2008 at 11:57:15PM +0900, FUJITA Tomonori wrote:
> > scsi_debug does at several places:
> > 
> > for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
> > 	kaddr = (unsigned char *)
> > 		kmap_atomic(sg_page(sg), KM_USER0);
> > 
> > 
> > We cannot do something like that with the clustering enabled (or we
> > can use scsi_kmap_atomic_sg).
> 
> Why not?  Is KM_USER0 used for something else with clstering enabled?

No, he means that kmap_atomic can only map a page of data.  This makes
single page only sg list entries and input assumption into this loop.
with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
accidentally works most of the time because of the way kmap functions.

James

Re: [PATCH] scsi_debug: disable clustering

[Matthew Wilcox]

On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> No, he means that kmap_atomic can only map a page of data.  This makes
> single page only sg list entries and input assumption into this loop.
> with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> accidentally works most of the time because of the way kmap functions.

Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
been working on ... this loop should work fine with clustering as it
takes account of the sg potentially having multiple pages:

        scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
                struct page *sgpage = sg_page(sg);
                unsigned int to_off = sg->offset;
                unsigned int sg_copy = sg->length;
                if (sg_copy > len)
                        sg_copy = len;
                len -= sg_copy;

                while (sg_copy > 0) {
                        char *vto, *vfrom;
                        unsigned int page_copy;

                        if (from_off > to_off)
                                page_copy = PAGE_SIZE - from_off;
                        else
                                page_copy = PAGE_SIZE - to_off;
                        if (page_copy > sg_copy)
                                page_copy = sg_copy;

                        vfrom = get_data_page(data_pfn);
                        vto = get_sg_page(sgpage);
                        memcpy(vto + to_off, vfrom + from_off, page_copy);
                        put_sg_page(vto);
                        put_data_page(vfrom);

                        from_off += page_copy;
                        if (from_off == PAGE_SIZE) {
                                from_off = 0;
                                data_pfn++;
                        }
                        to_off += page_copy;
                        if (to_off == PAGE_SIZE) {
                                to_off = 0;
                                sgpage++;
                        }
                        sg_copy -= page_copy;
                }
                if (!len)
                        break;
        }

get_data_page() and get_sg_page() each do a kmap_atomic, so I was
concerned that I might be out of KM_USER* pages.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] scsi_debug: disable clustering

[FUJITA Tomonori]

On Sun, 17 Feb 2008 07:28:48 -0700
Matthew Wilcox <matthew@wil.cx> wrote:

> On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> > No, he means that kmap_atomic can only map a page of data.  This makes
> > single page only sg list entries and input assumption into this loop.
> > with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> > accidentally works most of the time because of the way kmap functions.
> 
> Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
> been working on ... this loop should work fine with clustering as it
> takes account of the sg potentially having multiple pages:
> 
>         scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
>                 struct page *sgpage = sg_page(sg);
>                 unsigned int to_off = sg->offset;
>                 unsigned int sg_copy = sg->length;
>                 if (sg_copy > len)
>                         sg_copy = len;
>                 len -= sg_copy;

stex driver has a similar function to copies data between a buffer and
a scatter list. I think that scsi_kmap_atomic_sg is a bit primitive
(and not very popular).  I'll send a patch to add a helper function to
scsi_lib.c that copies data between a buffer and a scatter list. It
would be useful for several drivers.

Re: [PATCH] scsi_debug: disable clustering

[James Bottomley]

On Sun, 2008-02-17 at 23:52 +0900, FUJITA Tomonori wrote:
> On Sun, 17 Feb 2008 07:28:48 -0700
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> > > No, he means that kmap_atomic can only map a page of data.  This makes
> > > single page only sg list entries and input assumption into this loop.
> > > with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> > > accidentally works most of the time because of the way kmap functions.
> > 
> > Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
> > been working on ... this loop should work fine with clustering as it
> > takes account of the sg potentially having multiple pages:
> > 
> >         scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
> >                 struct page *sgpage = sg_page(sg);
> >                 unsigned int to_off = sg->offset;
> >                 unsigned int sg_copy = sg->length;
> >                 if (sg_copy > len)
> >                         sg_copy = len;
> >                 len -= sg_copy;
> 
> stex driver has a similar function to copies data between a buffer and
> a scatter list. I think that scsi_kmap_atomic_sg is a bit primitive
> (and not very popular).  I'll send a patch to add a helper function to
> scsi_lib.c that copies data between a buffer and a scatter list. It
> would be useful for several drivers.

Actually, if you're going to sweep up them all, libata also does this.

However, mapping and copying data isn't a SCSI specific function, it's
one any virtual block driver should do, so I think block might be the
correct location for such a function.

James

Re: [PATCH] scsi_debug: disable clustering

[FUJITA Tomonori]

On Sun, 17 Feb 2008 09:02:14 -0600
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> On Sun, 2008-02-17 at 23:52 +0900, FUJITA Tomonori wrote:
> > On Sun, 17 Feb 2008 07:28:48 -0700
> > Matthew Wilcox <matthew@wil.cx> wrote:
> > 
> > > On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> > > > No, he means that kmap_atomic can only map a page of data.  This makes
> > > > single page only sg list entries and input assumption into this loop.
> > > > with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> > > > accidentally works most of the time because of the way kmap functions.
> > > 
> > > Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
> > > been working on ... this loop should work fine with clustering as it
> > > takes account of the sg potentially having multiple pages:
> > > 
> > >         scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
> > >                 struct page *sgpage = sg_page(sg);
> > >                 unsigned int to_off = sg->offset;
> > >                 unsigned int sg_copy = sg->length;
> > >                 if (sg_copy > len)
> > >                         sg_copy = len;
> > >                 len -= sg_copy;
> > 
> > stex driver has a similar function to copies data between a buffer and
> > a scatter list. I think that scsi_kmap_atomic_sg is a bit primitive
> > (and not very popular).  I'll send a patch to add a helper function to
> > scsi_lib.c that copies data between a buffer and a scatter list. It
> > would be useful for several drivers.
> 
> Actually, if you're going to sweep up them all, libata also does this.

Thanks, I'll look at it.


> However, mapping and copying data isn't a SCSI specific function, it's
> one any virtual block driver should do, so I think block might be the
> correct location for such a function.

I see. It could go to lib/scatterlist.c or block/ I guess.

Re: [PATCH] scsi_debug: disable clustering

[James Bottomley]

On Mon, 2008-02-18 at 00:11 +0900, FUJITA Tomonori wrote:
> On Sun, 17 Feb 2008 09:02:14 -0600
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > 
> > On Sun, 2008-02-17 at 23:52 +0900, FUJITA Tomonori wrote:
> > > On Sun, 17 Feb 2008 07:28:48 -0700
> > > Matthew Wilcox <matthew@wil.cx> wrote:
> > > 
> > > > On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> > > > > No, he means that kmap_atomic can only map a page of data.  This makes
> > > > > single page only sg list entries and input assumption into this loop.
> > > > > with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> > > > > accidentally works most of the time because of the way kmap functions.
> > > > 
> > > > Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
> > > > been working on ... this loop should work fine with clustering as it
> > > > takes account of the sg potentially having multiple pages:
> > > > 
> > > >         scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
> > > >                 struct page *sgpage = sg_page(sg);
> > > >                 unsigned int to_off = sg->offset;
> > > >                 unsigned int sg_copy = sg->length;
> > > >                 if (sg_copy > len)
> > > >                         sg_copy = len;
> > > >                 len -= sg_copy;
> > > 
> > > stex driver has a similar function to copies data between a buffer and
> > > a scatter list. I think that scsi_kmap_atomic_sg is a bit primitive
> > > (and not very popular).  I'll send a patch to add a helper function to
> > > scsi_lib.c that copies data between a buffer and a scatter list. It
> > > would be useful for several drivers.
> > 
> > Actually, if you're going to sweep up them all, libata also does this.
> 
> Thanks, I'll look at it.
> 
> 
> > However, mapping and copying data isn't a SCSI specific function, it's
> > one any virtual block driver should do, so I think block might be the
> > correct location for such a function.
> 
> I see. It could go to lib/scatterlist.c or block/ I guess.

Actually, lib/scatterlist is probably better.

James