[PATCH 0/6] Reshape fixes (expansion)

[PATCH 0/6] Reshape fixes (expansion)

[Adam Kwolek]

The following series adds some fixes for reshape (expansion).

A while ago mdadm unit tests (suits 12 and 13) works correctly, now I've found problems.
I've learned that containers created for raid5 and raid0 has different number of blocks in mdstat.
I've put mdstat partial output to patch description:  UT FIX: imsm container can have different blocks number
I'm not 100% sure that this is fix (for changed behavior), not workaround for container creation problem.
This fix is first of fixes (3) that enables reshape (12, 13) UT.
If you find this more as workaround, this patch is good point for discussion what change is correct.

Patches:
      FIX: compare blocks on all data disks
      FIX: Compare the same units
makes changes for blocks computing verification. After those 2 more  patches UT (12, 13) works. 
Please let me know what you are thinking about them. It is possible that this is not final patch also,
but I've tried to find out what this condition should do, and change code to archive it (IMHO).

Rest of patches addresses other issues, I can observe during expansion.

BR
Adam

---

Adam Kwolek (6):
      FIX: md runs recovery instead reshape for growing single disk raid0 array
      FIX: Container can be left frozen
      FIX: compare blocks on all data disks
      FIX: Compare the same units
      UT FIX: imsm container can have different blocks number
      imsm: FIX: Wrong output string format


 Grow.c                  |    9 +++++++--
 super-intel.c           |    4 ++--
 tests/env-imsm-template |    2 +-
 3 files changed, 10 insertions(+), 5 deletions(-)

-- 
Signature

[PATCH 1/6] imsm: FIX: Wrong output string format

[Adam Kwolek]

On wrong starting container reshape conditions, mdadm returns information on console:
mdadmimsm: Operation is not allowed on this container

it is changed to:
mdadm: (imsm) Operation is not allowed on this container

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

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

