Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 6/7] blkcg: account requests instead of bios for request
 based request_queues
To:     Shaohua Li <shli@kernel.org>, Tejun Heo <tj@kernel.org>
Cc:     linux-kernel@vger.kernel.org, kernel-team@fb.com,
        lizefan@huawei.com, hannes@cmpxchg.org, cgroups@vger.kernel.org,
        guro@fb.com
References: <20171112222613.3613362-1-tj@kernel.org>
 <20171112222613.3613362-7-tj@kernel.org>
 <20171114232355.vjxlzfbqbqj5ihq4@kernel.org>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <9a2ddc6a-d618-a896-290c-254ffeb5e9d6@kernel.dk>
Date:   Wed, 15 Nov 2017 10:19:26 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171114232355.vjxlzfbqbqj5ihq4@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/14/2017 04:23 PM, Shaohua Li wrote:
> On Sun, Nov 12, 2017 at 02:26:12PM -0800, Tejun Heo wrote:
>> blkcg accounting is currently bio based, which is silly for request
>> based request_queues.  This is silly as the number of bios doesn't
>> have much to do with the actual number of IOs issued to the underlying
>> device (can be significantly higher or lower) and may change depending
>> on the implementation details on how the bios are issued (e.g. from
>> the recent split-bios-while-issuing change).
>>
>> request based request_queues have QUEUE_FLAG_IO_STAT set by default
>> which controls the gendisk accounting.  Do cgroup accounting for those
>> request_queues together with gendisk accounting on request completion.
>>
>> This makes cgroup accounting consistent with gendisk accounting and
>> what's happening on the system.
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> ---
>>  block/blk-core.c           |  3 +++
>>  include/linux/blk-cgroup.h | 18 +++++++++++++++++-
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 048be4a..ad23b96 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2429,6 +2429,7 @@ void blk_account_io_completion(struct request *req, unsigned int bytes)
>>  		cpu = part_stat_lock();
>>  		part = req->part;
>>  		part_stat_add(cpu, part, sectors[rw], bytes >> 9);
>> +		blkcg_account_io_completion(req, bytes);
>>  		part_stat_unlock();
>>  	}
>>  }
>> @@ -2454,6 +2455,8 @@ void blk_account_io_done(struct request *req)
>>  		part_round_stats(req->q, cpu, part);
>>  		part_dec_in_flight(req->q, part, rw);
>>  
>> +		blkcg_account_io_done(req);
>> +
>>  		hd_struct_put(part);
>>  		part_stat_unlock();
>>  	}
>> diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
>> index 96eed0f..f2f9691 100644
>> --- a/include/linux/blk-cgroup.h
>> +++ b/include/linux/blk-cgroup.h
>> @@ -715,7 +715,8 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
>>  
>>  	throtl = blk_throtl_bio(q, blkg, bio);
>>  
>> -	if (!throtl) {
>> +	/* if @q does io stat, blkcg stats are updated together with them */
>> +	if (!blk_queue_io_stat(q) && !throtl) {
> 
> Reviewed-by: Shaohua Li <shli@kernel.org>
> 
> One nitpick, can we use q->request_fn to determine request based queue? I think
> that is more reliable and usual way for this.

That won't work for mq - but there is a helper for this, queue_is_rq_based().

-- 
Jens Axboe


From 1584155485596217365@xxx Wed Nov 15 17:53:19 +0000 2017
X-GM-THRID: 1583900948124527465
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread