[PATCH] block: call elv_bio_merged() when merged

[PATCH] block: call elv_bio_merged() when merged

[Namhyung Kim]

Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Divyesh Shah <dpshah@google.com>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3fe00a14822a..4dc02ef5fc82 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
 	req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
 	drive_stat_acct(req, 0);
+	elv_bio_merged(q, req, bio);
 	return true;
 }
 
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
 	req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
 	drive_stat_acct(req, 0);
+	elv_bio_merged(q, req, bio);
 	return true;
 }
 
-- 
1.7.5

Re: [PATCH] block: call elv_bio_merged() when merged

[Shaohua Li]

2011/5/20 Namhyung Kim <namhyung@gmail.com>:
> Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
> removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
>
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> Cc: Divyesh Shah <dpshah@google.com>
> ---
>  block/blk-core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3fe00a14822a..4dc02ef5fc82 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
>
>        drive_stat_acct(req, 0);
> +       elv_bio_merged(q, req, bio);
>        return true;
>  }
>
> @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
>
>        drive_stat_acct(req, 0);
> +       elv_bio_merged(q, req, bio);
>        return true;
>  }
Looks you should do this in __make_request. when the routine is called
in attempt_plug_merge, the request isn't added to elevator yet.

Thanks,
Shaohua

Re: [PATCH] block: call elv_bio_merged() when merged

[Namhyung Kim]

Hello,

2011-05-20 (금), 16:31 +0800, Shaohua Li:
> 2011/5/20 Namhyung Kim <namhyung@gmail.com>:
> > Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
> > removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
> >
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > Cc: Divyesh Shah <dpshah@google.com>
> > ---
> >  block/blk-core.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 3fe00a14822a..4dc02ef5fc82 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
> >        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >
> >        drive_stat_acct(req, 0);
> > +       elv_bio_merged(q, req, bio);
> >        return true;
> >  }
> >
> > @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
> >        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >
> >        drive_stat_acct(req, 0);
> > +       elv_bio_merged(q, req, bio);
> >        return true;
> >  }
> Looks you should do this in __make_request. when the routine is called
> in attempt_plug_merge, the request isn't added to elevator yet.
> 

Hmm.. anyway it is merged. Is there any reason why we shouldn't collect
the stat - or invoke the callback routine - if the @req is not in the
elevator? Or we need to add a separate stat item for this case?

Thanks.

-- 
Regards,
Namhyung Kim

Re: [PATCH] block: call elv_bio_merged() when merged

[Jens Axboe]

On 2011-05-20 11:51, Namhyung Kim wrote:
> Hello,
> 
> 2011-05-20 (금), 16:31 +0800, Shaohua Li:
>> 2011/5/20 Namhyung Kim <namhyung@gmail.com>:
>>> Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
>>> removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
>>>
>>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
>>> Cc: Divyesh Shah <dpshah@google.com>
>>> ---
>>>  block/blk-core.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 3fe00a14822a..4dc02ef5fc82 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
>>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
>>>
>>>        drive_stat_acct(req, 0);
>>> +       elv_bio_merged(q, req, bio);
>>>        return true;
>>>  }
>>>
>>> @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
>>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
>>>
>>>        drive_stat_acct(req, 0);
>>> +       elv_bio_merged(q, req, bio);
>>>        return true;
>>>  }
>> Looks you should do this in __make_request. when the routine is called
>> in attempt_plug_merge, the request isn't added to elevator yet.
>>
> 
> Hmm.. anyway it is merged. Is there any reason why we shouldn't collect
> the stat - or invoke the callback routine - if the @req is not in the
> elevator? Or we need to add a separate stat item for this case?

Your patch should be safe, it's essentially only for the cgroup stuff
that does its own accounting and has appropriate protection for it. We'd
want to do this for both the plug and not-plugged merge case.

It's a bit of a shame to add this though, since now we are hitting the
cgroup lock for each merge.

-- 
Jens Axboe

Re: [PATCH] block: call elv_bio_merged() when merged

[Vivek Goyal]

