Re: QUEUE_FLAG_CLUSTER: not working in 2.6.24 ?

[Jens Axboe <jens.axboe@oracle.com>]

On Thu, Dec 13 2007, Mark Lord wrote:
> Jens Axboe wrote:
> >On Thu, Dec 13 2007, Mark Lord wrote:
> >>Jens Axboe wrote:
> >>>On Thu, Dec 13 2007, Jens Axboe wrote:
> >>>>On Thu, Dec 13 2007, Mark Lord wrote:
> >>>>>Jens Axboe wrote:
> >>>>>>On Thu, Dec 13 2007, Mark Lord wrote:
> >>>>>>>Mark Lord wrote:
> >>>>>>>>Jens Axboe wrote:
> >>>>>>>>>On Thu, Dec 13 2007, Mark Lord wrote:
> >>>>>>>>>>Matthew Wilcox wrote:
> >>>>>>>>>>>On Thu, Dec 13, 2007 at 01:48:18PM -0500, Mark Lord wrote:
> >>>>>>>>>>>>Problem confirmed.  2.6.23.8 regularly generates segments up to 
> >>>>>>>>>>>>64KB for libata,
> >>>>>>>>>>>>but 2.6.24 uses only 4KB segments and a *few* 8KB segments.
> >>>>>>>>>>>Just a suspicion ... could this be slab vs slub?  ie check your 
> >>>>>>>>>>>configs
> >>>>>>>>>>>are the same / similar between the two kernels.
> >>>>>>>>>>..
> >>>>>>>>>>
> >>>>>>>>>>Mmmm.. a good thought, that one.
> >>>>>>>>>>But I just rechecked, and both have CONFIG_SLAB=y
> >>>>>>>>>>
> >>>>>>>>>>My guess is that something got changed around when Jens
> >>>>>>>>>>reworked the block layer for 2.6.24.
> >>>>>>>>>>I'm going to dig around in there now.
> >>>>>>>>>I didn't rework the block layer for 2.6.24 :-). The core block 
> >>>>>>>>>layer
> >>>>>>>>>changes since 2.6.23 are:
> >>>>>>>>>
> >>>>>>>>>- Support for empty barriers. Not a likely candidate.
> >>>>>>>>>- Shared tag queue fixes. Totally unlikely.
> >>>>>>>>>- sg chaining support. Not likely.
> >>>>>>>>>- The bio changes from Neil. Of the bunch, the most likely 
> >>>>>>>>>suspects in
> >>>>>>>>>this area, since it changes some of the code involved with merges 
> >>>>>>>>>and
> >>>>>>>>>blk_rq_map_sg().
> >>>>>>>>>- Lots of simple stuff, again very unlikely.
> >>>>>>>>>
> >>>>>>>>>Anyway, it sounds odd for this to be a block layer problem if you 
> >>>>>>>>>do see
> >>>>>>>>>occasional segments being merged. So it sounds more like the input 
> >>>>>>>>>data
> >>>>>>>>>having changed.
> >>>>>>>>>
> >>>>>>>>>Why not just bisect it?
> >>>>>>>>..
> >>>>>>>>
> >>>>>>>>Because the early 2.6.24 series failed to boot on this machine
> >>>>>>>>due to bugs in the block layer -- so the code that caused this 
> >>>>>>>>regression
> >>>>>>>>is probably in the stuff from before the kernels became usable here.
> >>>>>>>..
> >>>>>>>
> >>>>>>>That sounds more harsh than intended --> the earlier 2.6.24 kernels 
> >>>>>>>(up to
> >>>>>>>the first couple of -rc* ones failed here because of 
> >>>>>>>incompatibilities
> >>>>>>>between the block/bio changes and libata.
> >>>>>>>
> >>>>>>>That's better, I think! 
> >>>>>>No worries, I didn't pick it up as harsh just as an odd conclusion :-)
> >>>>>>
> >>>>>>If I were you, I'd just start from the first -rc that booted for you. 
> >>>>>>If
> >>>>>>THAT has the bug, then we'll think of something else. If you don't get
> >>>>>>anywhere, I can run some tests tomorrow and see if I can reproduce it
> >>>>>>here.
> >>>>>..
> >>>>>
> >>>>>I believe that *anyone* can reproduce it, since it's broken long before
> >>>>>the requests ever get to SCSI or libata.  Which also means that 
> >>>>>*anyone*
> >>>>>who wants to can bisect it, as well.
> >>>>>
> >>>>>I don't do "bisects".
> >>>>It was just a suggestion on how to narrow it down, do as you see fit.
> >>>>
> >>>>>But I will dig a bit more and see if I can find the culprit.
> >>>>Sure, I'll dig around as well.
> >>>Just tried something simple. I only see one 12kb segment so far, so not
> >>>a lot by any stretch. I also DONT see any missed merges signs, so it
> >>>would appear that the pages in the request are simply not contigious
> >>>physically.
> >>>
> >>>diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> >>>index e30b1a4..1e34b6f 100644
> >>>--- a/block/ll_rw_blk.c
> >>>+++ b/block/ll_rw_blk.c
> >>>@@ -1330,6 +1330,8 @@ int blk_rq_map_sg(struct request_queue *q, struct 
> >>>request *rq,
> >>>				goto new_segment;
> >>>
> >>>			sg->length += nbytes;
> >>>+			if (sg->length > 8192)
> >>>+				printk("sg_len=%d\n", sg->length);
> >>>		} else {
> >>>new_segment:
> >>>			if (!sg)
> >>>@@ -1349,6 +1351,8 @@ new_segment:
> >>>				sg = sg_next(sg);
> >>>			}
> >>>
> >>>+			if (bvprv && (page_address(bvprv->bv_page) + 
> >>>bvprv->bv_len == page_address(bvec->bv_page)))
> >>>+				printk("missed merge\n");
> >>>			sg_set_page(sg, bvec->bv_page, nbytes, 
> >>>			bvec->bv_offset);
> >>>			nsegs++;
> >>>		}
> >>>
> >>..
> >>
> >>Yeah, the first part is similar to my own hack.
> >>
> >>For testing, try "dd if=/dev/sda of=/dev/null bs=4096k".
> >>That *really* should end up using contiguous pages on most systems.
> >>
> >>I figured out the git thing, and am now building some in-between kernels 
> >>to try.
> >
> >OK, it's a vm issue, I have tens of thousand "backward" pages after a
> >boot - IOW, bvec->bv_page is the page before bvprv->bv_page, not
> >reverse. So it looks like that bug got reintroduced.
> ...
> 
> Mmm.. shouldn't one of the front- or back- merge logics work for either 
> order?

I think you are misunderstanding the merging. The front/back bits are
for contig on disk, this is sg segment merging. We can only join pieces
that are contig in memory, otherwise the result would not be pretty :-)

-- 
Jens Axboe