[PATCH] trace bio queueing trial only when it occurs

[PATCH] trace bio queueing trial only when it occurs

[Minchan Kim]

If BIO is discarded or cross over end of device,
BIO queueing trial doesn't occur.

Let's trace it only when it happens.

Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 block/blk-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 5b59592..1a0cfd5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1463,8 +1463,6 @@ static inline void __generic_make_request(struct bio *bio)
 		if (old_sector != -1)
 			trace_block_remap(q, bio, old_dev, old_sector);

-		trace_block_bio_queue(q, bio);
-
 		old_sector = bio->bi_sector;
 		old_dev = bio->bi_bdev->bd_dev;

@@ -1477,6 +1475,8 @@ static inline void __generic_make_request(struct bio *bio)
 			goto end_io;
 		}

+		trace_block_bio_queue(q, bio);
+
 		ret = q->make_request_fn(q, bio);
 	} while (ret);

--
1.6.4



-- 
Kind regards,
Minchan Kim

[PATCH] trace bio queueing trial only when it occurs

[Minchan Kim]

It got lost in LKML mail storm.
I resend this.
Wu, Could you review this patch, please?

== CUT_HERE ==

If BIO is discarded or cross over end of device,
BIO queueing trial doesn't occur.

Let's trace it only when it happens.

Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 block/blk-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 5b59592..1a0cfd5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1463,8 +1463,6 @@ static inline void __generic_make_request(struct bio *bio)
 		if (old_sector != -1)
 			trace_block_remap(q, bio, old_dev, old_sector);

-		trace_block_bio_queue(q, bio);
-
 		old_sector = bio->bi_sector;
 		old_dev = bio->bi_bdev->bd_dev;

@@ -1477,6 +1475,8 @@ static inline void __generic_make_request(struct bio *bio)
 			goto end_io;
 		}

+		trace_block_bio_queue(q, bio);
+
 		ret = q->make_request_fn(q, bio);
 	} while (ret);

--
1.6.4



-- 
Kind regards,
Minchan Kim

Re: [PATCH] trace bio queueing trial only when it occurs

[Wu Fengguang]

Minchan,

I tend to agree with the change, but somehow confused by blkparse(1):

        Q -- queued   This notes *intent* to queue i/o at the given location.  No real requests exists yet.

Li Zefan has been working on blktrace and he can tell more.

Thanks,
Fengguang

On Tue, Sep 08, 2009 at 08:34:16AM +0800, Minchan Kim wrote:
> 
> It got lost in LKML mail storm.
> I resend this.
> Wu, Could you review this patch, please?
> 
> == CUT_HERE ==
> 
> If BIO is discarded or cross over end of device,
> BIO queueing trial doesn't occur.
> 
> Let's trace it only when it happens.
> 
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> ---
>  block/blk-core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 5b59592..1a0cfd5 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1463,8 +1463,6 @@ static inline void __generic_make_request(struct bio *bio)
>  		if (old_sector != -1)
>  			trace_block_remap(q, bio, old_dev, old_sector);
> 
> -		trace_block_bio_queue(q, bio);
> -
>  		old_sector = bio->bi_sector;
>  		old_dev = bio->bi_bdev->bd_dev;
> 
> @@ -1477,6 +1475,8 @@ static inline void __generic_make_request(struct bio *bio)
>  			goto end_io;
>  		}
> 
> +		trace_block_bio_queue(q, bio);
> +
>  		ret = q->make_request_fn(q, bio);
>  	} while (ret);
> 
> --
> 1.6.4
> 
> 
> 
> -- 
> Kind regards,
> Minchan Kim

Re: [PATCH] trace bio queueing trial only when it occurs

[Li Zefan]

Wu Fengguang wrote:
> Minchan,
> 
> I tend to agree with the change, but somehow confused by blkparse(1):
> 
>         Q -- queued   This notes *intent* to queue i/o at the given location.  No real requests exists yet.
> 
> Li Zefan has been working on blktrace and he can tell more.
> 

I work on ftrace-plugin blktrace and blk TRACE_EVENT, but don't know
much about the old blktrace history. ;)

I think the manpage says it's called before ->make_request_fn(), so
"No real requests exist yet".

Actually the trace was called just before make_request at first:
	2056a782f8e7e65fd4bfd027506b4ce1c5e9ccd4

And then 2 patches added some checks between them:
	5ddfe9691c91a244e8d1be597b6428fcefd58103
	51fd77bd9f512ab6cc9df0733ba1caaab89eb957

It seems to me it makes sense to have this patch.

> Thanks,
> Fengguang
> 
> On Tue, Sep 08, 2009 at 08:34:16AM +0800, Minchan Kim wrote:
>> It got lost in LKML mail storm.
>> I resend this.
>> Wu, Could you review this patch, please?
>>
>> == CUT_HERE ==
>>
>> If BIO is discarded or cross over end of device,
>> BIO queueing trial doesn't occur.
>>
>> Let's trace it only when it happens.
>>
>> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>> ---
>>  block/blk-core.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 5b59592..1a0cfd5 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1463,8 +1463,6 @@ static inline void __generic_make_request(struct bio *bio)
>>  		if (old_sector != -1)
>>  			trace_block_remap(q, bio, old_dev, old_sector);
>>
>> -		trace_block_bio_queue(q, bio);
>> -
>>  		old_sector = bio->bi_sector;
>>  		old_dev = bio->bi_bdev->bd_dev;
>>
>> @@ -1477,6 +1475,8 @@ static inline void __generic_make_request(struct bio *bio)
>>  			goto end_io;
>>  		}
>>
>> +		trace_block_bio_queue(q, bio);
>> +
>>  		ret = q->make_request_fn(q, bio);
>>  	} while (ret);
>>
>> --
>> 1.6.4
>>
>>
>>

Re: [PATCH] trace bio queueing trial only when it occurs

[Minchan Kim]

On Tue, Sep 8, 2009 at 11:02 AM, Li Zefan<lizf@cn.fujitsu.com> wrote:
> Wu Fengguang wrote:
>> Minchan,
>>
>> I tend to agree with the change, but somehow confused by blkparse(1):
>>
>>         Q -- queued   This notes *intent* to queue i/o at the given location.  No real requests exists yet.
>>
>> Li Zefan has been working on blktrace and he can tell more.
>>
>
> I work on ftrace-plugin blktrace and blk TRACE_EVENT, but don't know
> much about the old blktrace history. ;)
>
> I think the manpage says it's called before ->make_request_fn(), so
> "No real requests exist yet".
>
> Actually the trace was called just before make_request at first:
>        2056a782f8e7e65fd4bfd027506b4ce1c5e9ccd4
>
> And then 2 patches added some checks between them:
>        5ddfe9691c91a244e8d1be597b6428fcefd58103
>        51fd77bd9f512ab6cc9df0733ba1caaab89eb957
>
> It seems to me it makes sense to have this patch.

Thanks for good information.
Yes. It seems above 2 patches seem to break the rule.
Could I add your ACK?

>> Thanks,
>> Fengguang
>>
>> On Tue, Sep 08, 2009 at 08:34:16AM +0800, Minchan Kim wrote:
>>> It got lost in LKML mail storm.
>>> I resend this.
>>> Wu, Could you review this patch, please?
>>>
>>> == CUT_HERE ==
>>>
>>> If BIO is discarded or cross over end of device,
>>> BIO queueing trial doesn't occur.
>>>
>>> Let's trace it only when it happens.
>>>
>>> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>>> ---
>>>  block/blk-core.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 5b59592..1a0cfd5 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1463,8 +1463,6 @@ static inline void __generic_make_request(struct bio *bio)
>>>              if (old_sector != -1)
>>>                      trace_block_remap(q, bio, old_dev, old_sector);
>>>
>>> -            trace_block_bio_queue(q, bio);
>>> -
>>>              old_sector = bio->bi_sector;
>>>              old_dev = bio->bi_bdev->bd_dev;
>>>
>>> @@ -1477,6 +1475,8 @@ static inline void __generic_make_request(struct bio *bio)
>>>                      goto end_io;
>>>              }
>>>
>>> +            trace_block_bio_queue(q, bio);
>>> +
>>>              ret = q->make_request_fn(q, bio);
>>>      } while (ret);
>>>
>>> --
>>> 1.6.4
>>>
>>>
>>>
>



-- 
Kind regards,
Minchan Kim

Re: [PATCH] trace bio queueing trial only when it occurs

[Wu Fengguang]

On Tue, Sep 08, 2009 at 12:10:55PM +0800, Minchan Kim wrote:
> On Tue, Sep 8, 2009 at 11:02 AM, Li Zefan<lizf@cn.fujitsu.com> wrote:
> > Wu Fengguang wrote:
> >> Minchan,
> >>
> >> I tend to agree with the change, but somehow confused by blkparse(1):
> >>
> >>         Q -- queued   This notes *intent* to queue i/o at the given location.  No real requests exists yet.
> >>
> >> Li Zefan has been working on blktrace and he can tell more.
> >>
> >
> > I work on ftrace-plugin blktrace and blk TRACE_EVENT, but don't know
> > much about the old blktrace history. ;)
> >
> > I think the manpage says it's called before ->make_request_fn(), so
> > "No real requests exist yet".
> >
> > Actually the trace was called just before make_request at first:
> >        2056a782f8e7e65fd4bfd027506b4ce1c5e9ccd4
> >
> > And then 2 patches added some checks between them:
> >        5ddfe9691c91a244e8d1be597b6428fcefd58103
> >        51fd77bd9f512ab6cc9df0733ba1caaab89eb957
> >
> > It seems to me it makes sense to have this patch.
> 
> Thanks for good information.
> Yes. It seems above 2 patches seem to break the rule.
> Could I add your ACK?

