inode: don't memset the inode address space twice

PATCH In review Dave Chinner 4 participants 4 messages Mar 1, 2018 – Mar 7, 2018 VFS

lore

Share & tools

lore

Dave Chinner

author

2018-03-01 22:35 UTC #0 lore raw

PATCH 1/1 inode: don't memset the inode address space twice

From: Dave Chinner <[email protected]>

Noticed when looking at why cycling 600k inodes/s through the inode cache was taking a total of 8% cpu in memset() during inode initialisation. There is no need to zero the inode.i_data structure twice.

This increases single threaded bulkstat throughput from ~200,000 inodes/s to ~220,000 inodes/s, so we save a substantial amount of CPU time per inode init by doing this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/inode.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/inode.c +8 -3

index 6295f1415761..b153aeaa61ea 100644
@@ -346,9 +346,8 @@ void inc_nlink(struct inode *inode)
 }
 EXPORT_SYMBOL(inc_nlink);
 
-void address_space_init_once(struct address_space *mapping)
+static void __address_space_init_once(struct address_space *mapping)
 {
-	memset(mapping, 0, sizeof(*mapping));
 	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
 	spin_lock_init(&mapping->tree_lock);
 	init_rwsem(&mapping->i_mmap_rwsem);

@@ -356,6 +355,12 @@ void address_space_init_once(struct address_space *mapping)
 	spin_lock_init(&mapping->private_lock);
 	mapping->i_mmap = RB_ROOT_CACHED;
 }
