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