[PATCH 0/4] raid0<->raid10 takeover for imsm metadata

[PATCH 0/4] raid0<->raid10 takeover for imsm metadata

[Krzysztof Wojcik]

The following series implements raid0 <-> raid10 takeover support
for imsm metadata format.
It includes corrections according last code review notes.

---

Krzysztof Wojcik (4):
      Add raid10 -> raid0 takeover support
      raid0->raid10 takeover- create metadata update
      raid0->raid10 takeover- allocate memory for added disks
      raid0->raid10 takeover- process metadata update


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

-- 
Krzysztof Wojcik

[PATCH 1/4] Add raid10 -> raid0 takeover support

[Krzysztof Wojcik]

The patch introduces takeover form level 10 to level 0 for imsm
metadata. This patch contains procedures connected with preparing
and applying metadata update during 10 -> 0 takeover.
When performing takeover 10->0 mdmon should update the external
metadata (due to disk slot and level changes).
To achieve that mdadm calls reshape_super() and prepare
the "update_takeover" metadata update type.
Prepared update is processed by mdmon in process_update().

Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
 Grow.c        |    1 +
 super-intel.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 1 deletions(-)

diff --git a/Grow.c b/Grow.c
index ad13d6e..c4125ea 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1463,6 +1463,7 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
 			rv = 1;
 			goto release;
 		}
+		ping_monitor(container);
 	}
 
 	info.array = array;
diff --git a/super-intel.c b/super-intel.c
index 334e0f4..86b365d 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -299,6 +299,7 @@ enum imsm_update_type {
 	update_rename_array,
 	update_add_remove_disk,
 	update_reshape_container_disks,
+	update_takeover
 };
 
 struct imsm_update_activate_spare {
@@ -319,6 +320,15 @@ struct geo_params {
 	int raid_disks;
 };
 
+enum takeover_direction {
+	R10_TO_R0,
+	R0_TO_R10
+};
+struct imsm_update_takeover {
+	enum imsm_update_type type;
+	int subarray;
+	enum takeover_direction direction;
+};
 
 struct imsm_update_reshape {
 	enum imsm_update_type type;
@@ -5802,6 +5812,56 @@ update_reshape_exit:
 	return ret_val;
 }
 
+static int apply_takeover_update(struct imsm_update_takeover *u,
+				struct intel_super *super)
+{
+	struct imsm_dev *dev = NULL;
+	struct imsm_map *map;
+	struct dl *dm, *du;
+	struct intel_dev *dv;
+
+	for (dv = super->devlist; dv; dv = dv->next)
+		if (dv->index == (unsigned int)u->subarray) {
+			dev = dv->dev;
+			break;
+	}
+
+	if (dev == NULL)
+		return 0;
+
+	map = get_imsm_map(dev, 0);
+
+	if (u->direction == R10_TO_R0) {
+		/* iterate through devices to mark removed disks as spare */
+		for (dm = super->disks; dm; dm = dm->next) {
+			if (dm->disk.status & FAILED_DISK) {
+				int idx = dm->index;
+				/* update indexes on the disk list */
+				for (du = super->disks; du; du = du->next)
+					if (du->index > idx)
+						du->index--;
+				/* mark as spare disk */
+				dm->disk.status = SPARE_DISK;
+				dm->index = -1;
+			}
+		}
+
+		/* update map */
+		map->num_members = map->num_members / 2;
+		map->map_state = IMSM_T_STATE_NORMAL;
+		map->num_domains = 1;
+		map->raid_level = 0;
+		map->failed_disk_num = -1;
+	}
+
+	/* update disk order table */
+	for (du = super->disks; du; du = du->next)
+		if (du->index >= 0)
+			set_imsm_ord_tbl_ent(map, du->index, du->index);
+
+	return 1;
+}
+
 static void imsm_process_update(struct supertype *st,
 			        struct metadata_update *update)
 {
@@ -5844,6 +5904,13 @@ static void imsm_process_update(struct supertype *st,
 	mpb = super->anchor;
 
 	switch (type) {
+	case update_takeover: {
+		struct imsm_update_takeover *u = (void *)update->buf;
+		if (apply_takeover_update(u, super))
+			super->updates_pending++;
+		break;
+	}
+
 	case update_reshape_container_disks: {
 		struct imsm_update_reshape *u = (void *)update->buf;
 		if (apply_reshape_container_disks_update(
@@ -6691,6 +6758,35 @@ analyse_change_exit:
 	return change;
 }
 
+int imsm_takeover(struct supertype *st, struct geo_params *geo)
+{
+	struct intel_super *super = st->sb;
+	struct imsm_update_takeover *u;
+
+	u = malloc(sizeof(struct imsm_update_takeover));
+	if (u == NULL)
+		return 1;
+
+	u->type = update_takeover;
+	u->subarray = super->current_vol;
+
+	/* 10->0 transition */
+	if (geo->level == 0)
+		u->direction = R10_TO_R0;
+
+	/* update metadata locally */
+	imsm_update_metadata_locally(st, u,
+					sizeof(struct imsm_update_takeover));
+	/* and possibly remotely */
+	if (st->update_tail)
+		append_metadata_update(st, u,
+					sizeof(struct imsm_update_takeover));
+	else
+		free(u);
+
+	return 0;
+}
+
 static int imsm_reshape_super(struct supertype *st, long long size, int level,
 			      int layout, int chunksize, int raid_disks,
 			      char *backup, char *dev, int verbose)
@@ -6772,7 +6868,7 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
 		change = imsm_analyze_change(st, &geo);
 		switch (change) {
 			case CH_TAKEOVER:
-				ret_val = 0;
+				ret_val = imsm_takeover(st, &geo);
 				break;
 			case CH_CHUNK_MIGR:
 				ret_val = 0;

[PATCH 2/4] raid0->raid10 takeover- create metadata update

[Krzysztof Wojcik]

Create metadata update for raid0 -> raid10 takeover.
Because we have no mdmon running for raid0 we have to
update metadata using local update mechanism

Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
 super-intel.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 86b365d..3ba9962 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6774,6 +6774,10 @@ int imsm_takeover(struct supertype *st, struct geo_params *geo)
 	if (geo->level == 0)
 		u->direction = R10_TO_R0;
 
+	/* 0->10 transition */
+	if (geo->level == 10)
+		u->direction = R0_TO_R10;
+
 	/* update metadata locally */
 	imsm_update_metadata_locally(st, u,
 					sizeof(struct imsm_update_takeover));

[PATCH 3/4] raid0->raid10 takeover- allocate memory for added disks

[Krzysztof Wojcik]

Allocate memory will be used in process_update.
For raid0->raid10 takeover operation number of disks doubles
so we should allocate memory for additional disks
and one imsm_dev structure with extended order table.

Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
 super-intel.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 3ba9962..3b2a273 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6231,6 +6231,54 @@ static void imsm_prepare_update(struct supertype *st,
 	size_t len = 0;
 
 	switch (type) {
+	case update_takeover: {
+		struct imsm_update_takeover *u = (void *)update->buf;
+		if (u->direction == R0_TO_R10) {
+			void **tail = (void **)&update->space_list;
+			struct imsm_dev *dev = get_imsm_dev(super, u->subarray);
+			struct imsm_map *map = get_imsm_map(dev, 0);
+			int num_members = map->num_members;
+			void *space;
+			int size, i;
+			int err = 0;
+			/* allocate memory for added disks */
+			for (i = 0; i < num_members; i++) {
+				size = sizeof(struct dl);
+				space = malloc(size);
+				if (!space) {
+					err++;
+					goto update_takeover_exit;
+				}
+				*tail = space;
+				tail = space;
+				*tail = NULL;
+			}
+			/* allocate memory for new device */
+			size = sizeof_imsm_dev(super->devlist->dev, 0) +
+				(num_members * sizeof(__u32));
+			space = malloc(size);
+			if (!space) {
+				err++;
+				goto update_takeover_exit;
+			}
+			*tail = space;
+			tail = space;
+			*tail = NULL;
+update_takeover_exit:
+			if (!err) {
+				len = disks_to_mpb_size(num_members * 2);
+			} else {
+				/* if allocation didn't success, free buffer */
+				while (update->space_list) {
+					void **sp = update->space_list;
+					update->space_list = *sp;
+					free(sp);
+				}
+			}
+		}
+
+		break;
+	}
 	case update_reshape_container_disks: {
 		/* Every raid device in the container is about to
 		 * gain some more devices, and we will enter a

[PATCH 4/4] raid0->raid10 takeover- process metadata update

[Krzysztof Wojcik]

Implementation of raid0->raid10 takeover metadata update
at process_update level.
- We are using memory prievously alocated in prepare_update to
create two dummy disks will be inserted in the metadata and
new imsm_dev structure with expanded disk order table.
- Update indexes in disk list
- Update metadata map
- Update disk order table

Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
 super-intel.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 3b2a273..e284a4a 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5813,12 +5813,15 @@ update_reshape_exit:
 }
 
 static int apply_takeover_update(struct imsm_update_takeover *u,
-				struct intel_super *super)
+				struct intel_super *super,
+				void ***space_list)
 {
 	struct imsm_dev *dev = NULL;
+	struct intel_dev *dv;
+	struct imsm_dev *dev_new;
 	struct imsm_map *map;
 	struct dl *dm, *du;
-	struct intel_dev *dv;
+	int i;
 
 	for (dv = super->devlist; dv; dv = dv->next)
 		if (dv->index == (unsigned int)u->subarray) {
@@ -5845,7 +5848,6 @@ static int apply_takeover_update(struct imsm_update_takeover *u,
 				dm->index = -1;
 			}
 		}
-
 		/* update map */
 		map->num_members = map->num_members / 2;
 		map->map_state = IMSM_T_STATE_NORMAL;
@@ -5854,10 +5856,59 @@ static int apply_takeover_update(struct imsm_update_takeover *u,
 		map->failed_disk_num = -1;
 	}
 
+	if (u->direction == R0_TO_R10) {
+		void **space;
+		/* update slots in current disk list */
+		for (dm = super->disks; dm; dm = dm->next) {
+			if (dm->index >= 0)
+				dm->index *= 2;
+		}
+		/* create new *missing* disks */
+		for (i = 0; i < map->num_members; i++) {
+			space = *space_list;
+			if (!space)
+				continue;
+			*space_list = *space;
+			du = (void *)space;
+			memcpy(du, super->disks, sizeof(*du));
+			du->disk.status = FAILED_DISK;
+			du->disk.scsi_id = 0;
+			du->fd = -1;
+			du->minor = 0;
+			du->major = 0;
+			du->index = (i * 2) + 1;
+			sprintf((char *)du->disk.serial,
+				" MISSING_%d", du->index);
+			sprintf((char *)du->serial,
+				"MISSING_%d", du->index);
+			du->next = super->missing;
+			super->missing = du;
+		}
+		/* create new dev and map */
+		space = *space_list;
+		if (!space)
+			return 0;
+		*space_list = *space;
+		dev_new = (void *)space;
+		memcpy(dev_new, dev, sizeof(*dev));
+		/* update new map */
+		map = get_imsm_map(dev_new, 0);
+		map->failed_disk_num = map->num_members;
+		map->num_members = map->num_members * 2;
+		map->map_state = IMSM_T_STATE_NORMAL;
+		map->num_domains = 2;
+		map->raid_level = 1;
+		/* replace dev<->dev_new */
+		dv->dev = dev_new;
+	}
 	/* update disk order table */
 	for (du = super->disks; du; du = du->next)
 		if (du->index >= 0)
 			set_imsm_ord_tbl_ent(map, du->index, du->index);
+	for (du = super->missing; du; du = du->next)
+		if (du->index >= 0)
+			set_imsm_ord_tbl_ent(map, du->index,
+						du->index | IMSM_ORD_REBUILD);
 
 	return 1;
 }
@@ -5906,7 +5957,7 @@ static void imsm_process_update(struct supertype *st,
 	switch (type) {
 	case update_takeover: {
 		struct imsm_update_takeover *u = (void *)update->buf;
-		if (apply_takeover_update(u, super))
+		if (apply_takeover_update(u, super, &update->space_list))
 			super->updates_pending++;
 		break;
 	}

Re: [PATCH 1/4] Add raid10 -> raid0 takeover support

[Neil Brown]

On Mon, 17 Jan 2011 16:42:43 +0100
Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:

> The patch introduces takeover form level 10 to level 0 for imsm
> metadata. This patch contains procedures connected with preparing
> and applying metadata update during 10 -> 0 takeover.
> When performing takeover 10->0 mdmon should update the external
> metadata (due to disk slot and level changes).
> To achieve that mdadm calls reshape_super() and prepare
> the "update_takeover" metadata update type.
> Prepared update is processed by mdmon in process_update().

I've applied this, but I'm not sure that I like it.

I don't think mdadm needs to send a metadata update for 10->0
conversion.
It should:
 - fail all the devices that aren't wanted
 - remove them
 - mdmon notices and updates the metadata
 - mdadm changes the level to RAID0
 - mdmon notices and updates the metadata.

Currently mdmon doesn't monitor 'level', but it could and probably
should.
However in the interest of forward progress I won't insist on that for
now.

Other problems:

> 
> Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> ---
>  Grow.c        |    1 +
>  super-intel.c |   98
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files
> changed, 98 insertions(+), 1 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index ad13d6e..c4125ea 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1463,6 +1463,7 @@ int Grow_reshape(char *devname, int fd, int
> quiet, char *backup_file, rv = 1;
>  			goto release;
>  		}
> +		ping_monitor(container);

Why is this here??? EVERY change should be justified in the notes at
the top!!!!!!!


>  	}
>  
>  	info.array = array;
> diff --git a/super-intel.c b/super-intel.c
> index 334e0f4..86b365d 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -299,6 +299,7 @@ enum imsm_update_type {
>  	update_rename_array,
>  	update_add_remove_disk,
>  	update_reshape_container_disks,
> +	update_takeover
>  };
>  
>  struct imsm_update_activate_spare {
> @@ -319,6 +320,15 @@ struct geo_params {
>  	int raid_disks;
>  };
>  
> +enum takeover_direction {
> +	R10_TO_R0,
> +	R0_TO_R10
> +};
> +struct imsm_update_takeover {
> +	enum imsm_update_type type;
> +	int subarray;
> +	enum takeover_direction direction;
> +};
>  
>  struct imsm_update_reshape {
>  	enum imsm_update_type type;
> @@ -5802,6 +5812,56 @@ update_reshape_exit:
>  	return ret_val;
>  }
>  
> +static int apply_takeover_update(struct imsm_update_takeover *u,
> +				struct intel_super *super)
> +{
> +	struct imsm_dev *dev = NULL;
> +	struct imsm_map *map;
> +	struct dl *dm, *du;
> +	struct intel_dev *dv;
> +
> +	for (dv = super->devlist; dv; dv = dv->next)
> +		if (dv->index == (unsigned int)u->subarray) {
> +			dev = dv->dev;
> +			break;
> +	}
> +
> +	if (dev == NULL)
> +		return 0;
> +
> +	map = get_imsm_map(dev, 0);
> +
> +	if (u->direction == R10_TO_R0) {
> +		/* iterate through devices to mark removed disks as
> spare */
> +		for (dm = super->disks; dm; dm = dm->next) {
> +			if (dm->disk.status & FAILED_DISK) {
> +				int idx = dm->index;
> +				/* update indexes on the disk list */
> +				for (du = super->disks; du; du =
> du->next)
> +					if (du->index > idx)
> +						du->index--;
> +				/* mark as spare disk */
> +				dm->disk.status = SPARE_DISK;
> +				dm->index = -1;

This looks like it is probably wrong, but I'm not exactly sure what it
is trying to do so I cannot be sure.
In particular the changes to 'index' seem to be based on some
assumption that are not necessarily true.
Could you please explain exactly what this is trying to achieve?

> +			}
> +		}
> +
> +		/* update map */
> +		map->num_members = map->num_members / 2;
> +		map->map_state = IMSM_T_STATE_NORMAL;
> +		map->num_domains = 1;

I'm confused by this 'num_domains' thing.  Could you please explain
it for me.  The term "parity domains" is meaningless to me in this
context.

Thanks,
NeilBrown



> +		map->raid_level = 0;
> +		map->failed_disk_num = -1;
> +	}
> +
> +	/* update disk order table */
> +	for (du = super->disks; du; du = du->next)
> +		if (du->index >= 0)
> +			set_imsm_ord_tbl_ent(map, du->index,
> du->index); +
> +	return 1;
> +}
> +
>  static void imsm_process_update(struct supertype *st,
>  			        struct metadata_update *update)
>  {
> @@ -5844,6 +5904,13 @@ static void imsm_process_update(struct
> supertype *st, mpb = super->anchor;
>  
>  	switch (type) {
> +	case update_takeover: {
> +		struct imsm_update_takeover *u = (void *)update->buf;
> +		if (apply_takeover_update(u, super))
> +			super->updates_pending++;
> +		break;
> +	}
> +
>  	case update_reshape_container_disks: {
>  		struct imsm_update_reshape *u = (void *)update->buf;
>  		if (apply_reshape_container_disks_update(
> @@ -6691,6 +6758,35 @@ analyse_change_exit:
>  	return change;
>  }
>  
> +int imsm_takeover(struct supertype *st, struct geo_params *geo)
> +{
> +	struct intel_super *super = st->sb;
> +	struct imsm_update_takeover *u;
> +
> +	u = malloc(sizeof(struct imsm_update_takeover));
> +	if (u == NULL)
> +		return 1;
> +
> +	u->type = update_takeover;
> +	u->subarray = super->current_vol;
> +
> +	/* 10->0 transition */
> +	if (geo->level == 0)
> +		u->direction = R10_TO_R0;
> +
> +	/* update metadata locally */
> +	imsm_update_metadata_locally(st, u,
> +					sizeof(struct
> imsm_update_takeover));
> +	/* and possibly remotely */
> +	if (st->update_tail)
> +		append_metadata_update(st, u,
> +					sizeof(struct
> imsm_update_takeover));
> +	else
> +		free(u);
> +
> +	return 0;
> +}
> +
>  static int imsm_reshape_super(struct supertype *st, long long size,
> int level, int layout, int chunksize, int raid_disks,
>  			      char *backup, char *dev, int verbose)
> @@ -6772,7 +6868,7 @@ static int imsm_reshape_super(struct supertype
> *st, long long size, int level, change = imsm_analyze_change(st,
> &geo); switch (change) {
>  			case CH_TAKEOVER:
> -				ret_val = 0;
> +				ret_val = imsm_takeover(st, &geo);
>  				break;
>  			case CH_CHUNK_MIGR:
>  				ret_val = 0;

RE: [PATCH 1/4] Add raid10 -> raid0 takeover support

[Wojcik, Krzysztof]

> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Wednesday, January 26, 2011 1:59 AM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> Williams, Dan J; Ciechanowski, Ed
> Subject: Re: [PATCH 1/4] Add raid10 -> raid0 takeover support
> 
> On Mon, 17 Jan 2011 16:42:43 +0100
> Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
> 
> > The patch introduces takeover form level 10 to level 0 for imsm
> > metadata. This patch contains procedures connected with preparing
> > and applying metadata update during 10 -> 0 takeover.
> > When performing takeover 10->0 mdmon should update the external
> > metadata (due to disk slot and level changes).
> > To achieve that mdadm calls reshape_super() and prepare
> > the "update_takeover" metadata update type.
> > Prepared update is processed by mdmon in process_update().
> 
> I've applied this, but I'm not sure that I like it.
> 
> I don't think mdadm needs to send a metadata update for 10->0
> conversion.
> It should:
>  - fail all the devices that aren't wanted
>  - remove them
>  - mdmon notices and updates the metadata
>  - mdadm changes the level to RAID0
>  - mdmon notices and updates the metadata.
> 
> Currently mdmon doesn't monitor 'level', but it could and probably
> should.
> However in the interest of forward progress I won't insist on that for
> now.
Ok. Thanks for patch applying.
In the future we can consider to change
> 
> Other problems:
> 
> >
> > Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> > ---
> >  Grow.c        |    1 +
> >  super-intel.c |   98
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files
> > changed, 98 insertions(+), 1 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index ad13d6e..c4125ea 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -1463,6 +1463,7 @@ int Grow_reshape(char *devname, int fd, int
> > quiet, char *backup_file, rv = 1;
> >  			goto release;
> >  		}
> > +		ping_monitor(container);
> 
> Why is this here??? EVERY change should be justified in the notes at
> the top!!!!!!!
> 
> 
> >  	}
> >
> >  	info.array = array;
> > diff --git a/super-intel.c b/super-intel.c
> > index 334e0f4..86b365d 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -299,6 +299,7 @@ enum imsm_update_type {
> >  	update_rename_array,
> >  	update_add_remove_disk,
> >  	update_reshape_container_disks,
> > +	update_takeover
> >  };
> >
> >  struct imsm_update_activate_spare {
> > @@ -319,6 +320,15 @@ struct geo_params {
> >  	int raid_disks;
> >  };
> >
> > +enum takeover_direction {
> > +	R10_TO_R0,
> > +	R0_TO_R10
> > +};
> > +struct imsm_update_takeover {
> > +	enum imsm_update_type type;
> > +	int subarray;
> > +	enum takeover_direction direction;
> > +};
> >
> >  struct imsm_update_reshape {
> >  	enum imsm_update_type type;
> > @@ -5802,6 +5812,56 @@ update_reshape_exit:
> >  	return ret_val;
> >  }
> >
> > +static int apply_takeover_update(struct imsm_update_takeover *u,
> > +				struct intel_super *super)
> > +{
> > +	struct imsm_dev *dev = NULL;
> > +	struct imsm_map *map;
> > +	struct dl *dm, *du;
> > +	struct intel_dev *dv;
> > +
> > +	for (dv = super->devlist; dv; dv = dv->next)
> > +		if (dv->index == (unsigned int)u->subarray) {
> > +			dev = dv->dev;
> > +			break;
> > +	}
> > +
> > +	if (dev == NULL)
> > +		return 0;
> > +
> > +	map = get_imsm_map(dev, 0);
> > +
> > +	if (u->direction == R10_TO_R0) {
> > +		/* iterate through devices to mark removed disks as
> > spare */
> > +		for (dm = super->disks; dm; dm = dm->next) {
> > +			if (dm->disk.status & FAILED_DISK) {
> > +				int idx = dm->index;
> > +				/* update indexes on the disk list */
> > +				for (du = super->disks; du; du =
> > du->next)
> > +					if (du->index > idx)
> > +						du->index--;
> > +				/* mark as spare disk */
> > +				dm->disk.status = SPARE_DISK;
> > +				dm->index = -1;
> 
> This looks like it is probably wrong, but I'm not exactly sure what it
> is trying to do so I cannot be sure.
> In particular the changes to 'index' seem to be based on some
> assumption that are not necessarily true.
> Could you please explain exactly what this is trying to achieve?
When we remove disks from array, indexes of existing disks have to be updated to remove gaps between disks- if greater than removed disk index it should be decreased
Example:
1. Raid10:
Disk 1: index 0
Disk 2: index 1
Disk 3: index 2
Disk 4: index 3
2. Disk 2 and 4 have been removed
RAID10:
Disk 1: index 0
Disk 2(removed): index 1
Disk 3: index 2
Disk 4(removed): index 3
3. Level has been changed to "0"
RAID0:
Disk 1: index 0
Disk 2: index -1
Disk 3: index 1
Disk 4: index -1
4. At the end we have following layout:
RAID0:
Disk 1: index 0
Disk 2: index 1


> 
> > +			}
> > +		}
> > +
> > +		/* update map */
> > +		map->num_members = map->num_members / 2;
> > +		map->map_state = IMSM_T_STATE_NORMAL;
> > +		map->num_domains = 1;
> 
> I'm confused by this 'num_domains' thing.  Could you please explain
> it for me.  The term "parity domains" is meaningless to me in this
> context.
"num_domains" is used to distinguish level 10 and level 1:
For level 1:
	Level= 1
	Num_domains= 1
For level 10:
	Level= 1
	Num_domains= 2

For other levels (0, 5) num_domains= 1.

> 
> Thanks,
> NeilBrown
> 
> 
> 
> > +		map->raid_level = 0;
> > +		map->failed_disk_num = -1;
> > +	}
> > +
> > +	/* update disk order table */
> > +	for (du = super->disks; du; du = du->next)
> > +		if (du->index >= 0)
> > +			set_imsm_ord_tbl_ent(map, du->index,
> > du->index); +
> > +	return 1;
> > +}
> > +
> >  static void imsm_process_update(struct supertype *st,
> >  			        struct metadata_update *update)
> >  {
> > @@ -5844,6 +5904,13 @@ static void imsm_process_update(struct
> > supertype *st, mpb = super->anchor;
> >
> >  	switch (type) {
> > +	case update_takeover: {
> > +		struct imsm_update_takeover *u = (void *)update->buf;
> > +		if (apply_takeover_update(u, super))
> > +			super->updates_pending++;
> > +		break;
> > +	}
> > +
> >  	case update_reshape_container_disks: {
> >  		struct imsm_update_reshape *u = (void *)update->buf;
> >  		if (apply_reshape_container_disks_update(
> > @@ -6691,6 +6758,35 @@ analyse_change_exit:
> >  	return change;
> >  }
> >
> > +int imsm_takeover(struct supertype *st, struct geo_params *geo)
> > +{
> > +	struct intel_super *super = st->sb;
> > +	struct imsm_update_takeover *u;
> > +
> > +	u = malloc(sizeof(struct imsm_update_takeover));
> > +	if (u == NULL)
> > +		return 1;
> > +
> > +	u->type = update_takeover;
> > +	u->subarray = super->current_vol;
> > +
> > +	/* 10->0 transition */
> > +	if (geo->level == 0)
> > +		u->direction = R10_TO_R0;
> > +
> > +	/* update metadata locally */
> > +	imsm_update_metadata_locally(st, u,
> > +					sizeof(struct
> > imsm_update_takeover));
> > +	/* and possibly remotely */
> > +	if (st->update_tail)
> > +		append_metadata_update(st, u,
> > +					sizeof(struct
> > imsm_update_takeover));
> > +	else
> > +		free(u);
> > +
> > +	return 0;
> > +}
> > +
> >  static int imsm_reshape_super(struct supertype *st, long long size,
> > int level, int layout, int chunksize, int raid_disks,
> >  			      char *backup, char *dev, int verbose)
> > @@ -6772,7 +6868,7 @@ static int imsm_reshape_super(struct supertype
> > *st, long long size, int level, change = imsm_analyze_change(st,
> > &geo); switch (change) {
> >  			case CH_TAKEOVER:
> > -				ret_val = 0;
> > +				ret_val = imsm_takeover(st, &geo);
> >  				break;
> >  			case CH_CHUNK_MIGR:
> >  				ret_val = 0;

Re: [PATCH 1/4] Add raid10 -> raid0 takeover support

[Neil Brown]

On Wed, 26 Jan 2011 11:34:08 +0000
"Wojcik, Krzysztof" <krzysztof.wojcik@intel.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Neil Brown [mailto:neilb@suse.de]
> > Sent: Wednesday, January 26, 2011 1:59 AM
> > To: Wojcik, Krzysztof
> > Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> > Williams, Dan J; Ciechanowski, Ed
> > Subject: Re: [PATCH 1/4] Add raid10 -> raid0 takeover support
> > 
> > On Mon, 17 Jan 2011 16:42:43 +0100
> > Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
> > 
> > > The patch introduces takeover form level 10 to level 0 for imsm
> > > metadata. This patch contains procedures connected with preparing
> > > and applying metadata update during 10 -> 0 takeover.
> > > When performing takeover 10->0 mdmon should update the external
> > > metadata (due to disk slot and level changes).
> > > To achieve that mdadm calls reshape_super() and prepare
> > > the "update_takeover" metadata update type.
> > > Prepared update is processed by mdmon in process_update().
> > 
> > I've applied this, but I'm not sure that I like it.
> > 
> > I don't think mdadm needs to send a metadata update for 10->0
> > conversion.
> > It should:
> >  - fail all the devices that aren't wanted
> >  - remove them
> >  - mdmon notices and updates the metadata
> >  - mdadm changes the level to RAID0
> >  - mdmon notices and updates the metadata.
> > 
> > Currently mdmon doesn't monitor 'level', but it could and probably
> > should.
> > However in the interest of forward progress I won't insist on that
> > for now.
> Ok. Thanks for patch applying.
> In the future we can consider to change
> > 
> > Other problems:
> > 
> > >
> > > Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> > > ---
> > >  Grow.c        |    1 +
> > >  super-intel.c |   98
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files
> > > changed, 98 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index ad13d6e..c4125ea 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -1463,6 +1463,7 @@ int Grow_reshape(char *devname, int fd, int
> > > quiet, char *backup_file, rv = 1;
> > >  			goto release;
> > >  		}
> > > +		ping_monitor(container);
> > 
> > Why is this here??? EVERY change should be justified in the notes at
> > the top!!!!!!!
> > 
> > 
> > >  	}
> > >
> > >  	info.array = array;
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 334e0f4..86b365d 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -299,6 +299,7 @@ enum imsm_update_type {
> > >  	update_rename_array,
> > >  	update_add_remove_disk,
> > >  	update_reshape_container_disks,
> > > +	update_takeover
> > >  };
> > >
> > >  struct imsm_update_activate_spare {
> > > @@ -319,6 +320,15 @@ struct geo_params {
> > >  	int raid_disks;
> > >  };
> > >
> > > +enum takeover_direction {
> > > +	R10_TO_R0,
> > > +	R0_TO_R10
> > > +};
> > > +struct imsm_update_takeover {
> > > +	enum imsm_update_type type;
> > > +	int subarray;
> > > +	enum takeover_direction direction;
> > > +};
> > >
> > >  struct imsm_update_reshape {
> > >  	enum imsm_update_type type;
> > > @@ -5802,6 +5812,56 @@ update_reshape_exit:
> > >  	return ret_val;
> > >  }
> > >
> > > +static int apply_takeover_update(struct imsm_update_takeover *u,
> > > +				struct intel_super *super)
> > > +{
> > > +	struct imsm_dev *dev = NULL;
> > > +	struct imsm_map *map;
> > > +	struct dl *dm, *du;
> > > +	struct intel_dev *dv;
> > > +
> > > +	for (dv = super->devlist; dv; dv = dv->next)
> > > +		if (dv->index == (unsigned int)u->subarray) {
> > > +			dev = dv->dev;
> > > +			break;
> > > +	}
> > > +
> > > +	if (dev == NULL)
> > > +		return 0;
> > > +
> > > +	map = get_imsm_map(dev, 0);
> > > +
> > > +	if (u->direction == R10_TO_R0) {
> > > +		/* iterate through devices to mark removed disks
> > > as spare */
> > > +		for (dm = super->disks; dm; dm = dm->next) {
> > > +			if (dm->disk.status & FAILED_DISK) {
> > > +				int idx = dm->index;
> > > +				/* update indexes on the disk
> > > list */
> > > +				for (du = super->disks; du; du =
> > > du->next)
> > > +					if (du->index > idx)
> > > +						du->index--;
> > > +				/* mark as spare disk */
> > > +				dm->disk.status = SPARE_DISK;
> > > +				dm->index = -1;
> > 
> > This looks like it is probably wrong, but I'm not exactly sure what
> > it is trying to do so I cannot be sure.
> > In particular the changes to 'index' seem to be based on some
> > assumption that are not necessarily true.
> > Could you please explain exactly what this is trying to achieve?
> When we remove disks from array, indexes of existing disks have to be
> updated to remove gaps between disks- if greater than removed disk
> index it should be decreased Example: 1. Raid10:
> Disk 1: index 0
> Disk 2: index 1
> Disk 3: index 2
> Disk 4: index 3
> 2. Disk 2 and 4 have been removed
> RAID10:
> Disk 1: index 0
> Disk 2(removed): index 1
> Disk 3: index 2
> Disk 4(removed): index 3
> 3. Level has been changed to "0"
> RAID0:
> Disk 1: index 0
> Disk 2: index -1
> Disk 3: index 1
> Disk 4: index -1
> 4. At the end we have following layout:
> RAID0:
> Disk 1: index 0
> Disk 2: index 1

And does this work if the devices are in a different order, or if some
are missing?
It would make more sense to divide-by-2 the indexes of the devices that
you want to keep.


> 
> 
> > 
> > > +			}
> > > +		}
> > > +
> > > +		/* update map */
> > > +		map->num_members = map->num_members / 2;
> > > +		map->map_state = IMSM_T_STATE_NORMAL;
> > > +		map->num_domains = 1;
> > 
> > I'm confused by this 'num_domains' thing.  Could you please explain
> > it for me.  The term "parity domains" is meaningless to me in this
> > context.
> "num_domains" is used to distinguish level 10 and level 1:
> For level 1:
> 	Level= 1
> 	Num_domains= 1
> For level 10:
> 	Level= 1
> 	Num_domains= 2
> 
> For other levels (0, 5) num_domains= 1.

It would be great if you could include that in the code somewhere as a
comment, as the numbers seem to be entirely arbitrary and not follow
any clear pattern.

Thanks.
NeilBrown


> 
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > 
> > > +		map->raid_level = 0;
> > > +		map->failed_disk_num = -1;
> > > +	}
> > > +
> > > +	/* update disk order table */
> > > +	for (du = super->disks; du; du = du->next)
> > > +		if (du->index >= 0)
> > > +			set_imsm_ord_tbl_ent(map, du->index,
> > > du->index); +
> > > +	return 1;
> > > +}
> > > +
> > >  static void imsm_process_update(struct supertype *st,
> > >  			        struct metadata_update *update)
> > >  {
> > > @@ -5844,6 +5904,13 @@ static void imsm_process_update(struct
> > > supertype *st, mpb = super->anchor;
> > >
> > >  	switch (type) {
> > > +	case update_takeover: {
> > > +		struct imsm_update_takeover *u = (void
> > > *)update->buf;
> > > +		if (apply_takeover_update(u, super))
> > > +			super->updates_pending++;
> > > +		break;
> > > +	}
> > > +
> > >  	case update_reshape_container_disks: {
> > >  		struct imsm_update_reshape *u = (void
> > > *)update->buf; if (apply_reshape_container_disks_update(
> > > @@ -6691,6 +6758,35 @@ analyse_change_exit:
> > >  	return change;
> > >  }
> > >
> > > +int imsm_takeover(struct supertype *st, struct geo_params *geo)
> > > +{
> > > +	struct intel_super *super = st->sb;
> > > +	struct imsm_update_takeover *u;
> > > +
> > > +	u = malloc(sizeof(struct imsm_update_takeover));
> > > +	if (u == NULL)
> > > +		return 1;
> > > +
> > > +	u->type = update_takeover;
> > > +	u->subarray = super->current_vol;
> > > +
> > > +	/* 10->0 transition */
> > > +	if (geo->level == 0)
> > > +		u->direction = R10_TO_R0;
> > > +
> > > +	/* update metadata locally */
> > > +	imsm_update_metadata_locally(st, u,
> > > +					sizeof(struct
> > > imsm_update_takeover));
> > > +	/* and possibly remotely */
> > > +	if (st->update_tail)
> > > +		append_metadata_update(st, u,
> > > +					sizeof(struct
> > > imsm_update_takeover));
> > > +	else
> > > +		free(u);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int imsm_reshape_super(struct supertype *st, long long
> > > size, int level, int layout, int chunksize, int raid_disks,
> > >  			      char *backup, char *dev, int
> > > verbose) @@ -6772,7 +6868,7 @@ static int
> > > imsm_reshape_super(struct supertype *st, long long size, int
> > > level, change = imsm_analyze_change(st, &geo); switch (change) {
> > >  			case CH_TAKEOVER:
> > > -				ret_val = 0;
> > > +				ret_val = imsm_takeover(st,
> > > &geo); break;
> > >  			case CH_CHUNK_MIGR:
> > >  				ret_val = 0;