[PATCH 0/1] IMSM do not assume prev + current map in same state means reshape

[PATCH 0/1] IMSM do not assume prev + current map in same state means reshape

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

I believe there is a bug in the current IMSM code. It makes the
assumption that if the state of the previous map and the current map
are the same, it means we are trying to reshape the array. However if
you have a large array and just installed on it, and it was still
resyncing when you rebooted, it will come up in the same state and
'mdadm -Aa' will fail with an error that it is lacking a resync file.

This is what prevents an update Fedora 15 system from booting, such as
https://bugzilla.redhat.com/show_bug.cgi?id=736387

If I am missing something here, I'd appreciate to hear about it.

Thanks,
Jes

Jes Sorensen (1):
  IMSM only run reshape if number of disks has changed

 super-intel.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

-- 
1.7.6.4

[PATCH 1/1] IMSM only run reshape if number of disks has changed

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

The IMSM code incorrectly made the assumption that reshape also meant
resync, and used 1 to indicate resync and 2 to indicate a reshape.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 super-intel.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 62ccd15..426721d 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1766,18 +1766,14 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
 	info->custom_array_size   = __le32_to_cpu(dev->size_high);
 	info->custom_array_size   <<= 32;
 	info->custom_array_size   |= __le32_to_cpu(dev->size_low);
-	if (prev_map && map->map_state == prev_map->map_state) {
+	info->delta_disks	  = 0;
+	if (prev_map)
+		info->delta_disks = map->num_members - prev_map->num_members;
+	if (info->delta_disks) {
 		info->reshape_active = 1;
 		info->new_level = get_imsm_raid_level(map);
 		info->new_layout = imsm_level_to_layout(info->new_level);
 		info->new_chunk = __le16_to_cpu(map->blocks_per_strip) << 9;
-		info->delta_disks = map->num_members - prev_map->num_members;
-		if (info->delta_disks) {
-			/* this needs to be applied to every array
-			 * in the container.
-			 */
-			info->reshape_active = 2;
-		}
 		/* We shape information that we give to md might have to be
 		 * modify to cope with md's requirement for reshaping arrays.
 		 * For example, when reshaping a RAID0, md requires it to be
@@ -1812,7 +1808,6 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
 		info->new_level = UnSet;
 		info->new_layout = UnSet;
 		info->new_chunk = info->array.chunk_size;
-		info->delta_disks = 0;
 	}
 	info->disk.major = 0;
 	info->disk.minor = 0;
-- 
1.7.6.4

RE: [PATCH 0/1] IMSM do not assume prev + current map in same state means reshape

[Kwolek, Adam]

> -----Original Message-----
> From: Jes.Sorensen@redhat.com [mailto:Jes.Sorensen@redhat.com]
> Sent: Monday, October 17, 2011 10:40 PM
> To: linux-raid@vger.kernel.org
> Cc: neilb@suse.de; dledford@redhat.com; Dorau, Lukasz; Kwolek, Adam;
> Labun, Marcin
> Subject: [PATCH 0/1] IMSM do not assume prev + current map in same state
> means reshape
> 
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> I believe there is a bug in the current IMSM code. It makes the assumption
> that if the state of the previous map and the current map are the same, it
> means we are trying to reshape the array. However if you have a large array
> and just installed on it, and it was still resyncing when you rebooted, it will
> come up in the same state and 'mdadm -Aa' will fail with an error that it is
> lacking a resync file.
> 
> This is what prevents an update Fedora 15 system from booting, such as
> https://bugzilla.redhat.com/show_bug.cgi?id=736387
> 
> If I am missing something here, I'd appreciate to hear about it.
> 
> Thanks,
> Jes


Hi,
There is bug there as you pointed in your email, but it is already fixed in Neil's repository by commit :
	imsm: fix: stopped resync does not continue after auto-assemblation
	b601104eb4a4733a838fb86e9e279fed14ce9d3f

Your patch "IMSM do not assume prev + current map in same state means reshape" can break
e.g chunk size migration when number of disks doesn't change. 

Correct fix is pointed above and it tests migration type.

BR
Adam


> 
> Jes Sorensen (1):
>   IMSM only run reshape if number of disks has changed
> 
>  super-intel.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> --
> 1.7.6.4

Re: [PATCH 0/1] IMSM do not assume prev + current map in same state means reshape

[Jes Sorensen]

On 10/18/11 09:09, Kwolek, Adam wrote:
> 
> 
>> -----Original Message-----
>> From: Jes.Sorensen@redhat.com [mailto:Jes.Sorensen@redhat.com]
>> I believe there is a bug in the current IMSM code. It makes the assumption
>> that if the state of the previous map and the current map are the same, it
>> means we are trying to reshape the array. However if you have a large array
>> and just installed on it, and it was still resyncing when you rebooted, it will
>> come up in the same state and 'mdadm -Aa' will fail with an error that it is
>> lacking a resync file.
>>
>> This is what prevents an update Fedora 15 system from booting, such as
>> https://bugzilla.redhat.com/show_bug.cgi?id=736387
>>
>> If I am missing something here, I'd appreciate to hear about it.
>>
>> Thanks,
>> Jes
> 
> 
> Hi,
> There is bug there as you pointed in your email, but it is already fixed in Neil's repository by commit :
> 	imsm: fix: stopped resync does not continue after auto-assemblation
> 	b601104eb4a4733a838fb86e9e279fed14ce9d3f
> 
> Your patch "IMSM do not assume prev + current map in same state means reshape" can break
> e.g chunk size migration when number of disks doesn't change. 
> 
> Correct fix is pointed above and it tests migration type.

Interesting, this fix went in just after I started doing my testing so I
don't seem to have it in my tree. Missed it by a day or two :(

I'll refresh and try it out.

Jes