MD: improve raid1/10 write performance for fast storage

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If no bitmap, there is no point to queue bio to a thread and dispatch it in the thread. Directly dispatching bio doesn't impact correctness and removes above bottleneck.

Multiple threads dispatch requests could potentially reduce request merge and increase lock contention. For slow stroage, we just worry about request merge. Caller of .make_request should already have correct block plug set, which will take care of request merge and locking just like accessing raw device, so we don't need worry about this too much.

In a 4k randwrite test with a 2 disks setup, below patch can provide 20% ~ 50% performance improvements depending on numa binding.

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

---
 drivers/md/raid1.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Index: linux/drivers/md/raid1.c
===================================================================

drivers/md/raid1.c +0 -4

@@ -1187,10 +1187,13 @@ read_again:
 		mbio->bi_private = r1_bio;
 
 		atomic_inc(&r1_bio->remaining);
-		spin_lock_irqsave(&conf->device_lock, flags);
-		bio_list_add(&conf->pending_bio_list, mbio);
-		conf->pending_count++;
-		spin_unlock_irqrestore(&conf->device_lock, flags);

Quoted 36 lines · On Wed, 23 May 2012 15:26:20 +0800 Shaohua Li <[email protected]> wrote:

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If no bitmap, there is no point to queue bio to a thread and dispatch it in the thread. Directly dispatching bio doesn't impact correctness and removes above bottleneck.

Multiple threads dispatch requests could potentially reduce request merge and increase lock contention. For slow stroage, we just worry about request merge. Caller of .make_request should already have correct block plug set, which will take care of request merge and locking just like accessing raw device, so we don't need worry about this too much.

In a 4k randwrite test with a 2 disks setup, below patch can provide 20% ~ 50% performance improvements depending on numa binding.

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

---
 drivers/md/raid1.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Index: linux/drivers/md/raid1.c
===================================================================

drivers/md/raid1.c +0 -4

@@ -1187,10 +1187,13 @@ read_again:
 		mbio->bi_private = r1_bio;
 
 		atomic_inc(&r1_bio->remaining);
-		spin_lock_irqsave(&conf->device_lock, flags);
-		bio_list_add(&conf->pending_bio_list, mbio);
-		conf->pending_count++;
-		spin_unlock_irqrestore(&conf->device_lock, flags);

drivers/md/raid1.c read_again: · line 1199

Quoted 10 lines

drivers/md/raid1.c +7 -0

