On Fri, May 29, 2015 at 03:23:16PM +1000, NeilBrown wrote:

On Thu, 28 May 2015 17:33:48 -0700 Shaohua Li [off-list ref] wrote:

quoted

Since we have several stripe hash list, the conf->active_stripes doesn't
determine if there is free stripe in a specific hash list, so delete the
check. After this, the R5_INACTIVE_BLOCKED check is inappropriate. There
is no point not to wakeup a task if there is free stripe.

Signed-off-by: Shaohua Li <redacted>
---
 drivers/md/raid5.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 67626f3..4b5a03c 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -687,11 +687,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
 					&conf->cache_state);
 				wait_event_lock_irq(
 					conf->wait_for_stripe,
-					!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)),
+					!list_empty(conf->inactive_list + hash),
 					*(conf->hash_locks + hash));
 				clear_bit(R5_INACTIVE_BLOCKED,
 					  &conf->cache_state);

Have you actually tested this?  Because I do remember why I put that code in
and it made a very real performance improvement.

The idea is that once we run out of free stripes, we wait until there a lots
available.  That improves opportunities for batching.

So I would definitely needs some performance numbers with a patch like this.

I didn't realize this is related to performance. Your explaintation
makes a lot of sense, please ignore this.

Thanks,
Shaohua