+
+void address_space_init_once(struct address_space *mapping)
+{
+	memset(mapping, 0, sizeof(*mapping));
+	__address_space_init_once(mapping);
+}
 EXPORT_SYMBOL(address_space_init_once);
 
 /*

@@ -371,7 +376,7 @@ void inode_init_once(struct inode *inode)
 	INIT_LIST_HEAD(&inode->i_io_list);
 	INIT_LIST_HEAD(&inode->i_wb_list);
 	INIT_LIST_HEAD(&inode->i_lru);
-	address_space_init_once(&inode->i_data);
+	__address_space_init_once(&inode->i_data);
 	i_size_ordered_init(inode);
 }
 EXPORT_SYMBOL(inode_init_once);

Signature · 1 line

2.16.1

Carlos Maiolino

Reviewed-by on v1 1/1

2018-03-02 09:16 UTC #1 lore raw

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>
Noticed when looking at why cycling 600k inodes/s through the inode cache was taking a total of 8% cpu in memset() during inode initialisation. There is no need to zero the inode.i_data structure twice.

Quoted 5 lines

This increases single threaded bulkstat throughput from ~200,000 inodes/s to ~220,000 inodes/s, so we save a substantial amount of CPU time per inode init by doing this.
Signed-off-by Dave Chinner <[email protected]>

Looks good to me.

Reviewed-by Carlos Maiolino <[email protected]>

Quoted 48 lines

---
 fs/inode.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/inode.c +8 -3

index 6295f1415761..b153aeaa61ea 100644
@@ -346,9 +346,8 @@ void inc_nlink(struct inode *inode)
 }
 EXPORT_SYMBOL(inc_nlink);
 
-void address_space_init_once(struct address_space *mapping)
+static void __address_space_init_once(struct address_space *mapping)
 {
-	memset(mapping, 0, sizeof(*mapping));
 	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
 	spin_lock_init(&mapping->tree_lock);
 	init_rwsem(&mapping->i_mmap_rwsem);

@@ -356,6 +355,12 @@ void address_space_init_once(struct address_space *mapping)
 	spin_lock_init(&mapping->private_lock);
 	mapping->i_mmap = RB_ROOT_CACHED;
 }
+
+void address_space_init_once(struct address_space *mapping)
+{
+	memset(mapping, 0, sizeof(*mapping));
+	__address_space_init_once(mapping);
+}
 EXPORT_SYMBOL(address_space_init_once);
 
 /*

@@ -371,7 +376,7 @@ void inode_init_once(struct inode *inode)
 	INIT_LIST_HEAD(&inode->i_io_list);
 	INIT_LIST_HEAD(&inode->i_wb_list);
 	INIT_LIST_HEAD(&inode->i_lru);
-	address_space_init_once(&inode->i_data);
+	__address_space_init_once(&inode->i_data);
 	i_size_ordered_init(inode);
 }
 EXPORT_SYMBOL(inode_init_once);

Signature · 6 lines

2.16.1

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

Signature · 1 line

Carlos

Jan Kara

reviewer Reviewed-by on v1 1/1

2018-03-06 10:57 UTC #2 lore raw

In reply to Dave Chinner

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>
Noticed when looking at why cycling 600k inodes/s through the inode cache was taking a total of 8% cpu in memset() during inode initialisation. There is no need to zero the inode.i_data structure twice.

Quoted 5 lines

This increases single threaded bulkstat throughput from ~200,000 inodes/s to ~220,000 inodes/s, so we save a substantial amount of CPU time per inode init by doing this.
Signed-off-by Dave Chinner <[email protected]>

Nice and the patch looks good. You can add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 48 lines

---
 fs/inode.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/inode.c +8 -3

index 6295f1415761..b153aeaa61ea 100644
@@ -346,9 +346,8 @@ void inc_nlink(struct inode *inode)
 }
 EXPORT_SYMBOL(inc_nlink);
 
-void address_space_init_once(struct address_space *mapping)
+static void __address_space_init_once(struct address_space *mapping)
 {
-	memset(mapping, 0, sizeof(*mapping));
 	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
 	spin_lock_init(&mapping->tree_lock);
 	init_rwsem(&mapping->i_mmap_rwsem);

@@ -356,6 +355,12 @@ void address_space_init_once(struct address_space *mapping)
 	spin_lock_init(&mapping->private_lock);
 	mapping->i_mmap = RB_ROOT_CACHED;
 }
+
+void address_space_init_once(struct address_space *mapping)
+{
+	memset(mapping, 0, sizeof(*mapping));
+	__address_space_init_once(mapping);
+}
 EXPORT_SYMBOL(address_space_init_once);
 
 /*

@@ -371,7 +376,7 @@ void inode_init_once(struct inode *inode)
 	INIT_LIST_HEAD(&inode->i_io_list);
 	INIT_LIST_HEAD(&inode->i_wb_list);
 	INIT_LIST_HEAD(&inode->i_lru);
-	address_space_init_once(&inode->i_data);
+	__address_space_init_once(&inode->i_data);
 	i_size_ordered_init(inode);
 }
 EXPORT_SYMBOL(inode_init_once);

Signature · 6 lines

2.16.1

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

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Darrick J. Wong

Reviewed-by on v1 1/1

2018-03-07 01:32 UTC #3 lore raw

In reply to Dave Chinner

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>
Noticed when looking at why cycling 600k inodes/s through the inode cache was taking a total of 8% cpu in memset() during inode initialisation. There is no need to zero the inode.i_data structure twice.

Quoted 5 lines

This increases single threaded bulkstat throughput from ~200,000 inodes/s to ~220,000 inodes/s, so we save a substantial amount of CPU time per inode init by doing this.
Signed-off-by Dave Chinner <[email protected]>

Looks ok, will test...

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 44 lines

---
 fs/inode.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/inode.c +8 -3

index 6295f1415761..b153aeaa61ea 100644
@@ -346,9 +346,8 @@ void inc_nlink(struct inode *inode)
 }
 EXPORT_SYMBOL(inc_nlink);
 
-void address_space_init_once(struct address_space *mapping)
+static void __address_space_init_once(struct address_space *mapping)
 {
-	memset(mapping, 0, sizeof(*mapping));
 	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
 	spin_lock_init(&mapping->tree_lock);
 	init_rwsem(&mapping->i_mmap_rwsem);

@@ -356,6 +355,12 @@ void address_space_init_once(struct address_space *mapping)
 	spin_lock_init(&mapping->private_lock);
 	mapping->i_mmap = RB_ROOT_CACHED;
 }
+
+void address_space_init_once(struct address_space *mapping)
+{
+	memset(mapping, 0, sizeof(*mapping));
+	__address_space_init_once(mapping);
+}
 EXPORT_SYMBOL(address_space_init_once);
 
 /*

@@ -371,7 +376,7 @@ void inode_init_once(struct inode *inode)
 	INIT_LIST_HEAD(&inode->i_io_list);
 	INIT_LIST_HEAD(&inode->i_wb_list);
 	INIT_LIST_HEAD(&inode->i_lru);
-	address_space_init_once(&inode->i_data);
+	__address_space_init_once(&inode->i_data);
 	i_size_ordered_init(inode);
 }
 EXPORT_SYMBOL(inode_init_once);

Signature · 1 line

2.16.1