[PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[FUJITA Tomonori]

unsigned short is too small for sizeof(struct scatterlist) *
min(q->max_hw_segments, q->max_phys_segments).

This fixes memory leak with 4096 segments since 16 (likely sg size
with x86) * 4096 sets sglist_len to zero.

This might not happen without sg chaining support.

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

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 2fc24e7..2c44bb0 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -114,7 +114,7 @@ static struct class_interface sg_interface = {
 
 typedef struct sg_scatter_hold { /* holding area for scsi scatter gather info */
 	unsigned short k_use_sg; /* Count of kernel scatter-gather pieces */
-	unsigned short sglist_len; /* size of malloc'd scatter-gather list ++ */
+	unsigned sglist_len; /* size of malloc'd scatter-gather list ++ */
 	unsigned bufflen;	/* Size of (aggregate) data buffer */
 	unsigned b_malloc_len;	/* actual len malloc'ed in buffer */
 	struct scatterlist *buffer;/* scatter list */
-- 
1.5.2.4

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[Douglas Gilbert]

FUJITA Tomonori wrote:
> unsigned short is too small for sizeof(struct scatterlist) *
> min(q->max_hw_segments, q->max_phys_segments).
> 
> This fixes memory leak with 4096 segments since 16 (likely sg size
> with x86) * 4096 sets sglist_len to zero.
> 
> This might not happen without sg chaining support.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/sg.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 2fc24e7..2c44bb0 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -114,7 +114,7 @@ static struct class_interface sg_interface = {
>  
>  typedef struct sg_scatter_hold { /* holding area for scsi scatter gather info */
>  	unsigned short k_use_sg; /* Count of kernel scatter-gather pieces */
> -	unsigned short sglist_len; /* size of malloc'd scatter-gather list ++ */
> +	unsigned sglist_len; /* size of malloc'd scatter-gather list ++ */
>  	unsigned bufflen;	/* Size of (aggregate) data buffer */
>  	unsigned b_malloc_len;	/* actual len malloc'ed in buffer */
>  	struct scatterlist *buffer;/* scatter list */

Tomo,
Thanks.

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

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[FUJITA Tomonori]

On Sun, 05 Aug 2007 12:55:16 -0400
Douglas Gilbert <dougg@torque.net> wrote:

> FUJITA Tomonori wrote:
> > unsigned short is too small for sizeof(struct scatterlist) *
> > min(q->max_hw_segments, q->max_phys_segments).
> > 
> > This fixes memory leak with 4096 segments since 16 (likely sg size
> > with x86) * 4096 sets sglist_len to zero.
> > 
> > This might not happen without sg chaining support.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  drivers/scsi/sg.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> > index 2fc24e7..2c44bb0 100644
> > --- a/drivers/scsi/sg.c
> > +++ b/drivers/scsi/sg.c
> > @@ -114,7 +114,7 @@ static struct class_interface sg_interface = {
> >  
> >  typedef struct sg_scatter_hold { /* holding area for scsi scatter gather info */
> >  	unsigned short k_use_sg; /* Count of kernel scatter-gather pieces */
> > -	unsigned short sglist_len; /* size of malloc'd scatter-gather list ++ */
> > +	unsigned sglist_len; /* size of malloc'd scatter-gather list ++ */
> >  	unsigned bufflen;	/* Size of (aggregate) data buffer */
> >  	unsigned b_malloc_len;	/* actual len malloc'ed in buffer */
> >  	struct scatterlist *buffer;/* scatter list */
> 
> Tomo,
> Thanks.
> 
> Signed-off-by: Douglas Gilbert <dougg@torque.net>

Thanks for the quick reply.

Allocating 64K contiguous memory is not good so the next thing to do
is converting sg to use the sg chaining support fully. Or it might be
time to finish the overdue task, to convert sg to use the block layer
functions.

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[Mike Christie]

FUJITA Tomonori wrote:
> Allocating 64K contiguous memory is not good so the next thing to do
> is converting sg to use the sg chaining support fully. Or it might be

For LLDs like aic7xxx, I think we are stuck with a small 
scsi_host_template->sg_tablesize, so to continue to get large requests 
like before will we have to still allocate large segments?

Is block/scsi_ioctl.c converted to sg chaining in any tree yet? Is that 
in your tree or one of Jen's branches.

> time to finish the overdue task, to convert sg to use the block layer
> functions.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[FUJITA Tomonori]

On Tue, 07 Aug 2007 12:13:41 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> FUJITA Tomonori wrote:
> > Allocating 64K contiguous memory is not good so the next thing to do
> > is converting sg to use the sg chaining support fully. Or it might be
> 
> For LLDs like aic7xxx, I think we are stuck with a small 
> scsi_host_template->sg_tablesize, so to continue to get large requests 
> like before will we have to still allocate large segments?

No. sg.c has:

sizeof(struct scatterlist) * min(q->max_hw_segments, q->max_phys_segments)

If a lld has small max_hw_segments, it doesn't allocate big contiguous
memory.


> Is block/scsi_ioctl.c converted to sg chaining in any tree yet? Is that 
> in your tree or one of Jen's branches.

block/scsi_ioctl.c uses the standard block layer functions, there is
nothing to convert in it. sglist doesn't change the standard block
layer functions much since it doesn't allocate sg list. It changes
only blk_rq_map_sg.

Now only scsi-ml is changed to allocate chaining sg list
properly. Others like cciss are not converted yet, I think. It might
make sense to have the standard block layer functions to allocate
chaining sg list properly. So we could convert to potential consumers
(scsi-ml, sg, ccisss, etc) use them though I'm not sure how many non
scsi-ml needs chaining sg list.

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[Jens Axboe]

On Wed, Aug 08 2007, FUJITA Tomonori wrote:
> Now only scsi-ml is changed to allocate chaining sg list
> properly. Others like cciss are not converted yet, I think. It might
> make sense to have the standard block layer functions to allocate
> chaining sg list properly. So we could convert to potential consumers
> (scsi-ml, sg, ccisss, etc) use them though I'm not sure how many non
> scsi-ml needs chaining sg list.

The scsi chain table allocation/freeing could be made generic. The
reason I didn't do that is - as you list - that probably not many
non-scsi drivers need/want it. If they do, we can put that functionality
in the block layer.

The cciss hardware doesn't support more than 31 segments iirc. Newer
firmwares can do chaining as well, but the linux driver doesn't actually
support it. Once it does, we can add sg chaining support there too.

-- 
Jens Axboe

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[Mike Christie]

FUJITA Tomonori wrote:
> On Tue, 07 Aug 2007 12:13:41 -0500
> Mike Christie <michaelc@cs.wisc.edu> wrote:
> 
>> FUJITA Tomonori wrote:
>>> Allocating 64K contiguous memory is not good so the next thing to do
>>> is converting sg to use the sg chaining support fully. Or it might be
>> For LLDs like aic7xxx, I think we are stuck with a small 
>> scsi_host_template->sg_tablesize, so to continue to get large requests 
>> like before will we have to still allocate large segments?
> 
> No. sg.c has:
> 
> sizeof(struct scatterlist) * min(q->max_hw_segments, q->max_phys_segments)
> 
> If a lld has small max_hw_segments, it doesn't allocate big contiguous
> memory.
> 

Are we talking about the same thing? Are you saying that it does not 
allocate big continuous memory for the scatterlist right? I was asking 
about continuous memory for the buffer sg.c copies data to/from for the 
IO operation. I was saying that currently for something like aic if we 
want to continue to support 8 MB commands (or whatever it was) like 
before, then because its sg_tablesize/max_hw_segments is so small we 
have to continue allocating large IO buffers. That did not change right? 
If so let me know because you save me :)


> 
>> Is block/scsi_ioctl.c converted to sg chaining in any tree yet? Is that 
>> in your tree or one of Jen's branches.
> 
> block/scsi_ioctl.c uses the standard block layer functions, there is
> nothing to convert in it. sglist doesn't change the standard block
> layer functions much since it doesn't allocate sg list. It changes
> only blk_rq_map_sg.
> 
> Now only scsi-ml is changed to allocate chaining sg list
> properly. Others like cciss are not converted yet, I think. It might
> make sense to have the standard block layer functions to allocate
> chaining sg list properly. So we could convert to potential consumers
> (scsi-ml, sg, ccisss, etc) use them though I'm not sure how many non
> scsi-ml needs chaining sg list.

For drivers like sg and st, do mean the the sg list that is passed to 
functions like scsi_execute_async? If we kill that argument, and instead 
have sg.c and other scsi_execute_async callers just call blk helpers 
like blk_rq_map_user then we would not have to worry about drivers like 
sg needing to know about chaining right? I mean sg.c would not every 
interact with a scatterlist. It would just interact with a request and 
the blk helpers map data for it. The scatterlist that sg and st interact 
with is bogus. It gets thrown away in scsi_execute_async and is only 
used for book keeping.

I think it would be best to either have drivers like sg and st use 
blk_rq helpers to map data to requests like in my patches (this way they 
never know about scatterlists), or have sg and st allocate and setup a 
scatterlist properly and then just attach that to the request like we 
did before (we would have to add back in those checks to scsi-ml to 
check for that case again for this latter idea).

cciss is different right since it is doing blk_rq_map_sg and that 
itself. I am not getting into that :)

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[Mike Christie]

Mike Christie wrote:
> For drivers like sg and st, do mean the the sg list that is passed to 
> functions like scsi_execute_async? If we kill that argument, and instead 
> have sg.c and other scsi_execute_async callers just call blk helpers 
> like blk_rq_map_user then we would not have to worry about drivers like 
> sg needing to know about chaining right? I mean sg.c would not every 
> interact with a scatterlist. It would just interact with a request and 
> the blk helpers map data for it.

There should be a return there.

  The scatterlist that sg and st interact
> with is bogus. It gets thrown away in scsi_execute_async and is only 
> used for book keeping.

I mean currently the scatterlist that sg and st use is bogus and gets 
thrown away. If we convert sg and st to use blk_rq_map_user then those 
drivers will not have to interact with a scatterlist at all.

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[Benny Halevy]

Mike Christie wrote:
> Mike Christie wrote:
>> For drivers like sg and st, do mean the the sg list that is passed to 
>> functions like scsi_execute_async? If we kill that argument, and instead 
>> have sg.c and other scsi_execute_async callers just call blk helpers 
>> like blk_rq_map_user then we would not have to worry about drivers like 
>> sg needing to know about chaining right? I mean sg.c would not every 
>> interact with a scatterlist. It would just interact with a request and 
>> the blk helpers map data for it.
> 
> There should be a return there.
> 
>   The scatterlist that sg and st interact
>> with is bogus. It gets thrown away in scsi_execute_async and is only 
>> used for book keeping.
> 
> I mean currently the scatterlist that sg and st use is bogus and gets 
> thrown away. If we convert sg and st to use blk_rq_map_user then those 
> drivers will not have to interact with a scatterlist at all.

I'm not familiar with the dire details but this sounds like a good idea.

Benny

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[FUJITA Tomonori]

On Wed, 08 Aug 2007 11:58:14 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> FUJITA Tomonori wrote:
> > On Tue, 07 Aug 2007 12:13:41 -0500
> > Mike Christie <michaelc@cs.wisc.edu> wrote:
> > 
> >> FUJITA Tomonori wrote:
> >>> Allocating 64K contiguous memory is not good so the next thing to do
> >>> is converting sg to use the sg chaining support fully. Or it might be
> >> For LLDs like aic7xxx, I think we are stuck with a small 
> >> scsi_host_template->sg_tablesize, so to continue to get large requests 
> >> like before will we have to still allocate large segments?
> > 
> > No. sg.c has:
> > 
> > sizeof(struct scatterlist) * min(q->max_hw_segments, q->max_phys_segments)
> > 
> > If a lld has small max_hw_segments, it doesn't allocate big contiguous
> > memory.
> > 
> 
> Are we talking about the same thing? Are you saying that it does not 
> allocate big continuous memory for the scatterlist right? I was asking 
> about continuous memory for the buffer sg.c copies data to/from for the 
> IO operation. I was saying that currently for something like aic if we 
> want to continue to support 8 MB commands (or whatever it was) like 
> before, then because its sg_tablesize/max_hw_segments is so small we 
> have to continue allocating large IO buffers. That did not change right? 
> If so let me know because you save me :)

Oops, sorry. I think that nothing changes about large IO buffers. You
have to contiunue to allocate large IO buffers like before.

Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure

[FUJITA Tomonori]

On Wed, 08 Aug 2007 12:20:43 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> Mike Christie wrote:
> > For drivers like sg and st, do mean the the sg list that is passed to 
> > functions like scsi_execute_async? If we kill that argument, and instead 
> > have sg.c and other scsi_execute_async callers just call blk helpers 
> > like blk_rq_map_user then we would not have to worry about drivers like 
> > sg needing to know about chaining right? I mean sg.c would not every 
> > interact with a scatterlist. It would just interact with a request and 
> > the blk helpers map data for it.
> 
> There should be a return there.
> 
>   The scatterlist that sg and st interact
> > with is bogus. It gets thrown away in scsi_execute_async and is only 
> > used for book keeping.
> 
> I mean currently the scatterlist that sg and st use is bogus and gets 
> thrown away. If we convert sg and st to use blk_rq_map_user then those 
> drivers will not have to interact with a scatterlist at all.

Yeah, we should kill scsi_execute_async. sg should not be the consumer
even if the block layer has functions to allocate chaining sg.

Right now I'm happy as long as scsi-ml has the simple routine to
allocate chaining sg like Jens's branch. So we might easily move it to
the block layer or the block layer might just take care of the sg list
for scsi-ml, etc in the future.