Re: [PATCH 1/2] FIX: In Grow_restart() array parameters are wrongly used
From: NeilBrown <hidden>
Date: 2011-04-14 07:45:30
On Mon, 11 Apr 2011 15:39:08 +0200 Adam Kwolek [off-list ref] 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
quoted hunk ↗ jump to hunk
Signed-off-by: Adam Kwolek <redacted> --- 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