Re: [PATCH] BUG: nr_phys_segments cannot be less than nr_hw_segments

[Jens Axboe]

On Fri, Oct 10 2008, FUJITA Tomonori wrote:
> On Fri, 10 Oct 2008 14:03:44 +0200
> Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> 
> > On Tue, Oct 07 2008, FUJITA Tomonori wrote:
> > > On Thu, 2 Oct 2008 19:13:57 +0200
> > > Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> > > 
> > > > On Thu, Oct 02 2008, James Bottomley wrote:
> > > > > On Thu, 2008-10-02 at 18:58 +0200, Jens Axboe wrote:
> > > > > > On Thu, Oct 02 2008, James Bottomley wrote:
> > > > > > > The bug would appear to be that we sometimes only look at q->max_sectors
> > > > > > > when deciding on mergability.  Either we have to insist on max_sectors
> > > > > > > <= hw_max_sectors, or we have to start using min(q->max_sectors,
> > > > > > > q->max_hw_sectors) for this.
> > > > > > 
> > > > > > q->max_sectors MUST always be <= q->max_hw_sectors, otherwise we could
> > > > > > be sending down requests that are too large for the device to handle. So
> > > > > > that condition would be a big bug. The sysfs interface checks for this,
> > > > > > and blk_queue_max_sectors() makes sure that is true as well.
> > > > > 
> > > > > Yes, that seems always to be enforced.  Perhaps there are other ways of
> > > > > tripping this problem ... I'm still sure if it occurs it's because we do
> > > > > a physical merge where a virtual merge is forbidden.
> > > > > 
> > > > > > The fixes proposed still look weird. There is no phys vs hw segment
> > > > > > constraints, the request must adhere to the limits set by both. It's
> > > > > > mostly a moot point anyway, as 2.6.28 will get rid of the hw accounting
> > > > > > anyway.
> > > > > 
> > > > > Agree all three proposed fixes look wrong.  However, if it's what I
> > > > > think, just getting rid of hw accounting won't fix the problem because
> > > > > we'll still have the case where a physical merge is forbidden by iommu
> > > > > constraints ... this still needs to be accounted for.
> > > > > 
> > > > > What we really need is a demonstration of what actually is going
> > > > > wrong ...
> > > > 
> > > > Yep, IIRC we both asked for that the last time as well... Nikanth?
> > > 
> > > Possibly, blk_phys_contig_segment might miscalculate
> > > q->max_segment_size?
> > > 
> > > blk_phys_contig_segment does:
> > > 
> > > req->biotail->bi_size + next_req->bio->bi_size > q->max_segment_size;
> > > 
> > > But it's possible that req->biotail and the previous bio are supposed
> > > be merged into one segment? Then we could create too large segment
> > > here.
> > 
> > Hmm yes, that looks like it could indeed be a problem!
> 
> I think so.
> 
> 
> > We could fix this
> > with similar logic to what we used to do for the hw merging, keep track
> > of the current segment size that this bio belongs to, so it would end up
> > ala
> 
> Yeah, exactly.
> 
> You want a fix for this 2.6.28? Or disable this feature for 2.6.28?

Lets fix it. It wont be part of the initial merge, since it'll need some
dedicated testing, but we can get it there for 2.6.28. Shall I interpret
your message as willingness to write up the fix? :)

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/