[PATCH 0/3 v2] IMSM autolayout improvements

[PATCH 0/3 v2] IMSM autolayout improvements

[Mariusz Tkaczyk]

Following patchset modifies some parts of IMSM creation to ensure that
member's order is always same. It is ISM metadata requirement.

Additionally, as discussed with Jes I've started to implement more modern error
handling, by adding special enum for IMSM. Will be great to hear any comments
and opinion.

V2 improvements:
- typedef added in first patch
- changed order in enum

Mariusz Tkaczyk (3):
  imsm: introduce get_disk_slot_in_dev()
  imsm: use same slot across container
  imsm: block changing slots during creation

 super-intel.c        | 249 ++++++++++++++++++++++++++++---------------
 tests/09imsm-overlap |  28 -----
 2 files changed, 166 insertions(+), 111 deletions(-)
 delete mode 100644 tests/09imsm-overlap

-- 
2.26.2

[PATCH 1/3 v2] imsm: introduce get_disk_slot_in_dev()

[Mariusz Tkaczyk]

The routine was added to remove unnecessary get_imsm_dev() and
get_imsm_map() calls, used only to determine disk slot.

Additionally, enum for IMSM return statues was added for further usage.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 super-intel.c | 47 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index d5fad102..f0196948 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -366,6 +366,18 @@ struct migr_record {
 };
 ASSERT_SIZE(migr_record, 128)
 
