Re: Bugs on Linux 2.6.18-rc2 sg code?

[Douglas Gilbert <dougg@torque.net>]

Matthew Wilcox wrote:
> On Sat, Aug 19, 2006 at 10:41:09AM -0700, Andrew Morton wrote:
>> ow, GFP_ATOMIC is bad.  Can we avoid that?
> 
> I believe all the GFP_ATOMIC calls in sg.c can be downgraded to
> GFP_KERNEL.

Well, if memory serves, we have been around this loop
before. The GFP_ATOMIC flags were removed, users
complained about unexplained, unbounded waits unrelated
to the SCSI transport or device. So the GFP_ATOMIC
flags were put back again. The semantics of the
function that calls alloc_pages() is to cope with a
(fast) failure, and if it is an ambit claim (e.g. the
reserve buffer allocation on open() ) then lower the
order by 1 (halve the requested allocation) then try again.

The semantics of SCSI command injection in the sg driver
have been to fetch the resources needed and inject the
command to the mid level as quickly as possible.
If there is a resource problem, exit and tell the user
space promptly.

So the action required seems to be __GFP_NORETRY if that
works as its name suggests.

> sg_add_sfp is only called from sg_open(), so the GFP_ATOMIC there can
> be downgraded to GFP_KERNEL.

That is called on open() so GFP_KERNEL | __GFP_NORETRY
should be ok.

> There's one in sg_common_write that can be downgraded. It's called
> from sg_write and sg_ioctl (via sg_new_write).  No locks are held at
> those points.

That one is the last argument to scsi_execute_async().
I didn't add that and don't know what it is for.
Perhaps James could comment.

> st_map_user_pages uses GFP_ATOMIC.  It's only called from
> sg_build_direct, which is only called from sg_start_req, which is only
> called from sg_new_write, which is safe per above.

GFP_KERNEL | __GFP_NORETRY

> sg_page_malloc uses GFP_ATOMIC.  It's only called from sg_build_indirect.
> sg_build_indirect is called from sg_start_req (previously shown safe),
> and sg_build_reserve.  sg_build_reserve is safe as it's only called from
> sg_ioctl and sg_add_sfp (previously shown safe).
> 
> sg_build_sgat is called from sg_build_direct and sg_build_indirect,
> both of which have previously been shown safe.
> 
> Please check my reasoning, but I'm pretty confident that none of these
> paths are called with any locks held.

None of the kmalloc()s or alloc_pages() are called
from softirq callback ( sg_cmd_done() in sg's case) that
I can see. I don't think locks taken by the sg driver
should be a problem.


BTW I'm initially trying to fix a bug added by someone
else into the sg driver in lk 2.6.16 . The code I'm
proposing isn't optimal but, by reverting the code (more
or less) to what it was, at least I know it was well
tested :-) Thus I was hoping to get the fix into
lk 2.6.18 . The other changes being discussed could go
into the lk 2.6.19 cycle.

Doug Gilbert