btrfs: implement swap file support

Reads through the iov_iter infrastructure for kernel pages shouldn't be dirtied by the direct I/O code.

This is based on Dave Kleikamp's and Ming Lei's previously posted patches.

Cc Ming Lei <[email protected]>

Signed-off-by Omar Sandoval <[email protected]>

---
 fs/direct-io.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

fs/direct-io.c +4 -3

index e181b6b..e542ce4 100644
@@ -120,6 +120,7 @@ struct dio {
 	spinlock_t bio_lock;		/* protects BIO fields below */
 	int page_errors;		/* errno from get_user_pages() */
 	int is_async;			/* is IO async ? */
+	int should_dirty;		/* should we mark read pages dirty? */
 	bool defer_completion;		/* defer AIO completion to workqueue? */
 	int io_error;			/* IO error in completion path */
 	unsigned long refcount;		/* direct_io_worker() and bios */

@@ -392,7 +393,7 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio)
 	dio->refcount++;
 	spin_unlock_irqrestore(&dio->bio_lock, flags);
 
-	if (dio->is_async && dio->rw == READ)
+	if (dio->is_async && dio->rw == READ && dio->should_dirty)
 		bio_set_pages_dirty(bio);
 
 	if (sdio->submit_io)

@@ -463,13 +464,13 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
 	if (!uptodate)
 		dio->io_error = -EIO;
 