diff --git a/super-intel.c b/super-intel.c
index 59356e0..6a21b26 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -7097,8 +7097,8 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
 				free(u);
 
 		} else {
-			fprintf(stderr, Name "imsm: Operation is not allowed "
-				"on this container\n");
+			fprintf(stderr, Name ": (imsm) Operation "
+				"is not allowed on this container\n");
 		}
 	} else {
 		/* On volume level we support following operations

[PATCH 2/6] UT FIX: imsm container can have different blocks number

[Adam Kwolek]

When imsm container is created it have different blocks number
in /proc/mdstat depending on containing array raid level (raid0/raid5).
raid5 case:
md127 : inactive sdd[3](S) sde[2](S) sdc[1](S) sdb[0](S)
      2884 blocks super external:imsm

raid0 case:
md127 : inactive sdd[3](S) sde[2](S) sdc[1](S) sdb[0](S)
      836 blocks super external:imsm

Due to this, it cannot be compared to one value for both cases.

Test imsm container existence before unit test run only.

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

 tests/env-imsm-template |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tests/env-imsm-template b/tests/env-imsm-template
index 74c43c6..7a2890c 100644
--- a/tests/env-imsm-template
+++ b/tests/env-imsm-template
@@ -2,7 +2,7 @@ imsm_check() {
     udevadm settle
     case $1 in
     container )
-        grep -s "$(((418 * $2) / 2)) blocks super external:imsm" /proc/mdstat > /dev/null || {
+        grep -s "blocks super external:imsm" /proc/mdstat > /dev/null || {
             echo >&2 "**Fatal** Correctly formed container not found"; cat /proc/mdstat; exit 1; }
         ;;
     member )

[PATCH 3/6] FIX: Compare the same units

[Adam Kwolek]

In condition blocks and kbytes are compared.
The same units have to be compared, otherwise comparison
is meaningless.

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 8229b4d..6a9e81e 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1829,7 +1829,7 @@ started:
 		fprintf(stderr, Name ": Need to backup %luK of critical "
 			"section..\n", blocks/2);
 
-	if (blocks >= sra->component_size/2) {
+	if (blocks >= sra->component_size) {
 		fprintf(stderr, Name ": %s: Something wrong"
 			" - reshape aborted\n",
 			devname);

[PATCH 4/6] FIX: compare blocks on all data disks

[Adam Kwolek]

blocks are counted for all data disks.
component_size value is given for one array disk only.
This means we should multiple component_size by data disks number
to get similar (compare able) values.

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

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

diff --git a/Grow.c b/Grow.c
index 6a9e81e..09d7439 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1829,7 +1829,8 @@ started:
 		fprintf(stderr, Name ": Need to backup %luK of critical "
 			"section..\n", blocks/2);
 
-	if (blocks >= sra->component_size) {
+	disks = min(reshape.before.data_disks, reshape.after.data_disks);
+	if (blocks >= sra->component_size * disks) {
 		fprintf(stderr, Name ": %s: Something wrong"
 			" - reshape aborted\n",
 			devname);

[PATCH 5/6] FIX: Container can be left frozen

[Adam Kwolek]

When container operation fails before child process starts,
array can be left frozen because container_reshape() doesn't make
unfreeze() operation in all error cases, as it is responsible for.

add unfreeze() operation for error case scenarios in reshape_container()

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

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

diff --git a/Grow.c b/Grow.c
index 09d7439..e77ab80 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2123,8 +2123,10 @@ int reshape_container(char *container, int cfd, char *devname,
 	if (reshape_super(st, -1, info->new_level,
 			  info->new_layout, info->new_chunk,
 			  info->array.raid_disks + info->delta_disks,
-			  backup_file, devname, quiet))
+			  backup_file, devname, quiet)) {
+		unfreeze(st);
 		return 1;
+	}
 
 	sync_metadata(st);
 
@@ -2135,6 +2137,7 @@ int reshape_container(char *container, int cfd, char *devname,
 	switch (fork()) {
 	case -1: /* error */
 		perror("Cannot fork to complete reshape\n");
+		unfreeze(st);
 		return 1;
 	default: /* parent */
 		printf(Name ": multi-array reshape continues in background\n");

[PATCH 6/6] FIX: md runs recovery instead reshape for growing single disk raid0 array

[Adam Kwolek]

Problem occurs when we want to expand single disk raid0 array.
This is done via degraded 2 disks raid4 array. When new spare disk
for reshape is added to array, md immediately initiates recovery before
mdadm can configure and start reshape. This is due fact that 2 disk
raid4/5 array is special md case.
Mdmon does nothing here because container is blocked.
This is caused because after takeover array is not in frozen state in md.

Put array in to frozen state after takeover to allow mdadm to finish
configuration before reshape is executed in md.

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

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

diff --git a/Grow.c b/Grow.c
index e77ab80..88e03ef 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1677,6 +1677,7 @@ static int reshape_array(char *container, int fd, char *devname,
 			fprintf(stderr, Name ": level of %s changed to %s\n",
 				devname, c);	
 		orig_level = info->array.level;
+		sysfs_freeze_array(info);
 
 		if (reshape.level > 0 && st->ss->external) {
 			/* make sure mdmon is aware of the new level */

Re: [PATCH 0/6] Reshape fixes (expansion)

[NeilBrown]

On Tue, 08 Feb 2011 14:30:28 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> The following series adds some fixes for reshape (expansion).
> 
> A while ago mdadm unit tests (suits 12 and 13) works correctly, now I've found problems.
> I've learned that containers created for raid5 and raid0 has different number of blocks in mdstat.
> I've put mdstat partial output to patch description:  UT FIX: imsm container can have different blocks number
> I'm not 100% sure that this is fix (for changed behavior), not workaround for container creation problem.
> This fix is first of fixes (3) that enables reshape (12, 13) UT.
> If you find this more as workaround, this patch is good point for discussion what change is correct.
> 
> Patches:
>       FIX: compare blocks on all data disks
>       FIX: Compare the same units
> makes changes for blocks computing verification. After those 2 more  patches UT (12, 13) works. 
> Please let me know what you are thinking about them. It is possible that this is not final patch also,
> but I've tried to find out what this condition should do, and change code to archive it (IMHO).
> 
> Rest of patches addresses other issues, I can observe during expansion.
> 
> BR
> Adam
> 
> ---
> 
> Adam Kwolek (6):
>       FIX: md runs recovery instead reshape for growing single disk raid0 array
Applied.

>       FIX: Container can be left frozen
Applied.

>       FIX: compare blocks on all data disks
>       FIX: Compare the same units
No, I don't like these.
The point here is to compare the amount of data to be backed up with the
size of one of the spare devices, because in some cases the data will be
backed up to a spare device.
If the size of the data to be backed up is bigger than half the size of a
spare device, then it doesn't really fit and something is wrong.
Normally the amount of data to be backed up is much much smaller than the
size of any device.

Maybe we just need to make the test use larger devices or smaller chunk size,
or similar.


>       UT FIX: imsm container can have different blocks number
Applied (the block numbers here are completely meaningless, so ignoring them
is good).
>       imsm: FIX: Wrong output string format
Applied.

All pushed out to devel-3.2

Thanks.

NeilBrown

RE: [PATCH 0/6] Reshape fixes (expansion)

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Wednesday, February 09, 2011 4:11 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 0/6] Reshape fixes (expansion)
> 
> On Tue, 08 Feb 2011 14:30:28 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > The following series adds some fixes for reshape (expansion).
> >
> > A while ago mdadm unit tests (suits 12 and 13) works correctly, now
> I've found problems.
> > I've learned that containers created for raid5 and raid0 has
> different number of blocks in mdstat.
> > I've put mdstat partial output to patch description:  UT FIX: imsm
> container can have different blocks number
> > I'm not 100% sure that this is fix (for changed behavior), not
> workaround for container creation problem.
> > This fix is first of fixes (3) that enables reshape (12, 13) UT.
> > If you find this more as workaround, this patch is good point for
> discussion what change is correct.
> >
> > Patches:
> >       FIX: compare blocks on all data disks
> >       FIX: Compare the same units
> > makes changes for blocks computing verification. After those 2 more
> patches UT (12, 13) works.
> > Please let me know what you are thinking about them. It is possible
> that this is not final patch also,
> > but I've tried to find out what this condition should do, and change
> code to archive it (IMHO).
> >
> > Rest of patches addresses other issues, I can observe during
> expansion.
> >
> > BR
> > Adam
> >
> > ---
> >
> > Adam Kwolek (6):
> >       FIX: md runs recovery instead reshape for growing single disk
> raid0 array
> Applied.
> 
> >       FIX: Container can be left frozen
> Applied.
> 
> >       FIX: compare blocks on all data disks
> >       FIX: Compare the same units
> No, I don't like these.
> The point here is to compare the amount of data to be backed up with
> the
> size of one of the spare devices, because in some cases the data will
> be
> backed up to a spare device.
> If the size of the data to be backed up is bigger than half the size of
> a
> spare device, then it doesn't really fit and something is wrong.
> Normally the amount of data to be backed up is much much smaller than
> the
> size of any device.
> 
> Maybe we just need to make the test use larger devices or smaller chunk
> size,
> or similar.

Thank you for explanation. I'll prepare changes to UT.

BR
Adam

> 
> 
> >       UT FIX: imsm container can have different blocks number
> Applied (the block numbers here are completely meaningless, so ignoring
> them
> is good).
> >       imsm: FIX: Wrong output string format
> Applied.
> 
> All pushed out to devel-3.2
> 
> Thanks.
> 
> NeilBrown