[PATCH 1/4] raid5: wakeup raid5d when R5_ALLOC_MORE is set

[PATCH 1/4] raid5: wakeup raid5d when R5_ALLOC_MORE is set

[Shaohua Li]

The run time stripe allocation is done at raid5d. When we set the
R5_ALLOC_MORE flag, we should notify raid5d to handle it

Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/raid5.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 61e8e04..bfa2042 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -674,9 +674,11 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
 			if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
 				sh = get_free_stripe(conf, hash);
 				if (!sh && llist_empty(&conf->released_stripes) &&
-				    !test_bit(R5_DID_ALLOC, &conf->cache_state))
+				    !test_bit(R5_DID_ALLOC, &conf->cache_state)) {
 					set_bit(R5_ALLOC_MORE,
 						&conf->cache_state);
+					md_wakeup_thread(conf->mddev->thread);
+				}
 			}
 			if (noblock && sh == NULL)
 				break;
-- 
1.8.1

[PATCH 2/4] raid5: grown at least NR_STRIPE_HASH_LOCKS stripes

[Shaohua Li]

stripes are in hash list. If we are waiting for a free stripe, we must
make sure there is free stripe in corresponding hash list. To do this,
we simpliy allocate at lease NR_STRIPE_HASH_LOCKS stripes at runtime
stripe allocation.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/raid5.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index bfa2042..0cceb71 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5867,7 +5867,9 @@ static void raid5d(struct md_thread *thread)
 
 	spin_unlock_irq(&conf->device_lock);
 	if (test_and_clear_bit(R5_ALLOC_MORE, &conf->cache_state)) {
-		grow_one_stripe(conf, __GFP_NOWARN);
+		int i;
+		for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++)
+			grow_one_stripe(conf, __GFP_NOWARN);
 		/* Set flag even if allocation failed.  This helps
 		 * slow down allocation requests when mem is short
 		 */
-- 
1.8.1

[PATCH 3/4] raid5: ignore released_stripes check

[Shaohua Li]

conf->released_stripes list ins't always related if there is free
stripes pending. active stripes can be in the list too.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/raid5.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0cceb71..67626f3 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -673,8 +673,8 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
 		if (!sh) {
 			if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
 				sh = get_free_stripe(conf, hash);
-				if (!sh && llist_empty(&conf->released_stripes) &&
-				    !test_bit(R5_DID_ALLOC, &conf->cache_state)) {
+				if (!sh && !test_bit(R5_DID_ALLOC,
+				    &conf->cache_state)) {
 					set_bit(R5_ALLOC_MORE,
 						&conf->cache_state);
 					md_wakeup_thread(conf->mddev->thread);
-- 
1.8.1

[PATCH 4/4] raid5: fix wakeup condition

[Shaohua Li]

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 <shli@fb.com>
---
 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);
-- 
1.8.1

Re: [PATCH 1/4] raid5: wakeup raid5d when R5_ALLOC_MORE is set

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On Thu, 28 May 2015 17:33:45 -0700 Shaohua Li <shli@fb.com> wrote:

> The run time stripe allocation is done at raid5d. When we set the
> R5_ALLOC_MORE flag, we should notify raid5d to handle it
> 
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/raid5.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 61e8e04..bfa2042 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -674,9 +674,11 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
>  			if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
>  				sh = get_free_stripe(conf, hash);
>  				if (!sh && llist_empty(&conf->released_stripes) &&
> -				    !test_bit(R5_DID_ALLOC, &conf->cache_state))
> +				    !test_bit(R5_DID_ALLOC, &conf->cache_state)) {
>  					set_bit(R5_ALLOC_MORE,
>  						&conf->cache_state);
> +					md_wakeup_thread(conf->mddev->thread);
> +				}
>  			}
>  			if (noblock && sh == NULL)
>  				break;

Thanks for reviewing my code !!!

I'm not exactly against this patch, but I wonder if it is really needed.
R5_ALLOC_MORE is really just a hint - "You can allocate another stripe if you
like".  If the array is at all busy, raid5d will be called fairly often and
the allocation will happen.  If the array is idle, it doesn't matter if
memory is allocated for a while.  Does it?

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2/4] raid5: grown at least NR_STRIPE_HASH_LOCKS stripes

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On Thu, 28 May 2015 17:33:46 -0700 Shaohua Li <shli@fb.com> wrote:

> stripes are in hash list. If we are waiting for a free stripe, we must
> make sure there is free stripe in corresponding hash list. To do this,
> we simpliy allocate at lease NR_STRIPE_HASH_LOCKS stripes at runtime
> stripe allocation.
> 
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/raid5.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index bfa2042..0cceb71 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5867,7 +5867,9 @@ static void raid5d(struct md_thread *thread)
>  
>  	spin_unlock_irq(&conf->device_lock);
>  	if (test_and_clear_bit(R5_ALLOC_MORE, &conf->cache_state)) {
> -		grow_one_stripe(conf, __GFP_NOWARN);
> +		int i;
> +		for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++)
> +			grow_one_stripe(conf, __GFP_NOWARN);
>  		/* Set flag even if allocation failed.  This helps
>  		 * slow down allocation requests when mem is short
>  		 */

Again, if R5_ALLOC_MORE is just a hint, it certainly isn't true that we
"must" make sure there are free stripes.
It's fairly important that the pool of stripes grows slowly.  I don't think
it matters a lot how slowly, but on a busy array it should grow steadily
until it hits an equilibrium, and making that happen a bit faster doesn't
seem necessary.

