[PATCH 0/5] Level and chunk size migrations for imsm

[PATCH 0/5] Level and chunk size migrations for imsm

[Adam Kwolek]

The following series implements raid level (5<->0) and chunk size migrations.

Comparing to last patches you are looking at, there are the following differences:
1. Chunk size migrations added
2. Selecting spare device for raid0->raid5 migration is moved to mdmon (prepare_update())
        mdadm re-reads metadata for adding spares so sysfs-mdmon configuration is consistent
	mdadm puts migration parameters in update only:
		- new new level
		- new chunk size

The first patch in series addresses problem for raid5 -> raid0 migration when raid_disks sysfs
entry was set with too small value (value has to be large enough to allow takeover to raid0
at reshape end)

After this series mdadm passes unit tests suites: 12, 13, 14, 15, 16, 18 
(test 18imsm-r1_2d-takeover-r0_1d requires latest changes for raid1->raid0 takeover in md)

BR
Adam


---

Adam Kwolek (5):
      imsm: Add chunk size to metadata update
      imsm: process update for raid level migrations
      imsm: prepare memory for level migration update
      imsm: prepare update for level migrations reshape
      FIX: set delta_disks to 0 for raid5->raid0 transition


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

-- 
Signature

[PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0 transition

[Adam Kwolek]

We have to set proper value of delta_disks to avoid it wrongly being set
when it value remains UnSet for this level transition (Grow.c:1224).

This causes too small value set to "raid_disks" in sysfs
and reshape raid5->raid0 fails.

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 424d489..dba2825 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1073,6 +1073,7 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
 		switch (info->new_level) {
 		case 0:
 			delta_parity = -1;
+			info->delta_disks = 0;
 		case 4:
 			re->level = info->array.level;
 			re->before.data_disks = info->array.raid_disks - 1;

[PATCH 2/5] imsm: prepare update for level migrations reshape

[Adam Kwolek]

For level migrations:
1. raid0->raid5
2. raid5->raid0
metadata update is prepared

For migration raid0->raid5 spare device is required.
It is checked in mdadm for spares, but it is not included in to update.
Mdmon will decide what spare device

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

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

diff --git a/super-intel.c b/super-intel.c
index f3e248e..489340f 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -305,6 +305,7 @@ enum imsm_update_type {
 	update_rename_array,
 	update_add_remove_disk,
 	update_reshape_container_disks,
+	update_reshape_migration,
 	update_takeover
 };
 
@@ -340,6 +341,12 @@ struct imsm_update_reshape {
 	enum imsm_update_type type;
 	int old_raid_disks;
 	int new_raid_disks;
+	/* fields for array migration changes
+	 */
+	int subdev;
+	int new_level;
+	int new_layout;
+
 	int new_disks[1]; /* new_raid_disks - old_raid_disks makedev number */
 };
 
@@ -6133,6 +6140,9 @@ static void imsm_process_update(struct supertype *st,
 			super->updates_pending++;
 		break;
 	}
+	case update_reshape_migration: {
+		break;
+	}
 	case update_activate_spare: {
 		struct imsm_update_activate_spare *u = (void *) update->buf; 
 		struct imsm_dev *dev = get_imsm_dev(super, u->array);
@@ -6526,6 +6536,9 @@ static void imsm_prepare_update(struct supertype *st,
 		dprintf("New anchor length is %llu\n", (unsigned long long)len);
 		break;
 	}
+	case update_reshape_migration: {
+		break;
+	}
 	case update_create_array: {
 		struct imsm_update_create_array *u = (void *) update->buf;
 		struct intel_dev *dv;
@@ -6892,6 +6905,89 @@ abort:
 	return 0;
 }
 
+/******************************************************************************
+ * function: imsm_create_metadata_update_for_migration()
+ *           Creates update for IMSM array.
+ *
+ ******************************************************************************/
+static int imsm_create_metadata_update_for_migration(
+					struct supertype *st,
+					struct geo_params *geo,
+					struct imsm_update_reshape **updatep)
+{
+	struct intel_super *super = st->sb;
+	int update_memory_size = 0;
+	struct imsm_update_reshape *u = NULL;
+	int delta_disks = 0;
+	struct imsm_dev *dev;
+	int previous_level = -1;
+
+	dprintf("imsm_create_metadata_update_for_migration(enter)"
+		" New Level = %i\n", geo->level);
+	delta_disks = 0;
+
+	/* size of all update data without anchor */
+	update_memory_size = sizeof(struct imsm_update_reshape);
+
+	/* now add space for spare disks that we need to add. */
+	if (delta_disks > 0)
+		update_memory_size += sizeof(u->new_disks[0])
+					* (delta_disks - 1);
+
+	u = calloc(1, update_memory_size);
+	if (u == NULL) {
+		dprintf("error: cannot get memory for "
+			"imsm_create_metadata_update_for_migration\n");
+		return 0;
+	}
+	u->type = update_reshape_migration;
+	u->subdev = super->current_vol;
+	u->new_level = geo->level;
+	u->new_layout = geo->layout;
+	u->new_raid_disks = u->old_raid_disks = geo->raid_disks;
+	u->new_disks[0] = -1;
+
+	dev = get_imsm_dev(super, u->subdev);
+	if (dev) {
+		struct imsm_map *map;
+
+		map = get_imsm_map(dev, 0);
+		if (map)
+			previous_level = map->raid_level;
+	}
+	if ((geo->level == 5) && (previous_level == 0)) {
+		struct mdinfo *spares = NULL;
+
+		u->new_raid_disks++;
+		spares = get_spares_for_grow(st);
+		if (spares) {
+			struct dl *dl;
+			struct mdinfo *dev;
+
+			dev = spares->devs;
+			if (dev) {
+				u->new_disks[0] = makedev(dev->disk.major,
+							  dev->disk.minor);
+				dl = get_disk_super(super,
+						    dev->disk.major,
+						    dev->disk.minor);
+				dl->index = u->old_raid_disks;
+				dev = dev->next;
+			}
+			sysfs_free(spares);
+		} else {
+			free(u);
+			dprintf("error: cannot get spare device "
+				"for requested migration");
+			return 0;
+		}
+	}
+	dprintf("imsm: reshape update preparation : OK\n");
+	*updatep = u;
+
+	return update_memory_size;
+}
+
 static void imsm_update_metadata_locally(struct supertype *st,
 					 void *buf, int len)
 {
@@ -7146,9 +7242,25 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
 		case CH_TAKEOVER:
 			ret_val = imsm_takeover(st, &geo);
 			break;
-		case CH_MIGRATION:
+		case CH_MIGRATION: {
+			struct imsm_update_reshape *u = NULL;
+			int len =
+				imsm_create_metadata_update_for_migration(
+					st, &geo, &u);
+			if (len < 1) {
+				dprintf("imsm: "
+					"Cannot prepare update\n");
+			}
 			ret_val = 0;
-			break;
+			/* update metadata locally */
+			imsm_update_metadata_locally(st, u, len);
+			/* and possibly remotely */
+			if (st->update_tail)
+				append_metadata_update(st, u, len);
+			else
+				free(u);
+		}
+		break;
 		default:
 			ret_val = 1;
 		}

[PATCH 3/5] imsm: prepare memory for level migration update

[Adam Kwolek]

When level is changed from raid0 to raid5 memory is required for replace device
smaller device/array object. When update points to no spare device
(u->new_disks[0] == -1) memory for new missed disk will be required also.
This memory is allocated in manager context in prepare_update()

Prepare_update() is called in manager context so memory allocation are allowed
here. This allows us to look for spare devices for meta update.

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

 super-intel.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 489340f..b7ef428 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6439,6 +6439,8 @@ static void imsm_process_update(struct supertype *st,
 	}
 }
 
+static struct mdinfo *get_spares_for_grow(struct supertype *st);
+
 static void imsm_prepare_update(struct supertype *st,
 				struct metadata_update *update)
 {
@@ -6537,6 +6539,91 @@ static void imsm_prepare_update(struct supertype *st,
 		break;
 	}
 	case update_reshape_migration: {
+		/* for migration level 0->5 we need to add disks
+		 * so the same as for container operation we will copy
+		 * device to the bigger location.
+		 * in memory prepared device and new disk area are prepared
+		 * for usage in process update
+		 */
+		struct imsm_update_reshape *u = (void *)update->buf;
+		struct intel_dev *id;
+		void **space_tail = (void **)&update->space_list;
+		int size;
+		void *s;
+		int current_level = -1;
+
+		dprintf("imsm: imsm_prepare_update() for update_reshape\n");
+
+		/* add space for bigger array in update
+		 */
+		for (id = super->devlist; id; id = id->next) {
+			if (id->index == (unsigned)u->subdev) {
+				size = sizeof_imsm_dev(id->dev, 1);
+				if (u->new_raid_disks > u->old_raid_disks)
+					size += sizeof(__u32)*2*
+					(u->new_raid_disks - u->old_raid_disks);
+				s = malloc(size);
+				if (!s)
+					break;
+				*space_tail = s;
+				space_tail = s;
+				*space_tail = NULL;
+				break;
+			}
+		}
+		if (update->space_list == NULL)
+			break;
+
+		/* add space for disk in update
+		 */
+		size = sizeof(struct dl);
+		s = malloc(size);
+		if (!s) {
+			free(update->space_list);
+			update->space_list = NULL;
+			break;
+		}
+		*space_tail = s;
+		space_tail = s;
+		*space_tail = NULL;
+
+		/* add spare device to update
+		 */
+		for (id = super->devlist ; id; id = id->next)
+			if (id->index == (unsigned)u->subdev) {
+				struct imsm_dev *dev;
+				struct imsm_map *map;
+
+				dev = get_imsm_dev(super, u->subdev);
+				map = get_imsm_map(dev, 0);
+				current_level = map->raid_level;
+				break;
+			}
+		if ((u->new_level == 5) && (u->new_level != current_level)) {
+			struct mdinfo *spares;
+
+			u->new_raid_disks++;
+			spares = get_spares_for_grow(st);
+			if (spares) {
+				struct dl *dl;
+				struct mdinfo *dev;
+
+				dev = spares->devs;
+				if (dev) {
+					u->new_disks[0] = 
+						makedev(dev->disk.major,
+							dev->disk.minor);
+					dl = get_disk_super(super,
+							    dev->disk.major,
+							    dev->disk.minor);
+					dl->index = u->old_raid_disks;
+					dev = dev->next;
+				}
+				sysfs_free(spares);
+			}
+		}
+		len = disks_to_mpb_size(u->new_raid_disks);
+		dprintf("New anchor length is %llu\n", (unsigned long long)len);
 		break;
 	}
 	case update_create_array: {

[PATCH 4/5] imsm: process update for raid level migrations

[Adam Kwolek]

Received update and prepared memory is processed to update imsm metadata.
There is available cases:
1. Raid0->raid5(n+1 disk array) migration:
   a) no spares: degraded raid5 array is created
   b) spare disk is available: raid5 is created using spare device
2. Raid5(n disk array)->raid0(n disk array): array size is increased

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

 super-intel.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 121 insertions(+), 1 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index b7ef428..0f402bb 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5876,6 +5876,123 @@ static int add_remove_disk_update(struct intel_super *super)
 	return check_degraded;
 }
 
+
+static int apply_reshape_migration_update(struct imsm_update_reshape *u,
+						struct intel_super *super,
+						void ***space_list)
+{
+	struct intel_dev *id;
+	void **tofree = NULL;
+	int ret_val = 0;
+
+	dprintf("apply_reshape_migration_update()\n");
+	if ((u->subdev < 0) ||
+	    (u->subdev > 1)) {
+		dprintf("imsm: Error: Wrong subdev: %i\n", u->subdev);
+		return ret_val;
+	}
+	if ((space_list == NULL) || (*space_list == NULL)) {
+		dprintf("imsm: Error: Memory is not allocated\n");
+		return ret_val;
+	}
+
+	for (id = super->devlist ; id; id = id->next) {
+		if (id->index == (unsigned)u->subdev) {
+			struct imsm_dev *dev = get_imsm_dev(super, u->subdev);
+			struct imsm_map *map;
+			struct imsm_dev *new_dev =
+				(struct imsm_dev *)*space_list;
+			struct imsm_map *migr_map = get_imsm_map(dev, 1);
+			int to_state;
+			struct dl *new_disk;
+
+			if (new_dev == NULL)
+				return ret_val;
+			*space_list = **space_list;
+			memcpy(new_dev, dev, sizeof_imsm_dev(dev, 0));
+			map = get_imsm_map(new_dev, 0);
+			if (migr_map) {
+				dprintf("imsm: Error: migration in progress");
+				return ret_val;
+			}
+
+			to_state = map->map_state;
+			if ((u->new_level == 5) && (map->raid_level == 0)) {
+				map->num_members++;
+				if (u->new_disks[0] < 0) {
+					map->failed_disk_num =
+						map->num_members - 1;
+					to_state = IMSM_T_STATE_DEGRADED;
+				} else
+					to_state = IMSM_T_STATE_NORMAL;
+			}
+			migrate(new_dev, to_state, MIGR_GEN_MIGR);
+			if (u->new_level > -1)
+				map->raid_level = u->new_level;
+			migr_map = get_imsm_map(new_dev, 1);
+			if ((u->new_level == 5) &&
+			    (migr_map->raid_level == 0)) {
+				int ord = map->num_members - 1;
+				migr_map->num_members--;
+				if (u->new_disks[0] < 0)
+					ord |= IMSM_ORD_REBUILD;
+				set_imsm_ord_tbl_ent(map,
+						     map->num_members - 1,
+						     ord);
+			}
+			id->dev = new_dev;
+			tofree = (void **)dev;
+
+			/* add disk
+			 */
+			if ((u->new_level != 5) ||
+			    (migr_map->raid_level != 0) ||
+			    (migr_map->raid_level == map->raid_level))
+				goto skip_disk_add;
+
+			if (u->new_disks[0] >= 0) {
+				/* use passes spare
+				 */
+				new_disk = get_disk_super(super,
+							major(u->new_disks[0]),
+							minor(u->new_disks[0]));
+				dprintf("imsm: new disk for reshape is: %i:%i "
+					"(%p, index = %i)\n",
+					major(u->new_disks[0]),
+					minor(u->new_disks[0]),
+					new_disk, new_disk->index);
+				if (new_disk == NULL)
+					goto error_disk_add;
+
+				new_disk->index = map->num_members - 1;
+				/* slot to fill in autolayout
+				 */
+				new_disk->raiddisk = new_disk->index;
+				new_disk->disk.status |= CONFIGURED_DISK;
+				new_disk->disk.status &= ~SPARE_DISK;
+			} else
+				goto error_disk_add;
+
+skip_disk_add:
+			*tofree = *space_list;
+			/* calculate new size
+			 */
+			imsm_set_array_size(new_dev);
+
+			ret_val = 1;
+		}
+	}
+
+	if (tofree)
+		*space_list = tofree;
+	return ret_val;
+
+error_disk_add:
+	dprintf("Error: imsm: Cannot find disk.\n");
+	return ret_val;
+}
+
+
 static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
 						struct intel_super *super,
 						void ***space_list)