On Fri, May 20, 2011 at 08:50:47PM +0200, Jens Axboe wrote:
> On 2011-05-20 11:51, Namhyung Kim wrote:
> > Hello,
> > 
> > 2011-05-20 (금), 16:31 +0800, Shaohua Li:
> >> 2011/5/20 Namhyung Kim <namhyung@gmail.com>:
> >>> Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
> >>> removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
> >>>
> >>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> >>> Cc: Divyesh Shah <dpshah@google.com>
> >>> ---
> >>>  block/blk-core.c |    2 ++
> >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>> index 3fe00a14822a..4dc02ef5fc82 100644
> >>> --- a/block/blk-core.c
> >>> +++ b/block/blk-core.c
> >>> @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
> >>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >>>
> >>>        drive_stat_acct(req, 0);
> >>> +       elv_bio_merged(q, req, bio);
> >>>        return true;
> >>>  }
> >>>
> >>> @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
> >>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >>>
> >>>        drive_stat_acct(req, 0);
> >>> +       elv_bio_merged(q, req, bio);
> >>>        return true;
> >>>  }
> >> Looks you should do this in __make_request. when the routine is called
> >> in attempt_plug_merge, the request isn't added to elevator yet.
> >>
> > 
> > Hmm.. anyway it is merged. Is there any reason why we shouldn't collect
> > the stat - or invoke the callback routine - if the @req is not in the
> > elevator? Or we need to add a separate stat item for this case?
> 
> Your patch should be safe, it's essentially only for the cgroup stuff
> that does its own accounting and has appropriate protection for it. We'd
> want to do this for both the plug and not-plugged merge case.
> 
> It's a bit of a shame to add this though, since now we are hitting the
> cgroup lock for each merge.

I think wer can make MERGED per cpu as I have done in my other patch
series for following dispatch stats.

BLKIO_STAT_CPU_SECTORS,
/* Total bytes transferred */
BLKIO_STAT_CPU_SERVICE_BYTES,
/* Total IOs serviced, post merge */
BLKIO_STAT_CPU_SERVICED,

Jens are you planning to merge lockless throttling series? Once that gets
merged, we can convert this MERGED stat also per cpu and get rid of need of
taking blkg->stats_lock.

Thanks
Vivek

Re: [PATCH] block: call elv_bio_merged() when merged

[Jens Axboe]

On 2011-05-20 21:19, Vivek Goyal wrote:
> On Fri, May 20, 2011 at 08:50:47PM +0200, Jens Axboe wrote:
>> On 2011-05-20 11:51, Namhyung Kim wrote:
>>> Hello,
>>>
>>> 2011-05-20 (금), 16:31 +0800, Shaohua Li:
>>>> 2011/5/20 Namhyung Kim <namhyung@gmail.com>:
>>>>> Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
>>>>> removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
>>>>>
>>>>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
>>>>> Cc: Divyesh Shah <dpshah@google.com>
>>>>> ---
>>>>>  block/blk-core.c |    2 ++
>>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>> index 3fe00a14822a..4dc02ef5fc82 100644
>>>>> --- a/block/blk-core.c
>>>>> +++ b/block/blk-core.c
>>>>> @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
>>>>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
>>>>>
>>>>>        drive_stat_acct(req, 0);
>>>>> +       elv_bio_merged(q, req, bio);
>>>>>        return true;
>>>>>  }
>>>>>
>>>>> @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
>>>>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
>>>>>
>>>>>        drive_stat_acct(req, 0);
>>>>> +       elv_bio_merged(q, req, bio);
>>>>>        return true;
>>>>>  }
>>>> Looks you should do this in __make_request. when the routine is called
>>>> in attempt_plug_merge, the request isn't added to elevator yet.
>>>>
>>>
>>> Hmm.. anyway it is merged. Is there any reason why we shouldn't collect
>>> the stat - or invoke the callback routine - if the @req is not in the
>>> elevator? Or we need to add a separate stat item for this case?
>>
>> Your patch should be safe, it's essentially only for the cgroup stuff
>> that does its own accounting and has appropriate protection for it. We'd
>> want to do this for both the plug and not-plugged merge case.
>>
>> It's a bit of a shame to add this though, since now we are hitting the
>> cgroup lock for each merge.
> 
> I think wer can make MERGED per cpu as I have done in my other patch
> series for following dispatch stats.
> 
> BLKIO_STAT_CPU_SECTORS,
> /* Total bytes transferred */
> BLKIO_STAT_CPU_SERVICE_BYTES,
> /* Total IOs serviced, post merge */
> BLKIO_STAT_CPU_SERVICED,

Yep, lets please do that and we can re-instate the merge calls with
that, too.

> Jens are you planning to merge lockless throttling series? Once that gets
> merged, we can convert this MERGED stat also per cpu and get rid of need of
> taking blkg->stats_lock.

Merged them about an hour ago :-)

Send further updates relative to for-2.6.40/core.

-- 
Jens Axboe

Re: [PATCH] block: call elv_bio_merged() when merged

[Vivek Goyal]

