[PATCH 1/2] FIX: In Grow_restart() array parameters are wrongly used

Problem can be obvious visible when to 3-disks raid5 array 2 disks are added.
(added disks number + parity == old disks number)
Mdadm generates floating point exception and core file is generated
due to division by 0 (odata == 0).

During restart for external metadata, getinfo_super() returns
in mdinfo->array.raid_disks old disks number, that has to be increased
by delta_disks to receive new value. Function getinfo_super()
cannot return raid_disks field in different way, as array starting
geometry information. We are still going to final array geometry.
This Grow_restart() should have in mind and calculate local variables
in proper way.

This change makes Grow_restart() to interpret parameters in the same way
as reshape_array() did during reshape start, and it will do it in the same
way when it will be called from Continue_reshape().

Signed-off-by: Adam Kwolek
---

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

diff --git a/Grow.c b/Grow.c
index d4308bc..1fdee7e 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3102,13 +3102,17 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
unsigned long long nstripe, ostripe;
int ndata, odata;

- odata = info->array.raid_disks - info->delta_disks - 1;
+ if (st->ss->external) {
+ old_disks = info->array.raid_disks;
+ ndata = info->array.raid_disks + info->delta_disks - 1;
+ } else {
+ old_disks = info->array.raid_disks - info->delta_disks;
+ ndata = info->array.raid_disks - 1;
+ }
+ odata = old_disks - 1;
if (info->array.level == 6) odata--; /* number of data disks */
- ndata = info->array.raid_disks - 1;
if (info->new_level == 6) ndata--;

- old_disks = info->array.raid_disks - info->delta_disks;
-
if (info->delta_disks <= 0)
/* Didn't grow, so the backup file must have
* been used

--
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

[PATCH 2/2] FIX: Set reshape parameters for all disks belongs to new

For external metadata mdinfo->array.raid_disks reports "old" disks geometry.
To work in the same way as native metadata, new_disks has to be used
instead mdinfo->array.raid_disks.

Signed-off-by: Adam Kwolek
---

Grow.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/Grow.c b/Grow.c
index 1fdee7e..a954cfd 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3097,19 +3097,20 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
char *backup_file, int verbose)
{
int i, j;
- int old_disks;
+ int old_disks, new_disks;
unsigned long long *offsets;
unsigned long long nstripe, ostripe;
int ndata, odata;

if (st->ss->external) {
old_disks = info->array.raid_disks;
- ndata = info->array.raid_disks + info->delta_disks - 1;
+ new_disks = info->array.raid_disks + info->delta_disks;
} else {
old_disks = info->array.raid_disks - info->delta_disks;
- ndata = info->array.raid_disks - 1;
+ new_disks = info->array.raid_disks;
}
odata = old_disks - 1;
+ ndata = new_disks - 1;
if (info->array.level == 6) odata--; /* number of data disks */
if (info->new_level == 6) ndata--;

@@ -3268,8 +3269,8 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
goto second_fail; /* Cannot find leading superblock */

/* Now need the data offsets for all devices. */
- offsets = malloc(sizeof(*offsets)*info->array.raid_disks);
- for(j=0; jarray.raid_disks; j++) {
+ offsets = malloc(sizeof(*offsets)*new_disks);
+ for (j = 0; j < new_disks; j++) {
if (fdlist[j] < 0)
continue;
if (st->ss->load_super(st, fdlist[j], NULL))
@@ -3282,7 +3283,7 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
printf(Name ": restoring critical section\n");

if (restore_stripes(fdlist, offsets,
- info->array.raid_disks,
+ new_disks,
info->new_chunk,
info->new_level,
info->new_layout,
@@ -3298,7 +3299,7 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt

if (bsb.magic[15] == '2' &&
restore_stripes(fdlist, offsets,
- info->array.raid_disks,
+ new_disks,
info->new_chunk,
info->new_level,
info->new_layout,
@@ -3354,7 +3355,7 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
info->reshape_progress = p2;
}
}
- for (j=0; jarray.raid_disks; j++) {
+ for (j = 0; j < new_disks; j++) {
if (fdlist[j] < 0) continue;
if (st->ss->load_super(st, fdlist[j], NULL))
continue;

--
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 1/2] FIX: In Grow_restart() array parameters are wronglyused

On Mon, 11 Apr 2011 15:39:08 +0200 Adam Kwolek wrote:

> Problem can be obvious visible when to 3-disks raid5 array 2 disks are added.
> (added disks number + parity == old disks number)
> Mdadm generates floating point exception and core file is generated
> due to division by 0 (odata == 0).
>
> During restart for external metadata, getinfo_super() returns
> in mdinfo->array.raid_disks old disks number, that has to be increased
> by delta_disks to receive new value. Function getinfo_super()
> cannot return raid_disks field in different way, as array starting
> geometry information. We are still going to final array geometry.
> This Grow_restart() should have in mind and calculate local variables
> in proper way.
>
> This change makes Grow_restart() to interpret parameters in the same way
> as reshape_array() did during reshape start, and it will do it in the same
> way when it will be called from Continue_reshape().

sorry, but I don't like this change.
I never like having different handling for 'external' and 'non-external'.
Sometimes it is obviously necessary, but we should avoid it if possible.

Once reshape has started, 'raid_disks' is the new number of disks, not the
old number.

So sysfs_set_array should be changed to set raid_disks to
raid_disk - delta_disks
in the first instance, then if reshape is active, to raid_disks.

And getinfo_super should return the 'new' number of disks.

And reshape_array might need to fiddle with raid_disks before calling
analyse_change in the reshape_active case.

So it is more changes, but I think it is a better result.

Thanks,
NeilBrown

>
> Signed-off-by: Adam Kwolek
> ---
>
> Grow.c | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index d4308bc..1fdee7e 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3102,13 +3102,17 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
> unsigned long long nstripe, ostripe;
> int ndata, odata;
>
> - odata = info->array.raid_disks - info->delta_disks - 1;
> + if (st->ss->external) {
> + old_disks = info->array.raid_disks;
> + ndata = info->array.raid_disks + info->delta_disks - 1;
> + } else {
> + old_disks = info->array.raid_disks - info->delta_disks;
> + ndata = info->array.raid_disks - 1;
> + }
> + odata = old_disks - 1;
> if (info->array.level == 6) odata--; /* number of data disks */
> - ndata = info->array.raid_disks - 1;
> if (info->new_level == 6) ndata--;
>
> - old_disks = info->array.raid_disks - info->delta_disks;
> -
> if (info->delta_disks <= 0)
> /* Didn't grow, so the backup file must have
> * been used

--
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