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

---

Krzysztof Wojcik (6):
reshape_super reorganization
Define imsm_analyze_change function
Set delta_disks for raid10 -> raid0 takeover
FIX: Mistake in delta_disk comparison.
Remove code for non re-striping operations.
Add raid10 -> raid0 takeover support


Grow.c | 80 +++++---------
super-intel.c | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 334 insertions(+), 68 deletions(-)

--
Signature
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] reshape_super reorganization

Function has been divided into two clear parts:
1. Container operations
2. Volume operations

Prototype of imsm_analyze_change function has been added.

Signed-off-by: Krzysztof Wojcik
---
super-intel.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------------
1 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 0c70fda..8c1ed4e 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -284,6 +284,13 @@ struct extent {
unsigned long long start, size;
};

+/* definitions of reshape process types */
+enum imsm_reshape_type {
+ CH_TAKEOVER,
+ CH_CHUNK_MIGR,
+ CH_LEVEL_MIGRATION
+};
+
/* definition of messages passed to imsm_process_update */
enum imsm_update_type {
update_activate_spare,
@@ -6324,6 +6331,10 @@ static int imsm_reshape_is_allowed_on_container(struct supertype *st,
struct geo_params *geo,
int *old_raid_disks)
{
+ /* currently we only support increasing the number of devices
+ * for a container. This increases the number of device for each
+ * member array. They must all be RAID0 or RAID5.
+ */
int ret_val = 0;
struct mdinfo *info, *member;
int devices_that_can_grow = 0;
@@ -6538,15 +6549,21 @@ static void imsm_update_metadata_locally(struct supertype *st,
}
}

+/********************************************************** ****************************
+* Function: imsm_analyze_change
+* Description: Function analyze and validate change for single volume migration
+* Parameters: Geometry parameters, supertype structure
+* Returns: Operation type code on success, -1 if fail
+*********************************************************** **************************/
+enum imsm_reshape_type imsm_analyze_change(struct supertype *st, struct geo_params *geo)
+{
+ return -1;
+}
+
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)
{
- /* currently we only support increasing the number of devices
- * for a container. This increases the number of device for each
- * member array. They must all be RAID0 or RAID5.
- */
-
int ret_val = 1;
struct geo_params geo;

@@ -6555,6 +6572,7 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
memset(&geo, sizeof(struct geo_params), 0);

geo.dev_name = dev;
+ geo.dev_id = st->devnum;
geo.size = size;
geo.level = level;
geo.layout = layout;
@@ -6567,13 +6585,9 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
if (experimental() == 0)
return ret_val;

- /* verify reshape conditions
- * on container level we can only increase number of devices.
- */
if (st->container_dev == st->devnum) {
- /* check for delta_disks > 0
- * and supported raid levels 0 and 5 only in container
- */
+ /* On container level we can only increase number of devices. */
+ dprintf("imsm: info: Container operation\n");
int old_raid_disks = 0;
if (imsm_reshape_is_allowed_on_container(
st, &geo, &old_raid_disks)) {
@@ -6594,11 +6608,29 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
/* update metadata locally
*/
imsm_update_metadata_locally(st, st->updates);
- } else
+ } else {
fprintf(stderr, Name "imsm: Operation is not allowed "
"on this container\n");
- } else
- fprintf(stderr, Name "imsm: not a container operation\n");
+ }
+ } else {
+ /* On volume level we support following operations
+ * - takeover: raid10 -> raid0; raid0 -> raid10
+ * - chunk size migration
+ * - migration: raid5 -> raid0; raid0 -> raid5
+ */
+ int change;
+ dprintf("imsm: info: Volume operation\n");
+ change = imsm_analyze_change(st, &geo);
+ switch (change) {
+ case CH_TAKEOVER:
+ break;
+ case CH_CHUNK_MIGR:
+ break;
+ case CH_LEVEL_MIGRATION:
+ break;
+ }
+ ret_val = 0;
+ }

exit_imsm_reshape_super:
dprintf("imsm: reshape_super Exit code = %i\n", ret_val);

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] Define imsm_analyze_change function

Function intended to use for single volume migration.
Function analyze transition and validate if it is supported.

Signed-off-by: Krzysztof Wojcik
---
super-intel.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 8c1ed4e..81f8e81 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6549,15 +6549,127 @@ static void imsm_update_metadata_locally(struct supertype *st,
}
}

-/********************************************************** ****************************
+/********************************************************** *****************
+>>>>>>> 686d792... Define imsm_analyze_change function:super-intel.c
* Function: imsm_analyze_change
-* Description: Function analyze and validate change for single volume migration
+* Description: Function analyze change for single volume
+* and validate if transition is supported
* Parameters: Geometry parameters, supertype structure
* Returns: Operation type code on success, -1 if fail
-*********************************************************** **************************/
-enum imsm_reshape_type imsm_analyze_change(struct supertype *st, struct geo_params *geo)
+*********************************************************** *****************/
+enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
+ struct geo_params *geo)
{
- return -1;
+ int fd = -1;
+ struct mdinfo *sra = NULL;
+ int change = -1;
+ int check_devs = 0;
+
+ fd = open_dev(geo->dev_id);
+ if (fd < 0) {
+ printf("imsm: Error. Cannot open %s device\n", geo->dev_name);
+ return -1;
+ }
+
+ sra = sysfs_read(fd, 0,
+ GET_DISKS | GET_LAYOUT | GET_CHUNK |
+ GET_SIZE | GET_LEVEL | GET_DEVS);
+ if (!sra) {
+ dprintf("imsm: Error. Cannot get mdinfo for %s!\n", geo->dev_name);
+ goto analyse_change_exit;
+ }
+
+ if ((geo->level != sra->array.level) &&
+ (geo->level >= 0) &&
+ (geo->level != UnSet)) {
+ switch (sra->array.level) {
+ case 0:
+ if (geo->level == 5) {
+ change = CH_LEVEL_MIGRATION;
+ check_devs = 1;
+ }
+ if (geo->level == 10) {
+ change = CH_TAKEOVER;
+ check_devs = 1;
+ }
+ break;
+ case 5:
+ if (geo->level != 0)
+ change = CH_LEVEL_MIGRATION;
+ break;
+ case 10:
+ if (geo->level == 0) {
+ change = CH_TAKEOVER;
+ check_devs = 1;
+ }
+ break;
+ }
+ if (change == -1) {
+ dprintf("imsm: Error. Level Migration from %d to %d "\
+ "not supported!\n", sra->array.level, geo->level);
+ goto analyse_change_exit;
+ }
+ } else {
+ geo->level = sra->array.level;
+ }
+
+ if ((geo->layout != sra->array.layout)
+ && ((geo->layout != UnSet) && (geo->layout != -1))) {
+ change = CH_LEVEL_MIGRATION;
+ if ((sra->array.layout == 0) && (sra->array.level == 5)
+ && (geo->layout == 5)) {
+ /* reshape 5 -> 4 */
+ } else if ((sra->array.layout == 5)
+ && (sra->array.level == 5)
+ && (geo->layout == 0)) {
+ /* reshape 4 -> 5 */
+ geo->layout = 0;
+ geo->level = 5;
+ } else {
+ dprintf("imsm: Error. Layout Migration from %d to %d "\
+ "not supported!\n", sra->array.layout, geo->layout);
+ change = -1;
+ goto analyse_change_exit;
+ }
+ } else {
+ geo->layout = sra->array.layout;
+ }
+
+ if ((geo->chunksize > 0) && (geo->chunksize != UnSet)
+ && (geo->chunksize != sra->array.chunk_size)) {
+ change = CH_CHUNK_MIGR;
+ } else {
+ geo->chunksize = sra->array.chunk_size;
+ }
+
+ if (!validate_geometry_imsm(st,
+ geo->level,
+ geo->layout,
+ geo->raid_disks,
+ (geo->chunksize / 1024),
+ geo->size,
+ 0, 0, 1))
+ change = -1;
+
+ if (check_devs) {
+ struct intel_super *super = st->sb;
+ struct imsm_super *mpb = super->anchor;
+
+ if (mpb->num_raid_devs > 1) {
+ printf("imsm: Error. Cannot perform operation on %s"\
+ "- for this operation it MUST be single "\
+ "array in container\n",
+ geo->dev_name);
+ change = -1;
+ }
+ }
+
+analyse_change_exit:
+ sysfs_free(sra);
+ if (fd > -1)
+ close(fd);
+
+ return change;
}

