Re: [PATCH 6/7] blkcg: account requests instead of bios for request based request_queues

[Jens Axboe]

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@xxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxx>
> 
> 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