Re: [md PATCH 1/4] md: add block tracing for bio_remapping

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]

On Fri, Nov 18 2016, Shaohua Li wrote:

> On Thu, Nov 17, 2016 at 04:33:33PM +1100, Neil Brown wrote:
>> On Thu, Nov 17 2016, Shaohua Li wrote:
>> 
>> > On Mon, Nov 14, 2016 at 04:30:21PM +1100, Neil Brown wrote:
>> >> The block tracing infrastructure (accessed with blktrace/blkparse)
>> >> supports the tracing of mapping bios from one device to another.
>> >> This is currently used when a bio in a partition is mapped to the
>> >> whole device, when bios are mapped by dm, and for mapping in md/raid5.
>> >> Other md personalities do not include this tracing yet, so add it.
>> >> 
>> >> When a read-error is detected we redirect the request to a different device.
>> >> This could justifiably be seen as a new mapping for the originial bio,
>> >> or a secondary mapping for the bio that errors.  This patch uses
>> >> the second option.
>> >> 
>> >> When md is used under dm-raid, the mappings are not traced as we do
>> >> not have access to the block device number of the parent.
>> >
>> > Looks the the original sector (the last parameter of trace_block_bio_remap)
>> > isn't correct.
>> > - in linear/raid0, bio_split already updated bio->bi_iter.bi_sector
>> 
>> Oh yes, of course.  in the common case 'split == bio' so when
>> split->bi_iter.bi_sector  is adjusted, bio->.... is as well.
>> I'll fix that, and also add calls to trace_block_split() as appropriate.
>> 
>> 
>> > - in raid1/raid10, r1_bio->sector is updated before the bio is sent.
>> 
>> Here I really think my code is correct.  r1_bio->sector is always the
>> address in the array of the request.  It is only set once for each
>> r1_bio, and that is before the call to trace_block_io_remap().
>
> Oh, you are right, sorry. I think moving the trace_remap right before we add
> the bio to plug or pending list is better. It will make the code simpler. The
> timing of the trace doesn't matter.

I agree the timing doesn't matter

The reason I didn't do that before is that bio->bi_bdev points to the
rdev when the write bio is queued to the pending list.  I'm not sure if
trace_block_bio_remap() accesses ->bi_bdev, but it could.

So I'm not completely sure that it makes the code simpler, but I'll post
a version like that to see what you think.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]