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 from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
