From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757130Ab1ETVCh (ORCPT ); Fri, 20 May 2011 17:02:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30831 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757075Ab1ETVCd (ORCPT ); Fri, 20 May 2011 17:02:33 -0400 Date: Fri, 20 May 2011 17:02:28 -0400 From: Vivek Goyal To: Jens Axboe Cc: Namhyung Kim , Shaohua Li , linux-kernel@vger.kernel.org, Divyesh Shah Subject: Re: [PATCH] block: call elv_bio_merged() when merged Message-ID: <20110520210228.GA6789@redhat.com> References: <1305869337-4375-1-git-send-email-namhyung@gmail.com> <1305885108.1633.11.camel@leonhard> <4DD6B807.6040309@kernel.dk> <20110520191907.GA6379@redhat.com> <4DD6BF1F.7010403@kernel.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4DD6BF1F.7010403@kernel.dk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 : > >>>>> 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 > >>>>> Cc: Divyesh Shah > >>>>> --- > >>>>> 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