[PATCH 0/4] Accounting fixes

[PATCH 0/4] Accounting fixes

[Josef Bacik]

I've changed all of these a whole bunch recently, I think I've gotten all the
issues worked out, please review/test to make sure there's not some other
fucking corner I've missed.  Thanks,

Josef

[PATCH 1/4] Btrfs: account merges/splits properly

[Josef Bacik]

My fix

Btrfs: fix merge delalloc logic

only fixed half of the problems, it didn't fix the case where we have two large
extents on either side and then join them together with a new small extent.  We
need to instead keep track of how many extents we have accounted for with each
side of the new extent, and then see how many extents we need for the new large
extent.  If they match then we know we need to keep our reservation, otherwise
we need to drop our reservation.  This shows up with a case like this

[BTRFS_MAX_EXTENT_SIZE+4K][4K HOLE][BTRFS_MAX_EXTENT_SIZE+4K]

Previously the logic would have said that the number extents required for the
new size (3) is larger than the number of extents required for the largest side
(2) therefore we need to keep our reservation.  But this isn't the case, since
both sides require a reservation of 2 which leads to 4 for the whole range
currently reserved, but we only need 3, so we need to drop one of the
reservations.  The same problem existed for splits, we'd think we only need 3
extents when creating the hole but in reality we need 4.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/inode.c | 57 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 97b601b..bb74a41 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1542,30 +1542,17 @@ static void btrfs_split_extent_hook(struct inode *inode,
 		u64 new_size;
 
 		/*
-		 * We need the largest size of the remaining extent to see if we
-		 * need to add a new outstanding extent.  Think of the following
-		 * case
-		 *
-		 * [MEAX_EXTENT_SIZEx2 - 4k][4k]
-		 *
-		 * The new_size would just be 4k and we'd think we had enough
-		 * outstanding extents for this if we only took one side of the
-		 * split, same goes for the other direction.  We need to see if
-		 * the larger size still is the same amount of extents as the
-		 * original size, because if it is we need to add a new
-		 * outstanding extent.  But if we split up and the larger size
-		 * is less than the original then we are good to go since we've
-		 * already accounted for the extra extent in our original
-		 * accounting.
+		 * See the explanation in btrfs_merge_extent_hook, the same
+		 * applies here, just in reverse.
 		 */
 		new_size = orig->end - split + 1;
-		if ((split - orig->start) > new_size)
-			new_size = split - orig->start;
-
-		num_extents = div64_u64(size + BTRFS_MAX_EXTENT_SIZE - 1,
+		num_extents = div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
 					BTRFS_MAX_EXTENT_SIZE);
-		if (div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
-			      BTRFS_MAX_EXTENT_SIZE) < num_extents)
+		new_size = split - orig->start;
+		num_extents += div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
+					BTRFS_MAX_EXTENT_SIZE);
+		if (div64_u64(size + BTRFS_MAX_EXTENT_SIZE - 1,
+			      BTRFS_MAX_EXTENT_SIZE) >= num_extents)
 			return;
 	}
 
