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@xxxxxxxxxxxxxxx> > > > > > > > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > > > > > > > Reviewed-by: Lukas Czerner <lczerner@xxxxxxxxxx> > > > > > > > > --- > > > > > > > > > > > > > > > > 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@xxxxxxxx> SUSE Labs, CR
