Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 17 Sep 2019 21:31:01 +1000
From:   Matthew Bobrowski <mbobrowski@mbobrowski.org>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     tytso@mit.edu, jack@suse.cz, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        david@fromorbit.com, darrick.wong@oracle.com
Subject: Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap
 infrastructure
Message-ID: <20190917113101.GA17286@bobrowski>
References: <cover.1568282664.git.mbobrowski@mbobrowski.org>
 <db33705f9ba35ccbe20fc19b8ecbbf2078beff08.1568282664.git.mbobrowski@mbobrowski.org>
 <20190916121248.GD4005@infradead.org>
 <20190916223741.GA5936@bobrowski>
 <20190917090613.GC29487@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190917090613.GC29487@infradead.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On Tue, Sep 17, 2019 at 02:06:13AM -0700, Christoph Hellwig wrote:
> On Tue, Sep 17, 2019 at 08:37:41AM +1000, Matthew Bobrowski wrote:
> > > Independent of the error return issue you probably want to split
> > > modifying ext4_write_checks into a separate preparation patch.
> > 
> > Providing that there's no objections to introducing a possible performance
> > change with this separate preparation patch (overhead of calling
> > file_remove_privs/file_update_time twice), then I have no issues in doing so.
> 
> Well, we should avoid calling it twice.  But what caught my eye is that
> the buffered I/O path also called this function, so we are changing it as
> well here.  If that actually is safe (I didn't review these bits carefully
> and don't know ext4 that well) the overall refactoring of the write
> flow might belong into a separate prep patch (that is not relying
> on ->direct_IO, the checks changes, etc).

Yeah, I see what you're saying. From memory, in order to get this right, there
was a whole bunch of additional changes that needed to be done that would
effectively be removed in a subsequent patch. But, let me revisit this again
and see what I can do.

> > > > +	if (!inode_trylock(inode)) {
> > > > +		if (iocb->ki_flags & IOCB_NOWAIT)
> > > > +			return -EAGAIN;
> > > > +		inode_lock(inode);
> > > > +	}
> > > > +
> > > > +	if (!ext4_dio_checks(inode)) {
> > > > +		inode_unlock(inode);
> > > > +		/*
> > > > +		 * Fallback to buffered IO if the operation on the
> > > > +		 * inode is not supported by direct IO.
> > > > +		 */
> > > > +		return ext4_buffered_write_iter(iocb, from);
> > > 
> > > I think you want to lift the locking into the caller of this function
> > > so that you don't have to unlock and relock for the buffered write
> > > fallback.
> > 
> > I don't exactly know what you really mean by "lift the locking into the caller
> > of this function". I'm interpreting that as moving the inode_unlock()
> > operation into ext4_buffered_write_iter(), but I can't see how that would be
> > any different from doing it directly here? Wouldn't this also run the risk of
> > the locks becoming unbalanced as we'd need to add checks around whether the
> > resource is being contended? Maybe I'm misunderstanding something here...
> 
> With that I mean to acquire the inode lock in ext4_file_write_iter
> instead of the low-level buffered I/O or direct I/O routines.

Oh, I didn't think of that! But yes, that would in fact be nice and I cannot
see why we shouldn't be doing that at this point. It also helps with reducing
all the code duplication going on in the low-level buffered, direct, dax I/O
routines.

--<M>--