+		if (bitmap) {
+			spin_lock_irqsave(&conf->device_lock, flags);
+			bio_list_add(&conf->pending_bio_list, mbio);
+			conf->pending_count++;
+			spin_unlock_irqrestore(&conf->device_lock, flags);
+		} else
+			generic_make_request(mbio);
 	}
 	/* Mustn't call r1_bio_write_done before this next test,
 	 * as it could result in the bio being freed.

This looks like it should be 'obviously correct' but unfortunately it isn't.

If this raid1 is beneath a dm device (e.g. LVM), then generic_make_request will queue the request internally and not actually start it.
In particular this means that the cloned bio has no chance of being released before the next clone_bio call which is made on the next time around the loop.

This can lead to a deadlock as the clone_bio() might be waiting for that first cloned bio to be released (if memory is really tight).

i.e. when allocating multiple bios from a mempool, we need to arrange for them to be submitted by a separate thread.

Quoted 56 lines · NeilBrown

Quoted 36 lines · On Wed, 23 May 2012 15:26:20 +0800 Shaohua Li <[email protected]> wrote:

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If no bitmap, there is no point to queue bio to a thread and dispatch it in the thread. Directly dispatching bio doesn't impact correctness and removes above bottleneck.

Multiple threads dispatch requests could potentially reduce request merge and increase lock contention. For slow stroage, we just worry about request merge. Caller of .make_request should already have correct block plug set, which will take care of request merge and locking just like accessing raw device, so we don't need worry about this too much.

In a 4k randwrite test with a 2 disks setup, below patch can provide 20% ~ 50% performance improvements depending on numa binding.

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

---
 drivers/md/raid1.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Index: linux/drivers/md/raid1.c
===================================================================

drivers/md/raid1.c +0 -4

@@ -1187,10 +1187,13 @@ read_again:
 		mbio->bi_private = r1_bio;
 
 		atomic_inc(&r1_bio->remaining);
-		spin_lock_irqsave(&conf->device_lock, flags);
-		bio_list_add(&conf->pending_bio_list, mbio);
-		conf->pending_count++;
-		spin_unlock_irqrestore(&conf->device_lock, flags);

drivers/md/raid1.c read_again: · line 1199

Quoted 10 lines

drivers/md/raid1.c +7 -0

+		if (bitmap) {
+			spin_lock_irqsave(&conf->device_lock, flags);
+			bio_list_add(&conf->pending_bio_list, mbio);
+			conf->pending_count++;
+			spin_unlock_irqrestore(&conf->device_lock, flags);
+		} else
+			generic_make_request(mbio);
 	}
 	/* Mustn't call r1_bio_write_done before this next test,
 	 * as it could result in the bio being freed.

This looks like it should be 'obviously correct' but unfortunately it isn't.

If this raid1 is beneath a dm device (e.g. LVM), then generic_make_request will queue the request internally and not actually start it.
In particular this means that the cloned bio has no chance of being released before the next clone_bio call which is made on the next time around the loop.

Quoted 5 lines

This can lead to a deadlock as the clone_bio() might be waiting for that first cloned bio to be released (if memory is really tight).

i.e. when allocating multiple bios from a mempool, we need to arrange for them to be submitted by a separate thread.

If there isn't block plug, generic_make_request will dispatch the request directly.

sleep/schedule, which will trigger block unplug and dispatch the first bio, so no deadlock to me. Am I misunderstanding you?

Thanks,
Shaohua

Quoted 63 lines · On Thu, 24 May 2012 10:54:25 +0800 Shaohua Li <[email protected]> wrote:

Quoted 61 lines · NeilBrown

Quoted 36 lines · On Wed, 23 May 2012 15:26:20 +0800 Shaohua Li <[email protected]> wrote:

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If no bitmap, there is no point to queue bio to a thread and dispatch it in the thread. Directly dispatching bio doesn't impact correctness and removes above bottleneck.

Multiple threads dispatch requests could potentially reduce request merge and increase lock contention. For slow stroage, we just worry about request merge. Caller of .make_request should already have correct block plug set, which will take care of request merge and locking just like accessing raw device, so we don't need worry about this too much.

In a 4k randwrite test with a 2 disks setup, below patch can provide 20% ~ 50% performance improvements depending on numa binding.

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

---
 drivers/md/raid1.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Index: linux/drivers/md/raid1.c
===================================================================

drivers/md/raid1.c +0 -4

@@ -1187,10 +1187,13 @@ read_again:
 		mbio->bi_private = r1_bio;
 
 		atomic_inc(&r1_bio->remaining);
-		spin_lock_irqsave(&conf->device_lock, flags);
-		bio_list_add(&conf->pending_bio_list, mbio);
-		conf->pending_count++;
-		spin_unlock_irqrestore(&conf->device_lock, flags);

drivers/md/raid1.c read_again: · line 1199

Quoted 10 lines

drivers/md/raid1.c +7 -0

+		if (bitmap) {
+			spin_lock_irqsave(&conf->device_lock, flags);
+			bio_list_add(&conf->pending_bio_list, mbio);
+			conf->pending_count++;
+			spin_unlock_irqrestore(&conf->device_lock, flags);
+		} else
+			generic_make_request(mbio);
 	}
 	/* Mustn't call r1_bio_write_done before this next test,
 	 * as it could result in the bio being freed.

This looks like it should be 'obviously correct' but unfortunately it isn't.

If this raid1 is beneath a dm device (e.g. LVM), then generic_make_request will queue the request internally and not actually start it.
In particular this means that the cloned bio has no chance of being released before the next clone_bio call which is made on the next time around the loop.

This can lead to a deadlock as the clone_bio() might be waiting for that first cloned bio to be released (if memory is really tight).

i.e. when allocating multiple bios from a mempool, we need to arrange for them to be submitted by a separate thread.

Quoted 2 lines

If there isn't block plug, generic_make_request will dispatch the request directly.

Not necessarily. It might queue it:

	if (current->bio_list) {
		bio_list_add(current->bio_list, bio);
		return;
	}

Quoted 5 lines · NeilBrown

Not necessarily. It might queue it:

	if (current->bio_list) {
		bio_list_add(current->bio_list, bio);
		return;
	}

Ah, yes. Looks we can workaround it to increase bioset entry number to raid_disks*2. There isn't too much memory wasted with it.

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If bitmap support is enabled, write requests can only be dispatched after dirty bitmap is flushed out. After bitmap is flushed, how write requests are dispatched doesn't impact correctness. A natural idea is to distribute request dispatch to several threads. With this patch, requests are added to a percpu list first. After bitmap is flushed, then the percpu list requests will dispatched in a workqueue. In this way, above bottleneck is removed.

In a 4k randwrite test with a 2 disks setup, below patch can provide 10% ~ 50% performance improvements depending on numa binding.

Quoted 13 lines · On Wed, 23 May 2012 15:26:21 +0800 Shaohua Li <[email protected]> wrote:

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If bitmap support is enabled, write requests can only be dispatched after dirty bitmap is flushed out. After bitmap is flushed, how write requests are dispatched doesn't impact correctness. A natural idea is to distribute request dispatch to several threads. With this patch, requests are added to a percpu list first. After bitmap is flushed, then the percpu list requests will dispatched in a workqueue. In this way, above bottleneck is removed.

Quoted 2 lines

In a 4k randwrite test with a 2 disks setup, below patch can provide 10% ~ 50% performance improvements depending on numa binding.

Those numbers are quite impressive so there is certainly room for improvement here. I'm not sure that I'm entirely comfortable with the approach though.

Passing the request to a per-cpu thread does make sense, but a generic per-cpu thread feels dangerous as we don't know what else might be queued to that thread and because of the potential for deadlocks between memory allocation and generic_make_request (as mentioned in previous email) I find it hard to convince myself that this approach is entirely safe.

I wonder if we might take a very different approach and try to do everything in the one process. i.e. don't hand tasks off to other threads at all - at least in the common case.
So we could change plugger_unplug (in md.c) so that:

- if current->bio_list is NULL, (meaning all requests have been submitted and there is no risk of deadlock) we call bitmap_unplug and then submit all the queued writes.
- if current->bio_list is not NULL, then we just wakeup the md thread to do the work.

Quoted 21 lines · NeilBrown

Quoted 13 lines · On Wed, 23 May 2012 15:26:21 +0800 Shaohua Li <[email protected]> wrote:

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If bitmap support is enabled, write requests can only be dispatched after dirty bitmap is flushed out. After bitmap is flushed, how write requests are dispatched doesn't impact correctness. A natural idea is to distribute request dispatch to several threads. With this patch, requests are added to a percpu list first. After bitmap is flushed, then the percpu list requests will dispatched in a workqueue. In this way, above bottleneck is removed.

Quoted 2 lines

In a 4k randwrite test with a 2 disks setup, below patch can provide 10% ~ 50% performance improvements depending on numa binding.

Those numbers are quite impressive so there is certainly room for improvement here. I'm not sure that I'm entirely comfortable with the approach though.

Quoted 5 lines

Passing the request to a per-cpu thread does make sense, but a generic per-cpu thread feels dangerous as we don't know what else might be queued to that thread and because of the potential for deadlocks between memory allocation and generic_make_request (as mentioned in previous email) I find it hard to convince myself that this approach is entirely safe.

No problem, we can use a separate workqueue.

Quoted 11 lines

I wonder if we might take a very different approach and try to do everything in the one process. i.e. don't hand tasks off to other threads at all - at least in the common case.
So we could change plugger_unplug (in md.c) so that:

- if current->bio_list is NULL, (meaning all requests have been submitted and there is no risk of deadlock) we call bitmap_unplug and then submit all the queued writes.
- if current->bio_list is not NULL, then we just wakeup the md thread to do the work.

The current->bio_list check does make sense. I'm going to do the check for the 1 and 3 patches.

But I believe we can't call generic_make_request in unplug. For example, schedule()->unplug->generic_make_request->get_request_wait()->schedule(). At

least this will cause some request not dispatch. And last time I did similar experiment for raid0 (request is added to a per-disk plug_cb list, not directly dispatch) to reduce lock contention, I found nasty oops.

Thanks,
Shaohua

Quoted 28 lines · On Thu, 24 May 2012 13:17:09 +0800 Shaohua Li <[email protected]> wrote:

Quoted 26 lines · NeilBrown

Quoted 13 lines · On Wed, 23 May 2012 15:26:21 +0800 Shaohua Li <[email protected]> wrote:

In raid1, all write requests are dispatched in raid1d thread. In fast storage, the raid1d thread is a bottleneck, because it dispatches request too slow. Also raid1d thread migrates freely, which makes request completion cpu not match with submission cpu even driver/block layer has such capability. This will cause bad cache issue.

If bitmap support is enabled, write requests can only be dispatched after dirty bitmap is flushed out. After bitmap is flushed, how write requests are dispatched doesn't impact correctness. A natural idea is to distribute request dispatch to several threads. With this patch, requests are added to a percpu list first. After bitmap is flushed, then the percpu list requests will dispatched in a workqueue. In this way, above bottleneck is removed.

Quoted 2 lines

In a 4k randwrite test with a 2 disks setup, below patch can provide 10% ~ 50% performance improvements depending on numa binding.

Those numbers are quite impressive so there is certainly room for improvement here. I'm not sure that I'm entirely comfortable with the approach though.

Passing the request to a per-cpu thread does make sense, but a generic per-cpu thread feels dangerous as we don't know what else might be queued to that thread and because of the potential for deadlocks between memory allocation and generic_make_request (as mentioned in previous email) I find it hard to convince myself that this approach is entirely safe.

Quoted 1 line

No problem, we can use a separate workqueue.

A separate set of per-cpu workqueues for each md array doesn't sound like a good idea to me - if we can possibly avoid it.

Quoted 14 lines

Quoted 10 lines

I wonder if we might take a very different approach and try to do everything in the one process. i.e. don't hand tasks off to other threads at all - at least in the common case.
So we could change plugger_unplug (in md.c) so that:

- if current->bio_list is NULL, (meaning all requests have been submitted and there is no risk of deadlock) we call bitmap_unplug and then submit all the queued writes.
- if current->bio_list is not NULL, then we just wakeup the md thread to do the work.

The current->bio_list check does make sense. I'm going to do the check for the 1 and 3 patches.

Quoted 2 lines

But I believe we can't call generic_make_request in unplug. For example, schedule()->unplug->generic_make_request->get_request_wait()->schedule(). At

My ideas was to only submit the queued writes from a normal call to blk_finish_plug. Calls that come from schedule would use the current approach of signalling the md thread to take over. There must be a way to tell the difference - maybe just a new flag to the unplug callback.