@@ -1591,9 +1578,6 @@ static void btrfs_merge_extent_hook(struct inode *inode,
 	if (!(other->state & EXTENT_DELALLOC))
 		return;
 
-	old_size = other->end - other->start + 1;
-	if (old_size < (new->end - new->start + 1))
-		old_size = (new->end - new->start + 1);
 	if (new->start > other->start)
 		new_size = new->end - other->start + 1;
 	else
@@ -1608,13 +1592,32 @@ static void btrfs_merge_extent_hook(struct inode *inode,
 	}
 
 	/*
-	 * If we grew by another max_extent, just return, we want to keep that
-	 * reserved amount.
+	 * We have to add up either side to figure out how many extents were
+	 * accounted for before we merged into one big extent.  If the number of
+	 * extents we accounted for is <= the amount we need for the new range
+	 * then we can return, otherwise drop.  Think of it like this
+	 *
+	 * [ 4k][MAX_SIZE]
+	 *
+	 * So we've grown the extent by a MAX_SIZE extent, this would mean we
+	 * need 2 outstanding extents, on one side we have 1 and the other side
+	 * we have 1 so they are == and we can return.  But in this case
+	 *
+	 * [MAX_SIZE+4k][MAX_SIZE+4k]
+	 *
+	 * Each range on their own accounts for 2 extents, but merged together
+	 * they are only 3 extents worth of accounting, so we need to drop in
+	 * this case.
 	 */
+	old_size = other->end - other->start + 1;
 	num_extents = div64_u64(old_size + BTRFS_MAX_EXTENT_SIZE - 1,
 				BTRFS_MAX_EXTENT_SIZE);
+	old_size = new->end - new->start + 1;
+	num_extents += div64_u64(old_size + BTRFS_MAX_EXTENT_SIZE - 1,
+				 BTRFS_MAX_EXTENT_SIZE);
+
 	if (div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
-		      BTRFS_MAX_EXTENT_SIZE) > num_extents)
+		      BTRFS_MAX_EXTENT_SIZE) >= num_extents)
 		return;
 
 	spin_lock(&BTRFS_I(inode)->lock);
-- 
1.8.3.1

Re: [PATCH 1/4] Btrfs: account merges/splits properly

[Filipe David Manana]

On Tue, Mar 17, 2015 at 8:38 PM, Josef Bacik <jbacik@fb.com> wrote:
> My fix
>
> Btrfs: fix merge delalloc logic
>
> only fixed half of the problems, it didn't fix the case where we have two large
> extents on either side and then join them together with a new small extent.  We
> need to instead keep track of how many extents we have accounted for with each
> side of the new extent, and then see how many extents we need for the new large
> extent.  If they match then we know we need to keep our reservation, otherwise
> we need to drop our reservation.  This shows up with a case like this
>
> [BTRFS_MAX_EXTENT_SIZE+4K][4K HOLE][BTRFS_MAX_EXTENT_SIZE+4K]
>
> Previously the logic would have said that the number extents required for the
> new size (3) is larger than the number of extents required for the largest side
> (2) therefore we need to keep our reservation.  But this isn't the case, since
> both sides require a reservation of 2 which leads to 4 for the whole range
> currently reserved, but we only need 3, so we need to drop one of the
> reservations.  The same problem existed for splits, we'd think we only need 3
> extents when creating the hole but in reality we need 4.  Thanks,
>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
Tested-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>  fs/btrfs/inode.c | 57 +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 97b601b..bb74a41 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1542,30 +1542,17 @@ static void btrfs_split_extent_hook(struct inode *inode,
>                 u64 new_size;
>
>                 /*
> -                * We need the largest size of the remaining extent to see if we
> -                * need to add a new outstanding extent.  Think of the following
> -                * case
> -                *
> -                * [MEAX_EXTENT_SIZEx2 - 4k][4k]
> -                *
> -                * The new_size would just be 4k and we'd think we had enough
> -                * outstanding extents for this if we only took one side of the
> -                * split, same goes for the other direction.  We need to see if
> -                * the larger size still is the same amount of extents as the
> -                * original size, because if it is we need to add a new
> -                * outstanding extent.  But if we split up and the larger size
> -                * is less than the original then we are good to go since we've
> -                * already accounted for the extra extent in our original
> -                * accounting.
> +                * See the explanation in btrfs_merge_extent_hook, the same
> +                * applies here, just in reverse.
>                  */
>                 new_size = orig->end - split + 1;
> -               if ((split - orig->start) > new_size)
> -                       new_size = split - orig->start;
> -
> -               num_extents = div64_u64(size + BTRFS_MAX_EXTENT_SIZE - 1,
> +               num_extents = div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
>                                         BTRFS_MAX_EXTENT_SIZE);
> -               if (div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
> -                             BTRFS_MAX_EXTENT_SIZE) < num_extents)
> +               new_size = split - orig->start;
> +               num_extents += div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
> +                                       BTRFS_MAX_EXTENT_SIZE);
> +               if (div64_u64(size + BTRFS_MAX_EXTENT_SIZE - 1,
> +                             BTRFS_MAX_EXTENT_SIZE) >= num_extents)
>                         return;
>         }
>
> @@ -1591,9 +1578,6 @@ static void btrfs_merge_extent_hook(struct inode *inode,
>         if (!(other->state & EXTENT_DELALLOC))
>                 return;
>
> -       old_size = other->end - other->start + 1;
> -       if (old_size < (new->end - new->start + 1))
> -               old_size = (new->end - new->start + 1);
>         if (new->start > other->start)
>                 new_size = new->end - other->start + 1;
>         else
> @@ -1608,13 +1592,32 @@ static void btrfs_merge_extent_hook(struct inode *inode,
>         }
>
>         /*
> -        * If we grew by another max_extent, just return, we want to keep that
> -        * reserved amount.
> +        * We have to add up either side to figure out how many extents were
> +        * accounted for before we merged into one big extent.  If the number of
> +        * extents we accounted for is <= the amount we need for the new range
> +        * then we can return, otherwise drop.  Think of it like this
> +        *
> +        * [ 4k][MAX_SIZE]
> +        *
> +        * So we've grown the extent by a MAX_SIZE extent, this would mean we
> +        * need 2 outstanding extents, on one side we have 1 and the other side
> +        * we have 1 so they are == and we can return.  But in this case
> +        *
> +        * [MAX_SIZE+4k][MAX_SIZE+4k]
> +        *
> +        * Each range on their own accounts for 2 extents, but merged together
> +        * they are only 3 extents worth of accounting, so we need to drop in
> +        * this case.
>          */
> +       old_size = other->end - other->start + 1;
>         num_extents = div64_u64(old_size + BTRFS_MAX_EXTENT_SIZE - 1,
>                                 BTRFS_MAX_EXTENT_SIZE);
> +       old_size = new->end - new->start + 1;
> +       num_extents += div64_u64(old_size + BTRFS_MAX_EXTENT_SIZE - 1,
> +                                BTRFS_MAX_EXTENT_SIZE);
> +
>         if (div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
> -                     BTRFS_MAX_EXTENT_SIZE) > num_extents)
> +                     BTRFS_MAX_EXTENT_SIZE) >= num_extents)
>                 return;
>
>         spin_lock(&BTRFS_I(inode)->lock);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