-	if (dio->is_async && dio->rw == READ) {
+	if (dio->is_async && dio->rw == READ && dio->should_dirty) {
 		bio_check_pages_dirty(bio);	/* transfers ownership */
 	} else {
 		bio_for_each_segment_all(bvec, bio, i) {
 			struct page *page = bvec->bv_page;
 
-			if (dio->rw == READ && !PageCompound(page))
+			if (dio->rw == READ && !PageCompound(page) && dio->should_dirty)
 				set_page_dirty_lock(page);
 			page_cache_release(page);
 		}

With the new iov_iter infrastructure that supprots direct I/O to kernel pages please get rid of the ->readpage hack first. I'm still utterly disapoined that this crap ever got merged.

fs/btrfs/ioctl.c +6 -7

 		/*
-		 * Revert back under same assuptions as above
+		 * swap_activate checks that we don't swapon a copy-on-write
+		 * file, but we must also make sure that it doesn't become
+		 * copy-on-write.
 		 */
-		if (S_ISREG(mode)) {
-			if (inode->i_size == 0)
-				ip->flags &= ~(BTRFS_INODE_NODATACOW
-				             | BTRFS_INODE_NODATASUM);
-		} else {
-			ip->flags &= ~BTRFS_INODE_NODATACOW;
+		if (!IS_SWAPFILE(inode)) {
+			/*
+			 * Revert back under same assumptions as above

Implement the swap file a_ops on btrfs. Activation simply checks for a usable swap file: it must be fully allocated (no holes), support direct I/O (so no compressed or inline extents) and should be nocow (I'm not sure about that last one).

Signed-off-by Omar Sandoval <[email protected]>

---
 fs/btrfs/inode.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

fs/btrfs/inode.c +0 -0

index d23362f..b8fd36b 100644

fs/btrfs/inode.c +16 -0

+		return -EINVAL;
+	}
+	if (!(ip->flags & BTRFS_INODE_NODATACOW)) {
+		/* The swap file can't be copy-on-write. */
+		pr_err("BTRFS: swapfile is copy-on-write");
+		return -EINVAL;
+	}
+
+	lock_extent_bits(&ip->io_tree, 0, isize - 1, 0, &cached_state);
+
+	/*
+	 * All of the extents must be allocated and support direct I/O. Inline
+	 * extents and compressed extents fall back to buffered I/O, so those
+	 * are no good.
+	 */
+	start = 0;

Quoted 15 lines · Omar Sandoval

Implement the swap file a_ops on btrfs. Activation simply checks for a usable swap file: it must be fully allocated (no holes), support direct I/O (so no compressed or inline extents) and should be nocow (I'm not sure about that last one).

Signed-off-by Omar Sandoval <[email protected]>

---
 fs/btrfs/inode.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

fs/btrfs/inode.c +0 -0

index d23362f..b8fd36b 100644

fs/btrfs/inode.c out_inode: · line 9449

Quoted 8 lines

fs/btrfs/inode.c +5 -0

@@ -9442,6 +9442,75 @@ out_inode:
 
 }
 
+static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
+                            sector_t *span)
+{
+     struct inode *inode = file_inode(file);
+     struct btrfs_inode *ip = BTRFS_I(inode);

'ip' looks strange in context of a filesystem, please pick a different name (eg. 'inode' or whatever).

fs/btrfs/inode.c out_inode: · line 9458

Quoted 9 lines

fs/btrfs/inode.c +9 -0

+     int ret = 0;
+     u64 isize = inode->i_size;
+     struct extent_state *cached_state = NULL;
+     struct extent_map *em;
+     u64 start, len;
+
+     if (ip->flags & BTRFS_INODE_COMPRESS) {
+             /* Can't do direct I/O on a compressed file. */
+             pr_err("BTRFS: swapfile is compressed");

Please use the btrfs_err macros everywhere.

Quoted 22 lines

fs/btrfs/inode.c +22 -0

+             return -EINVAL;
+     }
+     if (!(ip->flags & BTRFS_INODE_NODATACOW)) {
+             /* The swap file can't be copy-on-write. */
+             pr_err("BTRFS: swapfile is copy-on-write");
+             return -EINVAL;
+     }
+
+     lock_extent_bits(&ip->io_tree, 0, isize - 1, 0, &cached_state);
+
+     /*
+      * All of the extents must be allocated and support direct I/O. Inline
+      * extents and compressed extents fall back to buffered I/O, so those
+      * are no good.
+      */
+     start = 0;
+     while (start < isize) {
+             len = isize - start;
+             em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
+             if (IS_ERR(em)) {
+                     ret = PTR_ERR(em);
+                     goto out;

fs/btrfs/inode.c out_inode: · line 9484

Quoted 4 lines

fs/btrfs/inode.c +4 -0

+             }
+
+             if (test_bit(EXTENT_FLAG_VACANCY, &em->flags) ||
+                 em->block_start == EXTENT_MAP_HOLE) {

If the no-holes feature is enabled on the filesystem, there won't be any such extent representing the hole. You have to check that each two consecutive extents are adjacent.

fs/btrfs/inode.c out_inode: · line 9490

Quoted 7 lines

fs/btrfs/inode.c +6 -0

+                     pr_err("BTRFS: swapfile has holes");
+                     ret = -EINVAL;
+                     goto out;
+             }
+             if (em->block_start == EXTENT_MAP_INLINE) {
+                     pr_err("BTRFS: swapfile is inline");

While the test is valid, this would mean that the file is smaller than the inline limit, which is now one page. I think the generic swap code would refuse such a small file anyway.

fs/btrfs/inode.c out_inode: · line 9498

Quoted 8 lines

fs/btrfs/inode.c +8 -0

+                     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.

fs/btrfs/inode.c out_inode: · line 9508

Quoted 10 lines

fs/btrfs/inode.c +10 -0

+
+             start = extent_map_end(em);
+             free_extent_map(em);
+     }
+
+out:
+     unlock_extent_cached(&ip->io_tree, 0, isize - 1, &cached_state,
+                          GFP_NOFS);
+     return ret;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

fs/btrfs/inode.c out_inode: · line 9490

Quoted 6 lines

fs/btrfs/inode.c +6 -0

+			pr_err("BTRFS: swapfile has holes");
+			ret = -EINVAL;
+			goto out;
+		}
+		if (em->block_start == EXTENT_MAP_INLINE) {
+			pr_err("BTRFS: swapfile is inline");

While the test is valid, this would mean that the file is smaller than the inline limit, which is now one page. I think the generic swap code would refuse such a small file anyway.

fs/btrfs/inode.c out_inode: · line 9498

Quoted 8 lines

fs/btrfs/inode.c +8 -0

+			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.

Quoted 14 lines · David Sterba

fs/btrfs/inode.c out_inode: · line 9498

Quoted 8 lines

fs/btrfs/inode.c +8 -0

+			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.

Quoted 3 lines

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.

[snip]

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.

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.

Sorry for the noise, looks like Christoph got back to me on the previous RFC just before I sent this out -- disregard this for now.

Quoted 2 lines · Omar Sandoval

Sorry for the noise, looks like Christoph got back to me on the previous RFC just before I sent this out -- disregard this for now.

If the NFS people are fine with this version I'd certainly welcome it as a first step. Additional improvements are of course always welcome.