[PATCH 0/5] Improve setup_items_for_insert

[PATCH 0/5] Improve setup_items_for_insert

[Nikolay Borisov]

here is a series to improve setup_items_for_insert. First patch is a simple
re-arranegement of statements to eliminate a duplication of calculation.
Patches 2 and 3 improve leaky interface of setup_items_for_insert as they
convey information about the function's implementation. Patch 4 adds a proper
kernel doc. Finally, patch 5 improves the error message in an exceptional
condition. As an added bonus after applying the whole series bloat-o-meter
output looks like:

add/remove: 0/0 grow/shrink: 1/7 up/down: 33/-99 (-66)
Function                                     old     new   delta
setup_items_for_insert                      1200    1233     +33
insert_extent                                448     445      -3
btrfs_duplicate_item                         260     254      -6
test_btrfs_split_item                       1784    1776      -8
insert_inode_item_key                        156     148      -8
__btrfs_drop_extents                        3637    3621     -16
btrfs_insert_delayed_items                  1153    1125     -28
btrfs_insert_empty_items                     177     147     -30
Total: Before=1113157, After=1113091, chg -0.01%

This has survived -g quick of xfstests

Nikolay Borisov (5):
  btrfs: Re-arrange statements in setup_items_for_insert
  btrfs: Eliminate total_size parameter from setup_items_for_insert
  btrfs: Sink total_data parameter in setup_items_for_insert
  btrfs: Add kerneldoc for setup_items_for_insert
  btrfs: improve error message in setup_items_for_insert

 fs/btrfs/ctree.c                     | 35 +++++++++++++++++-----------
 fs/btrfs/ctree.h                     |  2 +-
 fs/btrfs/delayed-inode.c             |  3 +--
 fs/btrfs/file.c                      |  6 +----
 fs/btrfs/tests/extent-buffer-tests.c |  3 +--
 fs/btrfs/tests/inode-tests.c         |  6 ++---
 6 files changed, 28 insertions(+), 27 deletions(-)

--
2.17.1

[PATCH 1/5] btrfs: Re-arrange statements in setup_items_for_insert

[Nikolay Borisov]

Rearrange statements calculating the offset of the newly added items so
that the calculation has to be done only once. No functional change.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 31e356240fce..0f325aaa5c1c 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4823,8 +4823,8 @@ void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 		btrfs_cpu_key_to_disk(&disk_key, cpu_key + i);
 		btrfs_set_item_key(leaf, &disk_key, slot + i);
 		item = btrfs_item_nr(slot + i);
-		btrfs_set_token_item_offset(&token, item, data_end - data_size[i]);
 		data_end -= data_size[i];
+		btrfs_set_token_item_offset(&token, item, data_end);
 		btrfs_set_token_item_size(&token, item, data_size[i]);
 	}
 
-- 
2.17.1

[PATCH 2/5] btrfs: Eliminate total_size parameter from setup_items_for_insert

[Nikolay Borisov]

The value of this argument can be derived from the total_data as it's
simply the value of the data size + size of btrfs_items being touched.
Move the parameter calculation inside the function. This results in a
simpler interface and also a minor size reduction:

./scripts/bloat-o-meter ctree.original fs/btrfs/ctree.o
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-34 (-34)
Function                                     old     new   delta
btrfs_duplicate_item                         260     259      -1
setup_items_for_insert                      1200    1190     -10
btrfs_insert_empty_items                     177     154     -23

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c                     | 11 +++++------
 fs/btrfs/ctree.h                     |  2 +-
 fs/btrfs/delayed-inode.c             |  4 ++--
 fs/btrfs/file.c                      |  5 +----
 fs/btrfs/tests/extent-buffer-tests.c |  3 +--
 fs/btrfs/tests/inode-tests.c         |  6 ++----
 6 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 0f325aaa5c1c..f14f728c4a5d 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4571,9 +4571,7 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
 		return ret;
 
 	path->slots[0]++;
-	setup_items_for_insert(root, path, new_key, &item_size,
-			       item_size, item_size +
-			       sizeof(struct btrfs_item), 1);
+	setup_items_for_insert(root, path, new_key, &item_size, item_size, 1);
 	leaf = path->nodes[0];
 	memcpy_extent_buffer(leaf,
 			     btrfs_item_ptr_offset(leaf, path->slots[0]),
@@ -4753,7 +4751,7 @@ void btrfs_extend_item(struct btrfs_path *path, u32 data_size)
  */
 void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 			    const struct btrfs_key *cpu_key, u32 *data_size,
-			    u32 total_data, u32 total_size, int nr)
+			    u32 total_data, int nr)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_item *item;
@@ -4764,6 +4762,8 @@ void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 	struct extent_buffer *leaf;
 	int slot;
 	struct btrfs_map_token token;