On Fri, May 20, 2011 at 09:21:03PM +0200, Jens Axboe wrote:
> On 2011-05-20 21:19, Vivek Goyal wrote:
> > On Fri, May 20, 2011 at 08:50:47PM +0200, Jens Axboe wrote:
> >> On 2011-05-20 11:51, Namhyung Kim wrote:
> >>> Hello,
> >>>
> >>> 2011-05-20 (금), 16:31 +0800, Shaohua Li:
> >>>> 2011/5/20 Namhyung Kim <namhyung@gmail.com>:
> >>>>> Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
> >>>>> removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
> >>>>>
> >>>>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> >>>>> Cc: Divyesh Shah <dpshah@google.com>
> >>>>> ---
> >>>>>  block/blk-core.c |    2 ++
> >>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>>>
> >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>>>> index 3fe00a14822a..4dc02ef5fc82 100644
> >>>>> --- a/block/blk-core.c
> >>>>> +++ b/block/blk-core.c
> >>>>> @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
> >>>>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >>>>>
> >>>>>        drive_stat_acct(req, 0);
> >>>>> +       elv_bio_merged(q, req, bio);
> >>>>>        return true;
> >>>>>  }
> >>>>>
> >>>>> @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
> >>>>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >>>>>
> >>>>>        drive_stat_acct(req, 0);
> >>>>> +       elv_bio_merged(q, req, bio);
> >>>>>        return true;
> >>>>>  }
> >>>> Looks you should do this in __make_request. when the routine is called
> >>>> in attempt_plug_merge, the request isn't added to elevator yet.
> >>>>
> >>>
> >>> Hmm.. anyway it is merged. Is there any reason why we shouldn't collect
> >>> the stat - or invoke the callback routine - if the @req is not in the
> >>> elevator? Or we need to add a separate stat item for this case?
> >>
> >> Your patch should be safe, it's essentially only for the cgroup stuff
> >> that does its own accounting and has appropriate protection for it. We'd
> >> want to do this for both the plug and not-plugged merge case.
> >>
> >> It's a bit of a shame to add this though, since now we are hitting the
> >> cgroup lock for each merge.
> > 
> > I think wer can make MERGED per cpu as I have done in my other patch
> > series for following dispatch stats.
> > 
> > BLKIO_STAT_CPU_SECTORS,
> > /* Total bytes transferred */
> > BLKIO_STAT_CPU_SERVICE_BYTES,
> > /* Total IOs serviced, post merge */
> > BLKIO_STAT_CPU_SERVICED,
> 
> Yep, lets please do that and we can re-instate the merge calls with
> that, too.
> 
> > Jens are you planning to merge lockless throttling series? Once that gets
> > merged, we can convert this MERGED stat also per cpu and get rid of need of
> > taking blkg->stats_lock.
> 
> Merged them about an hour ago :-)
> 
> Send further updates relative to for-2.6.40/core.

Sure. I am working on a patch now.

Thanks
Vivek

Re: [PATCH] block: call elv_bio_merged() when merged

[Shaohua Li]

On Sat, 2011-05-21 at 02:50 +0800, Jens Axboe wrote:
> On 2011-05-20 11:51, Namhyung Kim wrote:
> > Hello,
> > 
> > 2011-05-20 (금), 16:31 +0800, Shaohua Li:
> >> 2011/5/20 Namhyung Kim <namhyung@gmail.com>:
> >>> Commit 73c101011926 ("block: initial patch for on-stack per-task plugging")
> >>> removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
> >>>
> >>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> >>> Cc: Divyesh Shah <dpshah@google.com>
> >>> ---
> >>>  block/blk-core.c |    2 ++
> >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>> index 3fe00a14822a..4dc02ef5fc82 100644
> >>> --- a/block/blk-core.c
> >>> +++ b/block/blk-core.c
> >>> @@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
> >>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >>>
> >>>        drive_stat_acct(req, 0);
> >>> +       elv_bio_merged(q, req, bio);
> >>>        return true;
> >>>  }
> >>>
> >>> @@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
> >>>        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> >>>
> >>>        drive_stat_acct(req, 0);
> >>> +       elv_bio_merged(q, req, bio);
> >>>        return true;
> >>>  }
> >> Looks you should do this in __make_request. when the routine is called
> >> in attempt_plug_merge, the request isn't added to elevator yet.
> >>
> > 
> > Hmm.. anyway it is merged. Is there any reason why we shouldn't collect
> > the stat - or invoke the callback routine - if the @req is not in the
> > elevator? Or we need to add a separate stat item for this case?
> 
> Your patch should be safe, it's essentially only for the cgroup stuff
> that does its own accounting and has appropriate protection for it. 
I'm just worrying about there is duplicate accounting, but anyway it's
safe.