static int imsm_reshape_super(struct supertype *st, long long size, int level,
@@ -6623,13 +6735,17 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
change = imsm_analyze_change(st, &geo);
switch (change) {
case CH_TAKEOVER:
- break;
+ ret_val = 0;
+ break;
case CH_CHUNK_MIGR:
- break;
+ ret_val = 0;
+ break;
case CH_LEVEL_MIGRATION:
- break;
+ ret_val = 0;
+ break;
+ default:
+ ret_val = 1;
}
- ret_val = 0;
}

exit_imsm_reshape_super:

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] Set delta_disks for raid10 -> raid0 takeover

remove_disks_on_raid10_to_raid0_takeover function has been
changed to return number of removed disks.
Returned value is used to set info.delta_disks to proper value.
Part of code used to set delta_disks for case when raid_disks
is not zero has been moved above remove_disks_on_raid10_to_raid0_takeover
function to avoid overwrite delta_disks variable.

Signed-off-by: Krzysztof Wojcik
---
Grow.c | 34 +++++++++++++++++++---------------
1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/Grow.c b/Grow.c
index 3455115..c06561f 100644
--- a/Grow.c
+++ b/Grow.c
@@ -666,6 +666,7 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
int nr_of_copies;
struct mdinfo *remaining;
int slot;
+ int delta = 0;

nr_of_copies = layout & 0xff;

@@ -710,7 +711,7 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
e = &(*e)->next;
*e = sra->devs;
sra->devs = remaining;
- return 1;
+ return 0;
}

/* Remove all 'remaining' devices from the array */
@@ -725,8 +726,9 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
sd->disk.state &= ~(1< sd->next = sra->devs;
sra->devs = sd;
+ delta--;
}
- return 0;
+ return delta;
}

void reshape_free_fdlist(int *fdlist,
@@ -1456,6 +1458,17 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
size = array.size;
}