[PATCH 2/4] Btrfs: just free dummy extent buffers

[Josef Bacik]

If we fail during our sanity tests we could get NULL deref's because we unload
the module before the dummy extent buffers are free'd via RCU.  So check for
this case and just free the things directly.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/extent_io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 29850d4..d13cead 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4968,6 +4968,12 @@ static int release_extent_buffer(struct extent_buffer *eb)
 
 		/* Should be safe to release our pages at this point */
 		btrfs_release_extent_buffer_page(eb);
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
+			__free_extent_buffer(eb);
+			return 1;
+		}
+#endif
 		call_rcu(&eb->rcu_head, btrfs_release_extent_buffer_rcu);
 		return 1;
 	}
-- 
1.8.3.1

Re: [PATCH 2/4] Btrfs: just free dummy extent buffers

[David Sterba]

On Tue, Mar 17, 2015 at 04:38:01PM -0400, Josef Bacik wrote:
> If we fail during our sanity tests we could get NULL deref's because we unload
> the module before the dummy extent buffers are free'd via RCU.  So check for
> this case and just free the things directly.  Thanks,
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/extent_io.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 29850d4..d13cead 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4968,6 +4968,12 @@ static int release_extent_buffer(struct extent_buffer *eb)
>  
>  		/* Should be safe to release our pages at this point */
>  		btrfs_release_extent_buffer_page(eb);
> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

Please use btrfs_test_is_dummy_root, it's there to avoid the #ifdefs

> +		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
> +			__free_extent_buffer(eb);
> +			return 1;
> +		}
> +#endif
>  		call_rcu(&eb->rcu_head, btrfs_release_extent_buffer_rcu);
>  		return 1;
>  	}

