Re: [PATCH 2/3] block: prevent duplicated bio completion report

[Namhyung Kim <namhyung@gmail.com>]

Hi,

2012-01-18 2:45 AM, Tejun Heo wrote:
> Hello,
>
> On Tue, Jan 17, 2012 at 10:32:07AM +0900, Namhyung Kim wrote:
>> Since previous patch make block_bio_complete TP working,
>> it will generate duplicated BLK_TA_COMPLETEs for bounced
>> bios. Fix it.
>>
>> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
>> Cc: Tejun Heo <tj@kernel.org>
>
> This and the third patch should probably be merged to the first patch.
> As it currently stands, it introduces window where spurious events are
> generated.

OK, will do.


>
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 4053cbd4490e..45cd0074a1c8 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -97,6 +97,9 @@ struct bio {
>>   #define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
>>   #define bio_flagged(bio, flag)	((bio)->bi_flags&  (1<<  (flag)))
>>
>> +/* masked bio's won't report its completion via tracepoint */
>> +#define BIO_COMPLETE_MASK	(1<<  BIO_BOUNCED)
>
> And, who's setting this flag?

__blk_queue_bounce() does.


>
>>   /*
>>    * top 4 bits of bio flags indicate the pool this bio came from
>>    */
>> diff --git a/include/trace/events/block.h b/include/trace/events/block.h
>> index 96955f4828b3..72888542e186 100644
>> --- a/include/trace/events/block.h
>> +++ b/include/trace/events/block.h
>> @@ -219,7 +219,8 @@ TRACE_EVENT_CONDITION(block_bio_complete,
>>
>>   	TP_ARGS(q, bio, error),
>>
>> -	TP_CONDITION(bio->bi_bdev != NULL),
>> +	TP_CONDITION(bio->bi_bdev != NULL&&
>> +		     !(bio->bi_flags&  BIO_COMPLETE_MASK)),
>
> Bounced bio's are separate bio's too and I don't think masking its
> completion from the TP itself is a good idea.  As I wrote before, why
> not do this from blktrace code?

Because blktrace cannot know about the bi_flags, as I said before. :) 
And although the bounced bio's are separate ones, they aren't queued 
separately. They just get replaced on the way.

Besides, I think accounting wait_time of them will result in an invalid 
value unless it's handled somehow in block_bio_bounce and/or 
block_rq_issue TP.


Thanks,
Namhyung