Think of R5_ALLOC_MORE like a gentle push towards allocating more strips.  It
doesn't matter if it fails, and if it is really important there will be
plenty more gentle pushes coming.

Thanks,
NeilBrown


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 3/4] raid5: ignore released_stripes check

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

On Thu, 28 May 2015 17:33:47 -0700 Shaohua Li <shli@fb.com> wrote:

> conf->released_stripes list ins't always related if there is free
> stripes pending. active stripes can be in the list too.
> 
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/raid5.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0cceb71..67626f3 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -673,8 +673,8 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
>  		if (!sh) {
>  			if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
>  				sh = get_free_stripe(conf, hash);
> -				if (!sh && llist_empty(&conf->released_stripes) &&
> -				    !test_bit(R5_DID_ALLOC, &conf->cache_state)) {
> +				if (!sh && !test_bit(R5_DID_ALLOC,
> +				    &conf->cache_state)) {
>  					set_bit(R5_ALLOC_MORE,
>  						&conf->cache_state);
>  					md_wakeup_thread(conf->mddev->thread);

Yes, I think I agree with this. 
I don't clearly remember why I put that test in, but it is certainly hard to
justify it.
Even if there are free stripes in ->released_stripes, they were in use very
recently, so counting them as still in use is easily justified.

BTW, is an open parenthesis (or bracket or brace) is not the last character
in the line, then everything from there until the close must be to the right
of the open.  So I indented the "&conf->cache_state)) {" line some more.

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 4/4] raid5: fix wakeup condition

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On Thu, 28 May 2015 17:33:48 -0700 Shaohua Li <shli@fb.com> wrote:

> 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 <shli@fb.com>
> ---
>  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.

Thanks for the review anyway !!

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/4] raid5: wakeup raid5d when R5_ALLOC_MORE is set

[Shaohua Li]

On Fri, May 29, 2015 at 03:02:56PM +1000, NeilBrown wrote:
> On Thu, 28 May 2015 17:33:45 -0700 Shaohua Li <shli@fb.com> wrote:
> 
> > The run time stripe allocation is done at raid5d. When we set the
> > R5_ALLOC_MORE flag, we should notify raid5d to handle it
> > 
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> >  drivers/md/raid5.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 61e8e04..bfa2042 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -674,9 +674,11 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> >  			if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
> >  				sh = get_free_stripe(conf, hash);
> >  				if (!sh && llist_empty(&conf->released_stripes) &&
> > -				    !test_bit(R5_DID_ALLOC, &conf->cache_state))
> > +				    !test_bit(R5_DID_ALLOC, &conf->cache_state)) {
> >  					set_bit(R5_ALLOC_MORE,
> >  						&conf->cache_state);
> > +					md_wakeup_thread(conf->mddev->thread);
> > +				}
> >  			}
> >  			if (noblock && sh == NULL)
> >  				break;
> 
> Thanks for reviewing my code !!!
> 
> I'm not exactly against this patch, but I wonder if it is really needed.
> R5_ALLOC_MORE is really just a hint - "You can allocate another stripe if you
> like".  If the array is at all busy, raid5d will be called fairly often and
> the allocation will happen.  If the array is idle, it doesn't matter if
> memory is allocated for a while.  Does it?

So this is related to the usage in my raid5 cache patch. Say I need
allocate 100 stripes and dispatch them together. If there are only 99
free stripes, I want to allocate a new one, but the 99 stripes will not
be handled and freed, we can't steal one from the 99 stripes. In this
case, I hope the automatically stripe allocation is reliable. Handling
the stripes together is to reduce disk cache flush. I can workaround the
issue in other way (by increasing min stripe number), but thought making
the allocation reliable is better.

Thanks,
Shaohua

Re: [PATCH 4/4] raid5: fix wakeup condition

[Yuanhan Liu]

Hi Shaohua,

On Thu, May 28, 2015 at 05:33:48PM -0700, Shaohua Li wrote:
> 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

I happened to have considered something similar but with slight different
before. The stuff I came up with is to make active_stripes per stripe hash
as well, with which your above concerns could be eliminated.

I even wrote the code, and it became a bit uglier than I thought, what's
more, I see no obvious performance benifit from it. I then gave it up
and didn't sent it out for bothering you guys.

Well, I wrote that code before I came up with this lock contention fix[0],
it might be a different story if I wrote such code based on the fix. If you
are interested, I guess I could cook it up again and do some tests.

[0]: http://git.neil.brown.name/?p=md.git;a=commit;h=970e1a16559e1865b429e0ec408c9a06f2f83795


BTW, the reason I want to make active_stripes per stripe hash is to
keep the semantics of holding on to wake up processes before we get
1/4 stripes available, which, IMO, is for performance consideration.

And to be honest, I don't have too much clue how the performance will
behave differently in such two cases. I'm gonna to do some test to
figure it out, if you think that's necessary.

	--yliu

> 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 <shli@fb.com>
> ---
>  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);
> -- 
> 1.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] raid5: fix wakeup condition

[Shaohua Li]

On Fri, May 29, 2015 at 03:23:16PM +1000, NeilBrown wrote:
> On Thu, 28 May 2015 17:33:48 -0700 Shaohua Li <shli@fb.com> wrote:
> 
> > 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 <shli@fb.com>
> > ---
> >  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