Re: [RFC PATCH v2 07/19] fs/xfs: Teach xfs to use new dax_layout_busy_page()

[Dave Chinner]

On Mon, Aug 12, 2019 at 11:05:51AM -0700, Ira Weiny wrote:
> On Sat, Aug 10, 2019 at 09:30:37AM +1000, Dave Chinner wrote:
> > On Fri, Aug 09, 2019 at 03:58:21PM -0700, ira.weiny@xxxxxxxxx wrote:
> > > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > 
> > > dax_layout_busy_page() can now operate on a sub-range of the
> > > address_space provided.
> > > 
> > > Have xfs specify the sub range to dax_layout_busy_page()
> > 
> > Hmmm. I've got patches that change all these XFS interfaces to
> > support range locks. I'm not sure the way the ranges are passed here
> > is the best way to do it, and I suspect they aren't correct in some
> > cases, either....
> > 
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index ff3c1fae5357..f0de5486f6c1 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -1042,10 +1042,16 @@ xfs_vn_setattr(
> > >  		xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
> > >  		iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
> > >  
> > > -		error = xfs_break_layouts(inode, &iolock, BREAK_UNMAP);
> > > -		if (error) {
> > > -			xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
> > > -			return error;
> > > +		if (iattr->ia_size < inode->i_size) {
> > > +			loff_t                  off = iattr->ia_size;
> > > +			loff_t                  len = inode->i_size - iattr->ia_size;
> > > +
> > > +			error = xfs_break_layouts(inode, &iolock, off, len,
> > > +						  BREAK_UNMAP);
> > > +			if (error) {
> > > +				xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
> > > +				return error;
> > > +			}
> > 
> > This isn't right - truncate up still needs to break the layout on
> > the last filesystem block of the file,
> 
> I'm not following this?  From a user perspective they can't have done anything
> with the data beyond the EOF.  So isn't it safe to allow EOF to grow without
> changing the layout of that last block?


You're looking at this from the perspective of what RDMA page
pinning, not what the guarantees a filesystem has to provide layout
holders.

For example, truncate up has to zero the portion of the block beyond
EOF and that requires a data write. What happens if that block is a
shared extent and hence we have do a copy on write and alter the
file layout?

Or perhaps that tail block still has dirty data over it that is
marked for delayed allocation? Truncate up will have to write that
data to zero the delayed allocation extent that spans EOF, and hence
the truncate modifies the layout because it triggers allocation.

i.e. just because an operation does not change user data, it does
not mean that it will not change the file layout. There is a chance
that truncate up will modify the layout and so we need to break the
layout leases that span the range from the old size to the new
size...

> > and truncate down needs to
> > extend to "maximum file offset" because we remove all extents beyond
> > EOF on a truncate down.
> 
> Ok, I was trying to allow a user to extend the file without conflicts if they
> were to have a pin on the 'beginning' of the original file.

If we want to allow file extension under a layout lease, the lease
has to extend beyond EOF, otherwise the new section of the file is
not covered by a lease. If leases only extend to the existing
EOF, then once the new data is written and the file is extended,
then the lease owner needs to take a new lease on the range they
just wrote. SO the application ends up having to do write - lease
-write -lease - .... so that it has leases covering the range of the
file it is extending into.

Much better it to define a lease that extends to max file offset,
such that it always covers they range past the existing EOF and
extending writes will automatically be covered. What this then does
is to trigger layout break notifications on file size change, either
by write, truncate, fallocate, without having to actually know or
track the exactly file size in the lease....

> This sounds like
> you are saying that a layout lease must be dropped to do that?  In some ways I
> think I understand what you are driving at and I think I see how I may have
> been playing "fast and loose" with the strictness of the layout lease.  But
> from a user perspective if there is a part of the file which "does not exist"
> (beyond EOF) does it matter that the layout there may change?

Yes, it does, because userspace can directly manipulate the layout
beyond EOF via fallocate(). e.g. we can preallocation beyond EOF
without changing the file size, such that when we then do an
extending write no layout change actually takes place. The only
thing that happens from a layout point of view is that the file size
changes.

This becomes /interesting/ when you start doing things like

	lseek(fd, offset, SEEK_END);
	write(fd, buf, len);

which will trigger a write way beyond EOF into allocated space.
That will also trigger block zeroing at the old tail, and there may
be block zeroing around the write() as well. We've effectively
change the layout of the file at EOF,  We've effectively change the
layout of the file at EOF, and potentially beyond EOF.

Indeed, the app might be expecting the preallocation beyond EOF to
remain, so it might register a layout over that range to be notified
if the preallocation is removed or the EOF extends beyond it. It
needs to be notified on truncate down (which removes that
preallocated range the lease sits over) and EOF is moved beyond it
(layout range state has changed from inaccessable to valid file
data)....


> > i.e. when we use preallocation, the extent map extends beyond EOF,
> > and layout leases need to be able to extend beyond the current EOF
> > to allow the lease owner to do extending writes, extending truncate,
> > preallocation beyond EOF, etc safely without having to get a new
> > lease to cover the new region in the extended file...
> 
> I'm not following this.  What determines when preallocation is done?

The application can direct it via fallocate(FALLOC_FL_KEEPSIZE).
It's typically used for workloads that do appending O_DSYNC or
direct IO writes to minimise file fragmentation.

The filesystem can ialso choose to do allocation beyond EOFi
speculatively during writes. XFS does this extensively with delayed
allocation. And the filesystem can also remove this speculative
allocation beyond EOF, which it may do if there are no active pages
dirties on the inode for a period, it is reclaimed, the filesystem
is running low on space, the user/group is running low on quota
space, etc.

Again, just because user data does not change, it does not mean that
the file layout will not change....

> Forgive my ignorance on file systems but how can we have a layout for every
> file which is "maximum file offset" for every file even if a file is only 1
> page long?

The layout lease doesn't care what the file size it. It doesn't even
know what the file size is. The layout lease covers a range the
logical file offset with the intend that any change to the file
layout within that range will result in a notification. The layout
lease is not bound to the range of valid data in the file at all -
it doesn't matter if it points beyond EOF - if the file grows to
the size the it overlaps the layout lease, then that layout lease
needs to be notified by break_layouts....

I've had a stinking headache all day, so I'm struggling to make
sense right now. The best I can describe is that layout lease ranges
do not imply or require valid file data to exist within the range
they are taken over - they just cover a file offset range.

FWIW, the fcntl() locking interface uses a length of 0 to
indicate "to max file offset" rather than a specific length. e.g.
SETLK and friends:

	Specifying 0 for l_len has the special meaning: lock all
	bytes starting at the location specified by l_whence and
	l_start through to the end of file, no  matter
	how large the file grows.

That's exactly the semantics I'm talking about here - layout leases
need to be able to specify an extent anywhere within the valid file
offset range, and also to specify a nebulous "through to the end of
the layout range" so taht file growth can be done without needing
new leases to be taken as the file grows....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx