From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8F432C49ED7 for ; Mon, 16 Sep 2019 10:14:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 552242067D for ; Mon, 16 Sep 2019 10:14:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=mbobrowski-org.20150623.gappssmtp.com header.i=@mbobrowski-org.20150623.gappssmtp.com header.b="v6z5y0Mc" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728426AbfIPKOa (ORCPT ); Mon, 16 Sep 2019 06:14:30 -0400 Received: from mail-pg1-f193.google.com ([209.85.215.193]:43573 "EHLO mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726173AbfIPKOa (ORCPT ); Mon, 16 Sep 2019 06:14:30 -0400 Received: by mail-pg1-f193.google.com with SMTP id u72so19544504pgb.10 for ; Mon, 16 Sep 2019 03:14:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mbobrowski-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=YwusKXBUgyUMXSs68FocspxMjGXcgKSvza9r85FVtq4=; b=v6z5y0McOHGn6caqnH2fZHUUguAgMIWn1G5wkZy2mpOO5Vr6+zYEtdp5B1DnXNZlI9 EZWCcKune2GEpOoZmPN3Fd/xBqTPw3GeDjDHCrTbDGxRkcxQVRvZgaae45tiNWz4egdv 7aWNYbhwB4cnAhNaDOf89BHHxLVO1IBC/+9uxgIdOe8EFp4mc3LIMT2XcvnaYeRKqQqm Yiwb3IyFtUpA4Ev1VKh3dS8+BcwUFt3NgeiHb4dTUF5GGF13r6EgZNvrXUUnviBbd+Wk SJ2CKtC8oD+7uODionnjzRsjJjaPai1sErp6OMT7XZ+QlqSV+h0QR24i2CVmFXGruzsq R9Ag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=YwusKXBUgyUMXSs68FocspxMjGXcgKSvza9r85FVtq4=; b=YBxyoRQL6CDlYAiC8bEMJC+HfBs86YbAzbm7Z+AmtOYCupSjjpewK2cRNZutzfvXEm WEW4QF2yP26Wlwf2o+LxTKSL96PiUIha9Ai3hheMXkybV1X7u0oBtbBK+FG7ioomDybJ wRnUO5qL8BrpeCfxDU/QoJESsmfc1B4FKbRPac/Exj+gxorOcPnNTvTZahiIrtHQeGKj tbVZvHuTo1DQO60TpFXzAZCYRjMLKDmRohW4Efi1q3dhpHtCcmX4hUu9FzYiSPJH6FIS ZgtBsKifxnLihM/dFlfEI30IJeyIwsR9INU7LU+hw7Q9k17IVLHd4HAhsp450GkTg4Xg 84iw== X-Gm-Message-State: APjAAAXr4DGDqK/cjYIZ5ybcPUZixDluyDwArtxgxTdGM8rtrOTQfjJO JaCsOAhC3DcFpdu/jiPWmpn6 X-Google-Smtp-Source: APXvYqyL1KqvzAf2ZZKqgFwNVe0gZbGBEQJFslScWmzayJcvvmZjesltSh/2GJG9gQtx2930L95cZw== X-Received: by 2002:a63:5941:: with SMTP id j1mr55238748pgm.319.1568628864456; Mon, 16 Sep 2019 03:14:24 -0700 (PDT) Received: from bobrowski ([110.232.114.101]) by smtp.gmail.com with ESMTPSA id j4sm4197326pfn.29.2019.09.16.03.14.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Sep 2019 03:14:23 -0700 (PDT) Date: Mon, 16 Sep 2019 20:14:17 +1000 From: Matthew Bobrowski To: Ritesh Harjani Cc: tytso@mit.edu, jack@suse.cz, adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, david@fromorbit.com, hch@infradead.org, darrick.wong@oracle.com Subject: Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap infrastructure Message-ID: <20190916101417.GA4024@bobrowski> References: <20190916043741.BCBDEA4054@b06wcsmtp001.portsmouth.uk.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190916043741.BCBDEA4054@b06wcsmtp001.portsmouth.uk.ibm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Mon, Sep 16, 2019 at 10:07:41AM +0530, Ritesh Harjani wrote: > > @@ -213,12 +214,16 @@ static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) > > struct inode *inode = file_inode(iocb->ki_filp); > > ssize_t ret; > > > > + if (unlikely(IS_IMMUTABLE(inode))) > > + return -EPERM; > > + > > ret = generic_write_checks(iocb, from); > > if (ret <= 0) > > return ret; > > > > - if (unlikely(IS_IMMUTABLE(inode))) > > - return -EPERM; > > + ret = file_modified(iocb->ki_filp); > > + if (ret) > > + return 0; > > Why not return ret directly, otherwise we will be returning the wrong > error code to user space. Thoughts? You're right. I can't remember exactly why I decided to return '0', however looking at the code once again I don't see a reason why we don't just return 'ret', as any value other than '0' represents a failure in this case anyway. Thanks for picking that up. > Do you think simplification/restructuring of this API > "ext4_write_checks" can be a separate patch, so that this patch > only focuses on conversion of DIO write path to iomap? Hm, if we split it up so that it comes before this patch then it becomes hairy in the sense that a whole bunch of other changes would also need to come with what looks to be such a miniscule modification i.e. ext4_buffered_write_iter(), ext4_file_write_iter(), etc. Splitting it to come after just doesn't make any sense. To be honest, I don't really have any strong opinions around why we shouldn't split it up, nor do I have a strong opinion around why we should, so I think we should just leave it for now. > Also, I think we can make the function (ext4_write_checks()) > like below. This way we call for file_modified() only after we > have checked for write limits, at one place. No objections and I think it's a good idea. > static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter > *from) > { > struct inode *inode = file_inode(iocb->ki_filp); > ssize_t ret; > > if (unlikely(IS_IMMUTABLE(inode))) > return -EPERM; > > ret = generic_write_checks(iocb, from); > if (ret <= 0) > _ return ret; > /* > * If we have encountered a bitmap-format file, the size limit > * is smaller than s_maxbytes, which is for extent-mapped files. > */ > if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) { > struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); > > if (iocb->ki_pos >= sbi->s_bitmap_maxbytes) > return -EFBIG; > iov_iter_truncate(from, sbi->s_bitmap_maxbytes - > iocb->ki_pos); > } > + > + ret = file_modified(iocb->ki_filp); > + if (ret) > + return ret; > + > return iov_iter_count(from); > } ----