+ info.array = array;
+ sysfs_init(&info, fd, NoMdDev);
+ strcpy(info.text_version, sra->text_version);
+ info.component_size = size*2;
+ info.new_level = level;
+ info.new_chunk = chunksize * 1024;
+ if (raid_disks)
+ info.delta_disks = raid_disks - info.array.raid_disks;
+ else
+ info.delta_disks = UnSet;
+
/* ========= check for Raid10 -> Raid0 conversion ===============
* current implementation assumes that following conditions must be met:
* - far_copies == 1
@@ -1463,27 +1476,18 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
*/
if (level == 0 && array.level == 10 && sra &&
array.layout == ((1 << 8) + 2) && !(array.raid_disks & 1)) {
- int err;
- err = remove_disks_on_raid10_to_raid0_takeover(st, sra, array.layout);
- if (err) {
+ int rv;
+ rv = remove_disks_on_raid10_to_raid0_takeover(st, sra, array.layout);
+ if (!rv) {
dprintf(Name": Array cannot be reshaped\n");
if (cfd > -1)
close(cfd);
rv = 1;
goto release;
}
+ info.delta_disks = rv;
}

- info.array = array;
- sysfs_init(&info, fd, NoMdDev);
- strcpy(info.text_version, sra->text_version);
- info.component_size = size*2;
- info.new_level = level;
- info.new_chunk = chunksize * 1024;
- if (raid_disks)
- info.delta_disks = raid_disks - info.array.raid_disks;
- else
- info.delta_disks = UnSet;
if (layout_str == NULL) {
info.new_layout = UnSet;
if (info.array.level == 6 &&

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] FIX: Mistake in delta_disk comparison.

Signed-off-by: Krzysztof Wojcik
---
Grow.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Grow.c b/Grow.c
index c06561f..253f289 100644
--- a/Grow.c
+++ b/Grow.c
@@ -963,7 +963,7 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
return "RAID10 can only be changed to RAID0";
new_disks = (info->array.raid_disks
/ (info->array.layout & 0xff));
- if (info->delta_disks != UnSet) {
+ if (info->delta_disks == UnSet) {
info->delta_disks = (new_disks
- info->array.raid_disks);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] Remove code for non re-striping operations.

Neil,
How was your intention when you add this part of code?
I can't find the case when it will be necessary.
Moreover it breaks flow for takeover operations because
ioctl not succeeded when more than one parameter is changed.
I've remove this code temporally to be able test takeover
operations.
We can restore this code after we decide what it should do.

Signed-off-by: Krzysztof Wojcik
---
Grow.c | 43 +++++--------------------------------------
1 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/Grow.c b/Grow.c
index 253f289..6835f34 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1666,6 +1666,11 @@ static int reshape_array(char *container, int fd, char *devname,
ping_monitor(container);
}

+ if (reshape.backup_blocks == 0) {
+ /* If operation not not reuire re-striping we can finish */
+ return rv;
+ }
+
/* ->reshape_super might have chosen some spares from the
* container that it wants to be part of the new array.
* We can collect them with ->container_content and give
@@ -1692,44 +1697,6 @@ static int reshape_array(char *container, int fd, char *devname,
}
}

- if (reshape.backup_blocks == 0) {
- /* No restriping needed, but we might need to impose
- * some more changes: layout, raid_disks, chunk_size
- */
- if (info->new_layout != UnSet &&
- info->new_layout != info->array.layout) {
- info->array.layout = info->new_layout;
- if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
- fprintf(stderr, Name ": failed to set new layout\n");
- rv = 1;
- } else if (!quiet)
- printf("layout for %s set to %d\n",
- devname, info->array.layout);
- }
- if (info->delta_disks != UnSet &&
- info->delta_disks != 0) {
- info->array.raid_disks += info->delta_disks;
- if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
- fprintf(stderr, Name ": failed to set raid disks\n");
- rv = 1;
- } else if (!quiet)
- printf("raid_disks for %s set to %d\n",
- devname, info->array.raid_disks);
- }
- if (info->new_chunk != 0 &&
- info->new_chunk != info->array.chunk_size) {
- if (sysfs_set_num(info, NULL,
- "chunk_size", info->new_chunk) != 0) {
- fprintf(stderr, Name ": failed to set chunk size\n");
- rv = 1;
- } else if (!quiet)
- printf("chunk size for %s set to %d\n",
- devname, info->array.chunk_size);
- }
-
- return rv;
- }
-
/*
* There are three possibilities.
* 1/ The array will shrink.

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] Add raid10 -> raid0 takeover support

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
---
Grow.c | 1
super-intel.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 148 insertions(+), 1 deletions(-)

diff --git a/Grow.c b/Grow.c
index 6835f34..d5cd761 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1485,6 +1485,7 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
rv = 1;
goto release;
}
+ ping_monitor(container);
info.delta_disks = rv;
}

diff --git a/super-intel.c b/super-intel.c
index 81f8e81..90e0b98 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,12 @@ struct geo_params {
int raid_disks;
};

+#define TAKEOVER_R10_DISKS 2
+struct imsm_update_takeover {
+ enum imsm_update_type type;
+ int subarray;
+ int disks[TAKEOVER_R10_DISKS];
+};

struct imsm_update_reshape {
enum imsm_update_type type;
@@ -5764,6 +5771,68 @@ 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;
+ int *tab;
+ int i;
+
+ dev = get_imsm_dev(super, u->subarray);
+
+ if (dev == NULL)
+ return 0;
+
+ map = get_imsm_map(dev, 0);
+ tab = (int *)&map->disk_ord_tbl;
+
+for (i=0;i<4;i++)
+ dprintf("tab: idx= %i, val=%i\n", i, tab[i]);
+
+for (dm=super->disks;dm;dm=dm->next)
+ dprintf("disk: index= %i, minor= %i, major= %i\n", dm->index, dm->minor, dm->major);
+
+ /* iterate through devices to mark removed disks as spare
+ * and update order table */
+ for (i = 0; i < TAKEOVER_R10_DISKS; i++) {
+ for (dm = super->disks; dm; dm = dm->next) {
+ if (((unsigned int)dm->major != major(u->disks[i])) ||
+ ((unsigned int)dm->minor != minor(u->disks[i])))
+ continue;
+ for (du = super->disks; du; du = du->next)
+ if ((du->index > dm->index) && (du->index > 0))
+ du->index--;
+ dm->disk.status = SPARE_DISK;
+ dm->index = -1;
+ }
+ }
+
+ /* update disk order table */
+ i = 0;
+ for (du = super->disks; du; du = du->next) {
+ if (du->index >= 0) {
+dprintf("du->index= %i\n", du->index);
+ tab[du->index] = i;
+ i++;
+ }
+ }
+
+for (i=0;i<4;i++)
+ dprintf("AFTER tab: idx= %i, val=%i\n", i, tab[i]);
+for (dm=super->disks;dm;dm=dm->next)
+ dprintf("AFTER disk: index= %i, minor= %i, major= %i\n", dm->index, dm->minor, dm->major);
+
+
+ map->num_members = 2;
+ map->map_state = IMSM_T_STATE_NORMAL;
+ map->num_domains = 1;
+ map->raid_level = 0;
+ map->failed_disk_num = -1;
+ return 1;
+}
+
static void imsm_process_update(struct supertype *st,
struct metadata_update *update)
{
@@ -5806,6 +5875,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(
@@ -6672,6 +6748,76 @@ 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;
+ struct mdinfo *info;
+ struct mdinfo *newdi;
+ struct dl *dl;
+ int subarray, i, fd;
+ int found = 0;
+ int devnum;
+ char buf[PATH_MAX];
+
+ /* find requested device */
+ for (subarray = 0; subarray < super->anchor->num_raid_devs; subarray++) {
+ imsm_find_array_minor_by_subdev(subarray, st->container_dev, &devnum);
+ if (devnum == geo->dev_id) {
+ found = 1;
+ break;
+ }
+ }
+
+ if (!found) {
+ fprintf(stderr, Name "Cannot find %s (%i) subarray\n",
+ geo->dev_name, geo->dev_id);
+ return 1;
+ }
+
+ sprintf(buf, "/dev/md%i", devnum);
+ fd = open(buf, O_RDONLY);
+ if (!fd) {
+ fprintf(stderr, Name "Cannot open %s", buf);
+ return 1;
+ }
+ info = sysfs_read(fd, 0, GET_LEVEL|GET_VERSION|GET_DEVS|GET_STATE);
+ if (!info) {
+ fprintf(stderr, Name "Cannot load sysfs information for %s (%i)",
+ geo->dev_name, geo->dev_id);
+ return 1;
+ }
+
+ u = malloc(sizeof(struct imsm_update_takeover));
+ if (u == NULL)
+ return 1;
+
+ u->type = update_takeover;
+ u->subarray = subarray;
+
+ /* 10->0 transition - mark disks to remove */
+ if (geo->level == 0) {
+ i = 0;
+ for (dl = super->disks; dl; dl = dl->next) {
+ found = 0;
+ for (newdi = info->devs; newdi; newdi = newdi->next) {
+ if ((dl->major != newdi->disk.major) ||
+ (dl->minor != newdi->disk.minor) ||
+ (newdi->disk.raid_disk < 0))
+ continue;
+ found = 1;
+ break;
+ }
+ /* if disk not found, mark it for remove */
+ if ((found == 0) && (!(dl->disk.status & SPARE_DISK)))
+ u->disks[i++] = makedev(dl->major, dl->minor);
+ }
+ }
+
+ append_metadata_update(st, u, sizeof(struct imsm_update_takeover));
+ 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)
@@ -6735,7 +6881,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;

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] reshape_super reorganization

On Wed, 12 Jan 2011 17:00:58 +0100 Krzysztof Wojcik
wrote:

> Function has been divided into two clear parts:
> 1. Container operations
> 2. Volume operations
>
> Prototype of imsm_analyze_change function has been added.
>
> Signed-off-by: Krzysztof Wojcik
> ---
> super-intel.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 0c70fda..8c1ed4e 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -284,6 +284,13 @@ struct extent {
> unsigned long long start, size;
> };
>
> +/* definitions of reshape process types */
> +enum imsm_reshape_type {
> + CH_TAKEOVER,
> + CH_CHUNK_MIGR,
> + CH_LEVEL_MIGRATION
> +};
> +
> /* definition of messages passed to imsm_process_update */
> enum imsm_update_type {
> update_activate_spare,
> @@ -6324,6 +6331,10 @@ static int imsm_reshape_is_allowed_on_container(struct supertype *st,
> struct geo_params *geo,
> int *old_raid_disks)
> {
> + /* currently we only support increasing the number of devices
> + * for a container. This increases the number of device for each
> + * member array. They must all be RAID0 or RAID5.
> + */
> int ret_val = 0;
> struct mdinfo *info, *member;
> int devices_that_can_grow = 0;
> @@ -6538,15 +6549,21 @@ static void imsm_update_metadata_locally(struct supertype *st,
> }
> }
>
> +/********************************************************** ****************************
> +* Function: imsm_analyze_change
> +* Description: Function analyze and validate change for single volume migration
> +* Parameters: Geometry parameters, supertype structure
> +* Returns: Operation type code on success, -1 if fail
> +*********************************************************** **************************/
> +enum imsm_reshape_type imsm_analyze_change(struct supertype *st, struct geo_params *geo)
> +{
> + return -1;
> +}
> +
> 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)
> {
> - /* currently we only support increasing the number of devices
> - * for a container. This increases the number of device for each
> - * member array. They must all be RAID0 or RAID5.
> - */
> -
> int ret_val = 1;
> struct geo_params geo;
>
> @@ -6555,6 +6572,7 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
> memset(&geo, sizeof(struct geo_params), 0);
>
> geo.dev_name = dev;
> + geo.dev_id = st->devnum;
> geo.size = size;
> geo.level = level;
> geo.layout = layout;
> @@ -6567,13 +6585,9 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
> if (experimental() == 0)
> return ret_val;
>
> - /* verify reshape conditions
> - * on container level we can only increase number of devices.
> - */
> if (st->container_dev == st->devnum) {
> - /* check for delta_disks > 0
> - * and supported raid levels 0 and 5 only in container
> - */
> + /* On container level we can only increase number of devices. */
> + dprintf("imsm: info: Container operation\n");
> int old_raid_disks = 0;
> if (imsm_reshape_is_allowed_on_container(
> st, &geo, &old_raid_disks)) {
> @@ -6594,11 +6608,29 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
> /* update metadata locally
> */
> imsm_update_metadata_locally(st, st->updates);
> - } else
> + } else {
> fprintf(stderr, Name "imsm: Operation is not allowed "
> "on this container\n");
> - } else
> - fprintf(stderr, Name "imsm: not a container operation\n");
> + }
> + } else {
> + /* On volume level we support following operations
> + * - takeover: raid10 -> raid0; raid0 -> raid10
> + * - chunk size migration
> + * - migration: raid5 -> raid0; raid0 -> raid5
> + */
> + int change;
> + dprintf("imsm: info: Volume operation\n");
> + change = imsm_analyze_change(st, &geo);
> + switch (change) {
> + case CH_TAKEOVER:
> + break;
> + case CH_CHUNK_MIGR:
> + break;
> + case CH_LEVEL_MIGRATION:
> + break;
> + }
> + ret_val = 0;
> + }
>
> exit_imsm_reshape_super:
> dprintf("imsm: reshape_super Exit code = %i\n", ret_val);



Applied, thanks.

NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] Define imsm_analyze_change function

On Wed, 12 Jan 2011 17:01:06 +0100 Krzysztof Wojcik
wrote:

> Function intended to use for single volume migration.
> Function analyze transition and validate if it is supported.
>
> Signed-off-by: Krzysztof Wojcik
> ---
> super-intel.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 125 insertions(+), 9 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 8c1ed4e..81f8e81 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -6549,15 +6549,127 @@ static void imsm_update_metadata_locally(struct supertype *st,
> }
> }
>
> -/********************************************************** ****************************
> +/********************************************************** *****************
> +>>>>>>> 686d792... Define imsm_analyze_change function:super-intel.c

Leaving that sort of thing in the patch is a bad idea....



> * Function: imsm_analyze_change
> -* Description: Function analyze and validate change for single volume migration
> +* Description: Function analyze change for single volume
> +* and validate if transition is supported
> * Parameters: Geometry parameters, supertype structure
> * Returns: Operation type code on success, -1 if fail
> -*********************************************************** **************************/
> -enum imsm_reshape_type imsm_analyze_change(struct supertype *st, struct geo_params *geo)
> +*********************************************************** *****************/
> +enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
> + struct geo_params *geo)
> {
> - return -1;
> + int fd = -1;
> + struct mdinfo *sra = NULL;
> + int change = -1;
> + int check_devs = 0;
> +
> + fd = open_dev(geo->dev_id);
> + if (fd < 0) {
> + printf("imsm: Error. Cannot open %s device\n", geo->dev_name);
> + return -1;
> + }
> +
> + sra = sysfs_read(fd, 0,
> + GET_DISKS | GET_LAYOUT | GET_CHUNK |
> + GET_SIZE | GET_LEVEL | GET_DEVS);


loading info from the array is the wrong thing to do. You have to superblock
in st - you should use that to determine the current state of the array.
Maybe use getinfo_super_imsm or something.

Not applied.

NeilBrown



> + if (!sra) {
> + dprintf("imsm: Error. Cannot get mdinfo for %s!\n", geo->dev_name);
> + goto analyse_change_exit;
> + }
> +
> + if ((geo->level != sra->array.level) &&
> + (geo->level >= 0) &&
> + (geo->level != UnSet)) {
> + switch (sra->array.level) {
> + case 0:
> + if (geo->level == 5) {
> + change = CH_LEVEL_MIGRATION;
> + check_devs = 1;
> + }
> + if (geo->level == 10) {
> + change = CH_TAKEOVER;
> + check_devs = 1;
> + }
> + break;
> + case 5:
> + if (geo->level != 0)
> + change = CH_LEVEL_MIGRATION;
> + break;
> + case 10:
> + if (geo->level == 0) {
> + change = CH_TAKEOVER;
> + check_devs = 1;
> + }
> + break;
> + }
> + if (change == -1) {
> + dprintf("imsm: Error. Level Migration from %d to %d "\
> + "not supported!\n", sra->array.level, geo->level);
> + goto analyse_change_exit;
> + }
> + } else {
> + geo->level = sra->array.level;
> + }
> +
> + if ((geo->layout != sra->array.layout)
> + && ((geo->layout != UnSet) && (geo->layout != -1))) {
> + change = CH_LEVEL_MIGRATION;
> + if ((sra->array.layout == 0) && (sra->array.level == 5)
> + && (geo->layout == 5)) {
> + /* reshape 5 -> 4 */
> + } else if ((sra->array.layout == 5)
> + && (sra->array.level == 5)
> + && (geo->layout == 0)) {
> + /* reshape 4 -> 5 */
> + geo->layout = 0;
> + geo->level = 5;
> + } else {
> + dprintf("imsm: Error. Layout Migration from %d to %d "\
> + "not supported!\n", sra->array.layout, geo->layout);
> + change = -1;
> + goto analyse_change_exit;
> + }
> + } else {
> + geo->layout = sra->array.layout;
> + }
> +
> + if ((geo->chunksize > 0) && (geo->chunksize != UnSet)
> + && (geo->chunksize != sra->array.chunk_size)) {
> + change = CH_CHUNK_MIGR;
> + } else {
> + geo->chunksize = sra->array.chunk_size;
> + }
> +
> + if (!validate_geometry_imsm(st,
> + geo->level,
> + geo->layout,
> + geo->raid_disks,
> + (geo->chunksize / 1024),
> + geo->size,
> + 0, 0, 1))
> + change = -1;
> +
> + if (check_devs) {
> + struct intel_super *super = st->sb;
> + struct imsm_super *mpb = super->anchor;
> +
> + if (mpb->num_raid_devs > 1) {
> + printf("imsm: Error. Cannot perform operation on %s"\
> + "- for this operation it MUST be single "\
> + "array in container\n",
> + geo->dev_name);
> + change = -1;
> + }
> + }
> +
> +analyse_change_exit:
> + sysfs_free(sra);
> + if (fd > -1)
> + close(fd);
> +
> + return change;
> }
>
> static int imsm_reshape_super(struct supertype *st, long long size, int level,
> @@ -6623,13 +6735,17 @@ static int imsm_reshape_super(struct supertype *st, long long size, int level,
> change = imsm_analyze_change(st, &geo);
> switch (change) {
> case CH_TAKEOVER:
> - break;
> + ret_val = 0;
> + break;
> case CH_CHUNK_MIGR:
> - break;
> + ret_val = 0;
> + break;
> case CH_LEVEL_MIGRATION:
> - break;
> + ret_val = 0;
> + break;
> + default:
> + ret_val = 1;
> }
> - ret_val = 0;
> }
>
> exit_imsm_reshape_super:

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Set delta_disks for raid10 -> raid0 takeover

On Wed, 12 Jan 2011 17:01:15 +0100 Krzysztof Wojcik
wrote:

> remove_disks_on_raid10_to_raid0_takeover function has been
> changed to return number of removed disks.
> Returned value is used to set info.delta_disks to proper value.
> Part of code used to set delta_disks for case when raid_disks
> is not zero has been moved above remove_disks_on_raid10_to_raid0_takeover
> function to avoid overwrite delta_disks variable.
>
> Signed-off-by: Krzysztof Wojcik

I'm not really sure what this is trying to do, but it is doing it wrongly.

delta_disks should be the number to reduce 'raid_disks' by. So when
converting a copies=2 raid10 to a raid0 it must be exactly half of raid_disks.
But you are counting the number of active disks that get removed. That is
wrong and there could be some faulty disks that get removed as well.

Not applied.

NeilBrown





> ---
> Grow.c | 34 +++++++++++++++++++---------------
> 1 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 3455115..c06561f 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -666,6 +666,7 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
> int nr_of_copies;
> struct mdinfo *remaining;
> int slot;
> + int delta = 0;
>
> nr_of_copies = layout & 0xff;
>
> @@ -710,7 +711,7 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
> e = &(*e)->next;
> *e = sra->devs;
> sra->devs = remaining;
> - return 1;
> + return 0;
> }
>
> /* Remove all 'remaining' devices from the array */
> @@ -725,8 +726,9 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
> sd->disk.state &= ~(1< > sd->next = sra->devs;
> sra->devs = sd;
> + delta--;
> }
> - return 0;
> + return delta;
> }
>
> void reshape_free_fdlist(int *fdlist,
> @@ -1456,6 +1458,17 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
> size = array.size;
> }
>
> + info.array = array;
> + sysfs_init(&info, fd, NoMdDev);
> + strcpy(info.text_version, sra->text_version);
> + info.component_size = size*2;
> + info.new_level = level;
> + info.new_chunk = chunksize * 1024;
> + if (raid_disks)
> + info.delta_disks = raid_disks - info.array.raid_disks;
> + else
> + info.delta_disks = UnSet;
> +
> /* ========= check for Raid10 -> Raid0 conversion ===============
> * current implementation assumes that following conditions must be met:
> * - far_copies == 1
> @@ -1463,27 +1476,18 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
> */
> if (level == 0 && array.level == 10 && sra &&
> array.layout == ((1 << 8) + 2) && !(array.raid_disks & 1)) {
> - int err;
> - err = remove_disks_on_raid10_to_raid0_takeover(st, sra, array.layout);
> - if (err) {
> + int rv;
> + rv = remove_disks_on_raid10_to_raid0_takeover(st, sra, array.layout);
> + if (!rv) {
> dprintf(Name": Array cannot be reshaped\n");
> if (cfd > -1)
> close(cfd);
> rv = 1;
> goto release;
> }
> + info.delta_disks = rv;
> }
>
> - info.array = array;
> - sysfs_init(&info, fd, NoMdDev);
> - strcpy(info.text_version, sra->text_version);
> - info.component_size = size*2;
> - info.new_level = level;
> - info.new_chunk = chunksize * 1024;
> - if (raid_disks)
> - info.delta_disks = raid_disks - info.array.raid_disks;
> - else
> - info.delta_disks = UnSet;
> if (layout_str == NULL) {
> info.new_layout = UnSet;
> if (info.array.level == 6 &&

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] FIX: Mistake in delta_disk comparison.

On Wed, 12 Jan 2011 17:01:24 +0100 Krzysztof Wojcik
wrote:

> Signed-off-by: Krzysztof Wojcik
> ---
> Grow.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index c06561f..253f289 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -963,7 +963,7 @@ char *analyse_change(struct mdinfo *info, struct reshape *re)
> return "RAID10 can only be changed to RAID0";
> new_disks = (info->array.raid_disks
> / (info->array.layout & 0xff));
> - if (info->delta_disks != UnSet) {
> + if (info->delta_disks == UnSet) {
> info->delta_disks = (new_disks
> - info->array.raid_disks);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html


Applied, thanks.

NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

On Wed, 12 Jan 2011 17:01:40 +0100 Krzysztof Wojcik
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().

This depends on a previous patch which I didn't apply, so I didn't apply this
one either.

NeilBrown


>
> Signed-off-by: Krzysztof Wojcik
> ---
> Grow.c | 1
> super-intel.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 148 insertions(+), 1 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 6835f34..d5cd761 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1485,6 +1485,7 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
> rv = 1;
> goto release;
> }
> + ping_monitor(container);
> info.delta_disks = rv;
> }
>
> diff --git a/super-intel.c b/super-intel.c
> index 81f8e81..90e0b98 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,12 @@ struct geo_params {
> int raid_disks;
> };
>
> +#define TAKEOVER_R10_DISKS 2
> +struct imsm_update_takeover {
> + enum imsm_update_type type;
> + int subarray;
> + int disks[TAKEOVER_R10_DISKS];
> +};
>
> struct imsm_update_reshape {
> enum imsm_update_type type;
> @@ -5764,6 +5771,68 @@ 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;
> + int *tab;
> + int i;
> +
> + dev = get_imsm_dev(super, u->subarray);
> +
> + if (dev == NULL)
> + return 0;
> +
> + map = get_imsm_map(dev, 0);
> + tab = (int *)&map->disk_ord_tbl;
> +
> +for (i=0;i<4;i++)
> + dprintf("tab: idx= %i, val=%i\n", i, tab[i]);
> +
> +for (dm=super->disks;dm;dm=dm->next)
> + dprintf("disk: index= %i, minor= %i, major= %i\n", dm->index, dm->minor, dm->major);
> +
> + /* iterate through devices to mark removed disks as spare
> + * and update order table */
> + for (i = 0; i < TAKEOVER_R10_DISKS; i++) {
> + for (dm = super->disks; dm; dm = dm->next) {
> + if (((unsigned int)dm->major != major(u->disks[i])) ||
> + ((unsigned int)dm->minor != minor(u->disks[i])))
> + continue;
> + for (du = super->disks; du; du = du->next)
> + if ((du->index > dm->index) && (du->index > 0))
> + du->index--;
> + dm->disk.status = SPARE_DISK;
> + dm->index = -1;
> + }
> + }
> +
> + /* update disk order table */
> + i = 0;
> + for (du = super->disks; du; du = du->next) {
> + if (du->index >= 0) {
> +dprintf("du->index= %i\n", du->index);
> + tab[du->index] = i;
> + i++;
> + }
> + }
> +
> +for (i=0;i<4;i++)
> + dprintf("AFTER tab: idx= %i, val=%i\n", i, tab[i]);
> +for (dm=super->disks;dm;dm=dm->next)
> + dprintf("AFTER disk: index= %i, minor= %i, major= %i\n", dm->index, dm->minor, dm->major);
> +
> +
> + map->num_members = 2;
> + map->map_state = IMSM_T_STATE_NORMAL;
> + map->num_domains = 1;
> + map->raid_level = 0;
> + map->failed_disk_num = -1;
> + return 1;
> +}
> +
> static void imsm_process_update(struct supertype *st,
> struct metadata_update *update)
> {
> @@ -5806,6 +5875,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(
> @@ -6672,6 +6748,76 @@ 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;
> + struct mdinfo *info;
> + struct mdinfo *newdi;
> + struct dl *dl;
> + int subarray, i, fd;
> + int found = 0;
> + int devnum;
> + char buf[PATH_MAX];
> +
> + /* find requested device */
> + for (subarray = 0; subarray < super->anchor->num_raid_devs; subarray++) {
> + imsm_find_array_minor_by_subdev(subarray, st->container_dev, &devnum);
> + if (devnum == geo->dev_id) {
> + found = 1;
> + break;
> + }
> + }
> +
> + if (!found) {
> + fprintf(stderr, Name "Cannot find %s (%i) subarray\n",
> + geo->dev_name, geo->dev_id);
> + return 1;
> + }
> +
> + sprintf(buf, "/dev/md%i", devnum);
> + fd = open(buf, O_RDONLY);
> + if (!fd) {
> + fprintf(stderr, Name "Cannot open %s", buf);
> + return 1;
> + }
> + info = sysfs_read(fd, 0, GET_LEVEL|GET_VERSION|GET_DEVS|GET_STATE);
> + if (!info) {
> + fprintf(stderr, Name "Cannot load sysfs information for %s (%i)",
> + geo->dev_name, geo->dev_id);
> + return 1;
> + }
> +
> + u = malloc(sizeof(struct imsm_update_takeover));
> + if (u == NULL)
> + return 1;
> +
> + u->type = update_takeover;
> + u->subarray = subarray;
> +
> + /* 10->0 transition - mark disks to remove */
> + if (geo->level == 0) {
> + i = 0;
> + for (dl = super->disks; dl; dl = dl->next) {
> + found = 0;
> + for (newdi = info->devs; newdi; newdi = newdi->next) {
> + if ((dl->major != newdi->disk.major) ||
> + (dl->minor != newdi->disk.minor) ||
> + (newdi->disk.raid_disk < 0))
> + continue;
> + found = 1;
> + break;
> + }
> + /* if disk not found, mark it for remove */
> + if ((found == 0) && (!(dl->disk.status & SPARE_DISK)))
> + u->disks[i++] = makedev(dl->major, dl->minor);
> + }
> + }
> +
> + append_metadata_update(st, u, sizeof(struct imsm_update_takeover));
> + 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)
> @@ -6735,7 +6881,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;

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] Remove code for non re-striping operations.

On Wed, 12 Jan 2011 17:01:32 +0100 Krzysztof Wojcik
wrote:

> Neil,
> How was your intention when you add this part of code?
> I can't find the case when it will be necessary.
> Moreover it breaks flow for takeover operations because
> ioctl not succeeded when more than one parameter is changed.
> I've remove this code temporally to be able test takeover
> operations.
> We can restore this code after we decide what it should do.

Asking what the purpose of some code is is perfectly OK. Asking by sending a
patch which removes the code seems ... odd.

If the number of data devices is 1, then a RAID5 can have layout/chunksize
changed without any restriping.
This code handles that case.

NeilBrown


>
> Signed-off-by: Krzysztof Wojcik
> ---
> Grow.c | 43 +++++--------------------------------------
> 1 files changed, 5 insertions(+), 38 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 253f289..6835f34 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1666,6 +1666,11 @@ static int reshape_array(char *container, int fd, char *devname,
> ping_monitor(container);
> }
>
> + if (reshape.backup_blocks == 0) {
> + /* If operation not not reuire re-striping we can finish */
> + return rv;
> + }
> +
> /* ->reshape_super might have chosen some spares from the
> * container that it wants to be part of the new array.
> * We can collect them with ->container_content and give
> @@ -1692,44 +1697,6 @@ static int reshape_array(char *container, int fd, char *devname,
> }
> }
>
> - if (reshape.backup_blocks == 0) {
> - /* No restriping needed, but we might need to impose
> - * some more changes: layout, raid_disks, chunk_size
> - */
> - if (info->new_layout != UnSet &&
> - info->new_layout != info->array.layout) {
> - info->array.layout = info->new_layout;
> - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> - fprintf(stderr, Name ": failed to set new layout\n");
> - rv = 1;
> - } else if (!quiet)
> - printf("layout for %s set to %d\n",
> - devname, info->array.layout);
> - }
> - if (info->delta_disks != UnSet &&
> - info->delta_disks != 0) {
> - info->array.raid_disks += info->delta_disks;
> - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> - fprintf(stderr, Name ": failed to set raid disks\n");
> - rv = 1;
> - } else if (!quiet)
> - printf("raid_disks for %s set to %d\n",
> - devname, info->array.raid_disks);
> - }
> - if (info->new_chunk != 0 &&
> - info->new_chunk != info->array.chunk_size) {
> - if (sysfs_set_num(info, NULL,
> - "chunk_size", info->new_chunk) != 0) {
> - fprintf(stderr, Name ": failed to set chunk size\n");
> - rv = 1;
> - } else if (!quiet)
> - printf("chunk size for %s set to %d\n",
> - devname, info->array.chunk_size);
> - }
> -
> - return rv;
> - }
> -
> /*
> * There are three possibilities.
> * 1/ The array will shrink.

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/6] Remove code for non re-striping operations.

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, January 13, 2011 3:52 AM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> Williams, Dan J; Ciechanowski, Ed
> Subject: Re: [PATCH 5/6] Remove code for non re-striping operations.
>
> On Wed, 12 Jan 2011 17:01:32 +0100 Krzysztof Wojcik
> wrote:
>
> > Neil,
> > How was your intention when you add this part of code?
> > I can't find the case when it will be necessary.
> > Moreover it breaks flow for takeover operations because
> > ioctl not succeeded when more than one parameter is changed.
> > I've remove this code temporally to be able test takeover
> > operations.
> > We can restore this code after we decide what it should do.
>
> Asking what the purpose of some code is is perfectly OK. Asking by
> sending a
> patch which removes the code seems ... odd.

Sorry. It was just a proposal :)

>
> If the number of data devices is 1, then a RAID5 can have
> layout/chunksize
> changed without any restriping.
> This code handles that case.

So maybe it will be good to limit this part of code execution only for raid5?
(reshape.backup_blocks == 0) {
If (info->array.level != 5)
Return rv;
....

How do you mean?


>
> NeilBrown
>
>
> >
> > Signed-off-by: Krzysztof Wojcik
> > ---
> > Grow.c | 43 +++++--------------------------------------
> > 1 files changed, 5 insertions(+), 38 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 253f289..6835f34 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -1666,6 +1666,11 @@ static int reshape_array(char *container, int
> fd, char *devname,
> > ping_monitor(container);
> > }
> >
> > + if (reshape.backup_blocks == 0) {
> > + /* If operation not not reuire re-striping we can finish */
> > + return rv;
> > + }
> > +
> > /* ->reshape_super might have chosen some spares from the
> > * container that it wants to be part of the new array.
> > * We can collect them with ->container_content and give
> > @@ -1692,44 +1697,6 @@ static int reshape_array(char *container, int
> fd, char *devname,
> > }
> > }
> >
> > - if (reshape.backup_blocks == 0) {
> > - /* No restriping needed, but we might need to impose
> > - * some more changes: layout, raid_disks, chunk_size
> > - */
> > - if (info->new_layout != UnSet &&
> > - info->new_layout != info->array.layout) {
> > - info->array.layout = info->new_layout;
> > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> > - fprintf(stderr, Name ": failed to set new
> layout\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("layout for %s set to %d\n",
> > - devname, info->array.layout);
> > - }
> > - if (info->delta_disks != UnSet &&
> > - info->delta_disks != 0) {
> > - info->array.raid_disks += info->delta_disks;
> > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> > - fprintf(stderr, Name ": failed to set raid
> disks\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("raid_disks for %s set to %d\n",
> > - devname, info->array.raid_disks);
> > - }
> > - if (info->new_chunk != 0 &&
> > - info->new_chunk != info->array.chunk_size) {
> > - if (sysfs_set_num(info, NULL,
> > - "chunk_size", info->new_chunk) != 0) {
> > - fprintf(stderr, Name ": failed to set chunk
> size\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("chunk size for %s set to %d\n",
> > - devname, info->array.chunk_size);
> > - }
> > -
> > - return rv;
> > - }
> > -
> > /*
> > * There are three possibilities.
> > * 1/ The array will shrink.

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/6] Remove code for non re-striping operations.

Hi Neil,

So maybe it will be good to limit this part of code execution only for raid5?
(reshape.backup_blocks == 0) {
If (info->array.level != 5)
return rv;
....

Do you agree?

Regards- Krzysztof

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, January 13, 2011 3:52 AM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> Williams, Dan J; Ciechanowski, Ed
> Subject: Re: [PATCH 5/6] Remove code for non re-striping operations.
>
> On Wed, 12 Jan 2011 17:01:32 +0100 Krzysztof Wojcik
> wrote:
>
> > Neil,
> > How was your intention when you add this part of code?
> > I can't find the case when it will be necessary.
> > Moreover it breaks flow for takeover operations because
> > ioctl not succeeded when more than one parameter is changed.
> > I've remove this code temporally to be able test takeover
> > operations.
> > We can restore this code after we decide what it should do.
>
> Asking what the purpose of some code is is perfectly OK. Asking by
> sending a
> patch which removes the code seems ... odd.
>
> If the number of data devices is 1, then a RAID5 can have
> layout/chunksize
> changed without any restriping.
> This code handles that case.
>
> NeilBrown
>
>
> >
> > Signed-off-by: Krzysztof Wojcik
> > ---
> > Grow.c | 43 +++++--------------------------------------
> > 1 files changed, 5 insertions(+), 38 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 253f289..6835f34 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -1666,6 +1666,11 @@ static int reshape_array(char *container, int
> fd, char *devname,
> > ping_monitor(container);
> > }
> >
> > + if (reshape.backup_blocks == 0) {
> > + /* If operation not not reuire re-striping we can finish */
> > + return rv;
> > + }
> > +
> > /* ->reshape_super might have chosen some spares from the
> > * container that it wants to be part of the new array.
> > * We can collect them with ->container_content and give
> > @@ -1692,44 +1697,6 @@ static int reshape_array(char *container, int
> fd, char *devname,
> > }
> > }
> >
> > - if (reshape.backup_blocks == 0) {
> > - /* No restriping needed, but we might need to impose
> > - * some more changes: layout, raid_disks, chunk_size
> > - */
> > - if (info->new_layout != UnSet &&
> > - info->new_layout != info->array.layout) {
> > - info->array.layout = info->new_layout;
> > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> > - fprintf(stderr, Name ": failed to set new
> layout\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("layout for %s set to %d\n",
> > - devname, info->array.layout);
> > - }
> > - if (info->delta_disks != UnSet &&
> > - info->delta_disks != 0) {
> > - info->array.raid_disks += info->delta_disks;
> > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> > - fprintf(stderr, Name ": failed to set raid
> disks\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("raid_disks for %s set to %d\n",
> > - devname, info->array.raid_disks);
> > - }
> > - if (info->new_chunk != 0 &&
> > - info->new_chunk != info->array.chunk_size) {
> > - if (sysfs_set_num(info, NULL,
> > - "chunk_size", info->new_chunk) != 0) {
> > - fprintf(stderr, Name ": failed to set chunk
> size\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("chunk size for %s set to %d\n",
> > - devname, info->array.chunk_size);
> > - }
> > -
> > - return rv;
> > - }
> > -
> > /*
> > * There are three possibilities.
> > * 1/ The array will shrink.

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] Remove code for non re-striping operations.

On Mon, 17 Jan 2011 15:49:25 +0000 "Wojcik, Krzysztof"
wrote:

> Hi Neil,
>
> So maybe it will be good to limit this part of code execution only for raid5?
> (reshape.backup_blocks == 0) {
> If (info->array.level != 5)
> return rv;
> ...
>
> Do you agree?

No. Why would you want do to that?

The code is relevant in other cases too (I hadn't thought it through last
time I commented). There are other cases where re-striping isn't needed such
as when adding an extra device to a RAID1.

Why do you think this code needs any change at all?? Is it causing you some
sort of problem?

NeilBrown


>
> Regards- Krzysztof
>
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Thursday, January 13, 2011 3:52 AM
> > To: Wojcik, Krzysztof
> > Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> > Williams, Dan J; Ciechanowski, Ed
> > Subject: Re: [PATCH 5/6] Remove code for non re-striping operations.
> >
> > On Wed, 12 Jan 2011 17:01:32 +0100 Krzysztof Wojcik
> > wrote:
> >
> > > Neil,
> > > How was your intention when you add this part of code?
> > > I can't find the case when it will be necessary.
> > > Moreover it breaks flow for takeover operations because
> > > ioctl not succeeded when more than one parameter is changed.
> > > I've remove this code temporally to be able test takeover
> > > operations.
> > > We can restore this code after we decide what it should do.
> >
> > Asking what the purpose of some code is is perfectly OK. Asking by
> > sending a
> > patch which removes the code seems ... odd.
> >
> > If the number of data devices is 1, then a RAID5 can have
> > layout/chunksize
> > changed without any restriping.
> > This code handles that case.
> >
> > NeilBrown
> >
> >
> > >
> > > Signed-off-by: Krzysztof Wojcik
> > > ---
> > > Grow.c | 43 +++++--------------------------------------
> > > 1 files changed, 5 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index 253f289..6835f34 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -1666,6 +1666,11 @@ static int reshape_array(char *container, int
> > fd, char *devname,
> > > ping_monitor(container);
> > > }
> > >
> > > + if (reshape.backup_blocks == 0) {
> > > + /* If operation not not reuire re-striping we can finish */
> > > + return rv;
> > > + }
> > > +
> > > /* ->reshape_super might have chosen some spares from the
> > > * container that it wants to be part of the new array.
> > > * We can collect them with ->container_content and give
> > > @@ -1692,44 +1697,6 @@ static int reshape_array(char *container, int
> > fd, char *devname,
> > > }
> > > }
> > >
> > > - if (reshape.backup_blocks == 0) {
> > > - /* No restriping needed, but we might need to impose
> > > - * some more changes: layout, raid_disks, chunk_size
> > > - */
> > > - if (info->new_layout != UnSet &&
> > > - info->new_layout != info->array.layout) {
> > > - info->array.layout = info->new_layout;
> > > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> > > - fprintf(stderr, Name ": failed to set new
> > layout\n");
> > > - rv = 1;
> > > - } else if (!quiet)
> > > - printf("layout for %s set to %d\n",
> > > - devname, info->array.layout);
> > > - }
> > > - if (info->delta_disks != UnSet &&
> > > - info->delta_disks != 0) {
> > > - info->array.raid_disks += info->delta_disks;
> > > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
> > > - fprintf(stderr, Name ": failed to set raid
> > disks\n");
> > > - rv = 1;
> > > - } else if (!quiet)
> > > - printf("raid_disks for %s set to %d\n",
> > > - devname, info->array.raid_disks);
> > > - }
> > > - if (info->new_chunk != 0 &&
> > > - info->new_chunk != info->array.chunk_size) {
> > > - if (sysfs_set_num(info, NULL,
> > > - "chunk_size", info->new_chunk) != 0) {
> > > - fprintf(stderr, Name ": failed to set chunk
> > size\n");
> > > - rv = 1;
> > > - } else if (!quiet)
> > > - printf("chunk size for %s set to %d\n",
> > > - devname, info->array.chunk_size);
> > > - }
> > > -
> > > - return rv;
> > > - }
> > > -
> > > /*
> > > * There are three possibilities.
> > > * 1/ The array will shrink.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/6] Remove code for non re-striping operations.

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Wednesday, January 19, 2011 9:53 PM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam
> Subject: Re: [PATCH 5/6] Remove code for non re-striping operations.
>
> On Mon, 17 Jan 2011 15:49:25 +0000 "Wojcik, Krzysztof"
> wrote:
>
> > Hi Neil,
> >
> > So maybe it will be good to limit this part of code execution only
> for raid5?
> > (reshape.backup_blocks == 0) {
> > If (info->array.level != 5)
> > return rv;
> > ...
> >
> > Do you agree?
>
> No. Why would you want do to that?
>
> The code is relevant in other cases too (I hadn't thought it through
> last
> time I commented). There are other cases where re-striping isn't
> needed such
> as when adding an extra device to a RAID1.
>
> Why do you think this code needs any change at all?? Is it causing you
> some
> sort of problem?

Yes. Problem is when I execute raid10<->raid0 takeover operation, I have delta_disks set to -2 so condition in this code (delta_disks != UnSet) is true and ioctl is executed. It is not succeeded, Mdadm returns error:
Mdadm: failed to set disks
I don't know why ioctl returns error. Maybe because more than one parameters is changed (raid_disks and level) or it cannot overwrite raid_disks and level previously set to proper state.

I propose solutions:
1. Set delta_disks to UnSet for raid10->raid0 takeover or
2. Add new flag to reshape structure to indicate to skip raid_disk, chunksize, layout changes


>
> NeilBrown
>
>
> >
> > Regards- Krzysztof
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Thursday, January 13, 2011 3:52 AM
> > > To: Wojcik, Krzysztof
> > > Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> > > Williams, Dan J; Ciechanowski, Ed
> > > Subject: Re: [PATCH 5/6] Remove code for non re-striping
> operations.
> > >
> > > On Wed, 12 Jan 2011 17:01:32 +0100 Krzysztof Wojcik
> > > wrote:
> > >
> > > > Neil,
> > > > How was your intention when you add this part of code?
> > > > I can't find the case when it will be necessary.
> > > > Moreover it breaks flow for takeover operations because
> > > > ioctl not succeeded when more than one parameter is changed.
> > > > I've remove this code temporally to be able test takeover
> > > > operations.
> > > > We can restore this code after we decide what it should do.
> > >
> > > Asking what the purpose of some code is is perfectly OK. Asking by
> > > sending a
> > > patch which removes the code seems ... odd.
> > >
> > > If the number of data devices is 1, then a RAID5 can have
> > > layout/chunksize
> > > changed without any restriping.
> > > This code handles that case.
> > >
> > > NeilBrown
> > >
> > >
> > > >
> > > > Signed-off-by: Krzysztof Wojcik
> > > > ---
> > > > Grow.c | 43 +++++--------------------------------------
> > > > 1 files changed, 5 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/Grow.c b/Grow.c
> > > > index 253f289..6835f34 100644
> > > > --- a/Grow.c
> > > > +++ b/Grow.c
> > > > @@ -1666,6 +1666,11 @@ static int reshape_array(char *container,
> int
> > > fd, char *devname,
> > > > ping_monitor(container);
> > > > }
> > > >
> > > > + if (reshape.backup_blocks == 0) {
> > > > + /* If operation not not reuire re-striping we can
> finish */
> > > > + return rv;
> > > > + }
> > > > +
> > > > /* ->reshape_super might have chosen some spares from the
> > > > * container that it wants to be part of the new array.
> > > > * We can collect them with ->container_content and give
> > > > @@ -1692,44 +1697,6 @@ static int reshape_array(char *container,
> int
> > > fd, char *devname,
> > > > }
> > > > }
> > > >
> > > > - if (reshape.backup_blocks == 0) {
> > > > - /* No restriping needed, but we might need to impose
> > > > - * some more changes: layout, raid_disks, chunk_size
> > > > - */
> > > > - if (info->new_layout != UnSet &&
> > > > - info->new_layout != info->array.layout) {
> > > > - info->array.layout = info->new_layout;
> > > > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) !=
> 0) {
> > > > - fprintf(stderr, Name ": failed to set new
> > > layout\n");
> > > > - rv = 1;
> > > > - } else if (!quiet)
> > > > - printf("layout for %s set to %d\n",
> > > > - devname, info->array.layout);
> > > > - }
> > > > - if (info->delta_disks != UnSet &&
> > > > - info->delta_disks != 0) {
> > > > - info->array.raid_disks += info->delta_disks;
> > > > - if (ioctl(fd, SET_ARRAY_INFO, &info->array) !=
> 0) {
> > > > - fprintf(stderr, Name ": failed to set
> raid
> > > disks\n");
> > > > - rv = 1;
> > > > - } else if (!quiet)
> > > > - printf("raid_disks for %s set to %d\n",
> > > > - devname, info->array.raid_disks);
> > > > - }
> > > > - if (info->new_chunk != 0 &&
> > > > - info->new_chunk != info->array.chunk_size) {
> > > > - if (sysfs_set_num(info, NULL,
> > > > - "chunk_size", info->new_chunk) !=
> 0) {
> > > > - fprintf(stderr, Name ": failed to set
> chunk
> > > size\n");
> > > > - rv = 1;
> > > > - } else if (!quiet)
> > > > - printf("chunk size for %s set to %d\n",
> > > > - devname, info->array.chunk_size);
> > > > - }
> > > > -
> > > > - return rv;
> > > > - }
> > > > -
> > > > /*
> > > > * There are three possibilities.
> > > > * 1/ The array will shrink.
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html