writeback: avoid race when update bandwidth

PATCH In review Wanpeng Li 3 participants 9 messages Jun 12, 2012 – Jun 15, 2012

Subsystems (2)

Memory management Page cache

lore

Share & tools

lore

Wanpeng Li

author

2012-06-12 11:46 UTC #0 lore raw

PATCH v2 1/1 writeback: avoid race when update bandwidth

From: Wanpeng Li <[email protected]>

"V1 -> V2"
* remove dirty_lock

Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.

Signed-off-by Wanpeng Li <[email protected]>

---
 mm/page-writeback.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

mm/page-writeback.c +4 -5

index c833bf0..e28d36e 100644
@@ -815,7 +815,6 @@ static void global_update_bandwidth(unsigned long thresh,
 				    unsigned long dirty,
 				    unsigned long now)
 {
-	static DEFINE_SPINLOCK(dirty_lock);
 	static unsigned long update_time;
 
 	/*

@@ -824,12 +823,10 @@ static void global_update_bandwidth(unsigned long thresh,
 	if (time_before(now, update_time + BANDWIDTH_INTERVAL))
 		return;
 
-	spin_lock(&dirty_lock);
 	if (time_after_eq(now, update_time + BANDWIDTH_INTERVAL)) {
 		update_dirty_limit(thresh, dirty);
 		update_time = now;
 	}
-	spin_unlock(&dirty_lock);
 }
 
 /*

@@ -1032,12 +1029,14 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi,
 				 unsigned long bdi_dirty,
 				 unsigned long start_time)
 {
+	static DEFINE_SPINLOCK(bandwidth_lock);
+
 	if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL))
 		return;
-	spin_lock(&bdi->wb.list_lock);
+	spin_lock(&bandwidth_lock);
 	__bdi_update_bandwidth(bdi, thresh, bg_thresh, dirty,
 			       bdi_thresh, bdi_dirty, start_time);
-	spin_unlock(&bdi->wb.list_lock);
+	spin_unlock(&bandwidth_lock);
 }
 
 /*

Signature · 1 line

1.7.9.5

Fengguang Wu

2012-06-12 11:52 UTC #1 lore raw

Quoted 5 lines · Wanpeng Li

From: Wanpeng Li <[email protected]>
"V1 -> V2"
* remove dirty_lock

Quoted 7 lines

Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.
Signed-off-by Wanpeng Li <[email protected]>

Applied with a new title "writeback: use a standalone lock for updating write bandwidth". "race" is sensitive because it often refers to some locking error.

Thank you!

Fengguang

Wanpeng Li

author

2012-06-12 11:58 UTC #2 lore raw

Quoted 12 lines · Fengguang Wu

Quoted 11 lines · Wanpeng Li
From: Wanpeng Li <[email protected]>
"V1 -> V2"
* remove dirty_lock
Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.

Quoted 5 lines

Quoted 1 line
Signed-off-by Wanpeng Li <[email protected]>
Applied with a new title "writeback: use a standalone lock for updating write bandwidth". "race" is sensitive because it often refers to some locking error.

OK, Thanks a lot.

Regards,
Wanpeng Li

Dave Chinner

2012-06-13 03:59 UTC #3 lore raw

In reply to Fengguang Wu

Quoted 12 lines · Fengguang Wu

Quoted 11 lines · Wanpeng Li
From: Wanpeng Li <[email protected]>
"V1 -> V2"
* remove dirty_lock
Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.

Quoted 5 lines

Quoted 1 line
Signed-off-by Wanpeng Li <[email protected]>
Applied with a new title "writeback: use a standalone lock for updating write bandwidth". "race" is sensitive because it often refers to some locking error.

Fengguang - can we get some evidence that this is a contended lock before changing the scope of it? All of the previous "breaking up global locks" have been done based on lock contention data, so moving back to a global lock for this needs to have the same analysis provided...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Fengguang Wu

2012-06-13 12:14 UTC #4 lore raw

Quoted 19 lines · Dave Chinner

Quoted 12 lines · Fengguang Wu
Quoted 11 lines · Wanpeng Li
From: Wanpeng Li <[email protected]>
"V1 -> V2"
* remove dirty_lock
Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.
Quoted 5 lines
Quoted 1 line
Signed-off-by Wanpeng Li <[email protected]>
Applied with a new title "writeback: use a standalone lock for updating write bandwidth". "race" is sensitive because it often refers to some locking error.

Quoted 5 lines

Fengguang - can we get some evidence that this is a contended lock before changing the scope of it? All of the previous "breaking up global locks" have been done based on lock contention data, so moving back to a global lock for this needs to have the same analysis provided...

Good point. Attached is the lockstat for the case "10 disks each runs 100 dd dirtier tasks":

        lkp-ne02/JBOD-10HDD-thresh=4G/xfs-100dd-1-3.2.0-rc5

The wb->list_lock contention is much better than I expected, which is good. What stand out are
waittime-total

- &rq->lock             by double_rq_lock()             6738952.13
- clockevents_lock      by clockevents_notify()         2155554.37
- mapping->tree_lock    by test_clear_page_writeback()   931550.13
- sb_lock               by grab_super_passive()          918815.87
- &zone->lru_lock       by pagevec_lru_move_fn()         912681.05

- sysfs_mutex           by sysfs_permission()           24029975.20 # mutex
- ip->i_lock            by xfs_ilock()                  18428284.10 # mrlock

Thanks,
Fengguang

Dave Chinner

2012-06-14 02:06 UTC #5 lore raw

Quoted 26 lines · Fengguang Wu

Quoted 19 lines · Dave Chinner
Quoted 12 lines · Fengguang Wu
Quoted 11 lines · Wanpeng Li
From: Wanpeng Li <[email protected]>
"V1 -> V2"
* remove dirty_lock
Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.
Quoted 5 lines
Quoted 1 line
Signed-off-by Wanpeng Li <[email protected]>
Applied with a new title "writeback: use a standalone lock for updating write bandwidth". "race" is sensitive because it often refers to some locking error.
Quoted 5 lines
Fengguang - can we get some evidence that this is a contended lock before changing the scope of it? All of the previous "breaking up global locks" have been done based on lock contention data, so moving back to a global lock for this needs to have the same analysis provided...

Quoted 4 lines

Good point. Attached is the lockstat for the case "10 disks each runs 100 dd dirtier tasks":
        lkp-ne02/JBOD-10HDD-thresh=4G/xfs-100dd-1-3.2.0-rc5

(nothing attached)

Quoted 11 lines

The wb->list_lock contention is much better than I expected, which is good. What stand out are
waittime-total

- &rq->lock             by double_rq_lock()             6738952.13
- clockevents_lock      by clockevents_notify()         2155554.37
- mapping->tree_lock    by test_clear_page_writeback()   931550.13
- sb_lock               by grab_super_passive()          918815.87
- &zone->lru_lock       by pagevec_lru_move_fn()         912681.05

- sysfs_mutex           by sysfs_permission()           24029975.20 # mutex
- ip->i_lock            by xfs_ilock()                  18428284.10 # mrlock

The wait time is not really an indication of contention problems. Large wait time is usually an indication that the lock is being used a lot.

What matters is the number of contentions vs the number of acquisitions, and the number of those contentions that bounced the lock. If the number of contentions is >= 0.5% of the acquisitions, then the lock can be considered hot and needing some work. If I look here:

http://lists.linux.hp.com/~enw/ext4/3.2/3.2-full-lockstats.2/ffsb_fsscale.xfs.large_file_creates_threads=192/profiling/iteration.1/lock_stat

Which is a 192 thread concurrent write on a 48-core machine, the wb.list_lock shows 5,532 acquistions for the entire test, while the mapping tree lock took 440 million!. So your test isn't really one that shows wb.list_lock contention. The 192-thread mailserver workload from the same machine:

http://lists.linux.hp.com/~enw/ext4/3.2/3.2-full-lockstats.2/ffsb_fsscale.xfs.mail_server_threads=192/profiling/iteration.1/lock_stat

Shows about 7.1m acquisitions of the wb.list_lock, but only 28,000 contentions. So it isn't really contended enough to justify replacing it with a global lock.

FWIW, the third most contended lock on that workload is the XFS delayed write queue lock - 25M acquisitions for 600k contentions - a rate of about 2% which means quite severe contention. That lock no longer exists in 3.5 - Christoph completely reworked the delayed write buffer support to remove the global list and lock because it was showing up in profiles like this...

Indeed, that profile shows that XFS owns 7 of the 10 most contended locks, and 3 of them have had significant work done to reduce the contention since 3.2 as a result of recent profile results like this.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Fengguang Wu

2012-06-14 14:00 UTC #6 lore raw

Quoted 43 lines · Dave Chinner

Quoted 26 lines · Fengguang Wu
Quoted 19 lines · Dave Chinner
Quoted 12 lines · Fengguang Wu
Quoted 11 lines · Wanpeng Li
From: Wanpeng Li <[email protected]>
"V1 -> V2"
* remove dirty_lock
Since bdi->wb.list_lock is used to protect the b_* lists, so the flushers who call wb_writeback to writeback pages will stuck when bandwidth update policy holds this lock. In order to avoid this race we can introduce a new bandwidth_lock who is responsible for protecting bandwidth update policy.
Quoted 5 lines
Quoted 1 line
Signed-off-by Wanpeng Li <[email protected]>
Applied with a new title "writeback: use a standalone lock for updating write bandwidth". "race" is sensitive because it often refers to some locking error.
Quoted 5 lines
Fengguang - can we get some evidence that this is a contended lock before changing the scope of it? All of the previous "breaking up global locks" have been done based on lock contention data, so moving back to a global lock for this needs to have the same analysis provided...
Quoted 4 lines
Good point. Attached is the lockstat for the case "10 disks each runs 100 dd dirtier tasks":
        lkp-ne02/JBOD-10HDD-thresh=4G/xfs-100dd-1-3.2.0-rc5
(nothing attached)
Quoted 9 lines
The wb->list_lock contention is much better than I expected, which is good. What stand out are
waittime-total
- &rq->lock             by double_rq_lock()             6738952.13
- clockevents_lock      by clockevents_notify()         2155554.37
- mapping->tree_lock    by test_clear_page_writeback()   931550.13
- sb_lock               by grab_super_passive()          918815.87
- &zone->lru_lock       by pagevec_lru_move_fn()         912681.05

Quoted 6 lines

Quoted 2 lines
- sysfs_mutex           by sysfs_permission()           24029975.20 # mutex
- ip->i_lock            by xfs_ilock()                  18428284.10 # mrlock
The wait time is not really an indication of contention problems. Large wait time is usually an indication that the lock is being used a lot.

Right.

Quoted 5 lines

What matters is the number of contentions vs the number of acquisitions, and the number of those contentions that bounced the lock. If the number of contentions is >= 0.5% of the acquisitions, then the lock can be considered hot and needing some work. If I look here:

I wonder if anyone has a simple script for sorting lock_stat output based on that (and perhaps other selectable) criterion? It should be possible to write on myself, but still.. ;-)

Default lock_stat output is sorted by absolute number of contentions.

Quoted 8 lines

http://lists.linux.hp.com/~enw/ext4/3.2/3.2-full-lockstats.2/ffsb_fsscale.xfs.large_file_creates_threads=192/profiling/iteration.1/lock_stat
Which is a 192 thread concurrent write on a 48-core machine, the wb.list_lock shows 5,532 acquistions for the entire test, while the mapping tree lock took 440 million!. So your test isn't really one that shows wb.list_lock contention. The 192-thread mailserver workload from the same machine:

Quoted 5 lines

http://lists.linux.hp.com/~enw/ext4/3.2/3.2-full-lockstats.2/ffsb_fsscale.xfs.mail_server_threads=192/profiling/iteration.1/lock_stat
Shows about 7.1m acquisitions of the wb.list_lock, but only 28,000 contentions. So it isn't really contended enough to justify replacing it with a global lock.

Right.

Quoted 10 lines

FWIW, the third most contended lock on that workload is the XFS delayed write queue lock - 25M acquisitions for 600k contentions - a rate of about 2% which means quite severe contention. That lock no longer exists in 3.5 - Christoph completely reworked the delayed write buffer support to remove the global list and lock because it was showing up in profiles like this...
Indeed, that profile shows that XFS owns 7 of the 10 most contended locks, and 3 of them have had significant work done to reduce the contention since 3.2 as a result of recent profile results like this.

Nice work!

Thanks,
Fengguang

Dave Chinner

2012-06-15 00:06 UTC #7 lore raw

Quoted 7 lines · Fengguang Wu

Quoted 1 line · Dave Chinner
On Wed, Jun 13, 2012 at 08:14:34PM +0800, Fengguang Wu wrote:
I wonder if anyone has a simple script for sorting lock_stat output based on that (and perhaps other selectable) criterion? It should be possible to write on myself, but still.. ;-)
Default lock_stat output is sorted by absolute number of contentions.

No that I know of. The default is pretty sane, because a highly contended lock that is causing performance problems will always show up near the top. If it's not in the top 10, then it's usually not worth worrying about until you've dealt with the those above it....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Fengguang Wu

2012-06-15 00:30 UTC #8 lore raw

Quoted 7 lines · Dave Chinner

Quoted 6 lines · Fengguang Wu
Quoted 1 line · Dave Chinner
On Wed, Jun 13, 2012 at 08:14:34PM +0800, Fengguang Wu wrote:
I wonder if anyone has a simple script for sorting lock_stat output based on that (and perhaps other selectable) criterion? It should be possible to write on myself, but still.. ;-)

Quoted 6 lines

Quoted 1 line
Default lock_stat output is sorted by absolute number of contentions.
No that I know of. The default is pretty sane, because a highly contended lock that is causing performance problems will always show up near the top. If it's not in the top 10, then it's usually not worth worrying about until you've dealt with the those above it....

Okay, thanks for the tip!

Thanks,
Fengguang