Sure, thanks!

Acked-by: Wu Fengguang <fengguang.wu@intel.com>

> >> Thanks,
> >> Fengguang
> >>
> >> On Tue, Sep 08, 2009 at 08:34:16AM +0800, Minchan Kim wrote:
> >>> It got lost in LKML mail storm.
> >>> I resend this.
> >>> Wu, Could you review this patch, please?
> >>>
> >>> == CUT_HERE ==
> >>>
> >>> If BIO is discarded or cross over end of device,
> >>> BIO queueing trial doesn't occur.
> >>>
> >>> Let's trace it only when it happens.
> >>>
> >>> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> >>> ---
> >>>  block/blk-core.c |    4 ++--
> >>>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>> index 5b59592..1a0cfd5 100644
> >>> --- a/block/blk-core.c
> >>> +++ b/block/blk-core.c
> >>> @@ -1463,8 +1463,6 @@ static inline void __generic_make_request(struct bio *bio)
> >>>              if (old_sector != -1)
> >>>                      trace_block_remap(q, bio, old_dev, old_sector);
> >>>
> >>> -            trace_block_bio_queue(q, bio);
> >>> -
> >>>              old_sector = bio->bi_sector;
> >>>              old_dev = bio->bi_bdev->bd_dev;
> >>>
> >>> @@ -1477,6 +1475,8 @@ static inline void __generic_make_request(struct bio *bio)
> >>>                      goto end_io;
> >>>              }
> >>>
> >>> +            trace_block_bio_queue(q, bio);
> >>> +
> >>>              ret = q->make_request_fn(q, bio);
> >>>      } while (ret);
> >>>
> >>> --
> >>> 1.6.4
> >>>
> >>>
> >>>
> >
> 
> 
> 
> -- 
> Kind regards,
> Minchan Kim

Re: [PATCH] trace bio queueing trial only when it occurs

[Minchan Kim]

On Tue, Sep 8, 2009 at 1:13 PM, Wu Fengguang<fengguang.wu@intel.com> wrote:
> On Tue, Sep 08, 2009 at 12:10:55PM +0800, Minchan Kim wrote:
>> On Tue, Sep 8, 2009 at 11:02 AM, Li Zefan<lizf@cn.fujitsu.com> wrote:
>> > Wu Fengguang wrote:
>> >> Minchan,
>> >>
>> >> I tend to agree with the change, but somehow confused by blkparse(1):
>> >>
>> >>         Q -- queued   This notes *intent* to queue i/o at the given location.  No real requests exists yet.
>> >>
>> >> Li Zefan has been working on blktrace and he can tell more.
>> >>
>> >
>> > I work on ftrace-plugin blktrace and blk TRACE_EVENT, but don't know
>> > much about the old blktrace history. ;)
>> >
>> > I think the manpage says it's called before ->make_request_fn(), so
>> > "No real requests exist yet".
>> >
>> > Actually the trace was called just before make_request at first:
>> >        2056a782f8e7e65fd4bfd027506b4ce1c5e9ccd4
>> >
>> > And then 2 patches added some checks between them:
>> >        5ddfe9691c91a244e8d1be597b6428fcefd58103
>> >        51fd77bd9f512ab6cc9df0733ba1caaab89eb957
>> >
>> > It seems to me it makes sense to have this patch.
>>
>> Thanks for good information.
>> Yes. It seems above 2 patches seem to break the rule.
>> Could I add your ACK?
>
> Sure, thanks!
>
> Acked-by: Wu Fengguang <fengguang.wu@intel.com>

Thanks for careful review, Wu.



-- 
Kind regards,
Minchan Kim

Re: [PATCH] trace bio queueing trial only when it occurs

[Jens Axboe]

On Tue, Sep 08 2009, Wu Fengguang wrote:
> Minchan,
> 
> I tend to agree with the change, but somehow confused by blkparse(1):
> 
>         Q -- queued   This notes *intent* to queue i/o at the given location.  No real requests exists yet.
> 
> Li Zefan has been working on blktrace and he can tell more.

The documentation details the original implementation. I'm guessing the
need to move this trace hook is due to some user space tracking that
gets confused, because it cannot match the queue intent with a merge or
an insert.

I have no problems moving the trace call. If we had a matching bio_endio
trace, then I'd say moving it would lose information. But right now it
doesn't.

-- 
Jens Axboe