+	const u32 total_size = total_data + (nr * sizeof(struct btrfs_item));
+
 
 	if (path->slots[0] == 0) {
 		btrfs_cpu_key_to_disk(&disk_key, cpu_key);
@@ -4866,8 +4866,7 @@ int btrfs_insert_empty_items(struct btrfs_trans_handle *trans,
 	slot = path->slots[0];
 	BUG_ON(slot < 0);
 
-	setup_items_for_insert(root, path, cpu_key, data_size,
-			       total_data, total_size, nr);
+	setup_items_for_insert(root, path, cpu_key, data_size, total_data, nr);
 	return 0;
 }
 
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index af2bd059daae..ac53535412a6 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2713,7 +2713,7 @@ static inline int btrfs_del_item(struct btrfs_trans_handle *trans,
 
 void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 			    const struct btrfs_key *cpu_key, u32 *data_size,
-			    u32 total_data, u32 total_size, int nr);
+			    u32 total_data, int nr);
 int btrfs_insert_item(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		      const struct btrfs_key *key, void *data, u32 data_size);
 int btrfs_insert_empty_items(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 0727b10a9a89..0360e07d1b0f 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -768,8 +768,8 @@ static int btrfs_batch_insert_items(struct btrfs_root *root,
 	}
 
 	/* insert the keys of the items */
-	setup_items_for_insert(root, path, keys, data_size,
-			       total_data_size, total_size, nitems);
+	setup_items_for_insert(root, path, keys, data_size, total_data_size,
+			       nitems);
 
 	/* insert the dir index items */
 	slot = path->slots[0];
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 2cacb4424cd4..82f9bb78b86d 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1057,10 +1057,7 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,
 			if (btrfs_comp_cpu_keys(&key, &slot_key) > 0)
 				path->slots[0]++;
 		}
-		setup_items_for_insert(root, path, &key,
-				       &extent_item_size,
-				       extent_item_size,
-				       sizeof(struct btrfs_item) +
+		setup_items_for_insert(root, path, &key, &extent_item_size,
 				       extent_item_size, 1);
 		*key_inserted = 1;
 	}
diff --git a/fs/btrfs/tests/extent-buffer-tests.c b/fs/btrfs/tests/extent-buffer-tests.c
index a1b9f9b5978e..a792a1dfd6dd 100644
--- a/fs/btrfs/tests/extent-buffer-tests.c
+++ b/fs/btrfs/tests/extent-buffer-tests.c
@@ -60,8 +60,7 @@ static int test_btrfs_split_item(u32 sectorsize, u32 nodesize)
 	key.type = BTRFS_EXTENT_CSUM_KEY;
 	key.offset = 0;
 
-	setup_items_for_insert(root, path, &key, &value_len, value_len,
-			       value_len + sizeof(struct btrfs_item), 1);
+	setup_items_for_insert(root, path, &key, &value_len, value_len, 1);
 	item = btrfs_item_nr(0);
 	write_extent_buffer(eb, value, btrfs_item_ptr_offset(eb, 0),
 			    value_len);
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index 894a63a92236..068b6ef02cec 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -33,8 +33,7 @@ static void insert_extent(struct btrfs_root *root, u64 start, u64 len,
 	key.type = BTRFS_EXTENT_DATA_KEY;
 	key.offset = start;
 
-	setup_items_for_insert(root, &path, &key, &value_len, value_len,
-			       value_len + sizeof(struct btrfs_item), 1);
+	setup_items_for_insert(root, &path, &key, &value_len, value_len, 1);
 	fi = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item);
 	btrfs_set_file_extent_generation(leaf, fi, 1);
 	btrfs_set_file_extent_type(leaf, fi, type);
@@ -64,8 +63,7 @@ static void insert_inode_item_key(struct btrfs_root *root)
 	key.type = BTRFS_INODE_ITEM_KEY;
 	key.offset = 0;
 
-	setup_items_for_insert(root, &path, &key, &value_len, value_len,
-			       value_len + sizeof(struct btrfs_item), 1);
+	setup_items_for_insert(root, &path, &key, &value_len, value_len, 1);
 }
 
 /*
-- 
2.17.1

[PATCH 3/5] btrfs: Sink total_data parameter in setup_items_for_insert

[Nikolay Borisov]

That parameter can easily be derived based on the "data_size" and "nr"
parameters exploit this fact to simply the function's signature. No
functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c                     | 12 ++++++++----
 fs/btrfs/ctree.h                     |  2 +-
 fs/btrfs/delayed-inode.c             |  3 +--
 fs/btrfs/file.c                      |  3 +--
 fs/btrfs/tests/extent-buffer-tests.c |  2 +-
 fs/btrfs/tests/inode-tests.c         |  4 ++--
 6 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index f14f728c4a5d..26e653e92956 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4571,7 +4571,7 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
 		return ret;
 
 	path->slots[0]++;
-	setup_items_for_insert(root, path, new_key, &item_size, item_size, 1);
+	setup_items_for_insert(root, path, new_key, &item_size, 1);
 	leaf = path->nodes[0];
 	memcpy_extent_buffer(leaf,
 			     btrfs_item_ptr_offset(leaf, path->slots[0]),
@@ -4751,7 +4751,7 @@ void btrfs_extend_item(struct btrfs_path *path, u32 data_size)
  */
 void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 			    const struct btrfs_key *cpu_key, u32 *data_size,
-			    u32 total_data, int nr)
+			    int nr)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_item *item;
@@ -4762,8 +4762,12 @@ void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 	struct extent_buffer *leaf;
 	int slot;
 	struct btrfs_map_token token;
-	const u32 total_size = total_data + (nr * sizeof(struct btrfs_item));
+	u32 total_size;
+	u32 total_data = 0;
 
+	for (i = 0; i < nr; i++)
+		total_data += data_size[i];
+	total_size = total_data + (nr * sizeof(struct btrfs_item));
 
 	if (path->slots[0] == 0) {
 		btrfs_cpu_key_to_disk(&disk_key, cpu_key);
@@ -4866,7 +4870,7 @@ int btrfs_insert_empty_items(struct btrfs_trans_handle *trans,
 	slot = path->slots[0];
 	BUG_ON(slot < 0);
 
-	setup_items_for_insert(root, path, cpu_key, data_size, total_data, nr);
+	setup_items_for_insert(root, path, cpu_key, data_size, nr);
 	return 0;
 }
 
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index ac53535412a6..af5ef4dfb42c 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2713,7 +2713,7 @@ static inline int btrfs_del_item(struct btrfs_trans_handle *trans,
 
 void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 			    const struct btrfs_key *cpu_key, u32 *data_size,
-			    u32 total_data, int nr);
+			    int nr);
 int btrfs_insert_item(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		      const struct btrfs_key *key, void *data, u32 data_size);
 int btrfs_insert_empty_items(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 0360e07d1b0f..5aba81e16113 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -768,8 +768,7 @@ static int btrfs_batch_insert_items(struct btrfs_root *root,
 	}
 
 	/* insert the keys of the items */
-	setup_items_for_insert(root, path, keys, data_size, total_data_size,
-			       nitems);
+	setup_items_for_insert(root, path, keys, data_size, nitems);
 
 	/* insert the dir index items */
 	slot = path->slots[0];
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 82f9bb78b86d..e8deb5627223 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1057,8 +1057,7 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,
 			if (btrfs_comp_cpu_keys(&key, &slot_key) > 0)
 				path->slots[0]++;
 		}
-		setup_items_for_insert(root, path, &key, &extent_item_size,
-				       extent_item_size, 1);
+		setup_items_for_insert(root, path, &key, &extent_item_size, 1);
 		*key_inserted = 1;
 	}
 
diff --git a/fs/btrfs/tests/extent-buffer-tests.c b/fs/btrfs/tests/extent-buffer-tests.c
index a792a1dfd6dd..df54cdfdc250 100644
--- a/fs/btrfs/tests/extent-buffer-tests.c
+++ b/fs/btrfs/tests/extent-buffer-tests.c
@@ -60,7 +60,7 @@ static int test_btrfs_split_item(u32 sectorsize, u32 nodesize)
 	key.type = BTRFS_EXTENT_CSUM_KEY;
 	key.offset = 0;
 
-	setup_items_for_insert(root, path, &key, &value_len, value_len, 1);
+	setup_items_for_insert(root, path, &key, &value_len, 1);
 	item = btrfs_item_nr(0);
 	write_extent_buffer(eb, value, btrfs_item_ptr_offset(eb, 0),
 			    value_len);
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index 068b6ef02cec..cc54d4973a74 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -33,7 +33,7 @@ static void insert_extent(struct btrfs_root *root, u64 start, u64 len,
 	key.type = BTRFS_EXTENT_DATA_KEY;
 	key.offset = start;
 
-	setup_items_for_insert(root, &path, &key, &value_len, value_len, 1);
+	setup_items_for_insert(root, &path, &key, &value_len, 1);
 	fi = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item);
 	btrfs_set_file_extent_generation(leaf, fi, 1);
 	btrfs_set_file_extent_type(leaf, fi, type);
@@ -63,7 +63,7 @@ static void insert_inode_item_key(struct btrfs_root *root)
 	key.type = BTRFS_INODE_ITEM_KEY;
 	key.offset = 0;
 
-	setup_items_for_insert(root, &path, &key, &value_len, value_len, 1);
+	setup_items_for_insert(root, &path, &key, &value_len, 1);
 }
 
 /*
-- 
2.17.1

[PATCH 4/5] btrfs: Add kerneldoc for setup_items_for_insert

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 26e653e92956..bb89c0954ca1 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4744,10 +4744,16 @@ void btrfs_extend_item(struct btrfs_path *path, u32 data_size)
 	}
 }
 
-/*
- * this is a helper for btrfs_insert_empty_items, the main goal here is
- * to save stack depth by doing the bulk of the work in a function
- * that doesn't call btrfs_search_slot
+/**
+ * setup_items_for_insert - Helper called before inserting one or more items
+ * in a leaf. Main purpose is to save stack depth by doing the bulk of the work
+ * in a function that doesn't call btrfs_search_slot
+ *
+ * @root:	root we are inserting items in
+ * @path:	points to the leaf/slot where we are going to insert new items
+ * @cpu_key:	array of keys for items to be inserted
+ * @data_size:	size of the body of each item we are going to insert
+ * @nr:		size of @cpu_key/@data_size arrays
  */
 void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 			    const struct btrfs_key *cpu_key, u32 *data_size,
-- 
2.17.1

[PATCH 5/5] btrfs: improve error message in setup_items_for_insert

[Nikolay Borisov]

