From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934651AbbEMPcM (ORCPT ); Wed, 13 May 2015 11:32:12 -0400 Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:40792 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933546AbbEMPcK (ORCPT ); Wed, 13 May 2015 11:32:10 -0400 Message-ID: <55536E72.7070804@fb.com> Date: Wed, 13 May 2015 11:32:02 -0400 From: Jens Axboe User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Shaohua Li CC: Jeff Moyer , Subject: Re: [PATCH] blk: don't account discard request size References: <10cba6675b3e93f28b9c1c7a21e4b93b923e9531.1431467011.git.shli@fb.com> <55535D9C.3050706@fb.com> <20150513152159.GA3046840@devbig257.prn2.facebook.com> In-Reply-To: <20150513152159.GA3046840@devbig257.prn2.facebook.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [192.168.52.13] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-05-13_05:2015-05-13,2015-05-13,1970-01-01 signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/13/2015 11:22 AM, Shaohua Li wrote: > On Wed, May 13, 2015 at 10:20:12AM -0400, Jens Axboe wrote: >> On 05/13/2015 09:10 AM, Jeff Moyer wrote: >>> Shaohua Li writes: >>> >>>> In a workload with discard request, the IO throughput is generally much >>>> higher than expected. This is quite confusing checking iostat. Discard >>>> request doesn't really write data to drive, so don't account it. >>>> >>>> Signed-off-by: Shaohua Li >>>> --- >>>> block/blk-core.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/block/blk-core.c b/block/blk-core.c >>>> index fd154b9..0128d18 100644 >>>> --- a/block/blk-core.c >>>> +++ b/block/blk-core.c >>>> @@ -2138,7 +2138,11 @@ EXPORT_SYMBOL_GPL(blk_rq_err_bytes); >>>> >>>> void blk_account_io_completion(struct request *req, unsigned int bytes) >>>> { >>>> - if (blk_do_io_stat(req)) { >>>> + /* >>>> + * discard request doesn't really write @bytes to drive, >>>> + * doesn't account it >>>> + **/ >>>> + if (blk_do_io_stat(req) && !(req->cmd_flags & REQ_DISCARD)) { >>>> const int rw = rq_data_dir(req); >>>> struct hd_struct *part; >>>> int cpu; >>> >>> I think you want to modify __get_request to not set REQ_IO_STAT for >>> discard requests. This patch will still account the start of I/O, which >>> means in_flight will be off. >> >> That would be better. But I'm still not sure we want to turn off >> accounting for discards. For the mixed write/discard cases it's >> definitely confusing. The better option would be to account it as a >> discard and not a write. Preferably in a way that would not break >> existing tools, but so that they could get updated to support it. > > It's intentional discard IO start gets accounted, so tools will show > there is IO. I'm not sure if this is better though. > > Adding separate columns for discard (maybe flush too) is definitely > preferred. Is breaking existing tools really ok? We can't break then, I was just curious if adding a field to the end of the diskstats would potentially not break old applications. If not, they could just be updated to grab the new field too. -- Jens Axboe