+/**
+ * enum imsm_status - internal IMSM return values representation.
+ * @STATUS_OK: function succeeded.
+ * @STATUS_ERROR: General error ocurred (not specified).
+ *
+ * Typedefed to imsm_status_t.
+ */
+typedef enum imsm_status {
+	IMSM_STATUS_ERROR = -1,
+	IMSM_STATUS_OK = 0,
+} imsm_status_t;
+
 struct md_list {
 	/* usage marker:
 	 *  1: load metadata
@@ -1151,7 +1163,7 @@ static void set_imsm_ord_tbl_ent(struct imsm_map *map, int slot, __u32 ord)
 	map->disk_ord_tbl[slot] = __cpu_to_le32(ord);
 }
 
-static int get_imsm_disk_slot(struct imsm_map *map, unsigned idx)
+static int get_imsm_disk_slot(struct imsm_map *map, const unsigned int idx)
 {
 	int slot;
 	__u32 ord;
@@ -1162,7 +1174,7 @@ static int get_imsm_disk_slot(struct imsm_map *map, unsigned idx)
 			return slot;
 	}
 
-	return -1;
+	return IMSM_STATUS_ERROR;
 }
 
 static int get_imsm_raid_level(struct imsm_map *map)
@@ -1177,6 +1189,23 @@ static int get_imsm_raid_level(struct imsm_map *map)
 	return map->raid_level;
 }
 
+/**
+ * get_disk_slot_in_dev() - retrieve disk slot from &imsm_dev.
+ * @super: &intel_super pointer, not NULL.
+ * @dev_idx: imsm device index.
+ * @idx: disk index.
+ *
+ * Return: Slot on success, IMSM_STATUS_ERROR otherwise.
+ */
+static int get_disk_slot_in_dev(struct intel_super *super, const __u8 dev_idx,
+				const unsigned int idx)
+{
+	struct imsm_dev *dev = get_imsm_dev(super, dev_idx);
+	struct imsm_map *map = get_imsm_map(dev, MAP_0);
+
+	return get_imsm_disk_slot(map, idx);
+}
+
 static int cmp_extent(const void *av, const void *bv)
 {
 	const struct extent *a = av;
@@ -1193,13 +1222,9 @@ static int count_memberships(struct dl *dl, struct intel_super *super)
 	int memberships = 0;
 	int i;
 
-	for (i = 0; i < super->anchor->num_raid_devs; i++) {
-		struct imsm_dev *dev = get_imsm_dev(super, i);
-		struct imsm_map *map = get_imsm_map(dev, MAP_0);
-
-		if (get_imsm_disk_slot(map, dl->index) >= 0)
+	for (i = 0; i < super->anchor->num_raid_devs; i++)
+		if (get_disk_slot_in_dev(super, i, dl->index) >= 0)
 			memberships++;
-	}
 
 	return memberships;
 }
@@ -1909,6 +1934,7 @@ void examine_migr_rec_imsm(struct intel_super *super)
 
 		/* first map under migration */
 		map = get_imsm_map(dev, MAP_0);
+
 		if (map)
 			slot = get_imsm_disk_slot(map, super->disks->index);
 		if (map == NULL || slot > 1 || slot < 0) {
@@ -9629,10 +9655,9 @@ static int apply_update_activate_spare(struct imsm_update_activate_spare *u,
 		/* count arrays using the victim in the metadata */
 		found = 0;
 		for (a = active_array; a ; a = a->next) {
-			dev = get_imsm_dev(super, a->info.container_member);
-			map = get_imsm_map(dev, MAP_0);
+			int dev_idx = a->info.container_member;
 
-			if (get_imsm_disk_slot(map, victim) >= 0)
+			if (get_disk_slot_in_dev(super, dev_idx, victim) >= 0)
 				found++;
 		}
 
-- 
2.26.2

Re: [PATCH 1/3 v2] imsm: introduce get_disk_slot_in_dev()

[Coly Li]

> 2022年5月31日 18:27，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道：
> 
> The routine was added to remove unnecessary get_imsm_dev() and
> get_imsm_map() calls, used only to determine disk slot.
> 
> Additionally, enum for IMSM return statues was added for further usage.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
> super-intel.c | 47 ++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index d5fad102..f0196948 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -366,6 +366,18 @@ struct migr_record {
> };
> ASSERT_SIZE(migr_record, 128)
> 
> +/**
> + * enum imsm_status - internal IMSM return values representation.
> + * @STATUS_OK: function succeeded.
> + * @STATUS_ERROR: General error ocurred (not specified).
> + *
> + * Typedefed to imsm_status_t.
> + */
> +typedef enum imsm_status {
> +	IMSM_STATUS_ERROR = -1,
> +	IMSM_STATUS_OK = 0,
> +} imsm_status_t;
> +
> struct md_list {
> 	/* usage marker:
> 	 *  1: load metadata
> @@ -1151,7 +1163,7 @@ static void set_imsm_ord_tbl_ent(struct imsm_map *map, int slot, __u32 ord)
> 	map->disk_ord_tbl[slot] = __cpu_to_le32(ord);
> }
> 
> -static int get_imsm_disk_slot(struct imsm_map *map, unsigned idx)
> +static int get_imsm_disk_slot(struct imsm_map *map, const unsigned int idx)
> {
> 	int slot;
> 	__u32 ord;
> @@ -1162,7 +1174,7 @@ static int get_imsm_disk_slot(struct imsm_map *map, unsigned idx)
> 			return slot;
> 	}
> 
> -	return -1;
> +	return IMSM_STATUS_ERROR;
> }
> 
> static int get_imsm_raid_level(struct imsm_map *map)
> @@ -1177,6 +1189,23 @@ static int get_imsm_raid_level(struct imsm_map *map)
> 	return map->raid_level;
> }
> 
> +/**
> + * get_disk_slot_in_dev() - retrieve disk slot from &imsm_dev.
> + * @super: &intel_super pointer, not NULL.
> + * @dev_idx: imsm device index.
> + * @idx: disk index.
> + *
> + * Return: Slot on success, IMSM_STATUS_ERROR otherwise.
> + */
> +static int get_disk_slot_in_dev(struct intel_super *super, const __u8 dev_idx,
> +				const unsigned int idx)
> +{
> +	struct imsm_dev *dev = get_imsm_dev(super, dev_idx);
> +	struct imsm_map *map = get_imsm_map(dev, MAP_0);
> +
> +	return get_imsm_disk_slot(map, idx);
> +}
> +
> static int cmp_extent(const void *av, const void *bv)
> {
> 	const struct extent *a = av;
> @@ -1193,13 +1222,9 @@ static int count_memberships(struct dl *dl, struct intel_super *super)
> 	int memberships = 0;
> 	int i;
> 
> -	for (i = 0; i < super->anchor->num_raid_devs; i++) {
> -		struct imsm_dev *dev = get_imsm_dev(super, i);
> -		struct imsm_map *map = get_imsm_map(dev, MAP_0);
> -
> -		if (get_imsm_disk_slot(map, dl->index) >= 0)
> +	for (i = 0; i < super->anchor->num_raid_devs; i++)
> +		if (get_disk_slot_in_dev(super, i, dl->index) >= 0)
> 			memberships++;
> -	}
> 
> 	return memberships;
> }
> @@ -1909,6 +1934,7 @@ void examine_migr_rec_imsm(struct intel_super *super)
> 
> 		/* first map under migration */
> 		map = get_imsm_map(dev, MAP_0);
> +
> 		if (map)
> 			slot = get_imsm_disk_slot(map, super->disks->index);
> 		if (map == NULL || slot > 1 || slot < 0) {
> @@ -9629,10 +9655,9 @@ static int apply_update_activate_spare(struct imsm_update_activate_spare *u,
> 		/* count arrays using the victim in the metadata */
> 		found = 0;
> 		for (a = active_array; a ; a = a->next) {
> -			dev = get_imsm_dev(super, a->info.container_member);
> -			map = get_imsm_map(dev, MAP_0);
> +			int dev_idx = a->info.container_member;
> 
> -			if (get_imsm_disk_slot(map, victim) >= 0)
> +			if (get_disk_slot_in_dev(super, dev_idx, victim) >= 0)
> 				found++;
> 		}
> 
> -- 
> 2.26.2
> 

[PATCH 2/3 v2] imsm: use same slot across container

[Mariusz Tkaczyk]

Autolayout relies on drives order on super->disks list, but
it is not quaranted by readdir() in sysfs_read(). As a result
drive could be put in different slot in second volume.

Make it consistent by reffering to first volume, if exists.

Use enum imsm_status to unify error handling.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 super-intel.c | 169 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 108 insertions(+), 61 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index f0196948..3c02d2f6 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -7493,11 +7493,27 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
 	return 1;
 }
 
-static int imsm_get_free_size(struct supertype *st, int raiddisks,
-			 unsigned long long size, int chunk,
-			 unsigned long long *freesize)
+/**
+ * imsm_get_free_size() - get the biggest, common free space from members.
+ * @super: &intel_super pointer, not NULL.
+ * @raiddisks: number of raid disks.
+ * @size: requested size, could be 0 (means max size).
+ * @chunk: requested chunk.
+ * @freesize: pointer for returned size value.
+ *
+ * Return: &IMSM_STATUS_OK or &IMSM_STATUS_ERROR.
+ *
+ * @freesize is set to meaningful value, this can be @size, or calculated
+ * max free size.
+ * super->create_offset value is modified and set appropriately in
+ * merge_extends() for further creation.
+ */
+static imsm_status_t imsm_get_free_size(struct intel_super *super,
+					const int raiddisks,
+					unsigned long long size,
+					const int chunk,
+					unsigned long long *freesize)
 {
-	struct intel_super *super = st->sb;
 	struct imsm_super *mpb = super->anchor;
 	struct dl *dl;
 	int i;
@@ -7541,12 +7557,10 @@ static int imsm_get_free_size(struct supertype *st, int raiddisks,
 		/* chunk is in K */
 		minsize = chunk * 2;
 
-	if (cnt < raiddisks ||
-	    (super->orom && used && used != raiddisks) ||
-	    maxsize < minsize ||
-	    maxsize == 0) {
+	if (cnt < raiddisks || (super->orom && used && used != raiddisks) ||
+	    maxsize < minsize || maxsize == 0) {
 		pr_err("not enough devices with space to create array.\n");
-		return 0; /* No enough free spaces large enough */
+		return IMSM_STATUS_ERROR;
 	}
 
 	if (size == 0) {
@@ -7559,37 +7573,69 @@ static int imsm_get_free_size(struct supertype *st, int raiddisks,
 	}
 	if (mpb->num_raid_devs > 0 && size && size != maxsize)
 		pr_err("attempting to create a second volume with size less then remaining space.\n");
-	cnt = 0;
-	for (dl = super->disks; dl; dl = dl->next)
-		if (dl->e)
-			dl->raiddisk = cnt++;
-
 	*freesize = size;
 
 	dprintf("imsm: imsm_get_free_size() returns : %llu\n", size);
 
-	return 1;
+	return IMSM_STATUS_OK;
 }
 
-static int reserve_space(struct supertype *st, int raiddisks,
-			 unsigned long long size, int chunk,
-			 unsigned long long *freesize)
+/**
+ * autolayout_imsm() - automatically layout a new volume.
+ * @super: &intel_super pointer, not NULL.
+ * @raiddisks: number of raid disks.
+ * @size: requested size, could be 0 (means max size).
+ * @chunk: requested chunk.
+ * @freesize: pointer for returned size value.
+ *
+ * We are being asked to automatically layout a new volume based on the current
+ * contents of the container. If the parameters can be satisfied autolayout_imsm
+ * will record the disks, start offset, and will return size of the volume to
+ * be created. See imsm_get_free_size() for details.
+ * add_to_super() and getinfo_super() detect when autolayout is in progress.
+ * If first volume exists, slots are set consistently to it.
+ *
+ * Return: &IMSM_STATUS_OK on success, &IMSM_STATUS_ERROR otherwise.
+ *
+ * Disks are marked for creation via dl->raiddisk.
+ */
+static imsm_status_t autolayout_imsm(struct intel_super *super,
+				     const int raiddisks,
+				     unsigned long long size, const int chunk,
+				     unsigned long long *freesize)
 {
-	struct intel_super *super = st->sb;
-	struct dl *dl;
-	int cnt;
-	int rv = 0;
+	int curr_slot = 0;
+	struct dl *disk;
+	int vol_cnt = super->anchor->num_raid_devs;
+	imsm_status_t rv;
 
-	rv = imsm_get_free_size(st, raiddisks, size, chunk, freesize);
-	if (rv) {
-		cnt = 0;
-		for (dl = super->disks; dl; dl = dl->next)
-			if (dl->e)
-				dl->raiddisk = cnt++;
-		rv = 1;
+	rv = imsm_get_free_size(super, raiddisks, size, chunk, freesize);
+	if (rv != IMSM_STATUS_OK)
+		return IMSM_STATUS_ERROR;
+
+	for (disk = super->disks; disk; disk = disk->next) {
+		if (!disk->e)
+			continue;
+
+		if (curr_slot == raiddisks)
+			break;
+
+		if (vol_cnt == 0) {
+			disk->raiddisk = curr_slot;
+		} else {
+			int _slot = get_disk_slot_in_dev(super, 0, disk->index);
+
+			if (_slot == -1) {
+				pr_err("Disk %s is not used in first volume, aborting\n",
+				       disk->devname);
+				return IMSM_STATUS_ERROR;
+			}
+			disk->raiddisk = _slot;
+		}
+		curr_slot++;
 	}
 
-	return rv;
+	return IMSM_STATUS_OK;
 }
 
 static int validate_geometry_imsm(struct supertype *st, int level, int layout,
@@ -7625,35 +7671,35 @@ static int validate_geometry_imsm(struct supertype *st, int level, int layout,
 	}
 
 	if (!dev) {
-		if (st->sb) {
-			struct intel_super *super = st->sb;
-			if (!validate_geometry_imsm_orom(st->sb, level, layout,
-							 raiddisks, chunk, size,
-							 verbose))
+		struct intel_super *super = st->sb;
+
+		/*
+		 * Autolayout mode, st->sb and freesize must be set.
+		 */
+		if (!super || !freesize) {
+			pr_vrb("freesize and superblock must be set for autolayout, aborting\n");
+			return 1;
+		}
+
+		if (!validate_geometry_imsm_orom(st->sb, level, layout,
+						 raiddisks, chunk, size,
+						 verbose))
+			return 0;
+
+		if (super->orom) {
+			imsm_status_t rv;
+			int count = count_volumes(super->hba, super->orom->dpa,
+					      verbose);
+			if (super->orom->vphba <= count) {
+				pr_vrb("platform does not support more than %d raid volumes.\n",
+				       super->orom->vphba);
 				return 0;
-			/* we are being asked to automatically layout a
-			 * new volume based on the current contents of
-			 * the container.  If the the parameters can be
-			 * satisfied reserve_space will record the disks,
-			 * start offset, and size of the volume to be
-			 * created.  add_to_super and getinfo_super
-			 * detect when autolayout is in progress.
-			 */
-			/* assuming that freesize is always given when array is
-			   created */
-			if (super->orom && freesize) {
-				int count;
-				count = count_volumes(super->hba,
-						      super->orom->dpa, verbose);
-				if (super->orom->vphba <= count) {
-					pr_vrb("platform does not support more than %d raid volumes.\n",
-					       super->orom->vphba);
-					return 0;
-				}
 			}
-			if (freesize)
-				return reserve_space(st, raiddisks, size,
-						     *chunk, freesize);
+
+			rv = autolayout_imsm(super, raiddisks, size, *chunk,
+					     freesize);
+			if (rv != IMSM_STATUS_OK)
+				return 0;
 		}
 		return 1;
 	}
@@ -11524,7 +11570,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 	unsigned long long current_size;
 	unsigned long long free_size;
 	unsigned long long max_size;
-	int rv;
+	imsm_status_t rv;
 
 	getinfo_super_imsm_volume(st, &info, NULL);
 	if (geo->level != info.array.level && geo->level >= 0 &&
@@ -11643,9 +11689,10 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 		}
 		/* check the maximum available size
 		 */
-		rv =  imsm_get_free_size(st, dev->vol.map->num_members,
-					 0, chunk, &free_size);
-		if (rv == 0)
+		rv = imsm_get_free_size(super, dev->vol.map->num_members,
+					0, chunk, &free_size);
+
+		if (rv != IMSM_STATUS_OK)
 			/* Cannot find maximum available space
 			 */
 			max_size = 0;
-- 
2.26.2

Re: [PATCH 2/3 v2] imsm: use same slot across container

[Coly Li]

> 2022年5月31日 18:27，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道：
> 
> Autolayout relies on drives order on super->disks list, but
> it is not quaranted by readdir() in sysfs_read(). As a result
> drive could be put in different slot in second volume.
> 
> Make it consistent by reffering to first volume, if exists.
> 
> Use enum imsm_status to unify error handling.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Acked-by: Coly Li <colyli@suse.de>


Coly Li


> ---
> super-intel.c | 169 ++++++++++++++++++++++++++++++++------------------
> 1 file changed, 108 insertions(+), 61 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index f0196948..3c02d2f6 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -7493,11 +7493,27 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
> 	return 1;
> }
> 
> -static int imsm_get_free_size(struct supertype *st, int raiddisks,
> -			 unsigned long long size, int chunk,
> -			 unsigned long long *freesize)
> +/**
> + * imsm_get_free_size() - get the biggest, common free space from members.
> + * @super: &intel_super pointer, not NULL.
> + * @raiddisks: number of raid disks.
> + * @size: requested size, could be 0 (means max size).
> + * @chunk: requested chunk.
> + * @freesize: pointer for returned size value.
> + *
> + * Return: &IMSM_STATUS_OK or &IMSM_STATUS_ERROR.
> + *
> + * @freesize is set to meaningful value, this can be @size, or calculated
> + * max free size.
> + * super->create_offset value is modified and set appropriately in
> + * merge_extends() for further creation.
> + */
> +static imsm_status_t imsm_get_free_size(struct intel_super *super,
> +					const int raiddisks,
> +					unsigned long long size,
> +					const int chunk,
> +					unsigned long long *freesize)
> {
> -	struct intel_super *super = st->sb;
> 	struct imsm_super *mpb = super->anchor;
> 	struct dl *dl;
> 	int i;
> @@ -7541,12 +7557,10 @@ static int imsm_get_free_size(struct supertype *st, int raiddisks,
> 		/* chunk is in K */
> 		minsize = chunk * 2;
> 
> -	if (cnt < raiddisks ||
> -	    (super->orom && used && used != raiddisks) ||
> -	    maxsize < minsize ||
> -	    maxsize == 0) {
> +	if (cnt < raiddisks || (super->orom && used && used != raiddisks) ||
> +	    maxsize < minsize || maxsize == 0) {
> 		pr_err("not enough devices with space to create array.\n");
> -		return 0; /* No enough free spaces large enough */
> +		return IMSM_STATUS_ERROR;
> 	}
> 
> 	if (size == 0) {
> @@ -7559,37 +7573,69 @@ static int imsm_get_free_size(struct supertype *st, int raiddisks,
> 	}
> 	if (mpb->num_raid_devs > 0 && size && size != maxsize)
> 		pr_err("attempting to create a second volume with size less then remaining space.\n");
> -	cnt = 0;
> -	for (dl = super->disks; dl; dl = dl->next)
> -		if (dl->e)
> -			dl->raiddisk = cnt++;
> -
> 	*freesize = size;
> 
> 	dprintf("imsm: imsm_get_free_size() returns : %llu\n", size);
> 
> -	return 1;
> +	return IMSM_STATUS_OK;
> }
> 
> -static int reserve_space(struct supertype *st, int raiddisks,
> -			 unsigned long long size, int chunk,
> -			 unsigned long long *freesize)
> +/**
> + * autolayout_imsm() - automatically layout a new volume.
> + * @super: &intel_super pointer, not NULL.
> + * @raiddisks: number of raid disks.
> + * @size: requested size, could be 0 (means max size).
> + * @chunk: requested chunk.
> + * @freesize: pointer for returned size value.
> + *
> + * We are being asked to automatically layout a new volume based on the current
> + * contents of the container. If the parameters can be satisfied autolayout_imsm
> + * will record the disks, start offset, and will return size of the volume to
> + * be created. See imsm_get_free_size() for details.
> + * add_to_super() and getinfo_super() detect when autolayout is in progress.
> + * If first volume exists, slots are set consistently to it.
> + *
> + * Return: &IMSM_STATUS_OK on success, &IMSM_STATUS_ERROR otherwise.
> + *
> + * Disks are marked for creation via dl->raiddisk.
> + */
> +static imsm_status_t autolayout_imsm(struct intel_super *super,
> +				     const int raiddisks,
> +				     unsigned long long size, const int chunk,
> +				     unsigned long long *freesize)
> {
> -	struct intel_super *super = st->sb;
> -	struct dl *dl;
> -	int cnt;
> -	int rv = 0;
> +	int curr_slot = 0;
> +	struct dl *disk;
> +	int vol_cnt = super->anchor->num_raid_devs;
> +	imsm_status_t rv;
> 
> -	rv = imsm_get_free_size(st, raiddisks, size, chunk, freesize);
> -	if (rv) {
> -		cnt = 0;
> -		for (dl = super->disks; dl; dl = dl->next)
> -			if (dl->e)
> -				dl->raiddisk = cnt++;
> -		rv = 1;
> +	rv = imsm_get_free_size(super, raiddisks, size, chunk, freesize);
> +	if (rv != IMSM_STATUS_OK)
> +		return IMSM_STATUS_ERROR;
> +
> +	for (disk = super->disks; disk; disk = disk->next) {
> +		if (!disk->e)
> +			continue;
> +
> +		if (curr_slot == raiddisks)
> +			break;
> +
> +		if (vol_cnt == 0) {
> +			disk->raiddisk = curr_slot;
> +		} else {
> +			int _slot = get_disk_slot_in_dev(super, 0, disk->index);
> +
> +			if (_slot == -1) {
> +				pr_err("Disk %s is not used in first volume, aborting\n",
> +				       disk->devname);
> +				return IMSM_STATUS_ERROR;
> +			}
> +			disk->raiddisk = _slot;
> +		}
> +		curr_slot++;
> 	}
> 
> -	return rv;
> +	return IMSM_STATUS_OK;
> }
> 
> static int validate_geometry_imsm(struct supertype *st, int level, int layout,
> @@ -7625,35 +7671,35 @@ static int validate_geometry_imsm(struct supertype *st, int level, int layout,
> 	}
> 
> 	if (!dev) {
> -		if (st->sb) {
> -			struct intel_super *super = st->sb;
> -			if (!validate_geometry_imsm_orom(st->sb, level, layout,
> -							 raiddisks, chunk, size,
> -							 verbose))
> +		struct intel_super *super = st->sb;
> +
> +		/*
> +		 * Autolayout mode, st->sb and freesize must be set.
> +		 */
> +		if (!super || !freesize) {
> +			pr_vrb("freesize and superblock must be set for autolayout, aborting\n");
> +			return 1;
> +		}
> +
> +		if (!validate_geometry_imsm_orom(st->sb, level, layout,
> +						 raiddisks, chunk, size,
> +						 verbose))
> +			return 0;
> +
> +		if (super->orom) {
> +			imsm_status_t rv;
> +			int count = count_volumes(super->hba, super->orom->dpa,
> +					      verbose);
> +			if (super->orom->vphba <= count) {
> +				pr_vrb("platform does not support more than %d raid volumes.\n",
> +				       super->orom->vphba);
> 				return 0;
> -			/* we are being asked to automatically layout a
> -			 * new volume based on the current contents of
> -			 * the container.  If the the parameters can be
> -			 * satisfied reserve_space will record the disks,
> -			 * start offset, and size of the volume to be
> -			 * created.  add_to_super and getinfo_super
> -			 * detect when autolayout is in progress.
> -			 */
> -			/* assuming that freesize is always given when array is
> -			   created */
> -			if (super->orom && freesize) {
> -				int count;
> -				count = count_volumes(super->hba,
> -						      super->orom->dpa, verbose);
> -				if (super->orom->vphba <= count) {
> -					pr_vrb("platform does not support more than %d raid volumes.\n",
> -					       super->orom->vphba);
> -					return 0;
> -				}
> 			}
> -			if (freesize)
> -				return reserve_space(st, raiddisks, size,
> -						     *chunk, freesize);
> +
> +			rv = autolayout_imsm(super, raiddisks, size, *chunk,
> +					     freesize);
> +			if (rv != IMSM_STATUS_OK)
> +				return 0;
> 		}
> 		return 1;
> 	}
> @@ -11524,7 +11570,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
> 	unsigned long long current_size;
> 	unsigned long long free_size;
> 	unsigned long long max_size;
> -	int rv;
> +	imsm_status_t rv;
> 
> 	getinfo_super_imsm_volume(st, &info, NULL);
> 	if (geo->level != info.array.level && geo->level >= 0 &&
> @@ -11643,9 +11689,10 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
> 		}
> 		/* check the maximum available size
> 		 */
> -		rv =  imsm_get_free_size(st, dev->vol.map->num_members,
> -					 0, chunk, &free_size);
> -		if (rv == 0)
> +		rv = imsm_get_free_size(super, dev->vol.map->num_members,
> +					0, chunk, &free_size);
> +
> +		if (rv != IMSM_STATUS_OK)
> 			/* Cannot find maximum available space
> 			 */
> 			max_size = 0;
> -- 
> 2.26.2
> 

[PATCH 3/3 v2] imsm: block changing slots during creation

[Mariusz Tkaczyk]

If user specifies drives for array creation, then slot order across
volumes is not preserved.
Ideally, it should be checked in validate_geometry() but it is not
possible in current implementation (order is determined later).
Add verification in add_to_super_imsm_volume() and throw error if
mismatch is detected.
IMSM allows to use only same members within container.
This is not hardware dependency but metadata limitation.
Therefore, 09-imsm-overlap test is removed. Testing it is pointless.
After this patch, creation in this scenario is blocked. Offset
verification is covered in other tests.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 super-intel.c        | 33 ++++++++++++++++++++++-----------
 tests/09imsm-overlap | 28 ----------------------------
 2 files changed, 22 insertions(+), 39 deletions(-)
 delete mode 100644 tests/09imsm-overlap

diff --git a/super-intel.c b/super-intel.c
index 3c02d2f6..053f7e7e 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5760,6 +5760,10 @@ static int add_to_super_imsm_volume(struct supertype *st, mdu_disk_info_t *dk,
 	struct imsm_map *map;
 	struct dl *dl, *df;
 	int slot;
+	int autolayout = 0;
+
+	if (!is_fd_valid(fd))
+		autolayout = 1;
 
 	dev = get_imsm_dev(super, super->current_vol);
 	map = get_imsm_map(dev, MAP_0);
@@ -5770,25 +5774,32 @@ static int add_to_super_imsm_volume(struct supertype *st, mdu_disk_info_t *dk,
 		return 1;
 	}
 
-	if (!is_fd_valid(fd)) {
-		/* we're doing autolayout so grab the pre-marked (in
-		 * validate_geometry) raid_disk
-		 */
-		for (dl = super->disks; dl; dl = dl->next)
+	for (dl = super->disks; dl ; dl = dl->next) {
+		if (autolayout) {
 			if (dl->raiddisk == dk->raid_disk)
 				break;
-	} else {
-		for (dl = super->disks; dl ; dl = dl->next)
-			if (dl->major == dk->major &&
-			    dl->minor == dk->minor)
-				break;
+		} else if (dl->major == dk->major && dl->minor == dk->minor)
+			break;
 	}
 
 	if (!dl) {
-		pr_err("%s is not a member of the same container\n", devname);
+		if (!autolayout)
+			pr_err("%s is not a member of the same container.\n",
+			       devname);
 		return 1;
 	}
 
+	if (!autolayout && super->current_vol > 0) {
+		int _slot = get_disk_slot_in_dev(super, 0, dl->index);
+
+		if (_slot != dk->raid_disk) {
+			pr_err("Member %s is in %d slot for the first volume, but is in %d slot for a new volume.\n",
+			       dl->devname, _slot, dk->raid_disk);
+			pr_err("Raid members are in different order than for the first volume, aborting.\n");
+			return 1;
+		}
+	}
+
 	if (mpb->num_disks == 0)
 		if (!get_dev_sector_size(dl->fd, dl->devname,
 					 &super->sector_size))
diff --git a/tests/09imsm-overlap b/tests/09imsm-overlap
deleted file mode 100644
index ff5d2093..00000000
--- a/tests/09imsm-overlap
+++ /dev/null
@@ -1,28 +0,0 @@
-
-. tests/env-imsm-template
-
-# create raid arrays with varying degress of overlap
-mdadm -CR $container -e imsm -n 6 $dev0 $dev1 $dev2 $dev3 $dev4 $dev5
-imsm_check container 6
-
-size=1024
-level=1
-num_disks=2
-mdadm -CR $member0 $dev0 $dev1 -n $num_disks -l $level -z $size
-mdadm -CR $member1 $dev1 $dev2 -n $num_disks -l $level -z $size
-mdadm -CR $member2 $dev2 $dev3 -n $num_disks -l $level -z $size
-mdadm -CR $member3 $dev3 $dev4 -n $num_disks -l $level -z $size
-mdadm -CR $member4 $dev4 $dev5 -n $num_disks -l $level -z $size
-
-udevadm settle
-
-offset=0
-imsm_check member $member0 $num_disks $level $size 1024 $offset
-offset=$((offset+size+4096))
-imsm_check member $member1 $num_disks $level $size 1024 $offset
-offset=$((offset+size+4096))
-imsm_check member $member2 $num_disks $level $size 1024 $offset
-offset=$((offset+size+4096))
-imsm_check member $member3 $num_disks $level $size 1024 $offset
-offset=$((offset+size+4096))
-imsm_check member $member4 $num_disks $level $size 1024 $offset
-- 
2.26.2

Re: [PATCH 3/3 v2] imsm: block changing slots during creation

[Coly Li]

> 2022年5月31日 18:27，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道：
> 
> If user specifies drives for array creation, then slot order across
> volumes is not preserved.
> Ideally, it should be checked in validate_geometry() but it is not
> possible in current implementation (order is determined later).
> Add verification in add_to_super_imsm_volume() and throw error if
> mismatch is detected.
> IMSM allows to use only same members within container.
> This is not hardware dependency but metadata limitation.
> Therefore, 09-imsm-overlap test is removed. Testing it is pointless.
> After this patch, creation in this scenario is blocked. Offset
> verification is covered in other tests.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Acked-by: Coly Li <colyli@suse.de>


Coly Li


> ---
> super-intel.c        | 33 ++++++++++++++++++++++-----------
> tests/09imsm-overlap | 28 ----------------------------
> 2 files changed, 22 insertions(+), 39 deletions(-)
> delete mode 100644 tests/09imsm-overlap
> 
> diff --git a/super-intel.c b/super-intel.c
> index 3c02d2f6..053f7e7e 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5760,6 +5760,10 @@ static int add_to_super_imsm_volume(struct supertype *st, mdu_disk_info_t *dk,
> 	struct imsm_map *map;
> 	struct dl *dl, *df;
> 	int slot;
> +	int autolayout = 0;
> +
> +	if (!is_fd_valid(fd))
> +		autolayout = 1;
> 
> 	dev = get_imsm_dev(super, super->current_vol);
> 	map = get_imsm_map(dev, MAP_0);
> @@ -5770,25 +5774,32 @@ static int add_to_super_imsm_volume(struct supertype *st, mdu_disk_info_t *dk,
> 		return 1;
> 	}
> 
> -	if (!is_fd_valid(fd)) {
> -		/* we're doing autolayout so grab the pre-marked (in
> -		 * validate_geometry) raid_disk
> -		 */
> -		for (dl = super->disks; dl; dl = dl->next)
> +	for (dl = super->disks; dl ; dl = dl->next) {
> +		if (autolayout) {
> 			if (dl->raiddisk == dk->raid_disk)
> 				break;
> -	} else {
> -		for (dl = super->disks; dl ; dl = dl->next)
> -			if (dl->major == dk->major &&
> -			    dl->minor == dk->minor)
> -				break;
> +		} else if (dl->major == dk->major && dl->minor == dk->minor)
> +			break;
> 	}
> 
> 	if (!dl) {
> -		pr_err("%s is not a member of the same container\n", devname);
> +		if (!autolayout)
> +			pr_err("%s is not a member of the same container.\n",
> +			       devname);
> 		return 1;
> 	}
> 
> +	if (!autolayout && super->current_vol > 0) {
> +		int _slot = get_disk_slot_in_dev(super, 0, dl->index);
> +
> +		if (_slot != dk->raid_disk) {
> +			pr_err("Member %s is in %d slot for the first volume, but is in %d slot for a new volume.\n",
> +			       dl->devname, _slot, dk->raid_disk);
> +			pr_err("Raid members are in different order than for the first volume, aborting.\n");
> +			return 1;
> +		}
> +	}
> +
> 	if (mpb->num_disks == 0)
> 		if (!get_dev_sector_size(dl->fd, dl->devname,
> 					 &super->sector_size))
> diff --git a/tests/09imsm-overlap b/tests/09imsm-overlap
> deleted file mode 100644
> index ff5d2093..00000000
> --- a/tests/09imsm-overlap
> +++ /dev/null
> @@ -1,28 +0,0 @@
> -
> -. tests/env-imsm-template
> -
> -# create raid arrays with varying degress of overlap
> -mdadm -CR $container -e imsm -n 6 $dev0 $dev1 $dev2 $dev3 $dev4 $dev5
> -imsm_check container 6
> -
> -size=1024
> -level=1
> -num_disks=2
> -mdadm -CR $member0 $dev0 $dev1 -n $num_disks -l $level -z $size
> -mdadm -CR $member1 $dev1 $dev2 -n $num_disks -l $level -z $size
> -mdadm -CR $member2 $dev2 $dev3 -n $num_disks -l $level -z $size
> -mdadm -CR $member3 $dev3 $dev4 -n $num_disks -l $level -z $size
> -mdadm -CR $member4 $dev4 $dev5 -n $num_disks -l $level -z $size
> -
> -udevadm settle
> -
> -offset=0
> -imsm_check member $member0 $num_disks $level $size 1024 $offset
> -offset=$((offset+size+4096))
> -imsm_check member $member1 $num_disks $level $size 1024 $offset
> -offset=$((offset+size+4096))
> -imsm_check member $member2 $num_disks $level $size 1024 $offset
> -offset=$((offset+size+4096))
> -imsm_check member $member3 $num_disks $level $size 1024 $offset
> -offset=$((offset+size+4096))
> -imsm_check member $member4 $num_disks $level $size 1024 $offset
> -- 
> 2.26.2
> 

Re: [PATCH 0/3 v2] IMSM autolayout improvements

[Coly Li]

> 2022年5月31日 18:27，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道：
> 
> Following patchset modifies some parts of IMSM creation to ensure that
> member's order is always same. It is ISM metadata requirement.
> 
> Additionally, as discussed with Jes I've started to implement more modern error
> handling, by adding special enum for IMSM. Will be great to hear any comments
> and opinion.
> 
> V2 improvements:
> - typedef added in first patch
> - changed order in enum
> 
> Mariusz Tkaczyk (3):
>  imsm: introduce get_disk_slot_in_dev()
>  imsm: use same slot across container
>  imsm: block changing slots during creation

These patches look fine to me and I ack them all. Next step I will apply them into mdadm-CI 20220406-testing branch (not yet now) for further testing.

Thanks for the update.

Coly Li