Shrink some data structures

PATCH In review David Sterba 2 participants 8 messages Nov 15, 2017 – Nov 16, 2017 Btrfs file system

lore

Share & tools

lore

David Sterba

author maintainer

2017-11-15 17:44 UTC #3 lore raw

PATCH 3/6 btrfs: switch to refcount_t type for btrfs_trans_handle::use_count

In reply to David Sterba

The use_count is a reference counter, we can use the refcount_t type, though we don't use the atomicity. This is not a performance critical code and we could catch the underflows. The type is changed from long, but the number of references will fit an int.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/transaction.c | 12 ++++++------
 fs/btrfs/transaction.h |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

fs/btrfs/transaction.c +6 -6

index dac688c696c3..6348573e26a7 100644
@@ -495,8 +495,8 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 	if (current->journal_info) {
 		WARN_ON(type & TRANS_EXTWRITERS);
 		h = current->journal_info;
-		h->use_count++;
-		WARN_ON(h->use_count > 2);
+		refcount_inc(&h->use_count);
+		WARN_ON(refcount_read(&h->use_count) > 2);
 		h->orig_rsv = h->block_rsv;
 		h->block_rsv = NULL;
 		goto got_it;

@@ -567,7 +567,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 	h->transid = cur_trans->transid;
 	h->transaction = cur_trans;
 	h->root = root;
-	h->use_count = 1;
+	refcount_set(&h->use_count, 1);
 	h->fs_info = root->fs_info;
 
 	h->type = type;

@@ -837,8 +837,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 	int err = 0;
 	int must_run_delayed_refs = 0;
 
-	if (trans->use_count > 1) {
-		trans->use_count--;
+	if (refcount_read(&trans->use_count) > 1) {
+		refcount_dec(&trans->use_count);
 		trans->block_rsv = trans->orig_rsv;
 		return 0;
 	}

@@ -1868,7 +1868,7 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans,
 	struct btrfs_transaction *cur_trans = trans->transaction;
 	DEFINE_WAIT(wait);
 
-	WARN_ON(trans->use_count > 1);
+	WARN_ON(refcount_read(&trans->use_count) > 1);
 
 	btrfs_abort_transaction(trans, err);

fs/btrfs/transaction.h +1 -1

index c48a4a03f1b4..afa88f035654 100644
@@ -111,7 +111,7 @@ struct btrfs_trans_handle {
 	u64 transid;
 	u64 bytes_reserved;
 	u64 chunk_bytes_reserved;
-	unsigned long use_count;
+	refcount_t use_count;
 	unsigned long delayed_ref_updates;
 	struct btrfs_transaction *transaction;
 	struct btrfs_block_rsv *block_rsv;

Signature · 1 line

2.14.3

David Sterba

author maintainer

2017-11-15 17:44 UTC #5 lore raw

PATCH 5/6 btrfs: use narrower type for btrfs_transaction::num_dirty_bgs

In reply to David Sterba

The u64 is an overkill here, we could not possibly create that many blockgroups in one transaction.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/extent-tree.c | 2 +-
 fs/btrfs/transaction.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/extent-tree.c +1 -1

index 11b7d613140b..d59a65852447 100644
@@ -2894,7 +2894,7 @@ int btrfs_check_space_for_delayed_refs(struct btrfs_trans_handle *trans,
 	struct btrfs_block_rsv *global_rsv;
 	u64 num_heads = trans->transaction->delayed_refs.num_heads_ready;
 	u64 csum_bytes = trans->transaction->delayed_refs.pending_csums;
-	u64 num_dirty_bgs = trans->transaction->num_dirty_bgs;
+	unsigned int num_dirty_bgs = trans->transaction->num_dirty_bgs;
 	u64 num_bytes, num_dirty_bgs_bytes;
 	int ret = 0;

fs/btrfs/transaction.h +1 -1

index edf53112a6f2..1805fd101767 100644
@@ -70,7 +70,7 @@ struct btrfs_transaction {
 	struct list_head dirty_bgs;
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
-	u64 num_dirty_bgs;
+	unsigned int num_dirty_bgs;
 
 	/*
 	 * we need to make sure block group deletion doesn't race with

Signature · 1 line

2.14.3

David Sterba

author maintainer

2017-11-15 17:44 UTC #0 lore raw

cover Shrink some data structures

Unused members removed, other reordered.

struct btrfs_transaction:

-       /* size: 432, cachelines: 7, members: 27 */
-       /* sum members: 416, holes: 4, sum holes: 16 */
-       /* last cacheline: 48 bytes */
+       /* size: 416, cachelines: 7, members: 27 */
+       /* sum members: 412, holes: 1, sum holes: 4 */
+       /* last cacheline: 32 bytes */

struct btrfs_trans_handle:

-       /* size: 120, cachelines: 2, members: 20 */
-       /* sum members: 117, holes: 1, sum holes: 3 */
-       /* last cacheline: 56 bytes */
+       /* size: 104, cachelines: 2, members: 19 */
+       /* last cacheline: 40 bytes *

And incidentally the .text size goes down as well:

text data bss dec hex filename

 985430   75100   18560 1079090  107732 pre/btrfs.ko
 985243   75100   18560 1078903  107677 post/btrfs.ko

David Sterba (6):
btrfs: switch btrfs_trans_handle::adding_csums to bool
btrfs: remove unused member of btrfs_trans_handle
btrfs: switch to refcount_t type for btrfs_trans_handle::use_count
btrfs: reoder btrfs_trans_handle members for better packing
btrfs: use narrower type for btrfs_transaction::num_dirty_bgs
btrfs: reoder btrfs_transaction members for better packing

 fs/btrfs/extent-tree.c |  2 +-
 fs/btrfs/inode.c       |  4 ++--
 fs/btrfs/transaction.c | 12 ++++++------
 fs/btrfs/transaction.h | 11 +++++------
 4 files changed, 14 insertions(+), 15 deletions(-)

Signature · 1 line

2.14.3

David Sterba

author maintainer

2017-11-15 17:44 UTC #1 lore raw

PATCH 1/6 btrfs: switch btrfs_trans_handle::adding_csums to bool

The semantics of adding_csums matches bool, 'short' was most likely used to save space in a698d0755adb6f2 ("Btrfs: add a type field for the transaction handle").

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/inode.c       | 4 ++--
 fs/btrfs/transaction.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

fs/btrfs/inode.c +2 -2

index f352662e066f..3a32e782ccc4 100644
@@ -2036,10 +2036,10 @@ static noinline int add_pending_csums(struct btrfs_trans_handle *trans,
 	struct btrfs_ordered_sum *sum;
 
 	list_for_each_entry(sum, list, list) {
-		trans->adding_csums = 1;
+		trans->adding_csums = true;
 		btrfs_csum_file_blocks(trans,
 		       BTRFS_I(inode)->root->fs_info->csum_root, sum);
-		trans->adding_csums = 0;
+		trans->adding_csums = false;
 	}
 	return 0;
 }

fs/btrfs/transaction.h +1 -1

index c55e44560103..a673142c003e 100644
@@ -118,7 +118,7 @@ struct btrfs_trans_handle {
 	struct btrfs_block_rsv *block_rsv;
 	struct btrfs_block_rsv *orig_rsv;
 	short aborted;
-	short adding_csums;
+	bool adding_csums;
 	bool allocating_chunk;
 	bool can_flush_pending_bgs;
 	bool reloc_reserved;

Signature · 1 line

2.14.3

David Sterba

author maintainer

2017-11-15 17:44 UTC #4 lore raw

PATCH 4/6 btrfs: reoder btrfs_trans_handle members for better packing

In reply to David Sterba

Recent updates to the structure left some holes, reorder the types so the packing is tight. The size goes from 112 to 104 on 64bit.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/transaction.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/transaction.h +2 -2

index afa88f035654..edf53112a6f2 100644
@@ -111,11 +111,12 @@ struct btrfs_trans_handle {
 	u64 transid;
 	u64 bytes_reserved;
 	u64 chunk_bytes_reserved;
-	refcount_t use_count;
 	unsigned long delayed_ref_updates;
 	struct btrfs_transaction *transaction;
 	struct btrfs_block_rsv *block_rsv;
 	struct btrfs_block_rsv *orig_rsv;
+	refcount_t use_count;
+	unsigned int type;
 	short aborted;
 	bool adding_csums;
 	bool allocating_chunk;

@@ -123,7 +124,6 @@ struct btrfs_trans_handle {
 	bool reloc_reserved;
 	bool sync;
 	bool dirty;
-	unsigned int type;
 	struct btrfs_root *root;
 	struct btrfs_fs_info *fs_info;
 	struct list_head new_bgs;

Signature · 1 line

2.14.3

David Sterba

author maintainer

2017-11-15 17:44 UTC #2 lore raw

PATCH 2/6 btrfs: remove unused member of btrfs_trans_handle

In reply to David Sterba

Last user was removed in a monster commit a22285a6a32390195235171 ("Btrfs: Integrate metadata reservation with start_transaction") in 2010.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/transaction.h | 1 -
 1 file changed, 1 deletion(-)

fs/btrfs/transaction.h +0 -1

index a673142c003e..c48a4a03f1b4 100644
@@ -112,7 +112,6 @@ struct btrfs_trans_handle {
 	u64 bytes_reserved;
 	u64 chunk_bytes_reserved;
 	unsigned long use_count;
-	unsigned long blocks_reserved;
 	unsigned long delayed_ref_updates;
 	struct btrfs_transaction *transaction;
 	struct btrfs_block_rsv *block_rsv;

Signature · 1 line

2.14.3

David Sterba

author maintainer

2017-11-15 17:44 UTC #6 lore raw

PATCH 6/6 btrfs: reoder btrfs_transaction members for better packing

In reply to David Sterba

There are now 20 bytes of holes, we can reduce that to 4 by minor changes. Moving 'aborted' to the status and flags is also more logical, similar for num_dirty_bgs. The size goes from 432 to 416.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/transaction.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/transaction.h +2 -2

index 1805fd101767..6beee072b1bd 100644
@@ -58,6 +58,7 @@ struct btrfs_transaction {
 
 	/* Be protected by fs_info->trans_lock when we want to change it. */
 	enum btrfs_trans_state state;
+	int aborted;
 	struct list_head list;
 	struct extent_io_tree dirty_pages;
 	unsigned long start_time;

@@ -70,7 +71,6 @@ struct btrfs_transaction {
 	struct list_head dirty_bgs;
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
-	unsigned int num_dirty_bgs;
 
 	/*
 	 * we need to make sure block group deletion doesn't race with

@@ -79,11 +79,11 @@ struct btrfs_transaction {
 	 */
 	struct mutex cache_write_mutex;
 	spinlock_t dirty_bgs_lock;
+	unsigned int num_dirty_bgs;
 	/* Protected by spin lock fs_info->unused_bgs_lock. */
 	struct list_head deleted_bgs;
 	spinlock_t dropped_roots_lock;
 	struct btrfs_delayed_ref_root delayed_refs;
-	int aborted;
 	struct btrfs_fs_info *fs_info;
 };

Signature · 1 line

2.14.3

Liu Bo

Reviewed-by on v1 6/6

2017-11-16 23:30 UTC #7 lore raw

Re: [PATCH 6/6] btrfs: reoder btrfs_transaction members for better packing

Quoted 4 lines · David Sterba

There are now 20 bytes of holes, we can reduce that to 4 by minor changes. Moving 'aborted' to the status and flags is also more logical, similar for num_dirty_bgs. The size goes from 432 to 416.

Reviewed-by Liu Bo <[email protected]>

Thanks,

-liubo

Quoted 45 lines

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/transaction.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/transaction.h +2 -2

index 1805fd101767..6beee072b1bd 100644
@@ -58,6 +58,7 @@ struct btrfs_transaction {
 
 	/* Be protected by fs_info->trans_lock when we want to change it. */
 	enum btrfs_trans_state state;
+	int aborted;
 	struct list_head list;
 	struct extent_io_tree dirty_pages;
 	unsigned long start_time;

@@ -70,7 +71,6 @@ struct btrfs_transaction {
 	struct list_head dirty_bgs;
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
-	unsigned int num_dirty_bgs;
 
 	/*
 	 * we need to make sure block group deletion doesn't race with

@@ -79,11 +79,11 @@ struct btrfs_transaction {
 	 */
 	struct mutex cache_write_mutex;
 	spinlock_t dirty_bgs_lock;
+	unsigned int num_dirty_bgs;
 	/* Protected by spin lock fs_info->unused_bgs_lock. */
 	struct list_head deleted_bgs;
 	spinlock_t dropped_roots_lock;
 	struct btrfs_delayed_ref_root delayed_refs;
-	int aborted;
 	struct btrfs_fs_info *fs_info;
 };

Signature · 6 lines

2.14.3

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