Re: [PATCH 2/4] Btrfs: just free dummy extent buffers

[Josef Bacik]

On 03/18/2015 09:56 AM, David Sterba wrote:
> On Tue, Mar 17, 2015 at 04:38:01PM -0400, Josef Bacik wrote:
>> If we fail during our sanity tests we could get NULL deref's because we unload
>> the module before the dummy extent buffers are free'd via RCU.  So check for
>> this case and just free the things directly.  Thanks,
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>>   fs/btrfs/extent_io.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 29850d4..d13cead 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -4968,6 +4968,12 @@ static int release_extent_buffer(struct extent_buffer *eb)
>>
>>   		/* Should be safe to release our pages at this point */
>>   		btrfs_release_extent_buffer_page(eb);
>> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>
> Please use btrfs_test_is_dummy_root, it's there to avoid the #ifdefs

Don't have a root here.  Thanks,

Josef

[PATCH 3/4] Btrfs: add sanity test for outstanding_extents accounting

[Josef Bacik]

I introduced a regression wrt outstanding_extents accounting.  These are tricky
areas that aren't easily covered by xfstests as we could change MAX_EXTENT_SIZE
at any time.  So add sanity tests to cover the various conditions that are
tricky in order to make sure we don't introduce regressions in the future.
Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/ctree.h             |   3 +
 fs/btrfs/extent-tree.c       |   3 +
 fs/btrfs/inode.c             |  15 ++++
 fs/btrfs/tests/inode-tests.c | 197 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 217 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a0c90a3..5bd721a 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3910,6 +3910,9 @@ int btrfs_prealloc_file_range_trans(struct inode *inode,
 				    loff_t actual_len, u64 *alloc_hint);
 int btrfs_inode_check_errors(struct inode *inode);
 extern const struct dentry_operations btrfs_dentry_operations;
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+void btrfs_test_inode_set_ops(struct inode *inode);
+#endif
 
 /* ioctl.c */
 long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3ac3fef..d1ca86e 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5285,6 +5285,9 @@ void btrfs_delalloc_release_metadata(struct inode *inode, u64 num_bytes)
 	if (dropped > 0)
 		to_free += btrfs_calc_trans_metadata_size(root, dropped);
 
