[PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[ZhengYuan Liu]

The counter conf->empty_inactive_list_nr is only used for determine if the
raid5 is congested which is deal with in function raid5_congested().
It was increased in get_free_stripe() when conf->inactive_list got to be
empty and decreased in release_inactive_stripe_list() when splice
temp_inactive_list to conf->inactive_list. However, this may have a
problem when raid5_get_active_stripe or stripe_add_to_batch_list was called,
because these two functions may call list_del_init(&sh->lru) to delete sh from
"conf->inactive_list + hash" which may cause "conf->inactive_list + hash" to
be empty when atomic_inc_not_zero(&sh->count) got false. So a check should be
done at these two point and increase empty_inactive_list_nr accordingly.
Otherwise the counter may get to be negative number which would influence
async readahead from VFS.

Signed-off-by: ZhengYuan Liu <liuzhengyuan@kylinos.cn>
---
 drivers/md/raid5.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7c53861..1656c44 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -659,6 +659,7 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
 {
 	struct stripe_head *sh;
 	int hash = stripe_hash_locks_hash(sector);
+	int inc_empty_inactive_list_flag;
 
 	pr_debug("get_stripe, sector %llu\n", (unsigned long long)sector);
 
@@ -703,7 +704,12 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
 					atomic_inc(&conf->active_stripes);
 				BUG_ON(list_empty(&sh->lru) &&
 				       !test_bit(STRIPE_EXPANDING, &sh->state));
+				inc_empty_inactive_list_flag = 0;
+				if (!list_empty(conf->inactive_list + hash))
+					inc_empty_inactive_list_flag = 1;
 				list_del_init(&sh->lru);
+				if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
+					atomic_inc(&conf->empty_inactive_list_nr);
 				if (sh->group) {
 					sh->group->stripes_cnt--;
 					sh->group = NULL;
@@ -762,6 +768,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
 	sector_t head_sector, tmp_sec;
 	int hash;
 	int dd_idx;
+	int inc_empty_inactive_list_flag;
 
 	if (!stripe_can_batch(sh))
 		return;
@@ -781,7 +788,11 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
 				atomic_inc(&conf->active_stripes);
 			BUG_ON(list_empty(&head->lru) &&
 			       !test_bit(STRIPE_EXPANDING, &head->state));
+			if (!list_empty(conf->inactive_list + hash))
+				inc_empty_inactive_list_flag = 1;
 			list_del_init(&head->lru);
+			if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
+				atomic_inc(&conf->empty_inactive_list_nr);
 			if (head->group) {
 				head->group->stripes_cnt--;
 				head->group = NULL;
-- 
1.9.1

Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[Shaohua Li]

On Thu, Jul 28, 2016 at 02:22:14PM +0800, ZhengYuan Liu wrote:
> The counter conf->empty_inactive_list_nr is only used for determine if the
> raid5 is congested which is deal with in function raid5_congested().
> It was increased in get_free_stripe() when conf->inactive_list got to be
> empty and decreased in release_inactive_stripe_list() when splice
> temp_inactive_list to conf->inactive_list. However, this may have a
> problem when raid5_get_active_stripe or stripe_add_to_batch_list was called,
> because these two functions may call list_del_init(&sh->lru) to delete sh from
> "conf->inactive_list + hash" which may cause "conf->inactive_list + hash" to
> be empty when atomic_inc_not_zero(&sh->count) got false. So a check should be
> done at these two point and increase empty_inactive_list_nr accordingly.
> Otherwise the counter may get to be negative number which would influence
> async readahead from VFS.
> 
> Signed-off-by: ZhengYuan Liu <liuzhengyuan@kylinos.cn>
Applied, thanks!

Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[NeilBrown]

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

On Thu, Jul 28 2016, ZhengYuan Liu wrote:

> The counter conf->empty_inactive_list_nr is only used for determine if the
> raid5 is congested which is deal with in function raid5_congested().
> It was increased in get_free_stripe() when conf->inactive_list got to be
> empty and decreased in release_inactive_stripe_list() when splice
> temp_inactive_list to conf->inactive_list. However, this may have a
> problem when raid5_get_active_stripe or stripe_add_to_batch_list was called,
> because these two functions may call list_del_init(&sh->lru) to delete sh from
> "conf->inactive_list + hash" which may cause "conf->inactive_list + hash" to
> be empty when atomic_inc_not_zero(&sh->count) got false. So a check should be
> done at these two point and increase empty_inactive_list_nr accordingly.
> Otherwise the counter may get to be negative number which would influence
> async readahead from VFS.
>
> Signed-off-by: ZhengYuan Liu <liuzhengyuan@kylinos.cn>
> ---
>  drivers/md/raid5.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 7c53861..1656c44 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -659,6 +659,7 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
>  {
>  	struct stripe_head *sh;
>  	int hash = stripe_hash_locks_hash(sector);
> +	int inc_empty_inactive_list_flag;
>  
>  	pr_debug("get_stripe, sector %llu\n", (unsigned long long)sector);
>  
> @@ -703,7 +704,12 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
>  					atomic_inc(&conf->active_stripes);
>  				BUG_ON(list_empty(&sh->lru) &&
>  				       !test_bit(STRIPE_EXPANDING, &sh->state));
> +				inc_empty_inactive_list_flag = 0;
> +				if (!list_empty(conf->inactive_list + hash))
> +					inc_empty_inactive_list_flag = 1;
>  				list_del_init(&sh->lru);
> +				if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
> +					atomic_inc(&conf->empty_inactive_list_nr);

Maybe I'm forgetting an important detail, but this seems more
complicated than it needs to be.
The code just just confirmed that sh->count is zero, so sh must be on
the inactive list, mustn't it?
So inc_empty_inactive_list_flag can never be set to 1.

What am I missing?  Could sh not be on the inactive list at this point?

Same for the code below.

NeilBrown


>  				if (sh->group) {
>  					sh->group->stripes_cnt--;
>  					sh->group = NULL;
> @@ -762,6 +768,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
>  	sector_t head_sector, tmp_sec;
>  	int hash;
>  	int dd_idx;
> +	int inc_empty_inactive_list_flag;
>  
>  	if (!stripe_can_batch(sh))
>  		return;
> @@ -781,7 +788,11 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
>  				atomic_inc(&conf->active_stripes);
>  			BUG_ON(list_empty(&head->lru) &&
>  			       !test_bit(STRIPE_EXPANDING, &head->state));
> +			if (!list_empty(conf->inactive_list + hash))
> +				inc_empty_inactive_list_flag = 1;
>  			list_del_init(&head->lru);
> +			if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
> +				atomic_inc(&conf->empty_inactive_list_nr);
>  			if (head->group) {
>  				head->group->stripes_cnt--;
>  				head->group = NULL;
> -- 
> 1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[NeilBrown]

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

On Thu, Jul 28 2016, ZhengYuan Liu wrote:

> The counter conf->empty_inactive_list_nr is only used for determine if the
> raid5 is congested which is deal with in function raid5_congested().
> It was increased in get_free_stripe() when conf->inactive_list got to be
> empty and decreased in release_inactive_stripe_list() when splice
> temp_inactive_list to conf->inactive_list. However, this may have a
> problem when raid5_get_active_stripe or stripe_add_to_batch_list was called,
> because these two functions may call list_del_init(&sh->lru) to delete sh from
> "conf->inactive_list + hash" which may cause "conf->inactive_list + hash" to
> be empty when atomic_inc_not_zero(&sh->count) got false. So a check should be
> done at these two point and increase empty_inactive_list_nr accordingly.
> Otherwise the counter may get to be negative number which would influence
> async readahead from VFS.
>
> Signed-off-by: ZhengYuan Liu <liuzhengyuan@kylinos.cn>
> ---
>  drivers/md/raid5.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 7c53861..1656c44 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -659,6 +659,7 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
>  {
>  	struct stripe_head *sh;
>  	int hash = stripe_hash_locks_hash(sector);
> +	int inc_empty_inactive_list_flag;
>  
>  	pr_debug("get_stripe, sector %llu\n", (unsigned long long)sector);
>  
> @@ -703,7 +704,12 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
>  					atomic_inc(&conf->active_stripes);
>  				BUG_ON(list_empty(&sh->lru) &&
>  				       !test_bit(STRIPE_EXPANDING, &sh->state));
> +				inc_empty_inactive_list_flag = 0;
> +				if (!list_empty(conf->inactive_list + hash))
> +					inc_empty_inactive_list_flag = 1;
>  				list_del_init(&sh->lru);
> +				if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
> +					atomic_inc(&conf->empty_inactive_list_nr);

Maybe I'm forgetting an important detail, but this seems more
complicated than it needs to be.
The code just just confirmed that sh->count is zero, so sh must be on
the inactive list, mustn't it?
So inc_empty_inactive_list_flag can never be set to 1.

What am I missing?  Could sh not be on the inactive list at this point?

Same for the code below.

NeilBrown


>  				if (sh->group) {
>  					sh->group->stripes_cnt--;
>  					sh->group = NULL;
> @@ -762,6 +768,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
>  	sector_t head_sector, tmp_sec;
>  	int hash;
>  	int dd_idx;
> +	int inc_empty_inactive_list_flag;
>  
>  	if (!stripe_can_batch(sh))
>  		return;
> @@ -781,7 +788,11 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
>  				atomic_inc(&conf->active_stripes);
>  			BUG_ON(list_empty(&head->lru) &&
>  			       !test_bit(STRIPE_EXPANDING, &head->state));
> +			if (!list_empty(conf->inactive_list + hash))
> +				inc_empty_inactive_list_flag = 1;
>  			list_del_init(&head->lru);
> +			if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
> +				atomic_inc(&conf->empty_inactive_list_nr);
>  			if (head->group) {
>  				head->group->stripes_cnt--;
>  				head->group = NULL;
> -- 
> 1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[NeilBrown]

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

On Wed, Aug 03 2016, liuzhengyuan wrote:

> Thanks for you replay.
> I think it may be on the temp inactive list. An active sh was handled
> and put to temp inactive list firstly, then moved to inactive list.
> If sh is on the temp inactive list,  sh->count is zero too.

Hmmm... yes, you are probably right.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[liuzhengyuan]

Thanks for you replay.
I think it may be on the temp inactive list. An active sh was handled
and put to temp inactive list firstly, then moved to inactive list.
If sh is on the temp inactive list,  sh->count is zero too.

------------------ Original ------------------
From:  "NeilBrown"<neilb@suse.com>;
Date:  Wed, Aug 3, 2016 08:44 AM
To:  "ZhengYuan Liu"<liuzhengyuan@kylinos.cn>;
Cc:  "shli"<shli@kernel.org>; "liuzhengyuang521"<liuzhengyuang521@gmail.com>; "linux-raid"<linux-raid@vger.kernel.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "ZhengYuan Liu"<liuzhengyuan@kylinos.cn>;
Subject:  Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr
 
On Thu, Jul 28 2016, ZhengYuan Liu wrote:

> The counter conf->empty_inactive_list_nr is only used for determine if the
> raid5 is congested which is deal with in function raid5_congested().
> It was increased in get_free_stripe() when conf->inactive_list got to be
> empty and decreased in release_inactive_stripe_list() when splice
> temp_inactive_list to conf->inactive_list. However, this may have a
> problem when raid5_get_active_stripe or stripe_add_to_batch_list was called,
> because these two functions may call list_del_init(&sh->lru) to delete sh from
> "conf->inactive_list + hash" which may cause "conf->inactive_list + hash" to
> be empty when atomic_inc_not_zero(&sh->count) got false. So a check should be
> done at these two point and increase empty_inactive_list_nr accordingly.
> Otherwise the counter may get to be negative number which would influence
> async readahead from VFS.
>
> Signed-off-by: ZhengYuan Liu <liuzhengyuan@kylinos.cn>
> ---
>  drivers/md/raid5.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 7c53861..1656c44 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -659,6 +659,7 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
>  {
>  struct stripe_head *sh;
>  int hash = stripe_hash_locks_hash(sector);
> +	int inc_empty_inactive_list_flag;
>  
>  pr_debug("get_stripe, sector %llu\n", (unsigned long long)sector);
>  
> @@ -703,7 +704,12 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
>  atomic_inc(&conf->active_stripes);
>  BUG_ON(list_empty(&sh->lru) &&
>         !test_bit(STRIPE_EXPANDING, &sh->state));
> +	inc_empty_inactive_list_flag = 0;
> +	if (!list_empty(conf->inactive_list + hash))
> +	inc_empty_inactive_list_flag = 1;
>  list_del_init(&sh->lru);
> +	if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
> +	atomic_inc(&conf->empty_inactive_list_nr);

Maybe I'm forgetting an important detail, but this seems more
complicated than it needs to be.
The code just just confirmed that sh->count is zero, so sh must be on
the inactive list, mustn't it?
So inc_empty_inactive_list_flag can never be set to 1.

What am I missing?  Could sh not be on the inactive list at this point?

Same for the code below.

NeilBrown


>  if (sh->group) {
>  sh->group->stripes_cnt--;
>  sh->group = NULL;
> @@ -762,6 +768,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
>  sector_t head_sector, tmp_sec;
>  int hash;
>  int dd_idx;
> +	int inc_empty_inactive_list_flag;
>  
>  if (!stripe_can_batch(sh))
>  return;
> @@ -781,7 +788,11 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
>  atomic_inc(&conf->active_stripes);
>  BUG_ON(list_empty(&head->lru) &&
>         !test_bit(STRIPE_EXPANDING, &head->state));
> +	if (!list_empty(conf->inactive_list + hash))
> +	inc_empty_inactive_list_flag = 1;
>  list_del_init(&head->lru);
> +	if (list_empty(conf->inactive_list + hash) && inc_empty_inactive_list_flag)
> +	atomic_inc(&conf->empty_inactive_list_nr);
>  if (head->group) {
>  head->group->stripes_cnt--;
>  head->group = NULL;
> -- 
> 1.9.1

Re: [PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[Shaohua Li]

On Wed, Jul 20, 2016 at 05:22:00PM +0800, Zhengyuan Liu wrote:
> The counter conf->empty_inactive_list_nr is only used for determine if
> the raid5 is congested which is deal with in function
> raid5_congested(). It was increased in get_free_stripe() when
> conf->inactive_list got emptied and decreased in
> release_inactive_stripe_list() when  splice temp_inactive_list to
> conf->inactive_list. However, this could cause to problem when
> raid5_get_active_stripe was called, this function may call
> list_del_init(&sh->lru) to delete sh from  "conf->inactive_list +
> hash" which may cause  "conf->inactive_list + hash" to be empty when
> atomic_inc_not_zero(&sh->count) got false.
>  I have found conf->empty_inactive_list_nr to be negative number
> during my random test.
> Is there anything out of my thinking?

Good catch! I think you are right, we get a stripe from inactive list and
should decrement the count. Also we should do the same thing for
stripe_add_to_batch_list. Please resend the patch with proper format, see
Documentation/email-clients.txt

Thanks,
Shaohua

[PATCH] raid5: fix incorrectly counter of conf->empty_inactive_list_nr

[Zhengyuan Liu]

The counter conf->empty_inactive_list_nr is only used for determine if
the raid5 is congested which is deal with in function
raid5_congested(). It was increased in get_free_stripe() when
conf->inactive_list got emptied and decreased in
release_inactive_stripe_list() when  splice temp_inactive_list to
conf->inactive_list. However, this could cause to problem when
raid5_get_active_stripe was called, this function may call
list_del_init(&sh->lru) to delete sh from  "conf->inactive_list +
hash" which may cause  "conf->inactive_list + hash" to be empty when
atomic_inc_not_zero(&sh->count) got false.
 I have found conf->empty_inactive_list_nr to be negative number
during my random test.
Is there anything out of my thinking?

--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -659,6 +659,7 @@ raid5_get_active_stripe(struct r5conf *conf,
sector_t sector,
 {
        struct stripe_head *sh;
        int hash = stripe_hash_locks_hash(sector);
+       int inc_empty_inactive_list_flag;

        pr_debug("get_stripe, sector %llu\n", (unsigned long long)sector);

@@ -703,7 +704,14 @@ raid5_get_active_stripe(struct r5conf *conf,
sector_t sector,
                                        atomic_inc(&conf->active_stripes);
                                BUG_ON(list_empty(&sh->lru) &&
                                       !test_bit(STRIPE_EXPANDING, &sh->state));
+
+                               inc_empty_inactive_list_flag = 0;
+                               if(!list_empty(conf->inactive_list + hash))
+                                       inc_empty_inactive_list_flag = 1;
                                list_del_init(&sh->lru);
+                               if(list_empty(conf->inactive_list +
hash) && inc_empty_inactive_list_flag)
+
atomic_inc(&conf->empty_inactive_list_nr);
+
                                if (sh->group) {
                                        sh->group->stripes_cnt--;
                                        sh->group = NULL;