If I read code correctly, current wait_for_stripe actually has 2 usage:
- wait for there is enough free stripe cache, triggered when
get_free_stripe() failed. This is what wait_for_stripe intend
for literally.
- wait for quiesce == 0 or
active_aligned_reads == 0 && active_stripes == 0
It has nothing to do with wait_for_stripe literally, and releasing
an active stripe won't actually wake them up. On the contrary, wake_up
from under this case won't actually wake up the process waiting for
an free stripe being available.
Hence, we'd better split wait_for_stripe, and here I introduce
wait_for_quiesce for the second usage. The name may not well taken, or
even taken wrongly. Feel free to correct me then.
This is also a prepare patch for next patch: make wait_for_stripe
exclusive.
Signed-off-by: Yuanhan Liu <[email protected]>
---
drivers/md/raid5.c | 13 +++++++------
drivers/md/raid5.h | 1 +
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 9716319..b7e385f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
spin_lock_irq(conf->hash_locks + hash);
do {
- wait_event_lock_irq(conf->wait_for_stripe,
+ wait_event_lock_irq(conf->wait_for_quiesce,
conf->quiesce == 0 || noquiesce,
*(conf->hash_locks + hash));
sh = __find_stripe(conf, sector, conf->generation - previous);
@@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
raid_bi, 0);
bio_endio(raid_bi, 0);
if (atomic_dec_and_test(&conf->active_aligned_reads))
- wake_up(&conf->wait_for_stripe);
+ wake_up(&conf->wait_for_quiesce);
return;
}
@@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
align_bi->bi_iter.bi_sector += rdev->data_offset;
spin_lock_irq(&conf->device_lock);
- wait_event_lock_irq(conf->wait_for_stripe,
+ wait_event_lock_irq(conf->wait_for_quiesce,
conf->quiesce == 0,
conf->device_lock);
atomic_inc(&conf->active_aligned_reads);
@@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
bio_endio(raid_bio, 0);
}
if (atomic_dec_and_test(&conf->active_aligned_reads))
- wake_up(&conf->wait_for_stripe);
+ wake_up(&conf->wait_for_quiesce);
return handled;
}
@@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
goto abort;
spin_lock_init(&conf->device_lock);
seqcount_init(&conf->gen_lock);
+ init_waitqueue_head(&conf->wait_for_quiesce);
init_waitqueue_head(&conf->wait_for_stripe);
init_waitqueue_head(&conf->wait_for_overlap);
INIT_LIST_HEAD(&conf->handle_list);
@@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
* active stripes can drain
*/
conf->quiesce = 2;
- wait_event_cmd(conf->wait_for_stripe,
+ wait_event_cmd(conf->wait_for_quiesce,
atomic_read(&conf->active_stripes) == 0 &&
atomic_read(&conf->active_aligned_reads) == 0,
unlock_all_device_hash_locks_irq(conf),
@@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
case 0: /* re-enable writes */
lock_all_device_hash_locks_irq(conf);
conf->quiesce = 0;
- wake_up(&conf->wait_for_stripe);
+ wake_up(&conf->wait_for_quiesce);
wake_up(&conf->wait_for_overlap);
unlock_all_device_hash_locks_irq(conf);
break;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 7dc0dd8..fab53a3 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -508,6 +508,7 @@ struct r5conf {
struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
atomic_t empty_inactive_list_nr;
struct llist_head released_stripes;
+ wait_queue_head_t wait_for_quiesce;
wait_queue_head_t wait_for_stripe;
wait_queue_head_t wait_for_overlap;
unsigned long cache_state;
--
1.9.0
I noticed heavy spin lock contention at get_active_stripe() with fsmark
multiple thread write workloads.
Here is how this hot contention comes from. We have limited stripes, and
it's a multiple thread write workload. Hence, those stripes will be taken
soon, which puts later processes to sleep for waiting free stripes. When
enough stripes(> 1/4 total stripes) are released, all process are woken,
trying to get the lock. But there is one only being able to get this lock
for each hash lock, making other processes spinning out there for acquiring
the lock.
Thus, it's effectiveless to wakeup all processes and let them battle for
a lock that permits one to access only each time. Instead, we could make
it be a exclusive wake up: wake up one process only. That avoids the heavy
spin lock contention naturally.
Here are some test results I have got with this patch applied(all test run
3 times):
`fsmark.files_per_sec'
=====================
next-20150317 this patch
------------------------- -------------------------
metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
------------------------- ------------------------- -------- ------------------------------
25.600 ±0.0 92.700 ±2.5 262.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
25.600 ±0.0 77.800 ±0.6 203.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
32.000 ±0.0 93.800 ±1.7 193.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
32.000 ±0.0 81.233 ±1.7 153.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
48.800 ±14.5 99.667 ±2.0 104.2% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
6.400 ±0.0 12.800 ±0.0 100.0% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
63.133 ±8.2 82.800 ±0.7 31.2% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
245.067 ±0.7 306.567 ±7.9 25.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-f2fs-4M-30G-fsyncBeforeClose
17.533 ±0.3 21.000 ±0.8 19.8% ivb44/fsmark/1x-1t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
188.167 ±1.9 215.033 ±3.1 14.3% ivb44/fsmark/1x-1t-4BRD_12G-RAID5-btrfs-4M-30G-NoSync
254.500 ±1.8 290.733 ±2.4 14.2% ivb44/fsmark/1x-1t-9BRD_6G-RAID5-btrfs-4M-30G-NoSync
`time.system_time'
=====================
next-20150317 this patch
------------------------- -------------------------
metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
------------------------- ------------------------- -------- ------------------------------
7235.603 ±1.2 185.163 ±1.9 -97.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
7666.883 ±2.9 202.750 ±1.0 -97.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
14567.893 ±0.7 421.230 ±0.4 -97.1% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
3697.667 ±14.0 148.190 ±1.7 -96.0% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
5572.867 ±3.8 310.717 ±1.4 -94.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
5565.050 ±0.5 313.277 ±1.5 -94.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
2420.707 ±17.1 171.043 ±2.7 -92.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
3743.300 ±4.6 379.827 ±3.5 -89.9% ivb44/fsmark/1x-64t-3HDD-RAID5-ext4-4M-40G-fsyncBeforeClose
3308.687 ±6.3 363.050 ±2.0 -89.0% ivb44/fsmark/1x-64t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
Where,
1x: where 'x' means iterations or loop, corresponding to the 'L' option of fsmark
1t, 64t: where 't' means thread
4M: means the single file size, corresponding to the '-s' option of fsmark
40G, 30G, 120G: means the total test size
4BRD_12G: BRD is the ramdisk, where '4' means 4 ramdisk, and where '12G' means
the size of one ramdisk. So, it would be 48G in total. And we made a
raid on those ramdisk
As you can see, though there are no much performance gain for hard disk
workload, the system time is dropped heavily, up to 97%. And as expected,
the performance increased a lot, up to 260%, for fast device(ram disk).
Signed-off-by: Yuanhan Liu <[email protected]>
---
drivers/md/raid5.c | 46 +++++++++++++++++++++++++++++++++++-----------
drivers/md/raid5.h | 2 +-
2 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index b7e385f..2d8fcc1 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -344,7 +344,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
int hash)
{
int size;
- bool do_wakeup = false;
+ bool do_wakeup[NR_STRIPE_HASH_LOCKS] = { false, };
+ int i = 0;
unsigned long flags;
if (hash == NR_STRIPE_HASH_LOCKS) {
@@ -365,17 +366,22 @@ static void release_inactive_stripe_list(struct r5conf *conf,
!list_empty(list))
atomic_dec(&conf->empty_inactive_list_nr);
list_splice_tail_init(list, conf->inactive_list + hash);
- do_wakeup = true;
+ do_wakeup[size - 1] = true;
spin_unlock_irqrestore(conf->hash_locks + hash, flags);
}
size--;
hash--;
}
- if (do_wakeup) {
- wake_up(&conf->wait_for_stripe);
- if (conf->retry_read_aligned)
- md_wakeup_thread(conf->mddev->thread);
+ for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
+ bool waked_thread = false;
+ if (do_wakeup[i]) {
+ wake_up(&conf->wait_for_stripe[i]);
+ if (!waked_thread) {
+ waked_thread = true;
+ md_wakeup_thread(conf->mddev->thread);
+ }
+ }
}
}
@@ -655,6 +661,18 @@ static int has_failed(struct r5conf *conf)
return 0;
}
+/* XXX: might put it to linux/wait.h to be a public API? */
+#define raid_wait_event_exclusive_cmd(wq, condition, cmd1, cmd2) \
+do { \
+ if (condition) \
+ break; \
+ (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 1, 0, \
+ cmd1; \
+ schedule(); \
+ cmd2); \
+} while (0)
+
+
static struct stripe_head *
get_active_stripe(struct r5conf *conf, sector_t sector,
int previous, int noblock, int noquiesce)
@@ -684,14 +702,15 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
if (!sh) {
set_bit(R5_INACTIVE_BLOCKED,
&conf->cache_state);
- wait_event_lock_irq(
- conf->wait_for_stripe,
+ raid_wait_event_exclusive_cmd(
+ conf->wait_for_stripe[hash],
!list_empty(conf->inactive_list + hash) &&
(atomic_read(&conf->active_stripes)
< (conf->max_nr_stripes * 3 / 4)
|| !test_bit(R5_INACTIVE_BLOCKED,
&conf->cache_state)),
- *(conf->hash_locks + hash));
+ spin_unlock_irq(conf->hash_locks + hash),
+ spin_lock_irq(conf->hash_locks + hash));
clear_bit(R5_INACTIVE_BLOCKED,
&conf->cache_state);
} else {
@@ -716,6 +735,9 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
}
} while (sh == NULL);
+ if (!list_empty(conf->inactive_list + hash))
+ wake_up(&conf->wait_for_stripe[hash]);
+
spin_unlock_irq(conf->hash_locks + hash);
return sh;
}
@@ -2136,7 +2158,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
cnt = 0;
list_for_each_entry(nsh, &newstripes, lru) {
lock_device_hash_lock(conf, hash);
- wait_event_cmd(conf->wait_for_stripe,
+ raid_wait_event_exclusive_cmd(conf->wait_for_stripe[hash],
!list_empty(conf->inactive_list + hash),
unlock_device_hash_lock(conf, hash),
lock_device_hash_lock(conf, hash));
@@ -6391,7 +6413,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
spin_lock_init(&conf->device_lock);
seqcount_init(&conf->gen_lock);
init_waitqueue_head(&conf->wait_for_quiesce);
- init_waitqueue_head(&conf->wait_for_stripe);
+ for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
+ init_waitqueue_head(&conf->wait_for_stripe[i]);
+ }
init_waitqueue_head(&conf->wait_for_overlap);
INIT_LIST_HEAD(&conf->handle_list);
INIT_LIST_HEAD(&conf->hold_list);
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index fab53a3..cdad2d2 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -509,7 +509,7 @@ struct r5conf {
atomic_t empty_inactive_list_nr;
struct llist_head released_stripes;
wait_queue_head_t wait_for_quiesce;
- wait_queue_head_t wait_for_stripe;
+ wait_queue_head_t wait_for_stripe[NR_STRIPE_HASH_LOCKS];
wait_queue_head_t wait_for_overlap;
unsigned long cache_state;
#define R5_INACTIVE_BLOCKED 1 /* release of inactive stripes blocked,
--
1.9.0
On Fri, 24 Apr 2015 21:39:03 +0800 Yuanhan Liu <[email protected]>
wrote:
> If I read code correctly, current wait_for_stripe actually has 2 usage:
>
> - wait for there is enough free stripe cache, triggered when
> get_free_stripe() failed. This is what wait_for_stripe intend
> for literally.
>
> - wait for quiesce == 0 or
> active_aligned_reads == 0 && active_stripes == 0
>
> It has nothing to do with wait_for_stripe literally, and releasing
> an active stripe won't actually wake them up. On the contrary, wake_up
> from under this case won't actually wake up the process waiting for
> an free stripe being available.
I disagree. Releasing an active stripe *will* (or *can*) wake up that third
case, as it decrements "active_stripes" which will eventually reach zero.
I don't think your new code will properly wake up a process which is waiting
for "active_stripes == 0".
>
> Hence, we'd better split wait_for_stripe, and here I introduce
> wait_for_quiesce for the second usage. The name may not well taken, or
> even taken wrongly. Feel free to correct me then.
>
> This is also a prepare patch for next patch: make wait_for_stripe
> exclusive.
I think you have this commit description upside down :-)
The real motivation is that you are seeing contention on some spinlock and so
you want to split 'wait_for_stripe' up in to multiple wait_queues so that you
can use exclusive wakeup. As this is the main motivation, it should be
stated first.
Then explain that 'wait_for_stripe' is used to wait for the array to enter or
leave the quiescent state, and also to wait for an available stripe in each
of the hash lists.
So this patch splits the first usage off into a separate wait_queue, and the
next patch will split the second usage into one waitqueue for each hash value.
Then explain just is what is needed for that first step.
When you put it that way around, the patch makes lots of sense.
So: could you please resubmit with the description the right way around, and
with an appropriate wakeup call to ensure raid5_quiesce is woken up when
active_stripes reaches zero?
Thanks,
NeilBrown
>
> Signed-off-by: Yuanhan Liu <[email protected]>
> ---
> drivers/md/raid5.c | 13 +++++++------
> drivers/md/raid5.h | 1 +
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 9716319..b7e385f 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> spin_lock_irq(conf->hash_locks + hash);
>
> do {
> - wait_event_lock_irq(conf->wait_for_stripe,
> + wait_event_lock_irq(conf->wait_for_quiesce,
> conf->quiesce == 0 || noquiesce,
> *(conf->hash_locks + hash));
> sh = __find_stripe(conf, sector, conf->generation - previous);
> @@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
> raid_bi, 0);
> bio_endio(raid_bi, 0);
> if (atomic_dec_and_test(&conf->active_aligned_reads))
> - wake_up(&conf->wait_for_stripe);
> + wake_up(&conf->wait_for_quiesce);
> return;
> }
>
> @@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
> align_bi->bi_iter.bi_sector += rdev->data_offset;
>
> spin_lock_irq(&conf->device_lock);
> - wait_event_lock_irq(conf->wait_for_stripe,
> + wait_event_lock_irq(conf->wait_for_quiesce,
> conf->quiesce == 0,
> conf->device_lock);
> atomic_inc(&conf->active_aligned_reads);
> @@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
> bio_endio(raid_bio, 0);
> }
> if (atomic_dec_and_test(&conf->active_aligned_reads))
> - wake_up(&conf->wait_for_stripe);
> + wake_up(&conf->wait_for_quiesce);
> return handled;
> }
>
> @@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> goto abort;
> spin_lock_init(&conf->device_lock);
> seqcount_init(&conf->gen_lock);
> + init_waitqueue_head(&conf->wait_for_quiesce);
> init_waitqueue_head(&conf->wait_for_stripe);
> init_waitqueue_head(&conf->wait_for_overlap);
> INIT_LIST_HEAD(&conf->handle_list);
> @@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> * active stripes can drain
> */
> conf->quiesce = 2;
> - wait_event_cmd(conf->wait_for_stripe,
> + wait_event_cmd(conf->wait_for_quiesce,
> atomic_read(&conf->active_stripes) == 0 &&
> atomic_read(&conf->active_aligned_reads) == 0,
> unlock_all_device_hash_locks_irq(conf),
> @@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> case 0: /* re-enable writes */
> lock_all_device_hash_locks_irq(conf);
> conf->quiesce = 0;
> - wake_up(&conf->wait_for_stripe);
> + wake_up(&conf->wait_for_quiesce);
> wake_up(&conf->wait_for_overlap);
> unlock_all_device_hash_locks_irq(conf);
> break;
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 7dc0dd8..fab53a3 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -508,6 +508,7 @@ struct r5conf {
> struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
> atomic_t empty_inactive_list_nr;
> struct llist_head released_stripes;
> + wait_queue_head_t wait_for_quiesce;
> wait_queue_head_t wait_for_stripe;
> wait_queue_head_t wait_for_overlap;
> unsigned long cache_state;
On Fri, 24 Apr 2015 21:39:04 +0800 Yuanhan Liu <[email protected]>
wrote:
> I noticed heavy spin lock contention at get_active_stripe() with fsmark
> multiple thread write workloads.
>
> Here is how this hot contention comes from. We have limited stripes, and
> it's a multiple thread write workload. Hence, those stripes will be taken
> soon, which puts later processes to sleep for waiting free stripes. When
> enough stripes(> 1/4 total stripes) are released, all process are woken,
> trying to get the lock. But there is one only being able to get this lock
> for each hash lock, making other processes spinning out there for acquiring
> the lock.
>
> Thus, it's effectiveless to wakeup all processes and let them battle for
> a lock that permits one to access only each time. Instead, we could make
> it be a exclusive wake up: wake up one process only. That avoids the heavy
> spin lock contention naturally.
>
> Here are some test results I have got with this patch applied(all test run
> 3 times):
>
> `fsmark.files_per_sec'
> =====================
>
> next-20150317 this patch
> ------------------------- -------------------------
> metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
> ------------------------- ------------------------- -------- ------------------------------
> 25.600 ±0.0 92.700 ±2.5 262.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 25.600 ±0.0 77.800 ±0.6 203.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 32.000 ±0.0 93.800 ±1.7 193.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 32.000 ±0.0 81.233 ±1.7 153.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 48.800 ±14.5 99.667 ±2.0 104.2% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 6.400 ±0.0 12.800 ±0.0 100.0% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> 63.133 ±8.2 82.800 ±0.7 31.2% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 245.067 ±0.7 306.567 ±7.9 25.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-f2fs-4M-30G-fsyncBeforeClose
> 17.533 ±0.3 21.000 ±0.8 19.8% ivb44/fsmark/1x-1t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
> 188.167 ±1.9 215.033 ±3.1 14.3% ivb44/fsmark/1x-1t-4BRD_12G-RAID5-btrfs-4M-30G-NoSync
> 254.500 ±1.8 290.733 ±2.4 14.2% ivb44/fsmark/1x-1t-9BRD_6G-RAID5-btrfs-4M-30G-NoSync
>
> `time.system_time'
> =====================
>
> next-20150317 this patch
> ------------------------- -------------------------
> metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
> ------------------------- ------------------------- -------- ------------------------------
> 7235.603 ±1.2 185.163 ±1.9 -97.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 7666.883 ±2.9 202.750 ±1.0 -97.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 14567.893 ±0.7 421.230 ±0.4 -97.1% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> 3697.667 ±14.0 148.190 ±1.7 -96.0% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 5572.867 ±3.8 310.717 ±1.4 -94.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 5565.050 ±0.5 313.277 ±1.5 -94.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 2420.707 ±17.1 171.043 ±2.7 -92.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 3743.300 ±4.6 379.827 ±3.5 -89.9% ivb44/fsmark/1x-64t-3HDD-RAID5-ext4-4M-40G-fsyncBeforeClose
> 3308.687 ±6.3 363.050 ±2.0 -89.0% ivb44/fsmark/1x-64t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
>
> Where,
>
> 1x: where 'x' means iterations or loop, corresponding to the 'L' option of fsmark
>
> 1t, 64t: where 't' means thread
>
> 4M: means the single file size, corresponding to the '-s' option of fsmark
> 40G, 30G, 120G: means the total test size
>
> 4BRD_12G: BRD is the ramdisk, where '4' means 4 ramdisk, and where '12G' means
> the size of one ramdisk. So, it would be 48G in total. And we made a
> raid on those ramdisk
>
> As you can see, though there are no much performance gain for hard disk
> workload, the system time is dropped heavily, up to 97%. And as expected,
> the performance increased a lot, up to 260%, for fast device(ram disk).
>
> Signed-off-by: Yuanhan Liu <[email protected]>
> ---
> drivers/md/raid5.c | 46 +++++++++++++++++++++++++++++++++++-----------
> drivers/md/raid5.h | 2 +-
> 2 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b7e385f..2d8fcc1 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -344,7 +344,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> int hash)
> {
> int size;
> - bool do_wakeup = false;
> + bool do_wakeup[NR_STRIPE_HASH_LOCKS] = { false, };
I think I'd rather use an 'unsigned long' and set bits.
> + int i = 0;
> unsigned long flags;
>
> if (hash == NR_STRIPE_HASH_LOCKS) {
> @@ -365,17 +366,22 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> !list_empty(list))
> atomic_dec(&conf->empty_inactive_list_nr);
> list_splice_tail_init(list, conf->inactive_list + hash);
> - do_wakeup = true;
> + do_wakeup[size - 1] = true;
... so this becomes
do_wakeup |= 1 << (size - 1);
> spin_unlock_irqrestore(conf->hash_locks + hash, flags);
> }
> size--;
> hash--;
> }
>
> - if (do_wakeup) {
> - wake_up(&conf->wait_for_stripe);
> - if (conf->retry_read_aligned)
> - md_wakeup_thread(conf->mddev->thread);
> + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> + bool waked_thread = false;
> + if (do_wakeup[i]) {
> + wake_up(&conf->wait_for_stripe[i]);
> + if (!waked_thread) {
> + waked_thread = true;
> + md_wakeup_thread(conf->mddev->thread);
> + }
> + }
I don't think you want waked_thread to be local to this loop.
As it is, the "if (!waked_thread)" test *always* succeeds.
You can discard it if do_wakeup becomes and unsigned long, and just do
if (do_wakeup && conf->retry_read_aligned)
md_wakeup_thread(conf->mddev->thread);
And why have you removed the test on conf->retry_read_aligned??
> }
> }
>
> @@ -655,6 +661,18 @@ static int has_failed(struct r5conf *conf)
> return 0;
> }
>
> +/* XXX: might put it to linux/wait.h to be a public API? */
Yes, definitely put it in linux/wait.h
Thanks,
NeilBrown
> +#define raid_wait_event_exclusive_cmd(wq, condition, cmd1, cmd2) \
> +do { \
> + if (condition) \
> + break; \
> + (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 1, 0, \
> + cmd1; \
> + schedule(); \
> + cmd2); \
> +} while (0)
> +
> +
> static struct stripe_head *
> get_active_stripe(struct r5conf *conf, sector_t sector,
> int previous, int noblock, int noquiesce)
> @@ -684,14 +702,15 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> if (!sh) {
> set_bit(R5_INACTIVE_BLOCKED,
> &conf->cache_state);
> - wait_event_lock_irq(
> - conf->wait_for_stripe,
> + raid_wait_event_exclusive_cmd(
> + conf->wait_for_stripe[hash],
> !list_empty(conf->inactive_list + hash) &&
> (atomic_read(&conf->active_stripes)
> < (conf->max_nr_stripes * 3 / 4)
> || !test_bit(R5_INACTIVE_BLOCKED,
> &conf->cache_state)),
> - *(conf->hash_locks + hash));
> + spin_unlock_irq(conf->hash_locks + hash),
> + spin_lock_irq(conf->hash_locks + hash));
> clear_bit(R5_INACTIVE_BLOCKED,
> &conf->cache_state);
> } else {
> @@ -716,6 +735,9 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> }
> } while (sh == NULL);
>
> + if (!list_empty(conf->inactive_list + hash))
> + wake_up(&conf->wait_for_stripe[hash]);
> +
> spin_unlock_irq(conf->hash_locks + hash);
> return sh;
> }
> @@ -2136,7 +2158,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
> cnt = 0;
> list_for_each_entry(nsh, &newstripes, lru) {
> lock_device_hash_lock(conf, hash);
> - wait_event_cmd(conf->wait_for_stripe,
> + raid_wait_event_exclusive_cmd(conf->wait_for_stripe[hash],
> !list_empty(conf->inactive_list + hash),
> unlock_device_hash_lock(conf, hash),
> lock_device_hash_lock(conf, hash));
> @@ -6391,7 +6413,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> spin_lock_init(&conf->device_lock);
> seqcount_init(&conf->gen_lock);
> init_waitqueue_head(&conf->wait_for_quiesce);
> - init_waitqueue_head(&conf->wait_for_stripe);
> + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> + init_waitqueue_head(&conf->wait_for_stripe[i]);
> + }
> init_waitqueue_head(&conf->wait_for_overlap);
> INIT_LIST_HEAD(&conf->handle_list);
> INIT_LIST_HEAD(&conf->hold_list);
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index fab53a3..cdad2d2 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -509,7 +509,7 @@ struct r5conf {
> atomic_t empty_inactive_list_nr;
> struct llist_head released_stripes;
> wait_queue_head_t wait_for_quiesce;
> - wait_queue_head_t wait_for_stripe;
> + wait_queue_head_t wait_for_stripe[NR_STRIPE_HASH_LOCKS];
> wait_queue_head_t wait_for_overlap;
> unsigned long cache_state;
> #define R5_INACTIVE_BLOCKED 1 /* release of inactive stripes blocked,
On Mon, Apr 27, 2015 at 10:10:24AM +1000, NeilBrown wrote:
> On Fri, 24 Apr 2015 21:39:03 +0800 Yuanhan Liu <[email protected]>
> wrote:
>
> > If I read code correctly, current wait_for_stripe actually has 2 usage:
> >
> > - wait for there is enough free stripe cache, triggered when
> > get_free_stripe() failed. This is what wait_for_stripe intend
> > for literally.
> >
> > - wait for quiesce == 0 or
> > active_aligned_reads == 0 && active_stripes == 0
> >
> > It has nothing to do with wait_for_stripe literally, and releasing
> > an active stripe won't actually wake them up. On the contrary, wake_up
> > from under this case won't actually wake up the process waiting for
> > an free stripe being available.
>
> I disagree. Releasing an active stripe *will* (or *can*) wake up that third
> case, as it decrements "active_stripes" which will eventually reach zero.
>
> I don't think your new code will properly wake up a process which is waiting
> for "active_stripes == 0".
Right, and thanks for pointing it out. So, is this enough?
---
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 2d8fcc1..3f23035 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -383,6 +383,9 @@ static void release_inactive_stripe_list(struct
r5conf *conf,
}
}
}
+
+ if (!atomic_read(&conf->active_stripes))
+ wake_up(&conf->wait_for_quiesce);
}
/* should hold conf->device_lock already */
Or, should I put it a bit ahead, trying to invoke wake_up(&conf->wait_for_quiesce)
after each atomic_dec(&conf->active_stripes)?
if (atomic_dec_return(&conf->active_stripes) == 0)
wake_up(&conf->wait_for_quiesce);
>
> >
> > Hence, we'd better split wait_for_stripe, and here I introduce
> > wait_for_quiesce for the second usage. The name may not well taken, or
> > even taken wrongly. Feel free to correct me then.
> >
> > This is also a prepare patch for next patch: make wait_for_stripe
> > exclusive.
>
> I think you have this commit description upside down :-)
>
> The real motivation is that you are seeing contention on some spinlock and so
> you want to split 'wait_for_stripe' up in to multiple wait_queues so that you
> can use exclusive wakeup. As this is the main motivation, it should be
> stated first.
>
> Then explain that 'wait_for_stripe' is used to wait for the array to enter or
> leave the quiescent state, and also to wait for an available stripe in each
> of the hash lists.
>
> So this patch splits the first usage off into a separate wait_queue, and the
> next patch will split the second usage into one waitqueue for each hash value.
>
> Then explain just is what is needed for that first step.
>
> When you put it that way around, the patch makes lots of sense.
It does, and thanks!
>
> So: could you please resubmit with the description the right way around, and
To make sure I followed you correctly, my patch order is correct(I mean,
split lock first, and make wait_for_stripe per lock hash and exclusive
second), and what I need to do is re-writing the commit log as you suggested,
and fixing all issues you pointed out. Right?
--yliu
> with an appropriate wakeup call to ensure raid5_quiesce is woken up when
> active_stripes reaches zero?
>
> Thanks,
> NeilBrown
>
>
> >
> > Signed-off-by: Yuanhan Liu <[email protected]>
> > ---
> > drivers/md/raid5.c | 13 +++++++------
> > drivers/md/raid5.h | 1 +
> > 2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 9716319..b7e385f 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > spin_lock_irq(conf->hash_locks + hash);
> >
> > do {
> > - wait_event_lock_irq(conf->wait_for_stripe,
> > + wait_event_lock_irq(conf->wait_for_quiesce,
> > conf->quiesce == 0 || noquiesce,
> > *(conf->hash_locks + hash));
> > sh = __find_stripe(conf, sector, conf->generation - previous);
> > @@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
> > raid_bi, 0);
> > bio_endio(raid_bi, 0);
> > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > - wake_up(&conf->wait_for_stripe);
> > + wake_up(&conf->wait_for_quiesce);
> > return;
> > }
> >
> > @@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
> > align_bi->bi_iter.bi_sector += rdev->data_offset;
> >
> > spin_lock_irq(&conf->device_lock);
> > - wait_event_lock_irq(conf->wait_for_stripe,
> > + wait_event_lock_irq(conf->wait_for_quiesce,
> > conf->quiesce == 0,
> > conf->device_lock);
> > atomic_inc(&conf->active_aligned_reads);
> > @@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
> > bio_endio(raid_bio, 0);
> > }
> > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > - wake_up(&conf->wait_for_stripe);
> > + wake_up(&conf->wait_for_quiesce);
> > return handled;
> > }
> >
> > @@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> > goto abort;
> > spin_lock_init(&conf->device_lock);
> > seqcount_init(&conf->gen_lock);
> > + init_waitqueue_head(&conf->wait_for_quiesce);
> > init_waitqueue_head(&conf->wait_for_stripe);
> > init_waitqueue_head(&conf->wait_for_overlap);
> > INIT_LIST_HEAD(&conf->handle_list);
> > @@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > * active stripes can drain
> > */
> > conf->quiesce = 2;
> > - wait_event_cmd(conf->wait_for_stripe,
> > + wait_event_cmd(conf->wait_for_quiesce,
> > atomic_read(&conf->active_stripes) == 0 &&
> > atomic_read(&conf->active_aligned_reads) == 0,
> > unlock_all_device_hash_locks_irq(conf),
> > @@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > case 0: /* re-enable writes */
> > lock_all_device_hash_locks_irq(conf);
> > conf->quiesce = 0;
> > - wake_up(&conf->wait_for_stripe);
> > + wake_up(&conf->wait_for_quiesce);
> > wake_up(&conf->wait_for_overlap);
> > unlock_all_device_hash_locks_irq(conf);
> > break;
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index 7dc0dd8..fab53a3 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> > @@ -508,6 +508,7 @@ struct r5conf {
> > struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
> > atomic_t empty_inactive_list_nr;
> > struct llist_head released_stripes;
> > + wait_queue_head_t wait_for_quiesce;
> > wait_queue_head_t wait_for_stripe;
> > wait_queue_head_t wait_for_overlap;
> > unsigned long cache_state;
>
On Mon, Apr 27, 2015 at 10:24:05AM +1000, NeilBrown wrote:
> On Fri, 24 Apr 2015 21:39:04 +0800 Yuanhan Liu <[email protected]>
> wrote:
>
> > I noticed heavy spin lock contention at get_active_stripe() with fsmark
> > multiple thread write workloads.
> >
> > Here is how this hot contention comes from. We have limited stripes, and
> > it's a multiple thread write workload. Hence, those stripes will be taken
> > soon, which puts later processes to sleep for waiting free stripes. When
> > enough stripes(> 1/4 total stripes) are released, all process are woken,
> > trying to get the lock. But there is one only being able to get this lock
> > for each hash lock, making other processes spinning out there for acquiring
> > the lock.
> >
> > Thus, it's effectiveless to wakeup all processes and let them battle for
> > a lock that permits one to access only each time. Instead, we could make
> > it be a exclusive wake up: wake up one process only. That avoids the heavy
> > spin lock contention naturally.
> >
> > Here are some test results I have got with this patch applied(all test run
> > 3 times):
> >
> > `fsmark.files_per_sec'
> > =====================
> >
> > next-20150317 this patch
> > ------------------------- -------------------------
> > metric_value ?stddev metric_value ?stddev change testbox/benchmark/testcase-params
> > ------------------------- ------------------------- -------- ------------------------------
> > 25.600 ?0.0 92.700 ?2.5 262.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 25.600 ?0.0 77.800 ?0.6 203.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 32.000 ?0.0 93.800 ?1.7 193.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 32.000 ?0.0 81.233 ?1.7 153.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 48.800 ?14.5 99.667 ?2.0 104.2% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 6.400 ?0.0 12.800 ?0.0 100.0% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> > 63.133 ?8.2 82.800 ?0.7 31.2% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 245.067 ?0.7 306.567 ?7.9 25.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-f2fs-4M-30G-fsyncBeforeClose
> > 17.533 ?0.3 21.000 ?0.8 19.8% ivb44/fsmark/1x-1t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
> > 188.167 ?1.9 215.033 ?3.1 14.3% ivb44/fsmark/1x-1t-4BRD_12G-RAID5-btrfs-4M-30G-NoSync
> > 254.500 ?1.8 290.733 ?2.4 14.2% ivb44/fsmark/1x-1t-9BRD_6G-RAID5-btrfs-4M-30G-NoSync
> >
> > `time.system_time'
> > =====================
> >
> > next-20150317 this patch
> > ------------------------- -------------------------
> > metric_value ?stddev metric_value ?stddev change testbox/benchmark/testcase-params
> > ------------------------- ------------------------- -------- ------------------------------
> > 7235.603 ?1.2 185.163 ?1.9 -97.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 7666.883 ?2.9 202.750 ?1.0 -97.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 14567.893 ?0.7 421.230 ?0.4 -97.1% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> > 3697.667 ?14.0 148.190 ?1.7 -96.0% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 5572.867 ?3.8 310.717 ?1.4 -94.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 5565.050 ?0.5 313.277 ?1.5 -94.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 2420.707 ?17.1 171.043 ?2.7 -92.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 3743.300 ?4.6 379.827 ?3.5 -89.9% ivb44/fsmark/1x-64t-3HDD-RAID5-ext4-4M-40G-fsyncBeforeClose
> > 3308.687 ?6.3 363.050 ?2.0 -89.0% ivb44/fsmark/1x-64t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
> >
> > Where,
> >
> > 1x: where 'x' means iterations or loop, corresponding to the 'L' option of fsmark
> >
> > 1t, 64t: where 't' means thread
> >
> > 4M: means the single file size, corresponding to the '-s' option of fsmark
> > 40G, 30G, 120G: means the total test size
> >
> > 4BRD_12G: BRD is the ramdisk, where '4' means 4 ramdisk, and where '12G' means
> > the size of one ramdisk. So, it would be 48G in total. And we made a
> > raid on those ramdisk
> >
> > As you can see, though there are no much performance gain for hard disk
> > workload, the system time is dropped heavily, up to 97%. And as expected,
> > the performance increased a lot, up to 260%, for fast device(ram disk).
> >
> > Signed-off-by: Yuanhan Liu <[email protected]>
> > ---
> > drivers/md/raid5.c | 46 +++++++++++++++++++++++++++++++++++-----------
> > drivers/md/raid5.h | 2 +-
> > 2 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index b7e385f..2d8fcc1 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -344,7 +344,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> > int hash)
> > {
> > int size;
> > - bool do_wakeup = false;
> > + bool do_wakeup[NR_STRIPE_HASH_LOCKS] = { false, };
>
> I think I'd rather use an 'unsigned long' and set bits.
Will do that.
>
> > + int i = 0;
> > unsigned long flags;
> >
> > if (hash == NR_STRIPE_HASH_LOCKS) {
> > @@ -365,17 +366,22 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> > !list_empty(list))
> > atomic_dec(&conf->empty_inactive_list_nr);
> > list_splice_tail_init(list, conf->inactive_list + hash);
> > - do_wakeup = true;
> > + do_wakeup[size - 1] = true;
>
> ... so this becomes
> do_wakeup |= 1 << (size - 1);
>
> > spin_unlock_irqrestore(conf->hash_locks + hash, flags);
> > }
> > size--;
> > hash--;
> > }
> >
> > - if (do_wakeup) {
> > - wake_up(&conf->wait_for_stripe);
> > - if (conf->retry_read_aligned)
> > - md_wakeup_thread(conf->mddev->thread);
> > + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> > + bool waked_thread = false;
> > + if (do_wakeup[i]) {
> > + wake_up(&conf->wait_for_stripe[i]);
> > + if (!waked_thread) {
> > + waked_thread = true;
> > + md_wakeup_thread(conf->mddev->thread);
> > + }
> > + }
>
> I don't think you want waked_thread to be local to this loop.
> As it is, the "if (!waked_thread)" test *always* succeeds.
>
> You can discard it if do_wakeup becomes and unsigned long, and just do
>
> if (do_wakeup && conf->retry_read_aligned)
> md_wakeup_thread(conf->mddev->thread);
>
> And why have you removed the test on conf->retry_read_aligned??
Oops, a careless editing.
>
> > }
> > }
> >
> > @@ -655,6 +661,18 @@ static int has_failed(struct r5conf *conf)
> > return 0;
> > }
> >
> > +/* XXX: might put it to linux/wait.h to be a public API? */
>
> Yes, definitely put it in linux/wait.h
I will send a seperate patch for that.
Thanks.
--yliu
>
>
>
>
> > +#define raid_wait_event_exclusive_cmd(wq, condition, cmd1, cmd2) \
> > +do { \
> > + if (condition) \
> > + break; \
> > + (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 1, 0, \
> > + cmd1; \
> > + schedule(); \
> > + cmd2); \
> > +} while (0)
> > +
> > +
> > static struct stripe_head *
> > get_active_stripe(struct r5conf *conf, sector_t sector,
> > int previous, int noblock, int noquiesce)
> > @@ -684,14 +702,15 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > if (!sh) {
> > set_bit(R5_INACTIVE_BLOCKED,
> > &conf->cache_state);
> > - wait_event_lock_irq(
> > - conf->wait_for_stripe,
> > + raid_wait_event_exclusive_cmd(
> > + conf->wait_for_stripe[hash],
> > !list_empty(conf->inactive_list + hash) &&
> > (atomic_read(&conf->active_stripes)
> > < (conf->max_nr_stripes * 3 / 4)
> > || !test_bit(R5_INACTIVE_BLOCKED,
> > &conf->cache_state)),
> > - *(conf->hash_locks + hash));
> > + spin_unlock_irq(conf->hash_locks + hash),
> > + spin_lock_irq(conf->hash_locks + hash));
> > clear_bit(R5_INACTIVE_BLOCKED,
> > &conf->cache_state);
> > } else {
> > @@ -716,6 +735,9 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > }
> > } while (sh == NULL);
> >
> > + if (!list_empty(conf->inactive_list + hash))
> > + wake_up(&conf->wait_for_stripe[hash]);
> > +
> > spin_unlock_irq(conf->hash_locks + hash);
> > return sh;
> > }
> > @@ -2136,7 +2158,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
> > cnt = 0;
> > list_for_each_entry(nsh, &newstripes, lru) {
> > lock_device_hash_lock(conf, hash);
> > - wait_event_cmd(conf->wait_for_stripe,
> > + raid_wait_event_exclusive_cmd(conf->wait_for_stripe[hash],
> > !list_empty(conf->inactive_list + hash),
> > unlock_device_hash_lock(conf, hash),
> > lock_device_hash_lock(conf, hash));
> > @@ -6391,7 +6413,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> > spin_lock_init(&conf->device_lock);
> > seqcount_init(&conf->gen_lock);
> > init_waitqueue_head(&conf->wait_for_quiesce);
> > - init_waitqueue_head(&conf->wait_for_stripe);
> > + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> > + init_waitqueue_head(&conf->wait_for_stripe[i]);
> > + }
> > init_waitqueue_head(&conf->wait_for_overlap);
> > INIT_LIST_HEAD(&conf->handle_list);
> > INIT_LIST_HEAD(&conf->hold_list);
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index fab53a3..cdad2d2 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> > @@ -509,7 +509,7 @@ struct r5conf {
> > atomic_t empty_inactive_list_nr;
> > struct llist_head released_stripes;
> > wait_queue_head_t wait_for_quiesce;
> > - wait_queue_head_t wait_for_stripe;
> > + wait_queue_head_t wait_for_stripe[NR_STRIPE_HASH_LOCKS];
> > wait_queue_head_t wait_for_overlap;
> > unsigned long cache_state;
> > #define R5_INACTIVE_BLOCKED 1 /* release of inactive stripes blocked,
>
On Mon, 27 Apr 2015 10:12:49 +0800 Yuanhan Liu <[email protected]>
wrote:
> On Mon, Apr 27, 2015 at 10:10:24AM +1000, NeilBrown wrote:
> > On Fri, 24 Apr 2015 21:39:03 +0800 Yuanhan Liu <[email protected]>
> > wrote:
> >
> > > If I read code correctly, current wait_for_stripe actually has 2 usage:
> > >
> > > - wait for there is enough free stripe cache, triggered when
> > > get_free_stripe() failed. This is what wait_for_stripe intend
> > > for literally.
> > >
> > > - wait for quiesce == 0 or
> > > active_aligned_reads == 0 && active_stripes == 0
> > >
> > > It has nothing to do with wait_for_stripe literally, and releasing
> > > an active stripe won't actually wake them up. On the contrary, wake_up
> > > from under this case won't actually wake up the process waiting for
> > > an free stripe being available.
> >
> > I disagree. Releasing an active stripe *will* (or *can*) wake up that third
> > case, as it decrements "active_stripes" which will eventually reach zero.
> >
> > I don't think your new code will properly wake up a process which is waiting
> > for "active_stripes == 0".
>
> Right, and thanks for pointing it out. So, is this enough?
>
> ---
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 2d8fcc1..3f23035 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -383,6 +383,9 @@ static void release_inactive_stripe_list(struct
> r5conf *conf,
> }
> }
> }
> +
> + if (!atomic_read(&conf->active_stripes))
> + wake_up(&conf->wait_for_quiesce);
> }
>
> /* should hold conf->device_lock already */
>
>
> Or, should I put it a bit ahead, trying to invoke wake_up(&conf->wait_for_quiesce)
> after each atomic_dec(&conf->active_stripes)?
>
> if (atomic_dec_return(&conf->active_stripes) == 0)
> wake_up(&conf->wait_for_quiesce);
I think the first version is fine. While waiting for active_stripes to be
zero, ->quiesce is set to 2, and so no new stripes should get used.
>
> >
> > >
> > > Hence, we'd better split wait_for_stripe, and here I introduce
> > > wait_for_quiesce for the second usage. The name may not well taken, or
> > > even taken wrongly. Feel free to correct me then.
> > >
> > > This is also a prepare patch for next patch: make wait_for_stripe
> > > exclusive.
> >
> > I think you have this commit description upside down :-)
> >
> > The real motivation is that you are seeing contention on some spinlock and so
> > you want to split 'wait_for_stripe' up in to multiple wait_queues so that you
> > can use exclusive wakeup. As this is the main motivation, it should be
> > stated first.
> >
> > Then explain that 'wait_for_stripe' is used to wait for the array to enter or
> > leave the quiescent state, and also to wait for an available stripe in each
> > of the hash lists.
> >
> > So this patch splits the first usage off into a separate wait_queue, and the
> > next patch will split the second usage into one waitqueue for each hash value.
> >
> > Then explain just is what is needed for that first step.
> >
> > When you put it that way around, the patch makes lots of sense.
>
> It does, and thanks!
>
> >
> > So: could you please resubmit with the description the right way around, and
>
> To make sure I followed you correctly, my patch order is correct(I mean,
> split lock first, and make wait_for_stripe per lock hash and exclusive
> second), and what I need to do is re-writing the commit log as you suggested,
> and fixing all issues you pointed out. Right?
Correct.
Thanks,
NeilBrown
>
> --yliu
>
> > with an appropriate wakeup call to ensure raid5_quiesce is woken up when
> > active_stripes reaches zero?
> >
> > Thanks,
> > NeilBrown
> >
> >
> > >
> > > Signed-off-by: Yuanhan Liu <[email protected]>
> > > ---
> > > drivers/md/raid5.c | 13 +++++++------
> > > drivers/md/raid5.h | 1 +
> > > 2 files changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > > index 9716319..b7e385f 100644
> > > --- a/drivers/md/raid5.c
> > > +++ b/drivers/md/raid5.c
> > > @@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > > spin_lock_irq(conf->hash_locks + hash);
> > >
> > > do {
> > > - wait_event_lock_irq(conf->wait_for_stripe,
> > > + wait_event_lock_irq(conf->wait_for_quiesce,
> > > conf->quiesce == 0 || noquiesce,
> > > *(conf->hash_locks + hash));
> > > sh = __find_stripe(conf, sector, conf->generation - previous);
> > > @@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
> > > raid_bi, 0);
> > > bio_endio(raid_bi, 0);
> > > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > > - wake_up(&conf->wait_for_stripe);
> > > + wake_up(&conf->wait_for_quiesce);
> > > return;
> > > }
> > >
> > > @@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
> > > align_bi->bi_iter.bi_sector += rdev->data_offset;
> > >
> > > spin_lock_irq(&conf->device_lock);
> > > - wait_event_lock_irq(conf->wait_for_stripe,
> > > + wait_event_lock_irq(conf->wait_for_quiesce,
> > > conf->quiesce == 0,
> > > conf->device_lock);
> > > atomic_inc(&conf->active_aligned_reads);
> > > @@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
> > > bio_endio(raid_bio, 0);
> > > }
> > > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > > - wake_up(&conf->wait_for_stripe);
> > > + wake_up(&conf->wait_for_quiesce);
> > > return handled;
> > > }
> > >
> > > @@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> > > goto abort;
> > > spin_lock_init(&conf->device_lock);
> > > seqcount_init(&conf->gen_lock);
> > > + init_waitqueue_head(&conf->wait_for_quiesce);
> > > init_waitqueue_head(&conf->wait_for_stripe);
> > > init_waitqueue_head(&conf->wait_for_overlap);
> > > INIT_LIST_HEAD(&conf->handle_list);
> > > @@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > > * active stripes can drain
> > > */
> > > conf->quiesce = 2;
> > > - wait_event_cmd(conf->wait_for_stripe,
> > > + wait_event_cmd(conf->wait_for_quiesce,
> > > atomic_read(&conf->active_stripes) == 0 &&
> > > atomic_read(&conf->active_aligned_reads) == 0,
> > > unlock_all_device_hash_locks_irq(conf),
> > > @@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > > case 0: /* re-enable writes */
> > > lock_all_device_hash_locks_irq(conf);
> > > conf->quiesce = 0;
> > > - wake_up(&conf->wait_for_stripe);
> > > + wake_up(&conf->wait_for_quiesce);
> > > wake_up(&conf->wait_for_overlap);
> > > unlock_all_device_hash_locks_irq(conf);
> > > break;
> > > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > > index 7dc0dd8..fab53a3 100644
> > > --- a/drivers/md/raid5.h
> > > +++ b/drivers/md/raid5.h
> > > @@ -508,6 +508,7 @@ struct r5conf {
> > > struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
> > > atomic_t empty_inactive_list_nr;
> > > struct llist_head released_stripes;
> > > + wait_queue_head_t wait_for_quiesce;
> > > wait_queue_head_t wait_for_stripe;
> > > wait_queue_head_t wait_for_overlap;
> > > unsigned long cache_state;
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/