Re: fallocate on XFS for swap

[Dave Chinner <david@fromorbit.com>]

On Fri, Mar 09, 2018 at 05:17:07PM -0800, Darrick J. Wong wrote:
> On Sat, Mar 10, 2018 at 11:58:50AM +1100, Dave Chinner wrote:
> > On Fri, Mar 09, 2018 at 03:44:22PM -0800, Darrick J. Wong wrote:
> > > [you really ought to cc the xfs list]
> > > 
> > > On Fri, Mar 09, 2018 at 10:05:24PM +0000, Besogonov, Aleksei wrote:
> > > > Hi!
> > > > 
> > > > We’re working at Amazon on making XFS our default root filesystem for
> > > > the upcoming Amazon Linux 2 (now in prod preview). One of the problems
> > > > that we’ve encountered is inability to use fallocated files for swap
> > > > on XFS. This is really important for us, since we’re shipping our
> > > > current Amazon Linux with hibernation support .
> > > 
> > > <shudder>
> > > 
> > > > I’ve traced the problem to bmap(), used in generic_swapfile_activate
> > > > call, which returns 0 for blocks inside holes created by fallocate and
> > > > Dave Chinner confirmed it in a private email. I’m thinking about ways
> > > > to fix it, so far I see the following possibilities:
> > > > 
> > > > 1. Change bmap() to not return zeroes for blocks inside holes. But
> > > > this is an ABI change and it likely will break some obscure userspace
> > > > utility somewhere.
> > > 
> > > bmap is a horrible interface, let's leave it to wither and eventually go
> > > away.
> > > 
> > > > 2. Change generic_swap_activate to use a more modern interface, by
> > > > adding fiemap-like operation to address_space_operations with fallback
> > > > on bmap().
> > > 
> > > Probably the best idea, but see fs/iomap.c since we're basically leasing
> > > a chunk of file space to the kernel.  Leasing space to a user that wants
> > > direct access is becoming rather common (rdma, map_sync, etc.)
> > 
> > thing is, we don't want in-kernel users of fiemap. We've got other
> > block mapping interfaces that can be used, such as iomap...
> 
> Well yes, I was clumsily trying to suggest reimplementing
> generic_swap_activate with an iomap backend replacing/augmenting the old
> get_blocks thing... :)
> 
> > > > 3. Add an XFS-specific implementation of swapfile_activate.
> > > 
> > > Ugh no.
> > 
> > What we want is an iomap-based re-implementation of
> > generic_swap_activate(). One of the ways to plumb that in is to
> > use ->swapfile_activate() like so:
> 
> Is this distinct from the ->swap_activate function pointer in
> address_operations or a new one?  I think it'd be best to have it be a
> separate callback like you suggest:

No, we don't need to create a new one - the existing one is used by
a single caller and we can easily move all the functionality it
requires inside the NFS specific implementation - it's just mapping
the entire range as a single extent, but the callout is needed to
mark the sockets backing the file as in the memalloc path...

> > iomap_swapfile_activate()
> > {
> > 	return iomap_apply(... iomap_swapfile_add_extent, ...)
> > }
> > 
> > xfs_vm_swapfile_activate()
> > {
> > 	return iomap_swapfile_activate(xfs_iomap_ops);
> > }
> > 
> > 	.swapfile_activate = xfs_vm_swapfile_activate()
> > 
> > And massage the swapfile_activate callout be friendly to fragmented
> > files. i.e. change the nfs caller to run a
> > "add_single_swap_extent()" caller rather than have to do it in the
> > generic code on return....
> 
> But ugh, the names are confusing.  ->swapfile_activate, ->swap_activate,
> and generic_swapfile_activate.  Not sure what's needed to clean up the
> other filesystems to use a single mapping interface, though.

If they don't implement the callout, they use the
generic_swapfile_activate code that currently exists. Maybe with a
name change, but this way we don't have to touch them....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com