[PATCH 0/4] Set/get accessor cleanups

[PATCH 0/4] Set/get accessor cleanups

[David Sterba]

This patchset is first in a series to cleanup and optimize the
accessors, now removing code that's not needed anymore, explained with
references in the last patch.

Overall effects on .ko:

   text    data     bss     dec     hex filename
1463615  115665   16088 1595368  1857e8 pre/btrfs.ko
1456601  115665   16088 1588354  183c82 post/btrfs.ko

DELTA: -7014

And stack consumption:

__push_leaf_left                                   -32 (176 -> 144)
copy_for_split                                     -32 (144 -> 112)
fill_inode_item                                    -32 (80 -> 48)
btrfs_truncate_item                                -24 (152 -> 128)
btrfs_extend_item                                  -16 (104 -> 88)
btrfs_del_items                                    -16 (144 -> 128)
setup_items_for_insert                             -32 (144 -> 112)
__push_leaf_right                                  -24 (168 -> 144)

REMOVED (744):
        btrfs_get_token_32                          88
        btrfs_set_token_64                          96
        btrfs_get_token_64                          88
        btrfs_set_token_32                          96
        btrfs_get_token_16                          88
        btrfs_get_token_8                           88
        btrfs_set_token_16                          96
        btrfs_set_token_8                           96
        btrfs_init_map_token                         8

REMOVED/NEW DELTA:  -744
PRE/POST DELTA:     -952

David Sterba (4):
  btrfs: don't use token set/get accessors for btrfs_item members
  btrfs: don't use token set/get accessors in inode.c:fill_inode_item()
  btrfs: tree-log: don't use token set/get accessors in
    fill_inode_item()
  btrfs: accessors: delete token versions of set/get helpers

 fs/btrfs/accessors.c | 78 --------------------------------------------
 fs/btrfs/accessors.h | 37 ---------------------
 fs/btrfs/ctree.c     | 51 ++++++++++-------------------
 fs/btrfs/inode.c     | 50 ++++++++++++----------------
 fs/btrfs/tree-log.c  | 48 +++++++++++----------------
 5 files changed, 57 insertions(+), 207 deletions(-)

-- 
2.49.0

[PATCH 1/4] btrfs: don't use token set/get accessors for btrfs_item members

[David Sterba]

The token versions of set/get accessors will be removed, use the normal
helpers. The btrfs_item members use that interface the most but there
are no real benefits anymore.

This reduces stack consumption on x86_64 release config:

setup_items_for_insert                           -32 (144 -> 112)
__push_leaf_left                                 -32 (176 -> 144)
btrfs_extend_item                                -16 (104 -> 88)
copy_for_split                                   -32 (144 -> 112)
btrfs_del_items                                  -16 (144 -> 128)
btrfs_truncate_item                              -24 (152 -> 128)
__push_leaf_right                                -24 (168 -> 144)

and module size:

   text    data     bss     dec     hex filename
1463615  115665   16088 1595368  1857e8 pre/btrfs.ko
1463413  115665   16088 1595166  18571e post/btrfs.ko

