imsm: fix checking completion of RAID10 resync
PATCH 1/1 imsm: fix checking completion of RAID10 resync
If one creates RAID10 with IMSM metadata the is_resync_complete function returns '1' just when initial resync reaches 50%
IMSM version of the is_resync_complete function has been added that handles the case of IMSM RAID10 correctly.
--- super-intel.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
super-intel.c +18 -2
@@ -1021,6 +1021,22 @@ static int is_failed(struct imsm_disk *disk)return (disk->status &FAILED_DISK ) ==FAILED_DISK ; } +/* IMSM version of is_resync_complete helper routine +* to determine resync completion +* since MaxSector is a moving target +*/ +static int imsm_is_resync_complete (struct mdinfo *array) +{ +if (array->array.level !=10 ) { +if (array->resync_start >= array->component_size) +return 1 ; + }else { +if (array->resync_start >=2 *array->component_size) +return 1 ; + } +return 0 ; +} +/* try to determine how much space is reserved for metadata from * the last get_extents() entry on the smallest active disk, * otherwise fallback to the default
@@ -7119,12 +7135,12 @@ static int imsm_set_array_state(struct active_array *a, int consistent)handle_missing (super, dev);if (consistent ==2 && - (!is_resync_complete (&a->info) || + (!imsm_is_resync_complete (&a->info) || map_state !=IMSM_T_STATE_NORMAL || dev->vol.migr_state)) consistent =0 ; -if (is_resync_complete (&a->info)) { +if (imsm_is_resync_complete (&a->info)) {/* complete intialization / resync, * recovery and interrupted recovery is completed in * ->set_disk
Quoted 44 lines · On Tue, 30 Jul 2013 15:59:25 +0200 Pawel Baldysiak <[email protected]>…
If one creates RAID10 with IMSM metadata the
is_resync_completefunction returns '1' just when initial resync reaches 50%IMSM version of the
is_resync_completefunction has been added that handles the case of IMSM RAID10 correctly.Signed-off-by Pawel Baldysiak <[email protected]>--- super-intel.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)super-intel.c +16
-1@@ -1021,6 +1021,22 @@ static int is_failed(struct imsm_disk *disk)return (disk->status &FAILED_DISK ) ==FAILED_DISK ; } +/* IMSM version of is_resync_complete helper routine +* to determine resync completion +* since MaxSector is a moving target +*/ +static int imsm_is_resync_complete (struct mdinfo *array) +{ +if (array->array.level !=10 ) { +if (array->resync_start >= array->component_size) +return 1 ; + }else { +if (array->resync_start >=2 *array->component_size) +return 1 ; + } +return 0 ; +} +/* try to determine how much space is reserved for metadata from * the last get_extents() entry on the smallest active disk, * otherwise fallback to the default @@ -7119,12 +7135,12 @@ static int imsm_set_array_state(struct active_array *a, int consistent)handle_missing (super, dev); if (consistent == 2 && - (!is_resync_complete(&a->info) ||
super-intel.c static int imsm_set_array_state(struct active_array *a, int consistent) · line 7146
Quoted 10 lines
super-intel.c +2
-1+ (!imsm_is_resync_complete (&a->info) || map_state !=IMSM_T_STATE_NORMAL || dev->vol.migr_state)) consistent = 0; -if (is_resync_complete (&a->info)) { +if (imsm_is_resync_complete (&a->info)) {/* complete intialization / resync, * recovery and interrupted recovery is completed in * ->set_disk
Thanks.
However the bug is not specific to intel, so should be fixed in common code. And "2*" is not really very general.
The following patch should fix it properly. If you can confirm that it fixes the problem for you I would appreciate it.
Thanks,
NeilBrown
From 71d68ff62f945254240575cd836f5f2a09ced5d2 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Wed, 31 Jul 2013 09:18:57 +1000
Subject: [PATCH] Fix is_resync_complete for RAID10
For RAID10, 'sync' numbers go up to the array size rather than the component size. is_resync_complete() needs to allow for this.
mdmon.h +17 -3
@@ -91,7 +91,21 @@ extern int monitor_loop_cnt;*/ static inline int is_resync_complete (struct mdinfo *array) { -if (array->resync_start >= array->component_size) -return 1 ; -return 0 ; +unsigned long long sync_size =0 ; +int ncopies, l; +switch (array->array.level) { +case 1 : +case 4 : +case 5 : +case 6 : + sync_size = array->component_size; +break ; +case 10 : + l = array->array.layout; + ncopies = (l &0xff ) * ((l >>8 ) &&0xff ); + sync_size = array->component_size * array->array.raid_disks; + sync_size /= ncopies; +break ; + } +return array->resync_start >= sync_size; }
Quoted 64 lines · NeilBrown
super-intel.c static int imsm_set_array_state(struct
Quoted 40 lines · On Tue, 30 Jul 2013 15:59:25 +0200 Pawel Baldysiak <[email protected]>…
If one creates RAID10 with IMSM metadata the
is_resync_completefunction returns '1' just when initial resync reaches 50%IMSM version of the
is_resync_completefunction has been added that handles the case of IMSM RAID10 correctly.Signed-off-by Pawel Baldysiak <[email protected]>--- super-intel.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)super-intel.c +16
-0@@ -1021,6 +1021,22 @@ static int is_failed(struct imsm_disk *disk)return (disk->status &FAILED_DISK ) ==FAILED_DISK ; } +/* IMSM version of is_resync_complete helper routine +* to determine resync completion +* since MaxSector is a moving target +*/ +static int imsm_is_resync_complete (struct mdinfo *array) +{ +if (array->array.level !=10 ) { +if (array->resync_start >= array->component_size) +return 1 ; + }else { +if (array->resync_start >=2 *array->component_size) +return 1 ; + } +return 0 ; +} +/* try to determine how much space is reserved for metadata from * the last get_extents() entry on the smallest active disk, * otherwise fallback to the default @@ -7119,12 +7135,12 @@ static int imsm_set_array_state(struct
active_array*a, int consistent)Quoted 4 lines
super-intel.c +0
-1handle_missing (super, dev); if (consistent == 2 && - (!is_resync_complete(&a->info) ||super-intel.c static int imsm_set_array_state(struct · line 7146
Quoted 10 lines
super-intel.c +2
-1+ (!imsm_is_resync_complete (&a->info) || map_state !=IMSM_T_STATE_NORMAL || dev->vol.migr_state)) consistent = 0; -if (is_resync_complete (&a->info)) { +if (imsm_is_resync_complete (&a->info)) {/* complete intialization / resync, * recovery and interrupted recovery is completed in * ->set_disk Thanks.
However the bug is not specific to intel, so should be fixed in common code. And "2*" is not really very general.
Quoted 3 lines
The following patch should fix it properly. If you can confirm that it fixes the problem for you I would appreciate it.
Hi
Since Pawel is out of office I am responding instead of him.
Your patch does not work for IMSM now, because ncopies is equal 0 and mdmon crashes after dividing by zero - see details below:
Quoted 28 lines
Thanks,
NeilBrownFrom
71d68ff62f945254240575cd836f5f2a09ced5d2Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Wed, 31 Jul 2013 09:18:57 +1000
Subject: [PATCH] Fixis_resync_completefor RAID10For RAID10, 'sync' numbers go up to the array size rather than the component size.
is_resync_complete()needs to allow for this.Reported-by Pawel Baldysiak <[email protected]>Signed-off-by NeilBrown <[email protected]>mdmon.h +3
-3@@ -91,7 +91,21 @@ extern int monitor_loop_cnt;*/ static inline int is_resync_complete (struct mdinfo *array) { -if (array->resync_start >= array->component_size) -return 1 ; -return 0 ; +unsigned long long sync_size =0 ; +int ncopies, l; +switch (array->array.level) {
mdmon.h extern int monitor_loop_cnt; · line 106
Quoted 10 lines
mdmon.h +10
-0+case 1 : +case 4 : +case 5 : +case 6 : + sync_size = array->component_size; +break ; +case 10 : + l = array->array.layout; + ncopies = (l &0xff ) * ((l >>8 ) &&0xff ); + sync_size = array->component_size * array->array.raid_disks;
At this point of code the following variables equal (I have created RAID10 volume of size z=10G):
array->array.layout = 0
ncopies = 0array->component_size = 20971520array->array.raid_disks = 4sync_size = 83886080array->resync_start = 0
I will check why array->array.layout is equal 0.
Regards,
Lukasz
Quoted 5 lines
mdmon.h +4
-0+ sync_size /= ncopies; +break ; + } +return array->resync_start >= sync_size; }
Quoted 118 lines · Lukasz Dorau
Quoted 64 lines · NeilBrown
super-intel.c static int imsm_set_array_state(struct
Quoted 40 lines · On Tue, 30 Jul 2013 15:59:25 +0200 Pawel Baldysiak <[email protected]>…
If one creates RAID10 with IMSM metadata the
is_resync_completefunction returns '1' just when initial resync reaches 50%IMSM version of the
is_resync_completefunction has been added that handles the case of IMSM RAID10 correctly.Signed-off-by Pawel Baldysiak <[email protected]>--- super-intel.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)super-intel.c +16
-0@@ -1021,6 +1021,22 @@ static int is_failed(struct imsm_disk *disk)return (disk->status &FAILED_DISK ) ==FAILED_DISK ; } +/* IMSM version of is_resync_complete helper routine +* to determine resync completion +* since MaxSector is a moving target +*/ +static int imsm_is_resync_complete (struct mdinfo *array) +{ +if (array->array.level !=10 ) { +if (array->resync_start >= array->component_size) +return 1 ; + }else { +if (array->resync_start >=2 *array->component_size) +return 1 ; + } +return 0 ; +} +/* try to determine how much space is reserved for metadata from * the last get_extents() entry on the smallest active disk, * otherwise fallback to the default @@ -7119,12 +7135,12 @@ static int imsm_set_array_state(struct
active_array*a, int consistent)Quoted 4 lines
super-intel.c +0
-1handle_missing (super, dev); if (consistent == 2 && - (!is_resync_complete(&a->info) ||super-intel.c static int imsm_set_array_state(struct · line 7146
Quoted 10 lines
super-intel.c +2
-1+ (!imsm_is_resync_complete (&a->info) || map_state !=IMSM_T_STATE_NORMAL || dev->vol.migr_state)) consistent = 0; -if (is_resync_complete (&a->info)) { +if (imsm_is_resync_complete (&a->info)) {/* complete intialization / resync, * recovery and interrupted recovery is completed in * ->set_disk Thanks.
However the bug is not specific to intel, so should be fixed in common code. And "2*" is not really very general.Quoted 3 lines
The following patch should fix it properly. If you can confirm that it fixes the problem for you I would appreciate it.
Hi
Since Pawel is out of office I am responding instead of him. Your patch does not work for IMSM now, because ncopies is equal 0 and mdmon crashes after dividing by zero - see details below:
Quoted 4 lines
Thanks,
NeilBrownFrom
71d68ff62f945254240575cd836f5f2a09ced5d2Mon Sep 17 00:00:002001
Quoted 24 lines
From: NeilBrown <[email protected]>
Date: Wed, 31 Jul 2013 09:18:57 +1000
Subject: [PATCH] Fixis_resync_completefor RAID10For RAID10, 'sync' numbers go up to the array size rather than the component size.
is_resync_complete()needs to allow for this.Reported-by Pawel Baldysiak <[email protected]>Signed-off-by NeilBrown <[email protected]>mdmon.h +3
-3@@ -91,7 +91,21 @@ extern int monitor_loop_cnt;*/ static inline int is_resync_complete (struct mdinfo *array) { -if (array->resync_start >= array->component_size) -return 1 ; -return 0 ; +unsigned long long sync_size =0 ; +int ncopies, l; +switch (array->array.level) {mdmon.h extern int monitor_loop_cnt; · line 106
Quoted 10 lines
mdmon.h +10
-0+case 1 : +case 4 : +case 5 : +case 6 : + sync_size = array->component_size; +break ; +case 10 : + l = array->array.layout; + ncopies = (l &0xff ) * ((l >>8 ) &&0xff ); + sync_size = array->component_size * array->array.raid_disks;At this point of code the following variables equal (I have created RAID10 volume of size z=10G):
Quoted 9 lines
array->array.layout= 0
ncopies = 0array->component_size= 20971520array->array.raid_disks= 4sync_size= 83886080array->resync_start= 0I will check why
array->array.layoutis equal 0.
There is another, more serious, problem.
When we stop the array during initial resync (mdadm -Ss)
and the function is_resync_complete() is entered for the last time, array->array.raid_disks already equals 0, because it is zero'ed by manager:
a->info.array.raid_disks = mdstat->raid_disks;
at managemon.c:454.
As a result sync_size equals 0 and is_resync_complete() incorrectly returns 1 and resync finishes...
It seems to be a race condition between monitor and manager - manager changes value of array.raid_disks too fast.
Regards,
Lukasz
Quoted 11 lines
mdmon.h extern int monitor_loop_cnt; · line 111
Quoted 6 lines
mdmon.h +4
-0+ sync_size /= ncopies; +break ; + } +return array->resync_start >= sync_size; }--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · Lukasz
There is another, more serious, problem.
When we stop the array during initial resync (mdadm -Ss)
and the functionis_resync_complete()is entered for the last time,array->array.raid_disksalready equals 0, because it is zero'ed by manager:a->info.array.raid_disks = mdstat->raid_disks;at managemon.c:454.
As a resultsync_sizeequals 0 andis_resync_complete()incorrectly returns 1 and resync finishes...It seems to be a race condition between monitor and manager - manager changes value of array.raid_disks too fast.
Yes - that is a serious problem. Thanks for reporting it. I think this is the correct fix.
Thanks,
NeilBrown
From e49a8a80265ab2150c96b636450f5825bcd69d4a Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Mon, 5 Aug 2013 15:40:16 +1000
Subject: [PATCH] mdmon: don't use 'ghost' values from an inactive array.
It is possible for mdmon to see (in /proc/mdstat) and array in 'inactive' state, "mdadm -S" has written "inactive" to "array_state".
In this state values such as "raid_disk" are not meaningful and so should be ignored by manage_member().
managemon.c +5 -3
@@ -450,9 +450,11 @@ static void manage_member(struct mdstat_ent *mdstat,/* Raced with something */ return ; -// FIXME - a->info.array.raid_disks = mdstat->raid_disks; -// MORE +if (mdstat->active) { +// FIXME + a->info.array.raid_disks = mdstat->raid_disks; +// MORE + }if (sysfs_get_ll (&a->info,NULL ,"component_size" , &component_size) >=0 ) a->info.component_size = component_size <<1 ;