lkml.org 
[lkml]   [2008]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] BUG: nr_phys_segments cannot be less than nr_hw_segments
From
On Fri, 10 Oct 2008 14:37:19 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Fri, Oct 10 2008, FUJITA Tomonori wrote:
> > On Fri, 10 Oct 2008 14:03:44 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> >
> > > On Tue, Oct 07 2008, FUJITA Tomonori wrote:
> > > > On Thu, 2 Oct 2008 19:13:57 +0200
> > > > Jens Axboe <jens.axboe@oracle.com> 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? :)

Yeah, it's on this weekend todo list. :) I want to look at the code
again and make sure I correctly understand the problem.


\
 
 \ /
  Last update: 2008-10-10 14:59    [W:0.052 / U:1.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site