Re: [PATCH v2 5/5] btrfs: enable swap file support

[Omar Sandoval <osandov@osandov.com>]

On Sat, Nov 22, 2014 at 12:03:57PM -0800, Omar Sandoval wrote:
> On Fri, Nov 21, 2014 at 07:00:45PM +0100, David Sterba wrote:
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > > +		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
> > > +			pr_err("BTRFS: swapfile is compresed");
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > 
> > I think the preallocated extents should be refused as well. This means
> > the filesystem has enough space to hold the data but it would still have
> > to go through the allocation and could in turn stress the memory
> > management code that triggered the swapping activity in the first place.
> > 
> > Though it's probably still possible to reach such corner case even with
> > fully allocated nodatacow file, this should be reviewed anyway.
> > 
> I'll definitely take a closer look at this. In particular,
> btrfs_get_blocks_direct and btrfs_get_extent do allocations in some cases which
> I'll look into.
> 
Alright, I took a look at this. My understanding is that a PREALLOC extent
represents a region on disk that has already been allocated but isn't in use
yet, but please correct me if I'm wrong. Judging by this comment in
btrfs_get_blocks_direct, we don't have to worry about PREALLOC extents in
general:

/*
 * We don't allocate a new extent in the following cases
 *
 * 1) The inode is marked as NODATACOW.  In this case we'll just use the
 * existing extent.
 * 2) The extent is marked as PREALLOC.  We're good to go here and can
 * just use the extent.
 *
 */

A couple of other considerations that cropped up:

- btrfs_get_extent does a bunch of extra work if the extent is not cached in
  the extent map tree that would be nice to avoid when swapping
- We might still have to do a COW if the swap file is in a snapshot

We can avoid the btrfs_get_extent by pinning the extents in memory one way or
another in btrfs_swap_activate.

The snapshot issue is a little tricker to resolve. I see a few options:

1. Just do the COW and hope for the best
2. As part of btrfs_swap_activate, COW any shared extents. If a snapshot
happens while a swap file is active, we'll fall back to 1.
3. Clobber any swap file extents which are in a snapshot, i.e., always use the
existing extent.

I'm partial to 3, as it's the simplest approach, and I don't think it makes
much sense for a swap file to be in a snapshot anyways. I'd appreciate any
comments that anyone might have.

-- 
Omar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>