While at it also print the leaf content after the error.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index bb89c0954ca1..2c3f78cc6aa2 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4799,9 +4799,9 @@ void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
 		unsigned int old_data = btrfs_item_end_nr(leaf, slot);
 
 		if (old_data < data_end) {
-			btrfs_print_leaf(leaf);
-			btrfs_crit(fs_info, "slot %d old_data %d data_end %d",
+			btrfs_crit(fs_info, "item's (slot %d) data offset (%d) beyond data end of leaf (%d)",
 				   slot, old_data, data_end);
+			btrfs_print_leaf(leaf);
 			BUG();
 		}
 		/*
-- 
2.17.1

Re: [PATCH 1/5] btrfs: Re-arrange statements in setup_items_for_insert

[Josef Bacik]

On 9/1/20 10:39 AM, Nikolay Borisov wrote:
> Rearrange statements calculating the offset of the newly added items so
> that the calculation has to be done only once. No functional change.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 2/5] btrfs: Eliminate total_size parameter from setup_items_for_insert

[Josef Bacik]

On 9/1/20 10:39 AM, Nikolay Borisov wrote:
> The value of this argument can be derived from the total_data as it's
> simply the value of the data size + size of btrfs_items being touched.
> Move the parameter calculation inside the function. This results in a
> simpler interface and also a minor size reduction:
> 
> ./scripts/bloat-o-meter ctree.original fs/btrfs/ctree.o
> add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-34 (-34)
> Function                                     old     new   delta
> btrfs_duplicate_item                         260     259      -1
> setup_items_for_insert                      1200    1190     -10
> btrfs_insert_empty_items                     177     154     -23
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Some of these things, like btrfs_batch_insert_items, we could probably clean up 
and remove total_size later.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 3/5] btrfs: Sink total_data parameter in setup_items_for_insert

[Josef Bacik]

On 9/1/20 10:39 AM, Nikolay Borisov wrote:
> That parameter can easily be derived based on the "data_size" and "nr"
> parameters exploit this fact to simply the function's signature. No
> functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 4/5] btrfs: Add kerneldoc for setup_items_for_insert

[Josef Bacik]

On 9/1/20 10:40 AM, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 5/5] btrfs: improve error message in setup_items_for_insert

[Josef Bacik]

On 9/1/20 10:40 AM, Nikolay Borisov wrote:
> While at it also print the leaf content after the error.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 0/5] Improve setup_items_for_insert

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 5/5] btrfs: improve error message in setup_items_for_insert

[David Sterba]

On Tue, Sep 01, 2020 at 05:40:01PM +0300, Nikolay Borisov wrote:
> While at it also print the leaf content after the error.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index bb89c0954ca1..2c3f78cc6aa2 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -4799,9 +4799,9 @@ void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
>  		unsigned int old_data = btrfs_item_end_nr(leaf, slot);
>  
>  		if (old_data < data_end) {
> -			btrfs_print_leaf(leaf);
> -			btrfs_crit(fs_info, "slot %d old_data %d data_end %d",
> +			btrfs_crit(fs_info, "item's (slot %d) data offset (%d) beyond data end of leaf (%d)",
>  				   slot, old_data, data_end);
> +			btrfs_print_leaf(leaf);

This is a question if the order is right or vice versa. When something
goes wrong, the leaf dump fills a few pages or screens and it's not
always possible to scroll back.

The idea of 1) dump leaf 2) print the error message, is to keep the
specific error last hoping that it's still visible when somebody takes a
screenshot for a bugreport.

I've checked instances where we call print_leaf and this is the most
common ordering, so I'd rather keep it like that as it has some practial
impacts.

Re: [PATCH 0/5] Improve setup_items_for_insert

[David Sterba]

On Tue, Sep 01, 2020 at 05:39:56PM +0300, Nikolay Borisov wrote:
> here is a series to improve setup_items_for_insert. First patch is a simple
> re-arranegement of statements to eliminate a duplication of calculation.
> Patches 2 and 3 improve leaky interface of setup_items_for_insert as they
> convey information about the function's implementation. Patch 4 adds a proper
> kernel doc. Finally, patch 5 improves the error message in an exceptional
> condition. As an added bonus after applying the whole series bloat-o-meter
> output looks like:
> 
> add/remove: 0/0 grow/shrink: 1/7 up/down: 33/-99 (-66)
> Function                                     old     new   delta
> setup_items_for_insert                      1200    1233     +33
> insert_extent                                448     445      -3
> btrfs_duplicate_item                         260     254      -6
> test_btrfs_split_item                       1784    1776      -8
> insert_inode_item_key                        156     148      -8
> __btrfs_drop_extents                        3637    3621     -16
> btrfs_insert_delayed_items                  1153    1125     -28
> btrfs_insert_empty_items                     177     147     -30
> Total: Before=1113157, After=1113091, chg -0.01%
> 
> This has survived -g quick of xfstests
> 
> Nikolay Borisov (5):
>   btrfs: Re-arrange statements in setup_items_for_insert
>   btrfs: Eliminate total_size parameter from setup_items_for_insert
>   btrfs: Sink total_data parameter in setup_items_for_insert
>   btrfs: Add kerneldoc for setup_items_for_insert
>   btrfs: improve error message in setup_items_for_insert

Thanks, all seem straightforward, I'll add it to for-next and move to
misc-next once the tests finish.

Re: [PATCH 5/5] btrfs: improve error message in setup_items_for_insert

[David Sterba]

On Tue, Sep 01, 2020 at 05:40:01PM +0300, Nikolay Borisov wrote:
> While at it also print the leaf content after the error.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index bb89c0954ca1..2c3f78cc6aa2 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -4799,9 +4799,9 @@ void setup_items_for_insert(struct btrfs_root *root, struct btrfs_path *path,
>  		unsigned int old_data = btrfs_item_end_nr(leaf, slot);
>  
>  		if (old_data < data_end) {
> -			btrfs_print_leaf(leaf);
> -			btrfs_crit(fs_info, "slot %d old_data %d data_end %d",
> +			btrfs_crit(fs_info, "item's (slot %d) data offset (%d) beyond data end of leaf (%d)",
>  				   slot, old_data, data_end);

I've changed it to

"item at slot %d with data offset %u beyond data end of leaf %u"

as we don't use the ( ) elsewhere. The formats were mismatching old_data
and data_end, so they're now %u.

> +			btrfs_print_leaf(leaf);

The leaf is dumped as before, so the changelog got updated too.