Re: [LKP] [lkp] [xfs] 68a9f5e700: aim7.jobs-per-min -13.6% regression

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On Thu, Aug 11, 2016 at 8:56 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Aug 11, 2016 at 07:27:52PM -0700, Linus Torvalds wrote:
>>
>> I don't recall having ever seen the mapping tree_lock as a contention
>> point before, but it's not like I've tried that load either. So it
>> might be a regression (going back long, I suspect), or just an unusual
>> load that nobody has traditionally tested much.
>>
>> Single-threaded big file write one page at a time, was it?
>
> Yup. On a 4 node NUMA system.

Ok, I can't see any real contention on my single-node workstation
(running ext4 too, so there may be filesystem differences), but I
guess that shouldn't surprise me. The cacheline bouncing just isn't
expensive enough when it all stays on-die.

I can see the tree_lock in my profiles (just not very high), and at
least for ext4 the main caller ssems to be
__set_page_dirty_nobuffers().

And yes, looking at that, the biggest cost by _far_ inside the
spinlock seems to be the accounting.

Which doesn't even have to be inside the mapping lock, as far as I can
tell, and as far as comments go.

So a stupid patch to just move the dirty page accounting to outside
the spinlock might help a lot.

Does this attached patch help your contention numbers?

Adding a few people who get blamed for account_page_dirtied() and
inode_attach_wb() just to make sure that nobody expected the
mapping_lock spinlock to be held when calling account_page_dirtied().

I realize that this has nothing to do with the AIM7 regression (the
spinlock just isn't high enough in that profile), but your contention
numbers just aren't right, and updating accounting statistics inside a
critical spinlock when not needed is just wrong.

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2276 bytes --]

 fs/buffer.c         | 5 ++++-
 fs/xfs/xfs_aops.c   | 5 ++++-
 mm/page-writeback.c | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 9c8eb9b6db6a..f79a9d241589 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -628,15 +628,18 @@ static void __set_page_dirty(struct page *page, struct address_space *mapping,
 			     int warn)
 {
 	unsigned long flags;
+	bool account = false;
 
 	spin_lock_irqsave(&mapping->tree_lock, flags);
 	if (page->mapping) {	/* Race with truncate? */
 		WARN_ON_ONCE(warn && !PageUptodate(page));
-		account_page_dirtied(page, mapping);
 		radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
+		account = true;
 	}
 	spin_unlock_irqrestore(&mapping->tree_lock, flags);
+	if (account)
+		account_page_dirtied(page, mapping);
 }
 
 /*
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 7575cfc3ad15..59169c36765e 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -1490,15 +1490,18 @@ xfs_vm_set_page_dirty(
 	if (newly_dirty) {
 		/* sigh - __set_page_dirty() is static, so copy it here, too */
 		unsigned long flags;
+		bool account = false;
 
 		spin_lock_irqsave(&mapping->tree_lock, flags);
 		if (page->mapping) {	/* Race with truncate? */
 			WARN_ON_ONCE(!PageUptodate(page));
-			account_page_dirtied(page, mapping);
 			radix_tree_tag_set(&mapping->page_tree,
 					page_index(page), PAGECACHE_TAG_DIRTY);
+			account = true;
 		}
 		spin_unlock_irqrestore(&mapping->tree_lock, flags);
+		if (account)
+			account_page_dirtied(page, mapping);
 	}
 	unlock_page_memcg(page);
 	if (newly_dirty)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index f4cd7d8005c9..9a6a6b99acfe 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2517,10 +2517,10 @@ int __set_page_dirty_nobuffers(struct page *page)
 		spin_lock_irqsave(&mapping->tree_lock, flags);
 		BUG_ON(page_mapping(page) != mapping);
 		WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
-		account_page_dirtied(page, mapping);
 		radix_tree_tag_set(&mapping->page_tree, page_index(page),
 				   PAGECACHE_TAG_DIRTY);
 		spin_unlock_irqrestore(&mapping->tree_lock, flags);
+		account_page_dirtied(page, mapping);
 		unlock_page_memcg(page);
 
 		if (mapping->host) {