[PATCH 0/5] Make OLCE workeable

[PATCH 0/5] Make OLCE workeable

[Adam Kwolek]

This is first part of my comments for your latest changes on devel3-2 regarding reshape and reshape restart.
Unfortunately those changes breaks OLCE functionality. This patch series makes OLCE workable (mdadm UT suits 12 and 13 succeed).

Problems:
1. In passing in mdinfo (array.level) raid4 for transition raid0->raid0.
   This causes problems in takeover
   a.Raid0 takeover doesn't occur. array.level == reshape.level == 4
     Resolution: Decision about takeover has to be made based on real md level not info->array.level
   b.Decision about backward takeover cannot be always taken by mdadm. mdadm can think that original level is raid4
     Resolution: use level read from md as orig_level (not from metadata)

2.On the begin of reshape_array() array structure is read from md (via ioctrl). Md knows nothing about raid4 working level, so spares_needed is wrongly computed
  Resolution: use info->array.raid_disks instead array.raid_disks to compute  spares_needed.
3.Analyse_changes() delta_parity concept doesn't match information currently metadata reports. Result of this is wrongly computed reshape.after.data_disks field.
  Resolution: do not use delta_parity, base on information from metadata handle
4.Defects/potential defects
 a.delta_disks should be checked in sysfs_set_array()
 b.array assembly problem
reshape_active flag has to be evaluated. Existing second map doesn't means reshape always (rebuild, initialization)

I've tried to follow your idea in this series. Generally it seems ok (proved by UT). I like putting more array configuration in sysfs_set_array().

Now I'm going to look at patches at the reshape restart point of view.
I'll let you know results soon.

BR
Adam


---

Adam Kwolek (5):
      FIX: Compute spares_needed basing on metadata info
      FIX: Remove delta_parity concept
      FIX: Allow for takeover
      FIX: Set new raid disks when delta_disks is valid
      fix: array is reassembled inactive if stopped during resync


 Grow.c        |   22 +++++-----------------
 super-intel.c |    5 +++--
 sysfs.c       |    4 +++-
 3 files changed, 11 insertions(+), 20 deletions(-)

-- 
Signature

[PATCH 1/5] fix: array is reassembled inactive if stopped during resync

[Adam Kwolek]

This is adaptation of previous fix to avoid problem appear again.

If initial resync or recovery of a redundant array is not finished
before it is stopped then during assembly md will start it as inactive.
Writing readonly to array_state in assemble_container_content
fails because md thinks the array is during reshape.

In fact we only have a reshape if both current and previous map
states are the same. Otherwise we may have resync or recovery.
Setting reshape_active in such cases causes the issue.

Signed-off-by: Anna Czarnowska <anna.czarnowska@intel.com>
Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---

 super-intel.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 90faf27..4628f2d 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1758,8 +1758,9 @@ 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->reshape_active = 1;
+	info->reshape_active = (prev_map != NULL) &&
+			       (map->map_state == prev_map->map_state);
+	if (info->reshape_active) {
 		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;

[PATCH 2/5] FIX: Set new raid disks when delta_disks is valid

[Adam Kwolek]

(Possible problem)
When delta_disks is not equal to 0, and is not UnSet (INT_MAX)
new raid disks can be set. Especially in second case passed value
can cause an error in array configuration.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 sysfs.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sysfs.c b/sysfs.c
index a7dfcc2..571eab5 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -575,7 +575,9 @@ int sysfs_set_array(struct mdinfo *info, int vers)
 				    info->reshape_progress);
 		rv |= sysfs_set_num(info, NULL, "chunk_size", info->new_chunk);
 		rv |= sysfs_set_num(info, NULL, "layout", info->new_layout);
-		rv |= sysfs_set_num(info, NULL, "raid_disks",
+		if ((info->delta_disks != 0) &&
+		    (info->delta_disks != UnSet))
+			rv |= sysfs_set_num(info, NULL, "raid_disks",
 				    info->array.raid_disks + info->delta_disks);
 		/* We don't set 'new_level' here.  That can only happen
 		 * once the reshape completes.

[PATCH 3/5] FIX: Allow for takeover

[Adam Kwolek]

Takeover has to occur when md reports different raid level.
Previously it bases on information from metadata.
Now metadata reports 'working' level as array level.
This makes us to check it using information from md.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 Grow.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Grow.c b/Grow.c
index 88c8ad7..5c5a3a3 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1688,7 +1688,7 @@ static int reshape_array(char *container, int fd, char *devname,
 		goto release;
 	}
 
-	if (reshape.level != info->array.level) {
+	if (reshape.level != array.level) {
 		char *c = map_num(pers, reshape.level);
 		int err;
 		if (c == NULL)
@@ -1708,7 +1708,7 @@ static int reshape_array(char *container, int fd, char *devname,
 		if (!quiet)
 			fprintf(stderr, Name ": level of %s changed to %s\n",
 				devname, c);	
-		orig_level = info->array.level;
+		orig_level = array.level;
 		sysfs_freeze_array(info);
 
 		if (reshape.level > 0 && st->ss->external) {

[PATCH 4/5] FIX: Remove delta_parity concept

[Adam Kwolek]

When metadata reports level/raid_disks correct for level transition
delta_parity concept is in conflict with such approach.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 Grow.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/Grow.c b/Grow.c
index 5c5a3a3..a05bcef 100644
--- a/Grow.c
+++ b/Grow.c
@@ -893,10 +893,6 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 	 * when assembling an array that is undergoing reshape.
 	 */
 	int new_disks;
-	/* delta_parity records change in number of devices
-	 * caused by level change
-	 */
-	int delta_parity = 0;
 
 	/* If a new level not explicitly given, we assume no-change */
 	if (info->new_level == UnSet)
@@ -1045,18 +1041,15 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 		 */
 		switch (info->new_level) {
 		case 4:
-			delta_parity = 1;
 		case 0:
 			re->level = 4;
 			re->before.layout = 0;
 			break;
 		case 5:
-			delta_parity = 1;
 			re->level = 5;
 			re->before.layout = ALGORITHM_PARITY_N;
 			break;
 		case 6:
-			delta_parity = 2;
 			re->level = 6;
 			re->before.layout = ALGORITHM_PARITY_N;
 			break;
@@ -1072,7 +1065,6 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 	case 5:
 		switch (info->new_level) {
 		case 0:
-			delta_parity = -1;
 		case 4:
 			re->level = info->array.level;
 			re->before.data_disks = info->array.raid_disks - 1;
@@ -1084,7 +1076,6 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 			re->before.layout = info->array.layout;
 			break;
 		case 6:
-			delta_parity = 1;
 			re->level = 6;
 			re->before.data_disks = info->array.raid_disks - 1;
 			switch (info->array.layout) {
@@ -1127,7 +1118,6 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 		switch (info->new_level) {
 		case 4:
 		case 5:
-			delta_parity = -1;
 		case 6:
 			re->level = 6;
 			re->before.data_disks = info->array.raid_disks - 2;
@@ -1221,11 +1211,9 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 		return "Impossible level change requested";
 	}
 	if (info->delta_disks == UnSet)
-		info->delta_disks = delta_parity;
+		info->delta_disks = 0;
 
-	re->after.data_disks = (re->before.data_disks
-				+ info->delta_disks
-				- delta_parity);
+	re->after.data_disks = re->before.data_disks + info->delta_disks;
 	switch (re->level) {
 	case 6: re->parity = 2; break;
 	case 4:

[PATCH 5/5] FIX: Compute spares_needed basing on metadata info

[Adam Kwolek]

When metadata raid_disks information is correct for reshape working level,
this information has to be used for spares_needed calculation.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 Grow.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Grow.c b/Grow.c
index a05bcef..68962bf 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1661,7 +1661,7 @@ static int reshape_array(char *container, int fd, char *devname,
 	sysfs_freeze_array(info);
 	spares_needed = max(reshape.before.data_disks,
 			    reshape.after.data_disks)
-		+ reshape.parity - array.raid_disks;
+		+ reshape.parity - info->array.raid_disks;
 
 	if (!force &&
 	    info->new_level > 1 &&

Re: [PATCH 0/5] Make OLCE workeable

[NeilBrown]

On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> This is first part of my comments for your latest changes on devel3-2 regarding reshape and reshape restart.
> Unfortunately those changes breaks OLCE functionality. This patch series makes OLCE workable (mdadm UT suits 12 and 13 succeed).

That's really strange as the patches all look wrong.  I'll do some testing
my self and see what I can come up with.

> 
> Problems:
> 1. In passing in mdinfo (array.level) raid4 for transition raid0->raid0.
>    This causes problems in takeover
>    a.Raid0 takeover doesn't occur. array.level == reshape.level == 4
>      Resolution: Decision about takeover has to be made based on real md level not info->array.level

But takeover doesn't *need* to occur.
When we have a RAID0 array and we want to make it grow, we have to 'takeover'
to RAID4 first.
But when assembling an array that is in the middle of a growth we assemble
it as a RAID4 array - so it never was a RAID0 array.  It starts out as
RAID4.

>    b.Decision about backward takeover cannot be always taken by mdadm. mdadm can think that original level is raid4
>      Resolution: use level read from md as orig_level (not from metadata)

I see that is a problem.   We should record '0' as the 'new_level' and
get mdadm to make use of that information.

> 
> 2.On the begin of reshape_array() array structure is read from md (via ioctrl). Md knows nothing about raid4 working level, so spares_needed is wrongly computed
>   Resolution: use info->array.raid_disks instead array.raid_disks to compute  spares_needed.

md certainly should know about RAID4 working level because it is assembled
as RAID4.... hopefully some testing will show me what you really mean.

> 3.Analyse_changes() delta_parity concept doesn't match information currently metadata reports. Result of this is wrongly computed reshape.after.data_disks field.
>   Resolution: do not use delta_parity, base on information from metadata handle

You cannot just remove delta_parity like that.  It is needed when we start
a growth.  Maybe it is confusing when we restart a growth - I will look
in to in.

> 4.Defects/potential defects
>  a.delta_disks should be checked in sysfs_set_array()

No it shouldn't.  If delta_disks is UnSet, then the metadata handler has
done the wrong thing.

>  b.array assembly problem
> reshape_active flag has to be evaluated. Existing second map doesn't means reshape always (rebuild, initialization)

There is no problem here.  I agree with your second sentence, but that 
code already does this.
That first patch in your series makes *no* change to the meaning
of the code.
Before the patch it is 

  if (condition) {
     variable = 1;
     stuff;
     ...

After the patch it is

  variable = condition;
  if (variable) {
     stuff;
     ...


which has exactly the same effect.

So I haven't applied any of these matches as none of them make any sense to
me - sorry.

NeilBrown



> 
> I've tried to follow your idea in this series. Generally it seems ok (proved by UT). I like putting more array configuration in sysfs_set_array().
> 
> Now I'm going to look at patches at the reshape restart point of view.
> I'll let you know results soon.
> 
> BR
> Adam
> 
> 
> ---
> 
> Adam Kwolek (5):
>       FIX: Compute spares_needed basing on metadata info
>       FIX: Remove delta_parity concept
>       FIX: Allow for takeover
>       FIX: Set new raid disks when delta_disks is valid
>       fix: array is reassembled inactive if stopped during resync
> 
> 
>  Grow.c        |   22 +++++-----------------
>  super-intel.c |    5 +++--
>  sysfs.c       |    4 +++-
>  3 files changed, 11 insertions(+), 20 deletions(-)
> 

Re: [PATCH 3/5] FIX: Allow for takeover

[NeilBrown]

On Tue, 08 Mar 2011 14:24:55 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> Takeover has to occur when md reports different raid level.
> Previously it bases on information from metadata.
> Now metadata reports 'working' level as array level.
> This makes us to check it using information from md.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Grow.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 88c8ad7..5c5a3a3 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1688,7 +1688,7 @@ static int reshape_array(char *container, int fd, char *devname,
>  		goto release;
>  	}
>  
> -	if (reshape.level != info->array.level) {
> +	if (reshape.level != array.level) {
>  		char *c = map_num(pers, reshape.level);
>  		int err;
>  		if (c == NULL)

This patch is correct after all.  However the description
at the top doesn't really to justice to the subtly of why it is needed.
I have fixed that in the version that I committed.

NeilBrown

> @@ -1708,7 +1708,7 @@ static int reshape_array(char *container, int fd, char *devname,
>  		if (!quiet)
>  			fprintf(stderr, Name ": level of %s changed to %s\n",
>  				devname, c);	
> -		orig_level = info->array.level;
> +		orig_level = array.level;
>  		sysfs_freeze_array(info);
>  
>  		if (reshape.level > 0 && st->ss->external) {

RE: [PATCH 0/5] Make OLCE workeable

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Wednesday, March 09, 2011 1:53 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 0/5] Make OLCE workeable
> 
> On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > This is first part of my comments for your latest changes on devel3-2
> regarding reshape and reshape restart.
> > Unfortunately those changes breaks OLCE functionality. This patch
> series makes OLCE workable (mdadm UT suits 12 and 13 succeed).
> 
> That's really strange as the patches all look wrong.  I'll do some
> testing
> my self and see what I can come up with.
> 
> >
> > Problems:
> > 1. In passing in mdinfo (array.level) raid4 for transition raid0-
> >raid0.
> >    This causes problems in takeover
> >    a.Raid0 takeover doesn't occur. array.level == reshape.level == 4
> >      Resolution: Decision about takeover has to be made based on real
> md level not info->array.level
> 
> But takeover doesn't *need* to occur.
> When we have a RAID0 array and we want to make it grow, we have to
> 'takeover'
> to RAID4 first.
> But when assembling an array that is in the middle of a growth we
> assemble
> it as a RAID4 array - so it never was a RAID0 array.  It starts out as
> RAID4.

Takeover has to occur during 'normal' reshape (patches doesn't even touch restart problem).
When during normal start mdadm enters reshape_array() metadata is in reshape state already (reshape_active == 1)
This means that in case raid0, raid4 is reported but in md raid0 is still present.
Due to this 'takeover' has to be executed.

> 
> >    b.Decision about backward takeover cannot be always taken by mdadm.
> mdadm can think that original level is raid4
> >      Resolution: use level read from md as orig_level (not from
> metadata)
> 
> I see that is a problem.   We should record '0' as the 'new_level' and
> get mdadm to make use of that information.

This is true for success path. In error case mdadm exits via 'release' label, where 
md is switched to orig_level (not new_level). As I remember for 'succeess' path it works as you describes already.


> 
> >
> > 2.On the begin of reshape_array() array structure is read from md (via
> ioctrl). Md knows nothing about raid4 working level, so spares_needed is
> wrongly computed
> >   Resolution: use info->array.raid_disks instead array.raid_disks to
> compute  spares_needed.
> 
> md certainly should know about RAID4 working level because it is
> assembled
> as RAID4.... hopefully some testing will show me what you really mean.



During assembly - yes, but for regular reshape start no. 
Regular reshape start:
1. md knows raid0
2. metadata reports raid4 (metadata has reshape_active == 1 already)

Reshape restart
1. md knows raid4
2. metadata reports raid4


> 
> > 3.Analyse_changes() delta_parity concept doesn't match information
> currently metadata reports. Result of this is wrongly computed
> reshape.after.data_disks field.
> >   Resolution: do not use delta_parity, base on information from
> metadata handle
> 
> You cannot just remove delta_parity like that.  It is needed when we
> start
> a growth.  Maybe it is confusing when we restart a growth - I will look
> in to in.

It looks for me that when metadata handler takes responsibility for reporting
Different raid level and increasing raid disks by 'delta parity' we should not 
Count delta parity in different place. 
If you think that analyse_change() should support both cases:
1. reshape handler increases raid_disks by delta parity
2. reshape handler doesn't increase raid_disks by delta parity

I'll fix analyse_changes() for this.




> 
> > 4.Defects/potential defects
> >  a.delta_disks should be checked in sysfs_set_array()
> 
> No it shouldn't.  If delta_disks is UnSet, then the metadata handler has
> done the wrong thing.
> 
> >  b.array assembly problem
> > reshape_active flag has to be evaluated. Existing second map doesn't
> means reshape always (rebuild, initialization)
> 
> There is no problem here.  I agree with your second sentence, but that
> code already does this.
> That first patch in your series makes *no* change to the meaning
> of the code.
> Before the patch it is
> 
>   if (condition) {
>      variable = 1;
>      stuff;
>      ...
> 
> After the patch it is
> 
>   variable = condition;
>   if (variable) {
>      stuff;
>      ...
> 
> 
> which has exactly the same effect.

Of course you are right !!!, I don't know, how I've look at this condition yesterday.

BR
Adam

> 
> So I haven't applied any of these matches as none of them make any sense
> to
> me - sorry.
> 
> NeilBrown
> 
> 
> 
> >
> > I've tried to follow your idea in this series. Generally it seems ok
> (proved by UT). I like putting more array configuration in
> sysfs_set_array().
> >
> > Now I'm going to look at patches at the reshape restart point of view.
> > I'll let you know results soon.
> >
> > BR
> > Adam
> >
> >
> > ---
> >
> > Adam Kwolek (5):
> >       FIX: Compute spares_needed basing on metadata info
> >       FIX: Remove delta_parity concept
> >       FIX: Allow for takeover
> >       FIX: Set new raid disks when delta_disks is valid
> >       fix: array is reassembled inactive if stopped during resync
> >
> >
> >  Grow.c        |   22 +++++-----------------
> >  super-intel.c |    5 +++--
> >  sysfs.c       |    4 +++-
> >  3 files changed, 11 insertions(+), 20 deletions(-)
> >

Re: [PATCH 0/5] Make OLCE workeable

[NeilBrown]

On Wed, 9 Mar 2011 07:45:07 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> > 
> > >
> > > 2.On the begin of reshape_array() array structure is read from md (via
> > ioctrl). Md knows nothing about raid4 working level, so spares_needed is
> > wrongly computed
> > >   Resolution: use info->array.raid_disks instead array.raid_disks to
> > compute  spares_needed.
> > 
> > md certainly should know about RAID4 working level because it is
> > assembled
> > as RAID4.... hopefully some testing will show me what you really mean.
> 
> 
> 
> During assembly - yes, but for regular reshape start no. 
> Regular reshape start:
> 1. md knows raid0
> 2. metadata reports raid4 (metadata has reshape_active == 1 already)
> 
> Reshape restart
> 1. md knows raid4
> 2. metadata reports raid4

The difference was between a container reshape and a single array reshape.


> 
> 
> > 
> > > 3.Analyse_changes() delta_parity concept doesn't match information
> > currently metadata reports. Result of this is wrongly computed
> > reshape.after.data_disks field.
> > >   Resolution: do not use delta_parity, base on information from
> > metadata handle
> > 
> > You cannot just remove delta_parity like that.  It is needed when we
> > start
> > a growth.  Maybe it is confusing when we restart a growth - I will look
> > in to in.
> 
> It looks for me that when metadata handler takes responsibility for reporting
> Different raid level and increasing raid disks by 'delta parity' we should not 
> Count delta parity in different place. 
> If you think that analyse_change() should support both cases:
> 1. reshape handler increases raid_disks by delta parity
> 2. reshape handler doesn't increase raid_disks by delta parity
> 
> I'll fix analyse_changes() for this.
> 


Before you do, have a look at the changes I just pushed out.

With those, most of the tests pass - maybe all of them, I'm not sure.


Thanks,
NeilBrown

RE: [PATCH 0/5] Make OLCE workeable

[Kwolek, Adam]

> -----Original Message-----
> From: Kwolek, Adam
> Sent: Wednesday, March 09, 2011 8:45 AM
> To: 'NeilBrown'
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH 0/5] Make OLCE workeable
> 
> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Wednesday, March 09, 2011 1:53 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 0/5] Make OLCE workeable
> >
> > On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> >
> > > This is first part of my comments for your latest changes on devel3-
> 2
> > regarding reshape and reshape restart.
> > > Unfortunately those changes breaks OLCE functionality. This patch
> > series makes OLCE workable (mdadm UT suits 12 and 13 succeed).
> >
> > That's really strange as the patches all look wrong.  I'll do some
> > testing
> > my self and see what I can come up with.
> >
> > >
> > > Problems:
> > > 1. In passing in mdinfo (array.level) raid4 for transition raid0-
> > >raid0.
> > >    This causes problems in takeover
> > >    a.Raid0 takeover doesn't occur. array.level == reshape.level == 4
> > >      Resolution: Decision about takeover has to be made based on
> real
> > md level not info->array.level
> >
> > But takeover doesn't *need* to occur.
> > When we have a RAID0 array and we want to make it grow, we have to
> > 'takeover'
> > to RAID4 first.
> > But when assembling an array that is in the middle of a growth we
> > assemble
> > it as a RAID4 array - so it never was a RAID0 array.  It starts out as
> > RAID4.
> 
> Takeover has to occur during 'normal' reshape (patches doesn't even
> touch restart problem).
> When during normal start mdadm enters reshape_array() metadata is in
> reshape state already (reshape_active == 1)
> This means that in case raid0, raid4 is reported but in md raid0 is
> still present.
> Due to this 'takeover' has to be executed.
> 
> >
> > >    b.Decision about backward takeover cannot be always taken by
> mdadm.
> > mdadm can think that original level is raid4
> > >      Resolution: use level read from md as orig_level (not from
> > metadata)
> >
> > I see that is a problem.   We should record '0' as the 'new_level' and
> > get mdadm to make use of that information.
> 
> This is true for success path. In error case mdadm exits via 'release'
> label, where
> md is switched to orig_level (not new_level). As I remember for
> 'succeess' path it works as you describes already.
> 
> 
> >
> > >
> > > 2.On the begin of reshape_array() array structure is read from md
> (via
> > ioctrl). Md knows nothing about raid4 working level, so spares_needed
> is
> > wrongly computed
> > >   Resolution: use info->array.raid_disks instead array.raid_disks to
> > compute  spares_needed.
> >
> > md certainly should know about RAID4 working level because it is
> > assembled
> > as RAID4.... hopefully some testing will show me what you really mean.
> 
> 
> 
> During assembly - yes, but for regular reshape start no.
> Regular reshape start:
> 1. md knows raid0
> 2. metadata reports raid4 (metadata has reshape_active == 1 already)
> 
> Reshape restart
> 1. md knows raid4
> 2. metadata reports raid4
> 
> 
> >
> > > 3.Analyse_changes() delta_parity concept doesn't match information
> > currently metadata reports. Result of this is wrongly computed
> > reshape.after.data_disks field.
> > >   Resolution: do not use delta_parity, base on information from
> > metadata handle
> >
> > You cannot just remove delta_parity like that.  It is needed when we
> > start
> > a growth.  Maybe it is confusing when we restart a growth - I will
> look
> > in to in.
> 
> It looks for me that when metadata handler takes responsibility for
> reporting
> Different raid level and increasing raid disks by 'delta parity' we
> should not
> Count delta parity in different place.
> If you think that analyse_change() should support both cases:
> 1. reshape handler increases raid_disks by delta parity
> 2. reshape handler doesn't increase raid_disks by delta parity
> 
> I'll fix analyse_changes() for this.

I've have a look at this and problem is as follow:
Configuration during reshape start:
1. md: raid0, n disks
2. metadata: raid4, n+1 disks
3. new_level: raid0, delta_disks = 1

analyse_change() detects transition raid4->raid0 so:
1. delta_parity = -1
2. before.data_disks = n
3. after.data_disks = before.data_disks + delta_disks - delta_parity

This gives: after.raid_disks = n + 2

1. What parity disk has to do with data disks?
2. we have transition raid0->raid0, so data disks has to increase by delta_disks only
   How to differentiate cases:
	a) raid0->raid0
     	b) raid4->raid0
  For both:
  Array.level == 4 and new_level == 0
Pass md level to analyse changes() to make difference or add field to mdinfo (orig_level)?

3. If we set delta_parity to '-1' then I think it should be added (we removing it twice -> this means adding)?

Please let me know your opinion.

BR
Adam

> 
> 
> 
> 
> >
> > > 4.Defects/potential defects
> > >  a.delta_disks should be checked in sysfs_set_array()
> >
> > No it shouldn't.  If delta_disks is UnSet, then the metadata handler
> has
> > done the wrong thing.
> >
> > >  b.array assembly problem
> > > reshape_active flag has to be evaluated. Existing second map doesn't
> > means reshape always (rebuild, initialization)
> >
> > There is no problem here.  I agree with your second sentence, but that
> > code already does this.
> > That first patch in your series makes *no* change to the meaning
> > of the code.
> > Before the patch it is
> >
> >   if (condition) {
> >      variable = 1;
> >      stuff;
> >      ...
> >
> > After the patch it is
> >
> >   variable = condition;
> >   if (variable) {
> >      stuff;
> >      ...
> >
> >
> > which has exactly the same effect.
> 
> Of course you are right !!!, I don't know, how I've look at this
> condition yesterday.
> 
> BR
> Adam
> 
> >
> > So I haven't applied any of these matches as none of them make any
> sense
> > to
> > me - sorry.
> >
> > NeilBrown
> >
> >
> >
> > >
> > > I've tried to follow your idea in this series. Generally it seems ok
> > (proved by UT). I like putting more array configuration in
> > sysfs_set_array().
> > >
> > > Now I'm going to look at patches at the reshape restart point of
> view.
> > > I'll let you know results soon.
> > >
> > > BR
> > > Adam
> > >
> > >
> > > ---
> > >
> > > Adam Kwolek (5):
> > >       FIX: Compute spares_needed basing on metadata info
> > >       FIX: Remove delta_parity concept
> > >       FIX: Allow for takeover
> > >       FIX: Set new raid disks when delta_disks is valid
> > >       fix: array is reassembled inactive if stopped during resync
> > >
> > >
> > >  Grow.c        |   22 +++++-----------------
> > >  super-intel.c |    5 +++--
> > >  sysfs.c       |    4 +++-
> > >  3 files changed, 11 insertions(+), 20 deletions(-)
> > >

RE: [PATCH 0/5] Make OLCE workeable

[Kwolek, Adam]

I've tested current mdadm (devel3-2), expansion works again :).

Thanks
Adam

> -----Original Message-----
> From: Kwolek, Adam
> Sent: Wednesday, March 09, 2011 9:14 AM
> To: 'NeilBrown'
> Cc: 'linux-raid@vger.kernel.org'; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH 0/5] Make OLCE workeable
> 
> 
> 
> > -----Original Message-----
> > From: Kwolek, Adam
> > Sent: Wednesday, March 09, 2011 8:45 AM
> > To: 'NeilBrown'
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: RE: [PATCH 0/5] Make OLCE workeable
> >
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Wednesday, March 09, 2011 1:53 AM
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > > Neubauer, Wojciech
> > > Subject: Re: [PATCH 0/5] Make OLCE workeable
> > >
> > > On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek
> <adam.kwolek@intel.com>
> > > wrote:
> > >
> > > > This is first part of my comments for your latest changes on
> devel3-
> > 2
> > > regarding reshape and reshape restart.
> > > > Unfortunately those changes breaks OLCE functionality. This patch
> > > series makes OLCE workable (mdadm UT suits 12 and 13 succeed).
> > >
> > > That's really strange as the patches all look wrong.  I'll do some
> > > testing
> > > my self and see what I can come up with.
> > >
> > > >
> > > > Problems:
> > > > 1. In passing in mdinfo (array.level) raid4 for transition raid0-
> > > >raid0.
> > > >    This causes problems in takeover
> > > >    a.Raid0 takeover doesn't occur. array.level == reshape.level ==
> 4
> > > >      Resolution: Decision about takeover has to be made based on
> > real
> > > md level not info->array.level
> > >
> > > But takeover doesn't *need* to occur.
> > > When we have a RAID0 array and we want to make it grow, we have to
> > > 'takeover'
> > > to RAID4 first.
> > > But when assembling an array that is in the middle of a growth we
> > > assemble
> > > it as a RAID4 array - so it never was a RAID0 array.  It starts out
> as
> > > RAID4.
> >
> > Takeover has to occur during 'normal' reshape (patches doesn't even
> > touch restart problem).
> > When during normal start mdadm enters reshape_array() metadata is in
> > reshape state already (reshape_active == 1)
> > This means that in case raid0, raid4 is reported but in md raid0 is
> > still present.
> > Due to this 'takeover' has to be executed.
> >
> > >
> > > >    b.Decision about backward takeover cannot be always taken by
> > mdadm.
> > > mdadm can think that original level is raid4
> > > >      Resolution: use level read from md as orig_level (not from
> > > metadata)
> > >
> > > I see that is a problem.   We should record '0' as the 'new_level'
> and
> > > get mdadm to make use of that information.
> >
> > This is true for success path. In error case mdadm exits via 'release'
> > label, where
> > md is switched to orig_level (not new_level). As I remember for
> > 'succeess' path it works as you describes already.
> >
> >
> > >
> > > >
> > > > 2.On the begin of reshape_array() array structure is read from md
> > (via
> > > ioctrl). Md knows nothing about raid4 working level, so
> spares_needed
> > is
> > > wrongly computed
> > > >   Resolution: use info->array.raid_disks instead array.raid_disks
> to
> > > compute  spares_needed.
> > >
> > > md certainly should know about RAID4 working level because it is
> > > assembled
> > > as RAID4.... hopefully some testing will show me what you really
> mean.
> >
> >
> >
> > During assembly - yes, but for regular reshape start no.
> > Regular reshape start:
> > 1. md knows raid0
> > 2. metadata reports raid4 (metadata has reshape_active == 1 already)
> >
> > Reshape restart
> > 1. md knows raid4
> > 2. metadata reports raid4
> >
> >
> > >
> > > > 3.Analyse_changes() delta_parity concept doesn't match information
> > > currently metadata reports. Result of this is wrongly computed
> > > reshape.after.data_disks field.
> > > >   Resolution: do not use delta_parity, base on information from
> > > metadata handle
> > >
> > > You cannot just remove delta_parity like that.  It is needed when we
> > > start
> > > a growth.  Maybe it is confusing when we restart a growth - I will
> > look
> > > in to in.
> >
> > It looks for me that when metadata handler takes responsibility for
> > reporting
> > Different raid level and increasing raid disks by 'delta parity' we
> > should not
> > Count delta parity in different place.
> > If you think that analyse_change() should support both cases:
> > 1. reshape handler increases raid_disks by delta parity
> > 2. reshape handler doesn't increase raid_disks by delta parity
> >
> > I'll fix analyse_changes() for this.
> 
> I've have a look at this and problem is as follow:
> Configuration during reshape start:
> 1. md: raid0, n disks
> 2. metadata: raid4, n+1 disks
> 3. new_level: raid0, delta_disks = 1
> 
> analyse_change() detects transition raid4->raid0 so:
> 1. delta_parity = -1
> 2. before.data_disks = n
> 3. after.data_disks = before.data_disks + delta_disks - delta_parity
> 
> This gives: after.raid_disks = n + 2
> 
> 1. What parity disk has to do with data disks?
> 2. we have transition raid0->raid0, so data disks has to increase by
> delta_disks only
>    How to differentiate cases:
> 	a) raid0->raid0
>      	b) raid4->raid0
>   For both:
>   Array.level == 4 and new_level == 0
> Pass md level to analyse changes() to make difference or add field to
> mdinfo (orig_level)?
> 
> 3. If we set delta_parity to '-1' then I think it should be added (we
> removing it twice -> this means adding)?
> 
> Please let me know your opinion.
> 
> BR
> Adam
> 
> >
> >
> >
> >
> > >
> > > > 4.Defects/potential defects
> > > >  a.delta_disks should be checked in sysfs_set_array()
> > >
> > > No it shouldn't.  If delta_disks is UnSet, then the metadata handler
> > has
> > > done the wrong thing.
> > >
> > > >  b.array assembly problem
> > > > reshape_active flag has to be evaluated. Existing second map
> doesn't
> > > means reshape always (rebuild, initialization)
> > >
> > > There is no problem here.  I agree with your second sentence, but
> that
> > > code already does this.
> > > That first patch in your series makes *no* change to the meaning
> > > of the code.
> > > Before the patch it is
> > >
> > >   if (condition) {
> > >      variable = 1;
> > >      stuff;
> > >      ...
> > >
> > > After the patch it is
> > >
> > >   variable = condition;
> > >   if (variable) {
> > >      stuff;
> > >      ...
> > >
> > >
> > > which has exactly the same effect.
> >
> > Of course you are right !!!, I don't know, how I've look at this
> > condition yesterday.
> >
> > BR
> > Adam
> >
> > >
> > > So I haven't applied any of these matches as none of them make any
> > sense
> > > to
> > > me - sorry.
> > >
> > > NeilBrown
> > >
> > >
> > >
> > > >
> > > > I've tried to follow your idea in this series. Generally it seems
> ok
> > > (proved by UT). I like putting more array configuration in
> > > sysfs_set_array().
> > > >
> > > > Now I'm going to look at patches at the reshape restart point of
> > view.
> > > > I'll let you know results soon.
> > > >
> > > > BR
> > > > Adam
> > > >
> > > >
> > > > ---
> > > >
> > > > Adam Kwolek (5):
> > > >       FIX: Compute spares_needed basing on metadata info
> > > >       FIX: Remove delta_parity concept
> > > >       FIX: Allow for takeover
> > > >       FIX: Set new raid disks when delta_disks is valid
> > > >       fix: array is reassembled inactive if stopped during resync
> > > >
> > > >
> > > >  Grow.c        |   22 +++++-----------------
> > > >  super-intel.c |    5 +++--
> > > >  sysfs.c       |    4 +++-
> > > >  3 files changed, 11 insertions(+), 20 deletions(-)
> > > >