Re: [md PATCH 0/5] Stop using bi_phys_segments as a counter

[Shaohua Li <shli@kernel.org>]

On Tue, Nov 22, 2016 at 11:25:04AM +1100, Neil Brown wrote:
> On Tue, Nov 22 2016, Shaohua Li wrote:
> 
> > On Mon, Nov 21, 2016 at 12:19:43PM +1100, Neil Brown wrote:
> >> There are 2 problems with using bi_phys_segments as a counter
> >> 1/ we only use 16bits, which limits bios to 256M
> >> 2/ it is poor form to reuse a field like this.  It interferes
> >>    with other changes to bios.
> >> 
> >> We need to clean up a few things before we can change the use the
> >> counter which is now available inside a bio.
> >> 
> >> I have only tested this lightly.  More review and testing would be
> >> appreciated.
> >
> > So without the accounting, we:
> > - don't do bio completion trace
> 
> Yes, but hopefully that will be added back to bio_endio() soon.
> 
> > - call md_write_start/md_write_end excessively, which involves atomic operation.
> 
> raid5_inc_bio_active_stripes() did an atomic operation.  I don't think
> there is a net increase in the number of atomic operations.

That's different. md_write_start/end uses a global atomic.
raid5_inc_bio_active_stripes uses a bio atomic. So we have more cache bouncing now.
 
> >
> > Not big problems. But we are actually reusing __bi_remaining, I'm wondering why
> > we not explicitly reuse it. Eg, adds bio_dec_remaining_return() and uses it
> > like raid5_dec_bi_active_stripes.
> 
> Because using it exactly the same way that other places use it leads to
> fewer surprises, now or later.
> And I think that the effort to rearrange the code so that we could just
> call bio_endio() brought real improvements in code clarity and
> simplicity.

Not the same way. The return_bi list and retry list fix are still good. We can
replace the bio_endio in your patch with something like:
if (bio_dec_remaining_return() == 0) {
	trace_block_bio_complete()
	md_write_end()
	bio_endio();
}
This will give us better control when to end io.

Thanks,
Shaohua