@@ -6141,6 +6258,10 @@ static void imsm_process_update(struct supertype *st,
 		break;
 	}
 	case update_reshape_migration: {
+		struct imsm_update_reshape *u = (void *)update->buf;
+		if (apply_reshape_migration_update(
+			    u, super, &update->space_list))
+			super->updates_pending++;
 		break;
 	}
 	case update_activate_spare: {
@@ -6169,7 +6290,6 @@ static void imsm_process_update(struct supertype *st,
 		}
 
 		super->updates_pending++;
-
 		/* count failures (excluding rebuilds and the victim)
 		 * to determine map[0] state
 		 */

[PATCH 5/5] imsm: Add chunk size to metadata update

[Adam Kwolek]

Put information about chunk size change in to migration metadata update.

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

 super-intel.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 0f402bb..b04d209 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -346,6 +346,7 @@ struct imsm_update_reshape {
 	int subdev;
 	int new_level;
 	int new_layout;
+	int new_chunksize;
 
 	int new_disks[1]; /* new_raid_disks - old_raid_disks makedev number */
 };
@@ -5943,6 +5944,12 @@ static int apply_reshape_migration_update(struct imsm_update_reshape *u,
 			id->dev = new_dev;
 			tofree = (void **)dev;
 
+			/* update chunk size
+			 */
+			if (u->new_chunksize > 0)
+				map->blocks_per_strip =
+					__cpu_to_le16(u->new_chunksize * 2);
+
 			/* add disk
 			 */
 			if ((u->new_level != 5) ||
@@ -7153,14 +7160,25 @@ static int imsm_create_metadata_update_for_migration(
 	u->new_layout = geo->layout;
 	u->new_raid_disks = u->old_raid_disks = geo->raid_disks;
 	u->new_disks[0] = -1;
+	u->new_chunksize = -1;
 
 	dev = get_imsm_dev(super, u->subdev);
 	if (dev) {
 		struct imsm_map *map;
 
 		map = get_imsm_map(dev, 0);
-		if (map)
+		if (map) {
+			int current_chunk_size =
+				__le16_to_cpu(map->blocks_per_strip) / 2;
+
+			if (geo->chunksize != current_chunk_size) {
+				u->new_chunksize = geo->chunksize / 1024;
+				dprintf("imsm: "
+					"chunk size change from %i to %i\n",
+					current_chunk_size, u->new_chunksize);
+			}
 			previous_level = map->raid_level;
+		}
 	}
 	if ((geo->level == 5) && (previous_level == 0)) {
 		struct mdinfo *spares = NULL;

Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0 transition

[NeilBrown]

On Mon, 14 Feb 2011 14:12:49 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> We have to set proper value of delta_disks to avoid it wrongly being set
> when it value remains UnSet for this level transition (Grow.c:1224).
> 
> This causes too small value set to "raid_disks" in sysfs
> and reshape raid5->raid0 fails.
> 
> 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 424d489..dba2825 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1073,6 +1073,7 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
>  		switch (info->new_level) {
>  		case 0:
>  			delta_parity = -1;
> +			info->delta_disks = 0;
>  		case 4:
>  			re->level = info->array.level;
>  			re->before.data_disks = info->array.raid_disks - 1;

I think we have different expectations about what a RAID5 -> RAID0 transition
means.

To me, it means getting rid of the parity information.  So a 4-device RAID5
is converted to a 3-device RAID0 and stays the same size.

I think you want it to maintain the same number of devices, so a 4-device
RAID5 becomes a 4-device RAID0 and thus has larger storage.

If you want that, you need to say:
   mdadm -G /dev/md/xxx --level=0 --raid-disks=4

I'd be happy with functionality to do:

   mdadm -G /dev/md/xxx --level=0 --raid-disks=nochange

or something like that so it could be easily scripted easily, but I want the
default to do the simplest possible change.

Am I correct about your expectations?

Thanks,
NeilBrown

Re: [PATCH 2/5] imsm: prepare update for level migrations reshape

[NeilBrown]

On Mon, 14 Feb 2011 14:12:57 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> For level migrations:
> 1. raid0->raid5
> 2. raid5->raid0
> metadata update is prepared
> 
> For migration raid0->raid5 spare device is required.
> It is checked in mdadm for spares, but it is not included in to update.
> Mdmon will decide what spare device
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  super-intel.c |  116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 114 insertions(+), 2 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index f3e248e..489340f 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -305,6 +305,7 @@ enum imsm_update_type {
>  	update_rename_array,
>  	update_add_remove_disk,
>  	update_reshape_container_disks,
> +	update_reshape_migration,
>  	update_takeover
>  };
>  
> @@ -340,6 +341,12 @@ struct imsm_update_reshape {
>  	enum imsm_update_type type;
>  	int old_raid_disks;
>  	int new_raid_disks;
> +	/* fields for array migration changes
> +	 */
> +	int subdev;
> +	int new_level;
> +	int new_layout;
> +

I don't like how you are overloading this structure.  You are adding fields
to a structure that doesn't need them in its original context, and using a
structure containing old_raid_disks and new_raid_disks in a context where
they are needed.

It would be much better to have a separate structure for each different
imsm_update_type.


>  	int new_disks[1]; /* new_raid_disks - old_raid_disks makedev number */

Hmmm... new_disks is already there, isn't it.
I'm not at all sure that I'm happy about that.
It is used when increasing the number of devices in an array, but I would
like that to just change  the numbers and leave mdmon to add the devices.

'reshape_super'  should just verify that the change is legal, update the
metadata to show that the change has happened or is happening, and possibly
create an update to send to mdmon.
reshape_super should not choose or allocate spares.

'prepare_update' should *only* perform any allocations that process_update
will need.  It doesn't allocate spares either.  It *only* allocates memory.

'process_update' takes the update description and modifies the metadata, and
updates internal data structures so that they accurately reflect the
information in the metadata.  It doesn't allocate spares either.

'activate_spare' is the only place that should activate spares.  It analyses
the array, determines if any devices need to be added, finds them, and
creates a metadata update to describe the new devices.
manage_member() then adds them to the array and process_update eventually
records these new allocations in the metadata.

It is really important to keep these function separate and make sure each
function just does the one thing that it is supposed to do.  Otherwise all
the interactions become too complex and so are easily broken.


>  };
>  
> @@ -6133,6 +6140,9 @@ static void imsm_process_update(struct supertype *st,
>  			super->updates_pending++;
>  		break;
>  	}
> +	case update_reshape_migration: {
> +		break;
> +	}
>  	case update_activate_spare: {
>  		struct imsm_update_activate_spare *u = (void *) update->buf; 
>  		struct imsm_dev *dev = get_imsm_dev(super, u->array);
> @@ -6526,6 +6536,9 @@ static void imsm_prepare_update(struct supertype *st,
>  		dprintf("New anchor length is %llu\n", (unsigned long long)len);
>  		break;
>  	}
> +	case update_reshape_migration: {
> +		break;
> +	}
>  	case update_create_array: {
>  		struct imsm_update_create_array *u = (void *) update->buf;
>  		struct intel_dev *dv;
> @@ -6892,6 +6905,89 @@ abort:
>  	return 0;
>  }
>  
> +/******************************************************************************
> + * function: imsm_create_metadata_update_for_migration()
> + *           Creates update for IMSM array.
> + *
> + ******************************************************************************/
> +static int imsm_create_metadata_update_for_migration(
> +					struct supertype *st,
> +					struct geo_params *geo,
> +					struct imsm_update_reshape **updatep)
> +{
> +	struct intel_super *super = st->sb;
> +	int update_memory_size = 0;
> +	struct imsm_update_reshape *u = NULL;
> +	int delta_disks = 0;
> +	struct imsm_dev *dev;
> +	int previous_level = -1;
> +
> +	dprintf("imsm_create_metadata_update_for_migration(enter)"
> +		" New Level = %i\n", geo->level);
> +	delta_disks = 0;
> +
> +	/* size of all update data without anchor */
> +	update_memory_size = sizeof(struct imsm_update_reshape);
> +
> +	/* now add space for spare disks that we need to add. */
> +	if (delta_disks > 0)
> +		update_memory_size += sizeof(u->new_disks[0])
> +					* (delta_disks - 1);
> +
> +	u = calloc(1, update_memory_size);
> +	if (u == NULL) {
> +		dprintf("error: cannot get memory for "
> +			"imsm_create_metadata_update_for_migration\n");
> +		return 0;
> +	}
> +	u->type = update_reshape_migration;
> +	u->subdev = super->current_vol;
> +	u->new_level = geo->level;
> +	u->new_layout = geo->layout;
> +	u->new_raid_disks = u->old_raid_disks = geo->raid_disks;
> +	u->new_disks[0] = -1;
> +
> +	dev = get_imsm_dev(super, u->subdev);
> +	if (dev) {
> +		struct imsm_map *map;
> +
> +		map = get_imsm_map(dev, 0);
> +		if (map)
> +			previous_level = map->raid_level;
> +	}
> +	if ((geo->level == 5) && (previous_level == 0)) {
> +		struct mdinfo *spares = NULL;
> +
> +		u->new_raid_disks++;
> +		spares = get_spares_for_grow(st);

And here you are allocating spares in mdadm even though we agreed that you
weren't going to do that.


NeilBrown




> +		if (spares) {
> +			struct dl *dl;
> +			struct mdinfo *dev;
> +
> +			dev = spares->devs;
> +			if (dev) {
> +				u->new_disks[0] = makedev(dev->disk.major,
> +							  dev->disk.minor);
> +				dl = get_disk_super(super,
> +						    dev->disk.major,
> +						    dev->disk.minor);
> +				dl->index = u->old_raid_disks;
> +				dev = dev->next;
> +			}
> +			sysfs_free(spares);
> +		} else {
> +			free(u);
> +			dprintf("error: cannot get spare device "
> +				"for requested migration");
> +			return 0;
> +		}
> +	}
> +	dprintf("imsm: reshape update preparation : OK\n");
> +	*updatep = u;
> +
> +	return update_memory_size;
> +}
> +
>  static void imsm_update_metadata_locally(struct supertype *st,
>  					 void *buf, int len)
>  {
> @@ -7146,9 +7242,25 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
>  		case CH_TAKEOVER:
>  			ret_val = imsm_takeover(st, &geo);
>  			break;
> -		case CH_MIGRATION:
> +		case CH_MIGRATION: {
> +			struct imsm_update_reshape *u = NULL;
> +			int len =
> +				imsm_create_metadata_update_for_migration(
> +					st, &geo, &u);
> +			if (len < 1) {
> +				dprintf("imsm: "
> +					"Cannot prepare update\n");
> +			}
>  			ret_val = 0;
> -			break;
> +			/* update metadata locally */
> +			imsm_update_metadata_locally(st, u, len);
> +			/* and possibly remotely */
> +			if (st->update_tail)
> +				append_metadata_update(st, u, len);
> +			else
> +				free(u);
> +		}
> +		break;
>  		default:
>  			ret_val = 1;
>  		}

RE: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0 transition

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, February 15, 2011 1:32 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> transition
> 
> On Mon, 14 Feb 2011 14:12:49 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > We have to set proper value of delta_disks to avoid it wrongly being
> set
> > when it value remains UnSet for this level transition (Grow.c:1224).
> >
> > This causes too small value set to "raid_disks" in sysfs
> > and reshape raid5->raid0 fails.
> >
> > 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 424d489..dba2825 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -1073,6 +1073,7 @@ char *analyse_change(struct mdinfo *info,
> struct reshape *re)
> >  		switch (info->new_level) {
> >  		case 0:
> >  			delta_parity = -1;
> > +			info->delta_disks = 0;
> >  		case 4:
> >  			re->level = info->array.level;
> >  			re->before.data_disks = info->array.raid_disks - 1;
> 
> I think we have different expectations about what a RAID5 -> RAID0
> transition
> means.
> 
> To me, it means getting rid of the parity information.  So a 4-device
> RAID5
> is converted to a 3-device RAID0 and stays the same size.
> 
> I think you want it to maintain the same number of devices, so a 4-
> device
> RAID5 becomes a 4-device RAID0 and thus has larger storage.
> 
> If you want that, you need to say:
>    mdadm -G /dev/md/xxx --level=0 --raid-disks=4
> 
> I'd be happy with functionality to do:
> 
>    mdadm -G /dev/md/xxx --level=0 --raid-disks=nochange
> 
> or something like that so it could be easily scripted easily, but I
> want the
> default to do the simplest possible change.
> 
> Am I correct about your expectations?

Yes you are right.
Working in the way you described above, will probably need a change in md or mdadm should degrade array after reshape (before takeover).
If I recall takeover code correctly, to execute takeover from raid4/5->raid0, raid4/5 has to be a degraded array.
This a reason I've make no changes to raid_disks number, as such behavior seams fit current implementation. 

Please let me know the direction you think we should go.

BR
Adam



> 
> Thanks,
> NeilBrown

RE: [PATCH 2/5] imsm: prepare update for level migrations reshape

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, February 15, 2011 2:27 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 2/5] imsm: prepare update for level migrations
> reshape
> 
> On Mon, 14 Feb 2011 14:12:57 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > For level migrations:
> > 1. raid0->raid5
> > 2. raid5->raid0
> > metadata update is prepared
> >
> > For migration raid0->raid5 spare device is required.
> > It is checked in mdadm for spares, but it is not included in to
> update.
> > Mdmon will decide what spare device
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  super-intel.c |  116
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 114 insertions(+), 2 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index f3e248e..489340f 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -305,6 +305,7 @@ enum imsm_update_type {
> >  	update_rename_array,
> >  	update_add_remove_disk,
> >  	update_reshape_container_disks,
> > +	update_reshape_migration,
> >  	update_takeover
> >  };
> >
> > @@ -340,6 +341,12 @@ struct imsm_update_reshape {
> >  	enum imsm_update_type type;
> >  	int old_raid_disks;
> >  	int new_raid_disks;
> > +	/* fields for array migration changes
> > +	 */
> > +	int subdev;
> > +	int new_level;
> > +	int new_layout;
> > +
> 
> I don't like how you are overloading this structure.  You are adding
> fields
> to a structure that doesn't need them in its original context, and
> using a
> structure containing old_raid_disks and new_raid_disks in a context
> where
> they are needed.
> 
> It would be much better to have a separate structure for each different
> imsm_update_type.
> 
> 
> >  	int new_disks[1]; /* new_raid_disks - old_raid_disks makedev
> number */
> 
> Hmmm... new_disks is already there, isn't it.
> I'm not at all sure that I'm happy about that.
> It is used when increasing the number of devices in an array, but I
> would
> like that to just change  the numbers and leave mdmon to add the
> devices.
> 
> 'reshape_super'  should just verify that the change is legal, update
> the
> metadata to show that the change has happened or is happening, and
> possibly
> create an update to send to mdmon.
> reshape_super should not choose or allocate spares.
> 
> 'prepare_update' should *only* perform any allocations that
> process_update
> will need.  It doesn't allocate spares either.  It *only* allocates
> memory.
> 
> 'process_update' takes the update description and modifies the
> metadata, and
> updates internal data structures so that they accurately reflect the
> information in the metadata.  It doesn't allocate spares either.
> 
> 'activate_spare' is the only place that should activate spares.  It
> analyses
> the array, determines if any devices need to be added, finds them, and
> creates a metadata update to describe the new devices.
> manage_member() then adds them to the array and process_update
> eventually
> records these new allocations in the metadata.
> 
> It is really important to keep these function separate and make sure
> each
> function just does the one thing that it is supposed to do.  Otherwise
> all
> the interactions become too complex and so are easily broken.



I'll think how to connect all together.



> 
> 
> >  };
> >
> > @@ -6133,6 +6140,9 @@ static void imsm_process_update(struct
> supertype *st,
> >  			super->updates_pending++;
> >  		break;
> >  	}
> > +	case update_reshape_migration: {
> > +		break;
> > +	}
> >  	case update_activate_spare: {
> >  		struct imsm_update_activate_spare *u = (void *) update-
> >buf;
> >  		struct imsm_dev *dev = get_imsm_dev(super, u->array);
> > @@ -6526,6 +6536,9 @@ static void imsm_prepare_update(struct
> supertype *st,
> >  		dprintf("New anchor length is %llu\n", (unsigned long
> long)len);
> >  		break;
> >  	}
> > +	case update_reshape_migration: {
> > +		break;
> > +	}
> >  	case update_create_array: {
> >  		struct imsm_update_create_array *u = (void *) update->buf;
> >  		struct intel_dev *dv;
> > @@ -6892,6 +6905,89 @@ abort:
> >  	return 0;
> >  }
> >
> >
> +/*********************************************************************
> *********
> > + * function: imsm_create_metadata_update_for_migration()
> > + *           Creates update for IMSM array.
> > + *
> > +
> ***********************************************************************
> *******/
> > +static int imsm_create_metadata_update_for_migration(
> > +					struct supertype *st,
> > +					struct geo_params *geo,
> > +					struct imsm_update_reshape **updatep)
> > +{
> > +	struct intel_super *super = st->sb;
> > +	int update_memory_size = 0;
> > +	struct imsm_update_reshape *u = NULL;
> > +	int delta_disks = 0;
> > +	struct imsm_dev *dev;
> > +	int previous_level = -1;
> > +
> > +	dprintf("imsm_create_metadata_update_for_migration(enter)"
> > +		" New Level = %i\n", geo->level);
> > +	delta_disks = 0;
> > +
> > +	/* size of all update data without anchor */
> > +	update_memory_size = sizeof(struct imsm_update_reshape);
> > +
> > +	/* now add space for spare disks that we need to add. */
> > +	if (delta_disks > 0)
> > +		update_memory_size += sizeof(u->new_disks[0])
> > +					* (delta_disks - 1);
> > +
> > +	u = calloc(1, update_memory_size);
> > +	if (u == NULL) {
> > +		dprintf("error: cannot get memory for "
> > +			"imsm_create_metadata_update_for_migration\n");
> > +		return 0;
> > +	}
> > +	u->type = update_reshape_migration;
> > +	u->subdev = super->current_vol;
> > +	u->new_level = geo->level;
> > +	u->new_layout = geo->layout;
> > +	u->new_raid_disks = u->old_raid_disks = geo->raid_disks;
> > +	u->new_disks[0] = -1;
> > +
> > +	dev = get_imsm_dev(super, u->subdev);
> > +	if (dev) {
> > +		struct imsm_map *map;
> > +
> > +		map = get_imsm_map(dev, 0);
> > +		if (map)
> > +			previous_level = map->raid_level;
> > +	}
> > +	if ((geo->level == 5) && (previous_level == 0)) {
> > +		struct mdinfo *spares = NULL;
> > +
> > +		u->new_raid_disks++;
> > +		spares = get_spares_for_grow(st);
> 
> And here you are allocating spares in mdadm even though we agreed that
> you
> weren't going to do that.
> 
> 
> NeilBrown

Yes you are right, using spares below is my mistake during code rebasing.
Allocating spares I think is ok as spares check, but not using them.

Summarizing I have make some changes to all patches.
This have to wait few day, I'm working now on issue in expansion.
Details, I'll sent you shortly.

BR
Adam

> 
> 
> 
> 
> > +		if (spares) {
> > +			struct dl *dl;
> > +			struct mdinfo *dev;
> > +
> > +			dev = spares->devs;
> > +			if (dev) {
> > +				u->new_disks[0] = makedev(dev->disk.major,
> > +							  dev->disk.minor);
> > +				dl = get_disk_super(super,
> > +						    dev->disk.major,
> > +						    dev->disk.minor);
> > +				dl->index = u->old_raid_disks;
> > +				dev = dev->next;
> > +			}
> > +			sysfs_free(spares);
> > +		} else {
> > +			free(u);
> > +			dprintf("error: cannot get spare device "
> > +				"for requested migration");
> > +			return 0;
> > +		}
> > +	}
> > +	dprintf("imsm: reshape update preparation : OK\n");
> > +	*updatep = u;
> > +
> > +	return update_memory_size;
> > +}
> > +
> >  static void imsm_update_metadata_locally(struct supertype *st,
> >  					 void *buf, int len)
> >  {
> > @@ -7146,9 +7242,25 @@ static int imsm_reshape_super(struct supertype
> *st, long long size, int level,
> >  		case CH_TAKEOVER:
> >  			ret_val = imsm_takeover(st, &geo);
> >  			break;
> > -		case CH_MIGRATION:
> > +		case CH_MIGRATION: {
> > +			struct imsm_update_reshape *u = NULL;
> > +			int len =
> > +				imsm_create_metadata_update_for_migration(
> > +					st, &geo, &u);
> > +			if (len < 1) {
> > +				dprintf("imsm: "
> > +					"Cannot prepare update\n");
> > +			}
> >  			ret_val = 0;
> > -			break;
> > +			/* update metadata locally */
> > +			imsm_update_metadata_locally(st, u, len);
> > +			/* and possibly remotely */
> > +			if (st->update_tail)
> > +				append_metadata_update(st, u, len);
> > +			else
> > +				free(u);
> > +		}
> > +		break;
> >  		default:
> >  			ret_val = 1;
> >  		}

Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0 transition

[NeilBrown]

On Tue, 15 Feb 2011 08:30:22 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Tuesday, February 15, 2011 1:32 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> > transition
> > 
> > On Mon, 14 Feb 2011 14:12:49 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> > 
> > > We have to set proper value of delta_disks to avoid it wrongly being
> > set
> > > when it value remains UnSet for this level transition (Grow.c:1224).
> > >
> > > This causes too small value set to "raid_disks" in sysfs
> > > and reshape raid5->raid0 fails.
> > >
> > > 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 424d489..dba2825 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -1073,6 +1073,7 @@ char *analyse_change(struct mdinfo *info,
> > struct reshape *re)
> > >  		switch (info->new_level) {
> > >  		case 0:
> > >  			delta_parity = -1;
> > > +			info->delta_disks = 0;
> > >  		case 4:
> > >  			re->level = info->array.level;
> > >  			re->before.data_disks = info->array.raid_disks - 1;
> > 
> > I think we have different expectations about what a RAID5 -> RAID0
> > transition
> > means.
> > 
> > To me, it means getting rid of the parity information.  So a 4-device
> > RAID5
> > is converted to a 3-device RAID0 and stays the same size.
> > 
> > I think you want it to maintain the same number of devices, so a 4-
> > device
> > RAID5 becomes a 4-device RAID0 and thus has larger storage.
> > 
> > If you want that, you need to say:
> >    mdadm -G /dev/md/xxx --level=0 --raid-disks=4
> > 
> > I'd be happy with functionality to do:
> > 
> >    mdadm -G /dev/md/xxx --level=0 --raid-disks=nochange
> > 
> > or something like that so it could be easily scripted easily, but I
> > want the
> > default to do the simplest possible change.
> > 
> > Am I correct about your expectations?
> 
> Yes you are right.
> Working in the way you described above, will probably need a change in md or mdadm should degrade array after reshape (before takeover).
> If I recall takeover code correctly, to execute takeover from raid4/5->raid0, raid4/5 has to be a degraded array.
> This a reason I've make no changes to raid_disks number, as such behavior seams fit current implementation. 
> 
> Please let me know the direction you think we should go.
> 

I don't understand....
Working "the way I described" is just a slightly different way that arguments
passed to mdadm are interpreted.  The underlying process can still happen
exactly the same way - you just have to ask for it slightly differently.  No
change in md or mdadm required.

In any case, care is required if the RAID5 array goes degraded before you
switch it to RAID0.  The sensible thing would probably be to leave it in
RAID4 as the final switch to RAID0 would effectively destroy all your data.
I suspect the kernel wouldn't allow the final switch if the RAID5 were
degraded.

The direction that I want to go is exactly as stated above.  If no explicit
request is made to set the new value for raid-disks, then the change made
should be the simplest possible - leaving the number of data-disks unchanged.

NeilBrown

RE: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0 transition

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, February 15, 2011 9:58 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> transition
> 
> On Tue, 15 Feb 2011 08:30:22 +0000 "Kwolek, Adam"
> <adam.kwolek@intel.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Tuesday, February 15, 2011 1:32 AM
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > > Neubauer, Wojciech
> > > Subject: Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> > > transition
> > >
> > > On Mon, 14 Feb 2011 14:12:49 +0100 Adam Kwolek
> <adam.kwolek@intel.com>
> > > wrote:
> > >
> > > > We have to set proper value of delta_disks to avoid it wrongly
> being
> > > set
> > > > when it value remains UnSet for this level transition
> (Grow.c:1224).
> > > >
> > > > This causes too small value set to "raid_disks" in sysfs
> > > > and reshape raid5->raid0 fails.
> > > >
> > > > 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 424d489..dba2825 100644
> > > > --- a/Grow.c
> > > > +++ b/Grow.c
> > > > @@ -1073,6 +1073,7 @@ char *analyse_change(struct mdinfo *info,
> > > struct reshape *re)
> > > >  		switch (info->new_level) {
> > > >  		case 0:
> > > >  			delta_parity = -1;
> > > > +			info->delta_disks = 0;
> > > >  		case 4:
> > > >  			re->level = info->array.level;
> > > >  			re->before.data_disks = info->array.raid_disks
> - 1;
> > >
> > > I think we have different expectations about what a RAID5 -> RAID0
> > > transition
> > > means.
> > >
> > > To me, it means getting rid of the parity information.  So a 4-
> device
> > > RAID5
> > > is converted to a 3-device RAID0 and stays the same size.
> > >
> > > I think you want it to maintain the same number of devices, so a 4-
> > > device
> > > RAID5 becomes a 4-device RAID0 and thus has larger storage.
> > >
> > > If you want that, you need to say:
> > >    mdadm -G /dev/md/xxx --level=0 --raid-disks=4
> > >
> > > I'd be happy with functionality to do:
> > >
> > >    mdadm -G /dev/md/xxx --level=0 --raid-disks=nochange
> > >
> > > or something like that so it could be easily scripted easily, but I
> > > want the
> > > default to do the simplest possible change.
> > >
> > > Am I correct about your expectations?
> >
> > Yes you are right.
> > Working in the way you described above, will probably need a change
> in md or mdadm should degrade array after reshape (before takeover).
> > If I recall takeover code correctly, to execute takeover from
> raid4/5->raid0, raid4/5 has to be a degraded array.
> > This a reason I've make no changes to raid_disks number, as such
> behavior seams fit current implementation.
> >
> > Please let me know the direction you think we should go.
> >
> 
> I don't understand....
> Working "the way I described" is just a slightly different way that
> arguments
> passed to mdadm are interpreted.  The underlying process can still
> happen
> exactly the same way - you just have to ask for it slightly
> differently.  No
> change in md or mdadm required.
>
> In any case, care is required if the RAID5 array goes degraded before
> you
> switch it to RAID0.  The sensible thing would probably be to leave it
> in
> RAID4 as the final switch to RAID0 would effectively destroy all your
> data.
> I suspect the kernel wouldn't allow the final switch if the RAID5 were
> degraded.

Look raid0.c:586, raid5 cannot be takeovered to raid0 if it is not degraded.
Parity disk (in raid4) has to be "virtual". 
In situation when we want to throw away parity disk, we have to make array degraded first or change md behavior (imho).

> The direction that I want to go is exactly as stated above.  If no
> explicit
> request is made to set the new value for raid-disks, then the change
> made
> should be the simplest possible - leaving the number of data-disks
> unchanged.
> 
> NeilBrown

I wanted to maintain the same number of devices due to IMSM compatibility.
It there are 2 arrays in container, number of devices in array cannot be changed in one array only.
I wanted to avoid different command behavior depending on arrays number in container.
This can make user confused, so I've decided to keep raid_disks without change.

BR
Adam

RE: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0 transition

[Hawrylewicz Czarnowski, Przemyslaw]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Kwolek, Adam
> Sent: Tuesday, February 15, 2011 11:13 AM
> To: NeilBrown
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> transition
> 
> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Tuesday, February 15, 2011 9:58 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> > transition
> >
> > On Tue, 15 Feb 2011 08:30:22 +0000 "Kwolek, Adam"
> > <adam.kwolek@intel.com>
> > wrote:
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: NeilBrown [mailto:neilb@suse.de]
> > > > Sent: Tuesday, February 15, 2011 1:32 AM
> > > > To: Kwolek, Adam
> > > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > > > Neubauer, Wojciech
> > > > Subject: Re: [PATCH 1/5] FIX: set delta_disks to 0 for raid5->raid0
> > > > transition
> > > >
> > > > On Mon, 14 Feb 2011 14:12:49 +0100 Adam Kwolek
> > <adam.kwolek@intel.com>
> > > > wrote:
> > > >
> > > > > We have to set proper value of delta_disks to avoid it wrongly
> > being
> > > > set
> > > > > when it value remains UnSet for this level transition
> > (Grow.c:1224).
> > > > >
> > > > > This causes too small value set to "raid_disks" in sysfs
> > > > > and reshape raid5->raid0 fails.
> > > > >
> > > > > 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 424d489..dba2825 100644
> > > > > --- a/Grow.c
> > > > > +++ b/Grow.c
> > > > > @@ -1073,6 +1073,7 @@ char *analyse_change(struct mdinfo *info,
> > > > struct reshape *re)
> > > > >  		switch (info->new_level) {
> > > > >  		case 0:
> > > > >  			delta_parity = -1;
> > > > > +			info->delta_disks = 0;
> > > > >  		case 4:
> > > > >  			re->level = info->array.level;
> > > > >  			re->before.data_disks = info->array.raid_disks
> > - 1;
> > > >
> > > > I think we have different expectations about what a RAID5 -> RAID0
> > > > transition
> > > > means.
> > > >
> > > > To me, it means getting rid of the parity information.  So a 4-
> > device
> > > > RAID5
> > > > is converted to a 3-device RAID0 and stays the same size.
> > > >
> > > > I think you want it to maintain the same number of devices, so a 4-
> > > > device
> > > > RAID5 becomes a 4-device RAID0 and thus has larger storage.
> > > >
> > > > If you want that, you need to say:
> > > >    mdadm -G /dev/md/xxx --level=0 --raid-disks=4
> > > >
> > > > I'd be happy with functionality to do:
> > > >
> > > >    mdadm -G /dev/md/xxx --level=0 --raid-disks=nochange
> > > >
> > > > or something like that so it could be easily scripted easily, but I
> > > > want the
> > > > default to do the simplest possible change.
> > > >
> > > > Am I correct about your expectations?
> > >
> > > Yes you are right.
> > > Working in the way you described above, will probably need a change
> > in md or mdadm should degrade array after reshape (before takeover).
> > > If I recall takeover code correctly, to execute takeover from
> > raid4/5->raid0, raid4/5 has to be a degraded array.
> > > This a reason I've make no changes to raid_disks number, as such
> > behavior seams fit current implementation.
> > >
> > > Please let me know the direction you think we should go.
> > >
> >
> > I don't understand....
> > Working "the way I described" is just a slightly different way that
> > arguments
> > passed to mdadm are interpreted.  The underlying process can still
> > happen
> > exactly the same way - you just have to ask for it slightly
> > differently.  No
> > change in md or mdadm required.
> >
> > In any case, care is required if the RAID5 array goes degraded before
> > you
> > switch it to RAID0.  The sensible thing would probably be to leave it
> > in
> > RAID4 as the final switch to RAID0 would effectively destroy all your
> > data.
> > I suspect the kernel wouldn't allow the final switch if the RAID5 were
> > degraded.
> 
> Look raid0.c:586, raid5 cannot be takeovered to raid0 if it is not
> degraded.
> Parity disk (in raid4) has to be "virtual".
> In situation when we want to throw away parity disk, we have to make array
> degraded first or change md behavior (imho).
> 
> > The direction that I want to go is exactly as stated above.  If no
> > explicit
> > request is made to set the new value for raid-disks, then the change
> > made
> > should be the simplest possible - leaving the number of data-disks
> > unchanged.
> >
> > NeilBrown
> 
> I wanted to maintain the same number of devices due to IMSM compatibility.
> It there are 2 arrays in container, number of devices in array cannot be
> changed in one array only.
> I wanted to avoid different command behavior depending on arrays number in
> container.
> This can make user confused, so I've decided to keep raid_disks without
> change.
> 
> BR
> Adam
> 
Hi,

I am taking over the work on migration support for IMSM metadata. 
Right now I am focused on generic grow routines as at the current state they won't work for external metadata; especially when new chunk size differs (is greater) from original (some updates will appear on linux-raid shortly).
I would also like to agree on form of migration of Raid5->Raid0, as approach we consider the best for IMSM (with increase of data disks) is not the same as the most straightforward one (ie. leaving number of data disks constant).

-- 
Best Regards,
Przemyslaw Hawrylewicz-Czarnowski