+	if (btrfs_test_is_dummy_root(root))
+		return;
+
 	trace_btrfs_space_reservation(root->fs_info, "delalloc",
 				      btrfs_ino(inode), to_free, 0);
 	if (root->fs_info->quota_enabled) {
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index bb74a41..3717b3d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -108,6 +108,13 @@ static struct extent_map *create_pinned_em(struct inode *inode, u64 start,
 
 static int btrfs_dirty_inode(struct inode *inode);
 
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+void btrfs_test_inode_set_ops(struct inode *inode)
+{
+	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
+}
+#endif
+
 static int btrfs_init_inode_security(struct btrfs_trans_handle *trans,
 				     struct inode *inode,  struct inode *dir,
 				     const struct qstr *qstr)
@@ -1694,6 +1701,10 @@ static void btrfs_set_bit_hook(struct inode *inode,
 			spin_unlock(&BTRFS_I(inode)->lock);
 		}
 
+		/* For sanity tests */
+		if (btrfs_test_is_dummy_root(root))
+			return;
+
 		__percpu_counter_add(&root->fs_info->delalloc_bytes, len,
 				     root->fs_info->delalloc_batch);
 		spin_lock(&BTRFS_I(inode)->lock);
@@ -1749,6 +1760,10 @@ static void btrfs_clear_bit_hook(struct inode *inode,
 		    root != root->fs_info->tree_root)
 			btrfs_delalloc_release_metadata(inode, len);
 
+		/* For sanity tests. */
+		if (btrfs_test_is_dummy_root(root))
+			return;
+
 		if (root->root_key.objectid != BTRFS_DATA_RELOC_TREE_OBJECTID
 		    && do_list && !(state->state & EXTENT_NORESERVE))
 			btrfs_free_reserved_data_space(inode, len);
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index a116b55..054fc0d 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -911,6 +911,197 @@ out:
 	return ret;
 }
 
+static int test_extent_accounting(void)
+{
+	struct inode *inode = NULL;
+	struct btrfs_root *root = NULL;
+	int ret = -ENOMEM;
+
+	inode = btrfs_new_test_inode();
+	if (!inode) {
+		test_msg("Couldn't allocate inode\n");
+		return ret;
+	}
+
+	root = btrfs_alloc_dummy_root();
+	if (IS_ERR(root)) {
+		test_msg("Couldn't allocate root\n");
+		goto out;
+	}
+
+	root->fs_info = btrfs_alloc_dummy_fs_info();
+	if (!root->fs_info) {
+		test_msg("Couldn't allocate dummy fs info\n");
+		goto out;
+	}
+
+	BTRFS_I(inode)->root = root;
+	btrfs_test_inode_set_ops(inode);
+
+	/* [BTRFS_MAX_EXTENT_SIZE] */
+	BTRFS_I(inode)->outstanding_extents++;
+	ret = btrfs_set_extent_delalloc(inode, 0, BTRFS_MAX_EXTENT_SIZE - 1,
+					NULL);
+	if (ret) {
+		test_msg("btrfs_set_extent_delalloc returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 1) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 1, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/* [BTRFS_MAX_EXTENT_SIZE][4k] */
+	BTRFS_I(inode)->outstanding_extents++;
+	ret = btrfs_set_extent_delalloc(inode, BTRFS_MAX_EXTENT_SIZE,
+					BTRFS_MAX_EXTENT_SIZE + 4095, NULL);
+	if (ret) {
+		test_msg("btrfs_set_extent_delalloc returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 2) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 2, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/* [BTRFS_MAX_EXTENT_SIZE/2][4K HOLE][the rest] */
+	ret = clear_extent_bit(&BTRFS_I(inode)->io_tree,
+			       BTRFS_MAX_EXTENT_SIZE >> 1,
+			       (BTRFS_MAX_EXTENT_SIZE >> 1) + 4095,
+			       EXTENT_DELALLOC | EXTENT_DIRTY |
+			       EXTENT_UPTODATE | EXTENT_DO_ACCOUNTING, 0, 0,
+			       NULL, GFP_NOFS);
+	if (ret) {
+		test_msg("clear_extent_bit returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 2) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 2, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/* [BTRFS_MAX_EXTENT_SIZE][4K] */
+	BTRFS_I(inode)->outstanding_extents++;
+	ret = btrfs_set_extent_delalloc(inode, BTRFS_MAX_EXTENT_SIZE >> 1,
+					(BTRFS_MAX_EXTENT_SIZE >> 1) + 4095,
+					NULL);
+	if (ret) {
+		test_msg("btrfs_set_extent_delalloc returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 2) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 2, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/*
+	 * [BTRFS_MAX_EXTENT_SIZE+4K][4K HOLE][BTRFS_MAX_EXTENT_SIZE+4K]
+	 *
+	 * I'm artificially adding 2 to outstanding_extents because in the
+	 * buffered IO case we'd add things up as we go, but I don't feel like
+	 * doing that here, this isn't the interesting case we want to test.
+	 */
+	BTRFS_I(inode)->outstanding_extents += 2;
+	ret = btrfs_set_extent_delalloc(inode, BTRFS_MAX_EXTENT_SIZE + 8192,
+					(BTRFS_MAX_EXTENT_SIZE << 1) + 12287,
+					NULL);
+	if (ret) {
+		test_msg("btrfs_set_extent_delalloc returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 4) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 4, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/* [BTRFS_MAX_EXTENT_SIZE+4k][4k][BTRFS_MAX_EXTENT_SIZE+4k] */
+	BTRFS_I(inode)->outstanding_extents++;
+	ret = btrfs_set_extent_delalloc(inode, BTRFS_MAX_EXTENT_SIZE+4096,
+					BTRFS_MAX_EXTENT_SIZE+8191, NULL);
+	if (ret) {
+		test_msg("btrfs_set_extent_delalloc returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 3) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 3, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/* [BTRFS_MAX_EXTENT_SIZE+4k][4K HOLE][BTRFS_MAX_EXTENT_SIZE+4k] */
+	ret = clear_extent_bit(&BTRFS_I(inode)->io_tree,
+			       BTRFS_MAX_EXTENT_SIZE+4096,
+			       BTRFS_MAX_EXTENT_SIZE+8191,
+			       EXTENT_DIRTY | EXTENT_DELALLOC |
+			       EXTENT_DO_ACCOUNTING | EXTENT_UPTODATE, 0, 0,
+			       NULL, GFP_NOFS);
+	if (ret) {
+		test_msg("clear_extent_bit returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 4) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 4, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/*
+	 * Refill the hole again just for good measure, because I thought it
+	 * might fail and I'd rather satisfy my paranoia at this point.
+	 */
+	BTRFS_I(inode)->outstanding_extents++;
+	ret = btrfs_set_extent_delalloc(inode, BTRFS_MAX_EXTENT_SIZE+4096,
+					BTRFS_MAX_EXTENT_SIZE+8191, NULL);
+	if (ret) {
+		test_msg("btrfs_set_extent_delalloc returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents != 3) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 3, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+
+	/* Empty */
+	ret = clear_extent_bit(&BTRFS_I(inode)->io_tree, 0, (u64)-1,
+			       EXTENT_DIRTY | EXTENT_DELALLOC |
+			       EXTENT_DO_ACCOUNTING | EXTENT_UPTODATE, 0, 0,
+			       NULL, GFP_NOFS);
+	if (ret) {
+		test_msg("clear_extent_bit returned %d\n", ret);
+		goto out;
+	}
+	if (BTRFS_I(inode)->outstanding_extents) {
+		ret = -EINVAL;
+		test_msg("Miscount, wanted 0, got %u\n",
+			 BTRFS_I(inode)->outstanding_extents);
+		goto out;
+	}
+	ret = 0;
+out:
+	if (ret)
+		clear_extent_bit(&BTRFS_I(inode)->io_tree, 0, (u64)-1,
+				 EXTENT_DIRTY | EXTENT_DELALLOC |
+				 EXTENT_DO_ACCOUNTING | EXTENT_UPTODATE, 0, 0,
+				 NULL, GFP_NOFS);
+	iput(inode);
+	btrfs_free_dummy_root(root);
+	return ret;
+}
+
 int btrfs_test_inodes(void)
 {
 	int ret;
@@ -924,5 +1115,9 @@ int btrfs_test_inodes(void)
 	if (ret)
 		return ret;
 	test_msg("Running hole first btrfs_get_extent test\n");
-	return test_hole_first();
+	ret = test_hole_first();
+	if (ret)
+		return ret;
+	test_msg("Running outstanding_extents tests\n");
+	return test_extent_accounting();
 }
-- 
1.8.3.1

[PATCH 4/4] Btrfs: fix outstanding_extents accounting in DIO

[Josef Bacik]

We are keeping track of how many extents we need to reserve properly based on
the amount we want to write, but we were still incrementing outstanding_extents
if we wrote less than what we requested.  This isn't quite right since we will
be limited to our max extent size.  So instead lets do something horrible!  Keep
track of how many outstanding_extents we reserved, and decrement each time we
allocate an extent.  If we use our entire reserve make sure to jack up
outstanding_extents on the inode so the accounting works out properly.  Thanks,

Reported-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/inode.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3717b3d..aa1fb53 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7239,7 +7239,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 	u64 start = iblock << inode->i_blkbits;
 	u64 lockstart, lockend;
 	u64 len = bh_result->b_size;
-	u64 orig_len = len;
+	u64 *outstanding_extents = NULL;
 	int unlock_bits = EXTENT_LOCKED;
 	int ret = 0;
 
@@ -7251,6 +7251,16 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 	lockstart = start;
 	lockend = start + len - 1;
 
+	if (current->journal_info) {
+		/*
+		 * Need to pull our outstanding extents and set journal_info to NULL so
+		 * that anything that needs to check if there's a transction doesn't get
+		 * confused.
+		 */
+		outstanding_extents = current->journal_info;
+		current->journal_info = NULL;
+	}
+
 	/*
 	 * If this errors out it's because we couldn't invalidate pagecache for
 	 * this range and we need to fallback to buffered.
@@ -7374,11 +7384,20 @@ unlock:
 		if (start + len > i_size_read(inode))
 			i_size_write(inode, start + len);
 
-		if (len < orig_len) {
+		/*
+		 * If we have an outstanding_extents count still set then we're
+		 * within our reservation, otherwise we need to adjust our inode
+		 * counter appropriately.
+		 */
+		if (*outstanding_extents) {
+			(*outstanding_extents)--;
+		} else {
 			spin_lock(&BTRFS_I(inode)->lock);
 			BTRFS_I(inode)->outstanding_extents++;
 			spin_unlock(&BTRFS_I(inode)->lock);
 		}
+
+		current->journal_info = outstanding_extents;
 		btrfs_free_reserved_data_space(inode, len);
 	}
 
@@ -7402,6 +7421,8 @@ unlock:
 unlock_err:
 	clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, lockend,
 			 unlock_bits, 1, 0, &cached_state, GFP_NOFS);
+	if (outstanding_extents)
+		current->journal_info = outstanding_extents;
 	return ret;
 }
 
@@ -8101,6 +8122,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
+	u64 outstanding_extents = 0;
 	size_t count = 0;
 	int flags = 0;
 	bool wakeup = true;
@@ -8138,6 +8160,16 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
 		ret = btrfs_delalloc_reserve_space(inode, count);
 		if (ret)
 			goto out;
+		outstanding_extents = div64_u64(count +
+						BTRFS_MAX_EXTENT_SIZE - 1,
+						BTRFS_MAX_EXTENT_SIZE);
+
+		/*
+		 * We need to know how many extents we reserved so that we can
+		 * do the accounting properly if we go over the number we
+		 * originally calculated.  Abuse current->journal_info for this.
+		 */
+		current->journal_info = &outstanding_extents;
 	} else if (test_bit(BTRFS_INODE_READDIO_NEED_LOCK,
 				     &BTRFS_I(inode)->runtime_flags)) {
 		inode_dio_done(inode);
@@ -8150,6 +8182,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
 			iter, offset, btrfs_get_blocks_direct, NULL,
 			btrfs_submit_direct, flags);
 	if (rw & WRITE) {
+		current->journal_info = NULL;
 		if (ret < 0 && ret != -EIOCBQUEUED)
 			btrfs_delalloc_release_space(inode, count);
 		else if (ret >= 0 && (size_t)ret < count)
-- 
1.8.3.1

Re: [PATCH 4/4] Btrfs: fix outstanding_extents accounting in DIO

[Filipe David Manana]

On Tue, Mar 17, 2015 at 8:38 PM, Josef Bacik <jbacik@fb.com> wrote:
> We are keeping track of how many extents we need to reserve properly based on
> the amount we want to write, but we were still incrementing outstanding_extents
> if we wrote less than what we requested.  This isn't quite right since we will
> be limited to our max extent size.  So instead lets do something horrible!  Keep
> track of how many outstanding_extents we reserved, and decrement each time we
> allocate an extent.  If we use our entire reserve make sure to jack up
> outstanding_extents on the inode so the accounting works out properly.  Thanks,
>
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
Tested-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>  fs/btrfs/inode.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3717b3d..aa1fb53 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7239,7 +7239,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>         u64 start = iblock << inode->i_blkbits;
>         u64 lockstart, lockend;
>         u64 len = bh_result->b_size;
> -       u64 orig_len = len;
> +       u64 *outstanding_extents = NULL;
>         int unlock_bits = EXTENT_LOCKED;
>         int ret = 0;
>
> @@ -7251,6 +7251,16 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>         lockstart = start;
>         lockend = start + len - 1;
>
> +       if (current->journal_info) {
> +               /*
> +                * Need to pull our outstanding extents and set journal_info to NULL so
> +                * that anything that needs to check if there's a transction doesn't get
> +                * confused.
> +                */
> +               outstanding_extents = current->journal_info;
> +               current->journal_info = NULL;
> +       }
> +
>         /*
>          * If this errors out it's because we couldn't invalidate pagecache for
>          * this range and we need to fallback to buffered.
> @@ -7374,11 +7384,20 @@ unlock:
>                 if (start + len > i_size_read(inode))
>                         i_size_write(inode, start + len);
>
> -               if (len < orig_len) {
> +               /*
> +                * If we have an outstanding_extents count still set then we're
> +                * within our reservation, otherwise we need to adjust our inode
> +                * counter appropriately.
> +                */
> +               if (*outstanding_extents) {
> +                       (*outstanding_extents)--;
> +               } else {
>                         spin_lock(&BTRFS_I(inode)->lock);
>                         BTRFS_I(inode)->outstanding_extents++;
>                         spin_unlock(&BTRFS_I(inode)->lock);
>                 }
> +
> +               current->journal_info = outstanding_extents;
>                 btrfs_free_reserved_data_space(inode, len);
>         }
>
> @@ -7402,6 +7421,8 @@ unlock:
>  unlock_err:
>         clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, lockend,
>                          unlock_bits, 1, 0, &cached_state, GFP_NOFS);
> +       if (outstanding_extents)
> +               current->journal_info = outstanding_extents;
>         return ret;
>  }
>
> @@ -8101,6 +8122,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
>  {
>         struct file *file = iocb->ki_filp;
>         struct inode *inode = file->f_mapping->host;
> +       u64 outstanding_extents = 0;
>         size_t count = 0;
>         int flags = 0;
>         bool wakeup = true;
> @@ -8138,6 +8160,16 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
>                 ret = btrfs_delalloc_reserve_space(inode, count);
>                 if (ret)
>                         goto out;
> +               outstanding_extents = div64_u64(count +
> +                                               BTRFS_MAX_EXTENT_SIZE - 1,
> +                                               BTRFS_MAX_EXTENT_SIZE);
> +
> +               /*
> +                * We need to know how many extents we reserved so that we can
> +                * do the accounting properly if we go over the number we
> +                * originally calculated.  Abuse current->journal_info for this.
> +                */
> +               current->journal_info = &outstanding_extents;
>         } else if (test_bit(BTRFS_INODE_READDIO_NEED_LOCK,
>                                      &BTRFS_I(inode)->runtime_flags)) {
>                 inode_dio_done(inode);
> @@ -8150,6 +8182,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
>                         iter, offset, btrfs_get_blocks_direct, NULL,
>                         btrfs_submit_direct, flags);
>         if (rw & WRITE) {
> +               current->journal_info = NULL;
>                 if (ret < 0 && ret != -EIOCBQUEUED)
>                         btrfs_delalloc_release_space(inode, count);
>                 else if (ret >= 0 && (size_t)ret < count)
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 0/4] Accounting fixes

[Holger Hoffstätte]

On Tue, 17 Mar 2015 16:37:59 -0400, Josef Bacik wrote:

> I've changed all of these a whole bunch recently, I think I've gotten all the
> issues worked out, please review/test to make sure there's not some other
> fucking corner I've missed.  Thanks,

So at least for me these seem to fix the problems that happened since the
"account for large extents" patch. Large fallocate/rm cycles no longer trip
over each other with the block group collector, and balance no longer goes
into la-la land waiting for extent bit changes that never show up.

Thanks!

-h

Re: [PATCH 0/4] Accounting fixes

[David Sterba]

On Tue, Mar 17, 2015 at 04:37:59PM -0400, Josef Bacik wrote:
> I've changed all of these a whole bunch recently, I think I've gotten all the
> issues worked out, please review/test to make sure there's not some other
> fucking corner I've missed.  Thanks,

The dirty_bgs assert is gone and fstests have been cycling since
yesterday. Thanks.