linux-ext4 - Re: [PATCH v3 2/2] ext4: handle layout changes to pinned DAX mappings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 9 Jul 2018 21:49:56 +0200
From:   Jan Kara <jack@...e.cz>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     Jan Kara <jack@...e.cz>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Dave Chinner <david@...morbit.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Christoph Hellwig <hch@....de>, linux-nvdimm@...ts.01.org,
        Jeff Moyer <jmoyer@...hat.com>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v3 2/2] ext4: handle layout changes to pinned DAX mappings

On Mon 09-07-18 09:23:38, Darrick J. Wong wrote:
> On Mon, Jul 09, 2018 at 02:33:47PM +0200, Jan Kara wrote:
> > On Thu 05-07-18 10:53:10, Ross Zwisler wrote:
> > > On Wed, Jul 04, 2018 at 08:59:52PM -0700, Darrick J. Wong wrote:
> > > > On Thu, Jul 05, 2018 at 09:54:14AM +1000, Dave Chinner wrote:
> > > > > On Wed, Jul 04, 2018 at 02:27:23PM +0200, Jan Kara wrote:
> > > > > > On Wed 04-07-18 10:49:23, Dave Chinner wrote:
> > > > > > > On Mon, Jul 02, 2018 at 11:29:12AM -0600, Ross Zwisler wrote:
> > > > > > > > Follow the lead of xfs_break_dax_layouts() and add synchronization between
> > > > > > > > operations in ext4 which remove blocks from an inode (hole punch, truncate
> > > > > > > > down, etc.) and pages which are pinned due to DAX DMA operations.
> > > > > > > > 
> > > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> > > > > > > > Reviewed-by: Jan Kara <jack@...e.cz>
> > > > > > > > Reviewed-by: Lukas Czerner <lczerner@...hat.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Changes since v2:
> > > > > > > >  * Added a comment to ext4_insert_range() explaining why we don't call
> > > > > > > >    ext4_break_layouts(). (Jan)
> > > > > > > 
> > > > > > > Which I think is wrong and will cause data corruption.
> > > > > > > 
> > > > > > > > @@ -5651,6 +5663,11 @@ int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
> > > > > > > >  			LLONG_MAX);
> > > > > > > >  	if (ret)
> > > > > > > >  		goto out_mmap;
> > > > > > > > +	/*
> > > > > > > > +	 * We don't need to call ext4_break_layouts() because we aren't
> > > > > > > > +	 * removing any blocks from the inode.  We are just changing their
> > > > > > > > +	 * offset by inserting a hole.
> > > > > > > > +	 */
> > > > 
> > > > Does calling ext4_break_layouts from insert range not work?
> > > > 
> > > > It's my understanding that file leases work are a mechanism for the
> > > > filesystem to delegate some of its authority over physical space
> > > > mappings to "client" software.  AFAICT it's used for mmap'ing pmem
> > > > directly into userspace and exporting space on shared storage over
> > > > pNFS.  Some day we might use the same mechanism for the similar things
> > > > that RDMA does, or the swapfile code since that's essentially how it
> > > > works today.
> > > > 
> > > > The other part of these file leases is that the filesystem revokes them
> > > > any time it wants to perform a mapping operation on a file.  This breaks
> > > > my mental model of how leases work, and if you commit to this for ext4
> > > > then I'll have to remember that leases are different between xfs and
> > > > ext4.  Worse, since the reason for skipping ext4_break_layouts seems to
> > > > be the implementation detail that "DAX won't care", then someone else
> > > > wiring up pNFS/future RDMA/whatever will also have to remember to put it
> > > > back into ext4 or else kaboom.
> > > > 
> > > > Granted, Dave said all these things already, but I actually feel
> > > > strongly enough to reiterate.
> > > 
> > > Jan, would you like me to call ext4_break_layouts() in ext4_insert_range() to
> > > keep the lease mechanism consistent between ext4 and XFS, or would you prefer
> > > the s/ext4_break_layouts()/ext4_dax_unmap_pages()/ rename?
> > 
> > Let's just call it from ext4_insert_range(). I think the simple semantics
> > Dave and Darrick defend is more maintainable and insert range isn't really
> > performance critical operation.
> > 
> > The question remains whether equivalent of BREAK_UNMAP is really required
> > also for allocation of new blocks using fallocate. Because that doesn't
> > really seem fundamentally different from normal write which uses
> > BREAK_WRITE for xfs_break_layouts(). And that it more often used operation
> > so bothering with GUP synchronization when not needed could hurt there.
> > Dave, Darrick?
> 
> Hmm, IIRC BREAK_UNMAP is supposed to be for callers who are going to
> remove (or move) mappings that already exist, so that the caller blocks
> until the lessee acknowledges that they've forgotten all the mappings
> they knew about.  So I /think/ for fallocate mode 0 I think this could
> be BREAK_WRITE instead of _UNMAP, though (at least for xfs) the other
> modes all need _UNMAP.

OK, so we are on the same page here :)

> Side question: in xfs_file_aio_write_checks, do we need to do
> BREAK_UNMAP if is possible that writeback will end up performing a copy
> write?  Granted, the pnfs export and dax stuff don't support reflink or
> cow so I guess this is an academic question for now...

My naive understanding would be that yes, BREAK_UNMAP is needed in such
case...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR