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

[Jens Axboe <axboe@kernel.dk>]

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