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

[Namhyung Kim]

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@xxxxxxx>
> > Cc: Tejun Heo <tj@xxxxxxxxxx>
>
> 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

--
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/