From patchwork Wed May 11 13:23:12 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eryu Guan X-Patchwork-Id: 9069631 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id B253A9F1C1 for ; Wed, 11 May 2016 13:23:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C9D3420114 for ; Wed, 11 May 2016 13:23:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DABDC200ED for ; Wed, 11 May 2016 13:23:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932488AbcEKNXY (ORCPT ); Wed, 11 May 2016 09:23:24 -0400 Received: from mail-pa0-f65.google.com ([209.85.220.65]:36281 "EHLO mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932265AbcEKNXV (ORCPT ); Wed, 11 May 2016 09:23:21 -0400 Received: by mail-pa0-f65.google.com with SMTP id i5so3599212pag.3; Wed, 11 May 2016 06:23:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Qp5sJ6ySvrO6mrY1rhtVA5ygH0UBTtHxznfOdpCvhkg=; b=SOGDH/Ig2Rq7Ck8yU1VHqqAX9/hfjq/OhkqXsstljQYVTyCBibrvFSSQI3FT0xG6JN CFdNypDI0De4XHlwt6S2g7icbpD170w7ziu+773qMpbLT1dsNm7g8bbtLypEWV1Adw4k pZoMj9PlqTEJ/FCTiR52rIEJ/vbmSBKHA4ynGXAeIsy1h9OD+j6pFXRVKunMK2McXJ4b X2/Z29unFkfx4dXl0rjxRXm7rZljAC5c55kzotVAv+jADuNjyacPzYwsc97J12XzInLo WNZlPeWJqndvURAdfzzKeMXN2nS7Lf0YuuGzlYjqvW0LU3pLpP6gq9xg0udYZlSew08X Z3Lw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Qp5sJ6ySvrO6mrY1rhtVA5ygH0UBTtHxznfOdpCvhkg=; b=UDokQxjYpvqT4aEweEVD3hbKmVzM77K3WRDZfRfXJDOF8bwcVVZmtlmpJtsc7Jjtcw MdEZbGYZj/ZUw7EsSVQjVtKAv+Tn7j2Qxm2cEcRYHy5MYhYGFHHFxGRCYlDzGlyckeiP q4DC/V3SIorP+kwfxpEICrJr2ZZ7mOZHRL2EMDNka0zYv8B/ScAYRChRRB6fs8hNubIu 8yUoUT7hRMhWlUJZ+5+DRAMXNv6jVsGJaNhJuAXWx09vfumPKISIZ2FuILsqFd0uFjUY vmxcGNpFI3pRVhRanT8E1MJutcpP9W+dL2nXr7kdlI8JIlfUTaMAmuGCH/X1AfaBeJMn Y0hw== X-Gm-Message-State: AOPr4FXF6waxQpPc7no6D9raJD4HT6mGfy0xKe+ivyC1jVha50aZWmHSWoXyZVlWm5doQg== X-Received: by 10.66.118.70 with SMTP id kk6mr4964108pab.74.1462973000992; Wed, 11 May 2016 06:23:20 -0700 (PDT) Received: from localhost ([128.199.137.77]) by smtp.gmail.com with ESMTPSA id w187sm12325972pfw.50.2016.05.11.06.23.19 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 May 2016 06:23:19 -0700 (PDT) Date: Wed, 11 May 2016 21:23:12 +0800 From: Eryu Guan To: Jeff Moyer Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org Subject: Re: [PATCH v2 1/2] direct-io: cleanup get_more_blocks() Message-ID: <20160511132312.GD10350@eguan.usersys.redhat.com> References: <1462645910-23290-1-git-send-email-guaneryu@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, May 10, 2016 at 01:38:05PM -0400, Jeff Moyer wrote: > Eryu Guan writes: > > > Save one level of indention by returning error early. > > > > Introduce some local variables to make the code easier to read a bit, > > and do preparation for next patch. > > > > Signed-off-by: Eryu Guan > > Hi, Eryu, > > I don't think you have a full appreciation of the amount of optimization > that goes into this code. I don't see anything wrong with what you've > done, but I also don't want to introduce all these local variables and > change a branch in order to find out several months down the line that > we introduced some TPC-C regression of .5%. Agreed, I overdid it, v2 fix doesn't need the cleanup, I realized it after I sent it out. > > Look, I think this is all you need for the full fix: > > diff --git a/fs/direct-io.c b/fs/direct-io.c > index 4720377..f66754e 100644 > --- a/fs/direct-io.c > +++ b/fs/direct-io.c > @@ -639,8 +639,7 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio, > */ > create = dio->rw & WRITE; > if (dio->flags & DIO_SKIP_HOLES) { > - if (sdio->block_in_file < (i_size_read(dio->inode) >> > - sdio->blkbits)) > + if (fs_startblk < fs_count) > create = 0; > } > > > Can you just test that? I tested it and it did fix both of the issues for me. But it seems that it's a bit overkilled, in certain case block allocation should be allowed, but it still sets 'create' to 0. For example, append writing 8k to a 4k sparse file (so offset is also 4k), on a 4k block size filesystem, fs_startblk(1) is smaller than fs_count(2), so it still sets 'create' to 0. But block allocation should be allowed in this case, and both the original code and my patch do so. So I simplified my real fix to this (updates for comments not included): Do you think it's a proper fix? Thanks again for your time! Eryu --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/direct-io.c b/fs/direct-io.c index 4720377..0cace3e 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -639,8 +639,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio, */ create = dio->rw & WRITE; if (dio->flags & DIO_SKIP_HOLES) { - if (sdio->block_in_file < (i_size_read(dio->inode) >> - sdio->blkbits)) + if (fs_startblk <= ((i_size_read(dio->inode) - 1) >> + i_blkbits)) create = 0; }