DELTA: -202

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 51 +++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 2997f24207195c..b8b637267d678d 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -3109,7 +3109,6 @@ static noinline int __push_leaf_right(struct btrfs_trans_handle *trans,
 	struct btrfs_fs_info *fs_info = right->fs_info;
 	struct extent_buffer *left = path->nodes[0];
 	struct extent_buffer *upper = path->nodes[1];
-	struct btrfs_map_token token;
 	struct btrfs_disk_key disk_key;
 	int slot;
 	u32 i;
@@ -3183,13 +3182,12 @@ static noinline int __push_leaf_right(struct btrfs_trans_handle *trans,
 	copy_leaf_items(right, left, 0, left_nritems - push_items, push_items);
 
 	/* update the item pointers */
-	btrfs_init_map_token(&token, right);
 	right_nritems += push_items;
 	btrfs_set_header_nritems(right, right_nritems);
 	push_space = BTRFS_LEAF_DATA_SIZE(fs_info);
 	for (i = 0; i < right_nritems; i++) {
-		push_space -= btrfs_token_item_size(&token, i);
-		btrfs_set_token_item_offset(&token, i, push_space);
+		push_space -= btrfs_item_size(right, i);
+		btrfs_set_item_offset(right, i, push_space);
 	}
 
 	left_nritems -= push_items;
@@ -3332,7 +3330,6 @@ static noinline int __push_leaf_left(struct btrfs_trans_handle *trans,
 	int ret = 0;
 	u32 this_item_size;
 	u32 old_left_item_size;
-	struct btrfs_map_token token;
 
 	if (empty)
 		nr = min(right_nritems, max_slot);
@@ -3380,13 +3377,12 @@ static noinline int __push_leaf_left(struct btrfs_trans_handle *trans,
 	old_left_nritems = btrfs_header_nritems(left);
 	BUG_ON(old_left_nritems <= 0);
 
-	btrfs_init_map_token(&token, left);
 	old_left_item_size = btrfs_item_offset(left, old_left_nritems - 1);
 	for (i = old_left_nritems; i < old_left_nritems + push_items; i++) {
 		u32 ioff;
 
-		ioff = btrfs_token_item_offset(&token, i);
-		btrfs_set_token_item_offset(&token, i,
+		ioff = btrfs_item_offset(left, i);
+		btrfs_set_item_offset(left, i,
 		      ioff - (BTRFS_LEAF_DATA_SIZE(fs_info) - old_left_item_size));
 	}
 	btrfs_set_header_nritems(left, old_left_nritems + push_items);
@@ -3407,13 +3403,12 @@ static noinline int __push_leaf_left(struct btrfs_trans_handle *trans,
 				   btrfs_header_nritems(right) - push_items);
 	}
 
-	btrfs_init_map_token(&token, right);
 	right_nritems -= push_items;
 	btrfs_set_header_nritems(right, right_nritems);
 	push_space = BTRFS_LEAF_DATA_SIZE(fs_info);
 	for (i = 0; i < right_nritems; i++) {
-		push_space = push_space - btrfs_token_item_size(&token, i);
-		btrfs_set_token_item_offset(&token, i, push_space);
+		push_space = push_space - btrfs_item_size(right, i);
+		btrfs_set_item_offset(right, i, push_space);
 	}
 
 	btrfs_mark_buffer_dirty(trans, left);
@@ -3527,7 +3522,6 @@ static noinline int copy_for_split(struct btrfs_trans_handle *trans,
 	int i;
 	int ret;
 	struct btrfs_disk_key disk_key;
-	struct btrfs_map_token token;
 
 	nritems = nritems - mid;
 	btrfs_set_header_nritems(right, nritems);
@@ -3540,12 +3534,11 @@ static noinline int copy_for_split(struct btrfs_trans_handle *trans,
 
 	rt_data_off = BTRFS_LEAF_DATA_SIZE(fs_info) - btrfs_item_data_end(l, mid);
 
-	btrfs_init_map_token(&token, right);
 	for (i = 0; i < nritems; i++) {
 		u32 ioff;
 
-		ioff = btrfs_token_item_offset(&token, i);
-		btrfs_set_token_item_offset(&token, i, ioff + rt_data_off);
+		ioff = btrfs_item_offset(right, i);
+		btrfs_set_item_offset(right, i, ioff + rt_data_off);
 	}
 
 	btrfs_set_header_nritems(l, mid);
@@ -4011,7 +4004,6 @@ void btrfs_truncate_item(struct btrfs_trans_handle *trans,
 	unsigned int old_size;
 	unsigned int size_diff;
 	int i;
-	struct btrfs_map_token token;
 
 	leaf = path->nodes[0];
 	slot = path->slots[0];
@@ -4034,12 +4026,11 @@ void btrfs_truncate_item(struct btrfs_trans_handle *trans,
 	 * item0..itemN ... dataN.offset..dataN.size .. data0.size
 	 */
 	/* first correct the data pointers */
-	btrfs_init_map_token(&token, leaf);
 	for (i = slot; i < nritems; i++) {
 		u32 ioff;
 
-		ioff = btrfs_token_item_offset(&token, i);
-		btrfs_set_token_item_offset(&token, i, ioff + size_diff);
+		ioff = btrfs_item_offset(leaf, i);
+		btrfs_set_item_offset(leaf, i, ioff + size_diff);
 	}
 
 	/* shift the data */
@@ -4102,7 +4093,6 @@ void btrfs_extend_item(struct btrfs_trans_handle *trans,
 	unsigned int old_data;
 	unsigned int old_size;
 	int i;
-	struct btrfs_map_token token;
 
 	leaf = path->nodes[0];
 
@@ -4128,12 +4118,11 @@ void btrfs_extend_item(struct btrfs_trans_handle *trans,
 	 * item0..itemN ... dataN.offset..dataN.size .. data0.size
 	 */
 	/* first correct the data pointers */
-	btrfs_init_map_token(&token, leaf);
 	for (i = slot; i < nritems; i++) {
 		u32 ioff;
 
-		ioff = btrfs_token_item_offset(&token, i);
-		btrfs_set_token_item_offset(&token, i, ioff - data_size);
+		ioff = btrfs_item_offset(leaf, i);
+		btrfs_set_item_offset(leaf, i, ioff - data_size);
 	}
 
 	/* shift the data */
@@ -4173,7 +4162,6 @@ static void setup_items_for_insert(struct btrfs_trans_handle *trans,
 	struct btrfs_disk_key disk_key;
 	struct extent_buffer *leaf;
 	int slot;
-	struct btrfs_map_token token;
 	u32 total_size;
 
 	/*
@@ -4201,7 +4189,6 @@ static void setup_items_for_insert(struct btrfs_trans_handle *trans,
 		BUG();
 	}
 
-	btrfs_init_map_token(&token, leaf);
 	if (slot != nritems) {
 		unsigned int old_data = btrfs_item_data_end(leaf, slot);
 
@@ -4219,8 +4206,8 @@ static void setup_items_for_insert(struct btrfs_trans_handle *trans,
 		for (i = slot; i < nritems; i++) {
 			u32 ioff;
 
-			ioff = btrfs_token_item_offset(&token, i);
-			btrfs_set_token_item_offset(&token, i,
+			ioff = btrfs_item_offset(leaf, i);
+			btrfs_set_item_offset(leaf, i,
 						       ioff - batch->total_data_size);
 		}
 		/* shift the items */
@@ -4237,8 +4224,8 @@ static void setup_items_for_insert(struct btrfs_trans_handle *trans,
 		btrfs_cpu_key_to_disk(&disk_key, &batch->keys[i]);
 		btrfs_set_item_key(leaf, &disk_key, slot + i);
 		data_end -= batch->data_sizes[i];
-		btrfs_set_token_item_offset(&token, slot + i, data_end);
-		btrfs_set_token_item_size(&token, slot + i, batch->data_sizes[i]);
+		btrfs_set_item_offset(leaf, slot + i, data_end);
+		btrfs_set_item_size(leaf, slot + i, batch->data_sizes[i]);
 	}
 
 	btrfs_set_header_nritems(leaf, nritems + batch->nr);
@@ -4478,7 +4465,6 @@ int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	if (slot + nr != nritems) {
 		const u32 last_off = btrfs_item_offset(leaf, slot + nr - 1);
 		const int data_end = leaf_data_end(leaf);
-		struct btrfs_map_token token;
 		u32 dsize = 0;
 		int i;
 
@@ -4488,12 +4474,11 @@ int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		memmove_leaf_data(leaf, data_end + dsize, data_end,
 				  last_off - data_end);
 
-		btrfs_init_map_token(&token, leaf);
 		for (i = slot + nr; i < nritems; i++) {
 			u32 ioff;
 
-			ioff = btrfs_token_item_offset(&token, i);
-			btrfs_set_token_item_offset(&token, i, ioff + dsize);
+			ioff = btrfs_item_offset(leaf, i);
+			btrfs_set_item_offset(leaf, i, ioff + dsize);
 		}
 
 		memmove_leaf_items(leaf, slot, slot + nr, nritems - slot - nr);
-- 
2.49.0

[PATCH 2/4] btrfs: don't use token set/get accessors in inode.c:fill_inode_item()

[David Sterba]

The token versions of set/get accessors will be removed, use the normal
helpers.

There's additional overhead of the token helpers that update the cached
address in case it moves to another page/folio. The normal versions
don't need to do that.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 50 +++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6df7ef1b869b2e..508fb69a976dda 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4078,45 +4078,35 @@ static void fill_inode_item(struct btrfs_trans_handle *trans,
 			    struct btrfs_inode_item *item,
 			    struct inode *inode)
 {
-	struct btrfs_map_token token;
 	u64 flags;
 
-	btrfs_init_map_token(&token, leaf);
+	btrfs_set_inode_uid(leaf, item, i_uid_read(inode));
+	btrfs_set_inode_gid(leaf, item, i_gid_read(inode));
+	btrfs_set_inode_size(leaf, item, BTRFS_I(inode)->disk_i_size);
+	btrfs_set_inode_mode(leaf, item, inode->i_mode);
+	btrfs_set_inode_nlink(leaf, item, inode->i_nlink);
 
-	btrfs_set_token_inode_uid(&token, item, i_uid_read(inode));
-	btrfs_set_token_inode_gid(&token, item, i_gid_read(inode));
-	btrfs_set_token_inode_size(&token, item, BTRFS_I(inode)->disk_i_size);
-	btrfs_set_token_inode_mode(&token, item, inode->i_mode);
-	btrfs_set_token_inode_nlink(&token, item, inode->i_nlink);
+	btrfs_set_timespec_sec(leaf, &item->atime, inode_get_atime_sec(inode));
+	btrfs_set_timespec_nsec(leaf, &item->atime, inode_get_atime_nsec(inode));
 
-	btrfs_set_token_timespec_sec(&token, &item->atime,
-				     inode_get_atime_sec(inode));
-	btrfs_set_token_timespec_nsec(&token, &item->atime,
-				      inode_get_atime_nsec(inode));
+	btrfs_set_timespec_sec(leaf, &item->mtime, inode_get_mtime_sec(inode));
+	btrfs_set_timespec_nsec(leaf, &item->mtime, inode_get_mtime_nsec(inode));
 
-	btrfs_set_token_timespec_sec(&token, &item->mtime,
-				     inode_get_mtime_sec(inode));
-	btrfs_set_token_timespec_nsec(&token, &item->mtime,
-				      inode_get_mtime_nsec(inode));
+	btrfs_set_timespec_sec(leaf, &item->ctime, inode_get_ctime_sec(inode));
+	btrfs_set_timespec_nsec(leaf, &item->ctime, inode_get_ctime_nsec(inode));
 
-	btrfs_set_token_timespec_sec(&token, &item->ctime,
-				     inode_get_ctime_sec(inode));
-	btrfs_set_token_timespec_nsec(&token, &item->ctime,
-				      inode_get_ctime_nsec(inode));
+	btrfs_set_timespec_sec(leaf, &item->otime, BTRFS_I(inode)->i_otime_sec);
+	btrfs_set_timespec_nsec(leaf, &item->otime, BTRFS_I(inode)->i_otime_nsec);
 
-	btrfs_set_token_timespec_sec(&token, &item->otime, BTRFS_I(inode)->i_otime_sec);
-	btrfs_set_token_timespec_nsec(&token, &item->otime, BTRFS_I(inode)->i_otime_nsec);
-
-	btrfs_set_token_inode_nbytes(&token, item, inode_get_bytes(inode));
-	btrfs_set_token_inode_generation(&token, item,
-					 BTRFS_I(inode)->generation);
-	btrfs_set_token_inode_sequence(&token, item, inode_peek_iversion(inode));
-	btrfs_set_token_inode_transid(&token, item, trans->transid);
-	btrfs_set_token_inode_rdev(&token, item, inode->i_rdev);
+	btrfs_set_inode_nbytes(leaf, item, inode_get_bytes(inode));
+	btrfs_set_inode_generation(leaf, item, BTRFS_I(inode)->generation);
+	btrfs_set_inode_sequence(leaf, item, inode_peek_iversion(inode));
+	btrfs_set_inode_transid(leaf, item, trans->transid);
+	btrfs_set_inode_rdev(leaf, item, inode->i_rdev);
 	flags = btrfs_inode_combine_flags(BTRFS_I(inode)->flags,
 					  BTRFS_I(inode)->ro_flags);
-	btrfs_set_token_inode_flags(&token, item, flags);
-	btrfs_set_token_inode_block_group(&token, item, 0);
+	btrfs_set_inode_flags(leaf, item, flags);
+	btrfs_set_inode_block_group(leaf, item, 0);
 }
 
 /*
-- 
2.49.0

[PATCH 3/4] btrfs: tree-log: don't use token set/get accessors in fill_inode_item()

[David Sterba]

The token versions of set/get accessors will be removed, use the normal
helpers.

There's additional overhead of the token helpers that update the cached
address in case it moves to another page/folio. The normal versions
don't need to do that.

Note this is similar to fill_inode_item() in inode.c but with slight
differences. The two functions could be deduplicated eventually.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/tree-log.c | 48 ++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 97ed11788b470f..7e52d8f92e5bad 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4204,44 +4204,34 @@ static void fill_inode_item(struct btrfs_trans_handle *trans,
 			    struct inode *inode, int log_inode_only,
 			    u64 logged_isize)
 {
-	struct btrfs_map_token token;
 	u64 flags;
 
-	btrfs_init_map_token(&token, leaf);
-
 	if (log_inode_only) {
 		/* set the generation to zero so the recover code
 		 * can tell the difference between an logging
 		 * just to say 'this inode exists' and a logging
 		 * to say 'update this inode with these values'
 		 */
-		btrfs_set_token_inode_generation(&token, item, 0);
-		btrfs_set_token_inode_size(&token, item, logged_isize);
+		btrfs_set_inode_generation(leaf, item, 0);
+		btrfs_set_inode_size(leaf, item, logged_isize);
 	} else {
-		btrfs_set_token_inode_generation(&token, item,
-						 BTRFS_I(inode)->generation);
-		btrfs_set_token_inode_size(&token, item, inode->i_size);
+		btrfs_set_inode_generation(leaf, item, BTRFS_I(inode)->generation);
+		btrfs_set_inode_size(leaf, item, inode->i_size);
 	}
 
-	btrfs_set_token_inode_uid(&token, item, i_uid_read(inode));
-	btrfs_set_token_inode_gid(&token, item, i_gid_read(inode));
-	btrfs_set_token_inode_mode(&token, item, inode->i_mode);
-	btrfs_set_token_inode_nlink(&token, item, inode->i_nlink);
+	btrfs_set_inode_uid(leaf, item, i_uid_read(inode));
+	btrfs_set_inode_gid(leaf, item, i_gid_read(inode));
+	btrfs_set_inode_mode(leaf, item, inode->i_mode);
+	btrfs_set_inode_nlink(leaf, item, inode->i_nlink);
 
-	btrfs_set_token_timespec_sec(&token, &item->atime,
-				     inode_get_atime_sec(inode));
-	btrfs_set_token_timespec_nsec(&token, &item->atime,
-				      inode_get_atime_nsec(inode));
+	btrfs_set_timespec_sec(leaf, &item->atime, inode_get_atime_sec(inode));
+	btrfs_set_timespec_nsec(leaf, &item->atime, inode_get_atime_nsec(inode));
 
-	btrfs_set_token_timespec_sec(&token, &item->mtime,
-				     inode_get_mtime_sec(inode));
-	btrfs_set_token_timespec_nsec(&token, &item->mtime,
-				      inode_get_mtime_nsec(inode));
+	btrfs_set_timespec_sec(leaf, &item->mtime, inode_get_mtime_sec(inode));
+	btrfs_set_timespec_nsec(leaf, &item->mtime, inode_get_mtime_nsec(inode));
 
-	btrfs_set_token_timespec_sec(&token, &item->ctime,
-				     inode_get_ctime_sec(inode));
-	btrfs_set_token_timespec_nsec(&token, &item->ctime,
-				      inode_get_ctime_nsec(inode));
+	btrfs_set_timespec_sec(leaf, &item->ctime, inode_get_ctime_sec(inode));
+	btrfs_set_timespec_nsec(leaf, &item->ctime, inode_get_ctime_nsec(inode));
 
 	/*
 	 * We do not need to set the nbytes field, in fact during a fast fsync
@@ -4252,13 +4242,13 @@ static void fill_inode_item(struct btrfs_trans_handle *trans,
 	 * inode item in subvolume tree as needed (see overwrite_item()).
 	 */
 
-	btrfs_set_token_inode_sequence(&token, item, inode_peek_iversion(inode));
-	btrfs_set_token_inode_transid(&token, item, trans->transid);
-	btrfs_set_token_inode_rdev(&token, item, inode->i_rdev);
+	btrfs_set_inode_sequence(leaf, item, inode_peek_iversion(inode));
+	btrfs_set_inode_transid(leaf, item, trans->transid);
+	btrfs_set_inode_rdev(leaf, item, inode->i_rdev);
 	flags = btrfs_inode_combine_flags(BTRFS_I(inode)->flags,
 					  BTRFS_I(inode)->ro_flags);
-	btrfs_set_token_inode_flags(&token, item, flags);
-	btrfs_set_token_inode_block_group(&token, item, 0);
+	btrfs_set_inode_flags(leaf, item, flags);
+	btrfs_set_inode_block_group(leaf, item, 0);
 }
 
 static int log_inode_item(struct btrfs_trans_handle *trans,
-- 
2.49.0

[PATCH 4/4] btrfs: accessors: delete token versions of set/get helpers

[David Sterba]

Once upon a time there was a need to cache address of extent buffer
pages, as it was a costly operation (map_private_extent_buffer(),
cfed81a04eb555 ("Btrfs: add the ability to cache a pointer into the
eb")).  This was not even due to use of HIGHMEM, this had been removed
before that due to possible locking issues ( a65917156e3459 ("Btrfs:
stop using highmem for extent_buffers")).

Over the time the amount of work in the set/get helpers got reduced and
became quite straightforward bounds checking with an unaligned
read/write, commit db3756c879773c ("btrfs: remove unused
map_private_extent_buffer").

The actual caching of the page_address()/folio_address() in the token
was more work for very little gain. This depended on subsequent access
into the same page/folio, otherwise the cached pointer had to be
updated.

For metadata-heavy operations this showed up in the 'perf top' profile
where the btrfs_get_token_32 calls were at the top, on my testing
machine consuming about 2-3%. The other generic 32/64 bit helpers also
appeared in the profile with similar fraction.

After removing use of the token helpers we can remove them completely,
this leads to reduction of btrfs.ko by 6.7KiB on release config.

   text    data     bss     dec     hex filename
1463289  115665   16088 1595042  1856a2 pre/btrfs.ko
1456601  115665   16088 1588354  183c82 post/btrfs.ko

DELTA: -6688

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/accessors.c | 78 --------------------------------------------
 fs/btrfs/accessors.h | 37 ---------------------
 2 files changed, 115 deletions(-)

diff --git a/fs/btrfs/accessors.c b/fs/btrfs/accessors.c
index e3716516ca3876..5cfb0801700e6c 100644
--- a/fs/btrfs/accessors.c
+++ b/fs/btrfs/accessors.c
@@ -25,13 +25,6 @@ static bool check_setget_bounds(const struct extent_buffer *eb,
 	return true;
 }
 
-void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *eb)
-{
-	token->eb = eb;
-	token->kaddr = folio_address(eb->folios[0]);
-	token->offset = 0;
-}
-
 /*
  * Macro templates that define helpers to read/write extent buffer data of a
  * given size, that are also used via ctree.h for access to item members by
@@ -41,11 +34,6 @@ void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *e
  * - btrfs_set_8 (for 8/16/32/64)
  * - btrfs_get_8 (for 8/16/32/64)
  *
- * Generic helpers with a token (cached address of the most recently accessed
- * page):
- * - btrfs_set_token_8 (for 8/16/32/64)
- * - btrfs_get_token_8 (for 8/16/32/64)
- *
  * The set/get functions handle data spanning two pages transparently, in case
  * metadata block size is larger than page.  Every pointer to metadata items is
  * an offset into the extent buffer page array, cast to a specific type.  This
@@ -57,37 +45,6 @@ void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *e
  */
 
 #define DEFINE_BTRFS_SETGET_BITS(bits)					\
-u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
-			       const void *ptr, unsigned long off)	\
-{									\
-	const unsigned long member_offset = (unsigned long)ptr + off;	\
-	const unsigned long idx = get_eb_folio_index(token->eb, member_offset); \
-	const unsigned long oil = get_eb_offset_in_folio(token->eb,	\
-							 member_offset);\
-	const int unit_size = token->eb->folio_size;			\
-	const int unit_shift = token->eb->folio_shift;			\
-	const int size = sizeof(u##bits);				\
-	u8 lebytes[sizeof(u##bits)];					\
-	const int part = unit_size - oil;				\
-									\
-	ASSERT(token);							\
-	ASSERT(token->kaddr);						\
-	ASSERT(check_setget_bounds(token->eb, ptr, off, size));		\
-	if (token->offset <= member_offset &&				\
-	    member_offset + size <= token->offset + unit_size) {	\
-		return get_unaligned_le##bits(token->kaddr + oil);	\
-	}								\
-	token->kaddr = folio_address(token->eb->folios[idx]);		\
-	token->offset = idx << unit_shift;				\
-	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oil + size <= unit_size) \
-		return get_unaligned_le##bits(token->kaddr + oil);	\
-									\
-	memcpy(lebytes, token->kaddr + oil, part);			\
-	token->kaddr = folio_address(token->eb->folios[idx + 1]);	\
-	token->offset = (idx + 1) << unit_shift;			\
-	memcpy(lebytes + part, token->kaddr, size - part);		\
-	return get_unaligned_le##bits(lebytes);				\
-}									\
 u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
 			 const void *ptr, unsigned long off)		\
 {									\
@@ -110,41 +67,6 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
 	memcpy(lebytes + part, kaddr, size - part);			\
 	return get_unaligned_le##bits(lebytes);				\
 }									\
-void btrfs_set_token_##bits(struct btrfs_map_token *token,		\
-			    const void *ptr, unsigned long off,		\
-			    u##bits val)				\
-{									\
-	const unsigned long member_offset = (unsigned long)ptr + off;	\
-	const unsigned long idx = get_eb_folio_index(token->eb, member_offset); \
-	const unsigned long oil = get_eb_offset_in_folio(token->eb,	\
-							 member_offset);\
-	const int unit_size = token->eb->folio_size;			\
-	const int unit_shift = token->eb->folio_shift;			\
-	const int size = sizeof(u##bits);				\
-	u8 lebytes[sizeof(u##bits)];					\
-	const int part = unit_size - oil;				\
-									\
-	ASSERT(token);							\
-	ASSERT(token->kaddr);						\
-	ASSERT(check_setget_bounds(token->eb, ptr, off, size));		\
-	if (token->offset <= member_offset &&				\
-	    member_offset + size <= token->offset + unit_size) {	\
-		put_unaligned_le##bits(val, token->kaddr + oil);	\
-		return;							\
-	}								\
-	token->kaddr = folio_address(token->eb->folios[idx]);		\
-	token->offset = idx << unit_shift;				\
-	if (INLINE_EXTENT_BUFFER_PAGES == 1 ||				\
-	    oil + size <= unit_size) {					\
-		put_unaligned_le##bits(val, token->kaddr + oil);	\
-		return;							\
-	}								\
-	put_unaligned_le##bits(val, lebytes);				\
-	memcpy(token->kaddr + oil, lebytes, part);			\
-	token->kaddr = folio_address(token->eb->folios[idx + 1]);	\
-	token->offset = (idx + 1) << unit_shift;			\
-	memcpy(token->kaddr, lebytes + part, size - part);		\
-}									\
 void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
 		      unsigned long off, u##bits val)			\
 {									\
diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
index 15ea6348800b08..99b3ced12805bb 100644
--- a/fs/btrfs/accessors.h
+++ b/fs/btrfs/accessors.h
@@ -16,14 +16,6 @@
 
 struct extent_buffer;
 
-struct btrfs_map_token {
-	struct extent_buffer *eb;
-	char *kaddr;
-	unsigned long offset;
-};
-
-void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *eb);
-
 /*
  * Some macros to generate set/get functions for the struct fields.  This
  * assumes there is a lefoo_to_cpu for every type, so lets make a simple one
@@ -56,11 +48,6 @@ static inline void put_unaligned_le8(u8 val, void *p)
 			    sizeof_field(type, member)))
 
 #define DECLARE_BTRFS_SETGET_BITS(bits)					\
-u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
-			       const void *ptr, unsigned long off);	\
-void btrfs_set_token_##bits(struct btrfs_map_token *token,		\
-			    const void *ptr, unsigned long off,		\
-			    u##bits val);				\
 u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
 			 const void *ptr, unsigned long off);		\
 void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
@@ -83,18 +70,6 @@ static inline void btrfs_set_##name(const struct extent_buffer *eb, type *s, \
 {									\
 	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
 	btrfs_set_##bits(eb, s, offsetof(type, member), val);		\
-}									\
-static inline u##bits btrfs_token_##name(struct btrfs_map_token *token,	\
-					 const type *s)			\
-{									\
-	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
-	return btrfs_get_token_##bits(token, s, offsetof(type, member));\
-}									\
-static inline void btrfs_set_token_##name(struct btrfs_map_token *token,\
-					  type *s, u##bits val)		\
-{									\
-	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
-	btrfs_set_token_##bits(token, s, offsetof(type, member), val);	\
 }
 
 #define BTRFS_SETGET_HEADER_FUNCS(name, type, member, bits)		\
@@ -479,18 +454,6 @@ static inline void btrfs_set_item_##member(const struct extent_buffer *eb,	\
 					   int slot, u32 val)			\
 {										\
 	btrfs_set_raw_item_##member(eb, btrfs_item_nr(eb, slot), val);		\
-}										\
-static inline u32 btrfs_token_item_##member(struct btrfs_map_token *token,	\
-					    int slot)				\
-{										\
-	struct btrfs_item *item = btrfs_item_nr(token->eb, slot);		\
-	return btrfs_token_raw_item_##member(token, item);			\
-}										\
-static inline void btrfs_set_token_item_##member(struct btrfs_map_token *token,	\
-						 int slot, u32 val)		\
-{										\
-	struct btrfs_item *item = btrfs_item_nr(token->eb, slot);		\
-	btrfs_set_token_raw_item_##member(token, item, val);			\
 }
 
 BTRFS_ITEM_SETGET_FUNCS(offset)
-- 
2.49.0

Re: [PATCH 4/4] btrfs: accessors: delete token versions of set/get helpers

[Qu Wenruo]

在 2025/6/27 23:33, David Sterba 写道:
> Once upon a time there was a need to cache address of extent buffer
> pages, as it was a costly operation (map_private_extent_buffer(),
> cfed81a04eb555 ("Btrfs: add the ability to cache a pointer into the
> eb")).  This was not even due to use of HIGHMEM, this had been removed
> before that due to possible locking issues ( a65917156e3459 ("Btrfs:
> stop using highmem for extent_buffers")).
> 
> Over the time the amount of work in the set/get helpers got reduced and
> became quite straightforward bounds checking with an unaligned
> read/write, commit db3756c879773c ("btrfs: remove unused
> map_private_extent_buffer").

Thanks a lot for the history of the token accessors.

And this is also a great chance to us to sync the removal of token eb 
accessors to the progs.

If you haven't yet started that progs removal, I'm totally fine to do that.

Thanks,
Qu

> 
> The actual caching of the page_address()/folio_address() in the token
> was more work for very little gain. This depended on subsequent access
> into the same page/folio, otherwise the cached pointer had to be
> updated.
> 
> For metadata-heavy operations this showed up in the 'perf top' profile
> where the btrfs_get_token_32 calls were at the top, on my testing
> machine consuming about 2-3%. The other generic 32/64 bit helpers also
> appeared in the profile with similar fraction.
> 
> After removing use of the token helpers we can remove them completely,
> this leads to reduction of btrfs.ko by 6.7KiB on release config.
> 
>     text    data     bss     dec     hex filename
> 1463289  115665   16088 1595042  1856a2 pre/btrfs.ko
> 1456601  115665   16088 1588354  183c82 post/btrfs.ko
> 
> DELTA: -6688
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/accessors.c | 78 --------------------------------------------
>   fs/btrfs/accessors.h | 37 ---------------------
>   2 files changed, 115 deletions(-)
> 
> diff --git a/fs/btrfs/accessors.c b/fs/btrfs/accessors.c
> index e3716516ca3876..5cfb0801700e6c 100644
> --- a/fs/btrfs/accessors.c
> +++ b/fs/btrfs/accessors.c
> @@ -25,13 +25,6 @@ static bool check_setget_bounds(const struct extent_buffer *eb,
>   	return true;
>   }
>   
> -void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *eb)
> -{
> -	token->eb = eb;
> -	token->kaddr = folio_address(eb->folios[0]);
> -	token->offset = 0;
> -}
> -
>   /*
>    * Macro templates that define helpers to read/write extent buffer data of a
>    * given size, that are also used via ctree.h for access to item members by
> @@ -41,11 +34,6 @@ void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *e
>    * - btrfs_set_8 (for 8/16/32/64)
>    * - btrfs_get_8 (for 8/16/32/64)
>    *
> - * Generic helpers with a token (cached address of the most recently accessed
> - * page):
> - * - btrfs_set_token_8 (for 8/16/32/64)
> - * - btrfs_get_token_8 (for 8/16/32/64)
> - *
>    * The set/get functions handle data spanning two pages transparently, in case
>    * metadata block size is larger than page.  Every pointer to metadata items is
>    * an offset into the extent buffer page array, cast to a specific type.  This
> @@ -57,37 +45,6 @@ void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *e
>    */
>   
>   #define DEFINE_BTRFS_SETGET_BITS(bits)					\
> -u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
> -			       const void *ptr, unsigned long off)	\
> -{									\
> -	const unsigned long member_offset = (unsigned long)ptr + off;	\
> -	const unsigned long idx = get_eb_folio_index(token->eb, member_offset); \
> -	const unsigned long oil = get_eb_offset_in_folio(token->eb,	\
> -							 member_offset);\
> -	const int unit_size = token->eb->folio_size;			\
> -	const int unit_shift = token->eb->folio_shift;			\
> -	const int size = sizeof(u##bits);				\
> -	u8 lebytes[sizeof(u##bits)];					\
> -	const int part = unit_size - oil;				\
> -									\
> -	ASSERT(token);							\
> -	ASSERT(token->kaddr);						\
> -	ASSERT(check_setget_bounds(token->eb, ptr, off, size));		\
> -	if (token->offset <= member_offset &&				\
> -	    member_offset + size <= token->offset + unit_size) {	\
> -		return get_unaligned_le##bits(token->kaddr + oil);	\
> -	}								\
> -	token->kaddr = folio_address(token->eb->folios[idx]);		\
> -	token->offset = idx << unit_shift;				\
> -	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oil + size <= unit_size) \
> -		return get_unaligned_le##bits(token->kaddr + oil);	\
> -									\
> -	memcpy(lebytes, token->kaddr + oil, part);			\
> -	token->kaddr = folio_address(token->eb->folios[idx + 1]);	\
> -	token->offset = (idx + 1) << unit_shift;			\
> -	memcpy(lebytes + part, token->kaddr, size - part);		\
> -	return get_unaligned_le##bits(lebytes);				\
> -}									\
>   u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
>   			 const void *ptr, unsigned long off)		\
>   {									\
> @@ -110,41 +67,6 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
>   	memcpy(lebytes + part, kaddr, size - part);			\
>   	return get_unaligned_le##bits(lebytes);				\
>   }									\
> -void btrfs_set_token_##bits(struct btrfs_map_token *token,		\
> -			    const void *ptr, unsigned long off,		\
> -			    u##bits val)				\
> -{									\
> -	const unsigned long member_offset = (unsigned long)ptr + off;	\
> -	const unsigned long idx = get_eb_folio_index(token->eb, member_offset); \
> -	const unsigned long oil = get_eb_offset_in_folio(token->eb,	\
> -							 member_offset);\
> -	const int unit_size = token->eb->folio_size;			\
> -	const int unit_shift = token->eb->folio_shift;			\
> -	const int size = sizeof(u##bits);				\
> -	u8 lebytes[sizeof(u##bits)];					\
> -	const int part = unit_size - oil;				\
> -									\
> -	ASSERT(token);							\
> -	ASSERT(token->kaddr);						\
> -	ASSERT(check_setget_bounds(token->eb, ptr, off, size));		\
> -	if (token->offset <= member_offset &&				\
> -	    member_offset + size <= token->offset + unit_size) {	\
> -		put_unaligned_le##bits(val, token->kaddr + oil);	\
> -		return;							\
> -	}								\
> -	token->kaddr = folio_address(token->eb->folios[idx]);		\
> -	token->offset = idx << unit_shift;				\
> -	if (INLINE_EXTENT_BUFFER_PAGES == 1 ||				\
> -	    oil + size <= unit_size) {					\
> -		put_unaligned_le##bits(val, token->kaddr + oil);	\
> -		return;							\
> -	}								\
> -	put_unaligned_le##bits(val, lebytes);				\
> -	memcpy(token->kaddr + oil, lebytes, part);			\
> -	token->kaddr = folio_address(token->eb->folios[idx + 1]);	\
> -	token->offset = (idx + 1) << unit_shift;			\
> -	memcpy(token->kaddr, lebytes + part, size - part);		\
> -}									\
>   void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
>   		      unsigned long off, u##bits val)			\
>   {									\
> diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
> index 15ea6348800b08..99b3ced12805bb 100644
> --- a/fs/btrfs/accessors.h
> +++ b/fs/btrfs/accessors.h
> @@ -16,14 +16,6 @@
>   
>   struct extent_buffer;
>   
> -struct btrfs_map_token {
> -	struct extent_buffer *eb;
> -	char *kaddr;
> -	unsigned long offset;
> -};
> -
> -void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *eb);
> -
>   /*
>    * Some macros to generate set/get functions for the struct fields.  This
>    * assumes there is a lefoo_to_cpu for every type, so lets make a simple one
> @@ -56,11 +48,6 @@ static inline void put_unaligned_le8(u8 val, void *p)
>   			    sizeof_field(type, member)))
>   
>   #define DECLARE_BTRFS_SETGET_BITS(bits)					\
> -u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
> -			       const void *ptr, unsigned long off);	\
> -void btrfs_set_token_##bits(struct btrfs_map_token *token,		\
> -			    const void *ptr, unsigned long off,		\
> -			    u##bits val);				\
>   u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
>   			 const void *ptr, unsigned long off);		\
>   void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
> @@ -83,18 +70,6 @@ static inline void btrfs_set_##name(const struct extent_buffer *eb, type *s, \
>   {									\
>   	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
>   	btrfs_set_##bits(eb, s, offsetof(type, member), val);		\
> -}									\
> -static inline u##bits btrfs_token_##name(struct btrfs_map_token *token,	\
> -					 const type *s)			\
> -{									\
> -	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
> -	return btrfs_get_token_##bits(token, s, offsetof(type, member));\
> -}									\
> -static inline void btrfs_set_token_##name(struct btrfs_map_token *token,\
> -					  type *s, u##bits val)		\
> -{									\
> -	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
> -	btrfs_set_token_##bits(token, s, offsetof(type, member), val);	\
>   }
>   
>   #define BTRFS_SETGET_HEADER_FUNCS(name, type, member, bits)		\
> @@ -479,18 +454,6 @@ static inline void btrfs_set_item_##member(const struct extent_buffer *eb,	\
>   					   int slot, u32 val)			\
>   {										\
>   	btrfs_set_raw_item_##member(eb, btrfs_item_nr(eb, slot), val);		\
> -}										\
> -static inline u32 btrfs_token_item_##member(struct btrfs_map_token *token,	\
> -					    int slot)				\
> -{										\
> -	struct btrfs_item *item = btrfs_item_nr(token->eb, slot);		\
> -	return btrfs_token_raw_item_##member(token, item);			\
> -}										\
> -static inline void btrfs_set_token_item_##member(struct btrfs_map_token *token,	\
> -						 int slot, u32 val)		\
> -{										\
> -	struct btrfs_item *item = btrfs_item_nr(token->eb, slot);		\
> -	btrfs_set_token_raw_item_##member(token, item, val);			\
>   }
>   
>   BTRFS_ITEM_SETGET_FUNCS(offset)

Re: [PATCH 0/4] Set/get accessor cleanups

[Qu Wenruo]

在 2025/6/27 23:33, David Sterba 写道:
> This patchset is first in a series to cleanup and optimize the
> accessors, now removing code that's not needed anymore, explained with
> references in the last patch.
> 
> Overall effects on .ko:
> 
>     text    data     bss     dec     hex filename
> 1463615  115665   16088 1595368  1857e8 pre/btrfs.ko
> 1456601  115665   16088 1588354  183c82 post/btrfs.ko
> 
> DELTA: -7014
> 
> And stack consumption:
> 
> __push_leaf_left                                   -32 (176 -> 144)
> copy_for_split                                     -32 (144 -> 112)
> fill_inode_item                                    -32 (80 -> 48)
> btrfs_truncate_item                                -24 (152 -> 128)
> btrfs_extend_item                                  -16 (104 -> 88)
> btrfs_del_items                                    -16 (144 -> 128)
> setup_items_for_insert                             -32 (144 -> 112)
> __push_leaf_right                                  -24 (168 -> 144)
> 
> REMOVED (744):
>          btrfs_get_token_32                          88
>          btrfs_set_token_64                          96
>          btrfs_get_token_64                          88
>          btrfs_set_token_32                          96
>          btrfs_get_token_16                          88
>          btrfs_get_token_8                           88
>          btrfs_set_token_16                          96
>          btrfs_set_token_8                           96
>          btrfs_init_map_token                         8
> 
> REMOVED/NEW DELTA:  -744
> PRE/POST DELTA:     -952
> 
> David Sterba (4):
>    btrfs: don't use token set/get accessors for btrfs_item members
>    btrfs: don't use token set/get accessors in inode.c:fill_inode_item()
>    btrfs: tree-log: don't use token set/get accessors in
>      fill_inode_item()
>    btrfs: accessors: delete token versions of set/get helpers

Looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Although it also exposed that we have two different fill_inode_item() 
functions.
I know the tree log code needs some special handling, the 
fill_inode_item() in inode.c looks like can be implemented by the one in 
tree-log.

It may be a good time to merge them into one in another patch.

Thanks,
Qu

> 
>   fs/btrfs/accessors.c | 78 --------------------------------------------
>   fs/btrfs/accessors.h | 37 ---------------------
>   fs/btrfs/ctree.c     | 51 ++++++++++-------------------
>   fs/btrfs/inode.c     | 50 ++++++++++++----------------
>   fs/btrfs/tree-log.c  | 48 +++++++++++----------------
>   5 files changed, 57 insertions(+), 207 deletions(-)
> 

Re: [PATCH 0/4] Set/get accessor cleanups

[Johannes Thumshirn]

Nice work,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/4] Set/get accessor cleanups

[David Sterba]

On Sun, Jun 29, 2025 at 10:48:15AM +0930, Qu Wenruo wrote:
> > David Sterba (4):
> >    btrfs: don't use token set/get accessors for btrfs_item members
> >    btrfs: don't use token set/get accessors in inode.c:fill_inode_item()
> >    btrfs: tree-log: don't use token set/get accessors in
> >      fill_inode_item()
> >    btrfs: accessors: delete token versions of set/get helpers
> 
> Looks good to me.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Although it also exposed that we have two different fill_inode_item() 
> functions.
> I know the tree log code needs some special handling, the 
> fill_inode_item() in inode.c looks like can be implemented by the one in 
> tree-log.
> 
> It may be a good time to merge them into one in another patch.

Yes, the difference is not that big so it makes sense to merge them.

Re: [PATCH 4/4] btrfs: accessors: delete token versions of set/get helpers

[David Sterba]

On Sun, Jun 29, 2025 at 10:43:26AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/6/27 23:33, David Sterba 写道:
> > Once upon a time there was a need to cache address of extent buffer
> > pages, as it was a costly operation (map_private_extent_buffer(),
> > cfed81a04eb555 ("Btrfs: add the ability to cache a pointer into the
> > eb")).  This was not even due to use of HIGHMEM, this had been removed
> > before that due to possible locking issues ( a65917156e3459 ("Btrfs:
> > stop using highmem for extent_buffers")).
> > 
> > Over the time the amount of work in the set/get helpers got reduced and
> > became quite straightforward bounds checking with an unaligned
> > read/write, commit db3756c879773c ("btrfs: remove unused
> > map_private_extent_buffer").
> 
> Thanks a lot for the history of the token accessors.
> 
> And this is also a great chance to us to sync the removal of token eb 
> accessors to the progs.
> 
> If you haven't yet started that progs removal, I'm totally fine to do that.

I usually do the kernel->progs sync before a release, inspired by the
kernel git log where it applies. I haven't started with the accessors
updates as I have a few more changes to those files pending but I
wouldn't mind if you do it now.