[ Resending in plain text... sorry for the duplicate ] Hi, On Mon, Jul 30, 2012 at 6:14 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Tue, Jul 31, 2012 at 08:55:59AM +0800, majianpeng wrote: > > On 2012-07-31 05:42 Dave Chinner <david@xxxxxxxxxxxxx> Wrote: > > >On Mon, Jul 30, 2012 at 03:14:28PM +0800, majianpeng wrote: > > >> When exec bio_alloc, the bi_rw is zero.But after calling > > >> bio_add_page, > > >> it will use bi_rw. > > >> Fox example, in functiion __bio_add_page,it will call > > >> merge_bvec_fn(). > > >> The merge_bvec_fn of raid456 will use the bi_rw to judge the merge. > > >> >> if ((bvm->bi_rw & 1) == WRITE) > > >> >> return biovec->bv_len; /* always allow writes to be mergeable */ > > > > > >So if bio_add_page() requires bi_rw to be set, then shouldn't it be > > >set up for every caller? I noticed there are about 50 call sites for > > >bio_add_page(), and you've only touched about 10 of them. Indeed, I > > >notice that the RAID0/1 code uses bio_add_page, and as that can be > > >stacked on top of RAID456, it also needs to set bi_rw correctly. > > >As a result, your patch set is nowhere near complete, not does it > > >document that bio_add_page requires that bi_rw be set before calling > > >(which is the new API requirement, AFAICT). > > There are many place call bio_add_page and I send some of those. Because > > my abilty, so I only send > > some patchs which i understand clearly. > > Sure, but my point is that there is no point changing only a few and > ignoring the great majority of callers. Either fix them all, fix it > some other way (e.g. API change), or remove the code from the RAID5 > function that requires it. A while back, we tried to address this by changing the alloc functions to take rw argument and set it (as per Jens suggestion). I guess the patch did not make it in. Please check: https://lkml.org/lkml/2011/7/11/275 and the follow ups. If needed, I can dust up that patch and resend it. > <snip> > > It's entirely possible that when bi_rw was added to struct > bvec_merge_data, the person who added it was mistaken that bi_rw was > set at this point in time when in fact it never has been. Hence it's > presence and reliance on it would be a bug. > > That's what I'm asking - is this actually beneificial, or should it > simply be removed from struct bvec_merge_data? Data is needed to > answer that question.... There are cases where we found it really beneficial to know the rw field to decide if the can be really merged or not. Regards, Muthu > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx > -- > 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/ -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
