Btrfs: fix our overcommit math

Commit Message

Josef Bacik Sept. 6, 2012, 9:03 p.m. UTC

I noticed I was seeing large lags when running my torrent test in a vm on my
laptop.  While trying to make it lag less I noticed that our overcommit math
was taking into account the number of bytes we wanted to reclaim, not the
number of bytes we actually wanted to allocate, which means we wouldn't
overcommit as often.  This patch fixes the overcommit math and makes
shrink_delalloc() use that logic so that it will stop looping faster.  We
still have pretty high spikes of latency, but the test now takes 3 minutes
less time (about 5% faster).  Thanks,

Signed-off-by: Josef Bacik <[e-mail]>
---
 fs/btrfs/extent-tree.c |   70 ++++++++++++++++++++++++++++--------------------
 1 files changed, 41 insertions(+), 29 deletions(-)

Comments

Zach Brown Sept. 6, 2012, 9:21 p.m. UTC | #1

> +	used = space_info->bytes_used + space_info->bytes_reserved +
> +		space_info->bytes_pinned + space_info->bytes_readonly +
> +		space_info->bytes_may_use;
> +		spin_lock(&root->fs_info->free_chunk_lock);
> +	avail = root->fs_info->free_chunk_space;

Is that lock only protecting the read of free_chunk_space?  Might as
well just acquire and release around that and get all that local math
out of the critical section?

(I first thought the spin_lock() was accidentally dropped until I saw it
hiding all tabbed in and smushed against the used assignment.)

>  		spin_lock(&space_info->lock);
> -		if (space_info->bytes_used + space_info->bytes_reserved +
> -		    space_info->bytes_pinned + space_info->bytes_readonly +
> -		    space_info->bytes_may_use + orig <=
> -		    space_info->total_bytes) {
> +		if (can_overcommit(root, space_info, orig, !trans)) {
>  			spin_unlock(&space_info->lock);

*spots lock nesting, puts on his "only guy who runs with lockdep"
worrying hat*

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

Patch

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4bb546da..5f94ca2 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3663,6 +3663,45 @@  out:
 	return ret;
 }
 
+static int can_overcommit(struct btrfs_root *root,
+			  struct btrfs_space_info *space_info, u64 bytes,
+			  int flush)
+{
+	u64 profile = btrfs_get_alloc_profile(root, 0);
+	u64 avail;
+	u64 used;
+
+	used = space_info->bytes_used + space_info->bytes_reserved +
+		space_info->bytes_pinned + space_info->bytes_readonly +
+		space_info->bytes_may_use;
+		spin_lock(&root->fs_info->free_chunk_lock);
+	avail = root->fs_info->free_chunk_space;
+
+	/*
+	 * If we have dup, raid1 or raid10 then only half of the free
+	 * space is actually useable.
+	 */
+	if (profile & (BTRFS_BLOCK_GROUP_DUP |
+		       BTRFS_BLOCK_GROUP_RAID1 |
+		       BTRFS_BLOCK_GROUP_RAID10))
+		avail >>= 1;
+
+	/*
+	 * If we aren't flushing don't let us overcommit too much, say
+	 * 1/8th of the space.  If we can flush, let it overcommit up to
+	 * 1/2 of the space.
+	 */
+	if (flush)
+		avail >>= 3;
+	else
+		avail >>= 1;
+	 spin_unlock(&root->fs_info->free_chunk_lock);
+
+	if (used + bytes < space_info->total_bytes + avail)
+		return 1;
+	return 0;
+}
+
 /*
  * shrink metadata reservation for delalloc
  */
@@ -3706,10 +3745,7 @@  static void shrink_delalloc(struct btrfs_root *root, u64 to_reclaim, u64 orig,
 			  !atomic_read(&root->fs_info->async_delalloc_pages));
 
 		spin_lock(&space_info->lock);
-		if (space_info->bytes_used + space_info->bytes_reserved +
-		    space_info->bytes_pinned + space_info->bytes_readonly +
-		    space_info->bytes_may_use + orig <=
-		    space_info->total_bytes) {
+		if (can_overcommit(root, space_info, orig, !trans)) {
 			spin_unlock(&space_info->lock);
 			break;
 		}
@@ -3925,7 +3961,6 @@  again:
 	}
 
 	if (ret) {
-		u64 profile = btrfs_get_alloc_profile(root, 0);
 		u64 avail;
 
 		/*
@@ -3946,30 +3981,7 @@  again:
 			goto again;
 		}
 
-		spin_lock(&root->fs_info->free_chunk_lock);
-		avail = root->fs_info->free_chunk_space;
-
-		/*
-		 * If we have dup, raid1 or raid10 then only half of the free
-		 * space is actually useable.
-		 */
-		if (profile & (BTRFS_BLOCK_GROUP_DUP |
-			       BTRFS_BLOCK_GROUP_RAID1 |
-			       BTRFS_BLOCK_GROUP_RAID10))
-			avail >>= 1;
-
-		/*
-		 * If we aren't flushing don't let us overcommit too much, say
-		 * 1/8th of the space.  If we can flush, let it overcommit up to
-		 * 1/2 of the space.
-		 */
-		if (flush)
-			avail >>= 3;
-		else
-			avail >>= 1;
-		 spin_unlock(&root->fs_info->free_chunk_lock);
-
-		if (used + num_bytes < space_info->total_bytes + avail) {
+		if (can_overcommit(root, space_info, orig_bytes, flush)) {
 			space_info->bytes_may_use += orig_bytes;
 			trace_btrfs_space_reservation(root->fs_info,
 				"space_info", space_info->flags, orig_bytes, 1);