writeback: avoid race when update bandwidth

From: Wanpeng Li <[email protected]>

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 |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/page-writeback.c +4 -2

index c8945e0..b3b08fb 100644
@@ -1032,12 +1032,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

Quoted 1 line · Wanpeng Li

From: Wanpeng Li <[email protected]>

That email address is no longer in use?

Quoted 6 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.

This looks good to me. wb.list_lock could be contended and it's better for bdi_update_bandwidth() to use a standalone and hardly contended lock.

btw, with this change, the dirty_lock in global_update_bandwidth() can be eliminated.

Quoted 29 lines

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

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

mm/page-writeback.c +4 -2

index c8945e0..b3b08fb 100644
@@ -1032,12 +1032,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

Quoted 4 lines · Fengguang Wu

Quoted 1 line · Wanpeng Li

From: Wanpeng Li <[email protected]>

That email address is no longer in use?

No, :), you better use "Wanpeng Li <[email protected]>" to commit my patch. Because next month this email address will be available again, and this also can help my colleagues to add my patches count this year, thanks a lot.

Quoted 1 line · Wanpeng Li

From: Wanpeng Li <[email protected]>

That email address is no longer in use?

Quoted 5 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.

This looks good to me. wb.list_lock could be contended and it's better for bdi_update_bandwidth() to use a standalone and hardly contended lock.

Quoted 5 lines · Fengguang Wu

Quoted 1 line · Wanpeng Li

From: Wanpeng Li <[email protected]>

That email address is no longer in use?

Quoted 5 lines

Quoted 5 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.

This is not a race condition - it is a lock contention condition.

Quoted 3 lines

This looks good to me. wb.list_lock could be contended and it's better for bdi_update_bandwidth() to use a standalone and hardly contended lock.

I'm not sure it will be "hardly contended". That's a global lock, so now we'll end up with updates on different bdis contending and it's not uncommon to see a couple of thousand processes on large machines beating on balance_dirty_pages(). Putting a global scope lock around such a function doesn't seem like a good solution to me.

Oh, and if you want to remove the dirty_lock from global_update_limit(), then replacing the lock with a cmpxchg loop will do it just fine....

Quoted 6 lines · Dave Chinner

Quoted 5 lines · Fengguang Wu

Quoted 1 line · Wanpeng Li

From: Wanpeng Li <[email protected]>

That email address is no longer in use?

Quoted 7 lines

Quoted 5 lines

Quoted 5 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.

This is not a race condition - it is a lock contention condition.

Nod.

Quoted 9 lines

Quoted 3 lines

This looks good to me. wb.list_lock could be contended and it's better for bdi_update_bandwidth() to use a standalone and hardly contended lock.

I'm not sure it will be "hardly contended". That's a global lock, so now we'll end up with updates on different bdis contending and it's not uncommon to see a couple of thousand processes on large machines beating on balance_dirty_pages(). Putting a global scope lock around such a function doesn't seem like a good solution to me.

It's more about the number of bdi's than the number of processes that matters. Because here is a per-bdi 200ms ratelimit:

bdi_update_bandwidth():

       if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL))
                return;         
       // lock it

So a global should be enough when there are only dozens of disks.

However, the global bandwidth_lock will probably become a problem when there comes hundreds of disks. If there are (or will be) such setups, I'm fine to revert to the old per-bdi locking.

Quoted 3 lines

Oh, and if you want to remove the dirty_lock from global_update_limit(), then replacing the lock with a cmpxchg loop will do it just fine....

Yes. But to be frank, I don't care about that dirty_lock at all, because it has its own 200ms rate limiting :-)

Quoted 32 lines · Fengguang Wu

Quoted 6 lines · Dave Chinner

Quoted 5 lines · Fengguang Wu

Quoted 1 line · Wanpeng Li

From: Wanpeng Li <[email protected]>

That email address is no longer in use?

Quoted 7 lines

Quoted 5 lines

Quoted 5 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.

This is not a race condition - it is a lock contention condition.

Nod.

Quoted 9 lines

Quoted 3 lines

This looks good to me. wb.list_lock could be contended and it's better for bdi_update_bandwidth() to use a standalone and hardly contended lock.

I'm not sure it will be "hardly contended". That's a global lock, so now we'll end up with updates on different bdis contending and it's not uncommon to see a couple of thousand processes on large machines beating on balance_dirty_pages(). Putting a global scope lock around such a function doesn't seem like a good solution to me.

It's more about the number of bdi's than the number of processes that matters. Because here is a per-bdi 200ms ratelimit:

bdi_update_bandwidth():

Quoted 3 lines

       if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL))
                return;         
       // lock it

So now you get a thousand processes on a thousand CPUs all hit that case at the same time because they are all writing to disk at the same time, all nicely synchronised by MPI. Lock contention ahoy!

Quoted 1 line

So a global should be enough when there are only dozens of disks.

Only needs one bdi, just with lots of processes trying to hit it at the same time such that they all pass the time after check.

Quoted 3 lines

However, the global bandwidth_lock will probably become a problem when there comes hundreds of disks. If there are (or will be) such setups, I'm fine to revert to the old per-bdi locking.

There are setups with hundreds of disks. They also tend to have hundreds of CPUs, too....

Quoted 6 lines

Quoted 3 lines

Oh, and if you want to remove the dirty_lock from global_update_limit(), then replacing the lock with a cmpxchg loop will do it just fine....

Yes. But to be frank, I don't care about that dirty_lock at all, because it has its own 200ms rate limiting :-)

That has the same problem, only it's currently nested inside another lock which isolates it from contention. This is why measurement is important - until there is that evidence shows that the lock contention is a problem, don't change it because it generally has a unpredictable cascading effect that often results in worse contention that was there originally....