[PATCH 0/6] imsm: expand improvements

[PATCH 0/6] imsm: expand improvements

[Mariusz Tkaczyk]

merge_extents() was initially designed to support creation only. Expand
feature was added later and the current code was not updated properly.
Now, we can see two issues:
1. --size=max used with expand and create result in different array size.
2. In scenarios, where volume were deleted an recreated it may not be
possible to expand the volume.

The patchset addresses listed issues and removes limitation to the last
volume for expand.

Mariusz Tkaczyk (6):
  imsm: move sum_extents calculations to merge_extents()
  imsm: imsm_get_free_size() refactor.
  imsm: introduce round_member_size_to_mb()
  imsm: move expand verification code into new function
  imsm: return free space after volume for expand
  imsm: fix free space calculations

 super-intel.c | 363 ++++++++++++++++++++++++++++----------------------
 1 file changed, 202 insertions(+), 161 deletions(-)

-- 
2.26.2

[PATCH 1/6] imsm: move sum_extents calculations to merge_extents()

[Mariusz Tkaczyk]

This logic is only used by merge_extents() code, there is no need to pass
it as parameter. Move it up. Add proper description.

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

diff --git a/super-intel.c b/super-intel.c
index 8ffe485c..81d6ecd9 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6824,21 +6824,31 @@ static unsigned long long find_size(struct extent *e, int *idx, int num_extents)
 	return end - base_start;
 }
 
-static unsigned long long merge_extents(struct intel_super *super, int sum_extents)
+/** merge_extents() - analyze extents and get max common free size.
+ * @super: Intel metadata, not NULL.
+ *
+ * Build a composite disk with all known extents and generate a new maxsize
+ * given the "all disks in an array must share a common start offset"
+ * constraint.
+ *
+ * Return: Max free space or 0 on failure.
+ */
+static unsigned long long merge_extents(struct intel_super *super)
 {
-	/* build a composite disk with all known extents and generate a new
-	 * 'maxsize' given the "all disks in an array must share a common start
-	 * offset" constraint
-	 */
-	struct extent *e = xcalloc(sum_extents, sizeof(*e));
+	struct extent *e;
 	struct dl *dl;
 	int i, j;
-	int start_extent;
+	int start_extent, sum_extents = 0;
 	unsigned long long pos;
 	unsigned long long start = 0;
 	unsigned long long maxsize;
 	unsigned long reserve;
 
+	for (dl = super->disks; dl; dl = dl->next)
+		if (dl->e)
+			sum_extents += dl->extent_cnt;
+	e = xcalloc(sum_extents, sizeof(struct extent));
+
 	/* coalesce and sort all extents. also, check to see if we need to
 	 * reserve space between member arrays
 	 */
@@ -7497,13 +7507,7 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
 		return 0;
 	}
 
-	/* count total number of extents for merge */
-	i = 0;
-	for (dl = super->disks; dl; dl = dl->next)
-		if (dl->e)
-			i += dl->extent_cnt;
-
-	maxsize = merge_extents(super, i);
+	maxsize = merge_extents(super);
 
 	if (mpb->num_raid_devs > 0 && size && size != maxsize)
 		pr_err("attempting to create a second volume with size less then remaining space.\n");
@@ -7557,7 +7561,6 @@ static imsm_status_t imsm_get_free_size(struct intel_super *super,
 	struct imsm_super *mpb = super->anchor;
 	struct dl *dl;
 	int i;
-	int extent_cnt;
 	struct extent *e;
 	unsigned long long maxsize;
 	unsigned long long minsize;
@@ -7566,7 +7569,6 @@ static imsm_status_t imsm_get_free_size(struct intel_super *super,
 
 	/* find the largest common start free region of the possible disks */
 	used = 0;
-	extent_cnt = 0;
 	cnt = 0;
 	for (dl = super->disks; dl; dl = dl->next) {
 		dl->raiddisk = -1;
@@ -7587,11 +7589,10 @@ static imsm_status_t imsm_get_free_size(struct intel_super *super,
 			;
 		dl->e = e;
 		dl->extent_cnt = i;
-		extent_cnt += i;
 		cnt++;
 	}
 
-	maxsize = merge_extents(super, extent_cnt);
+	maxsize = merge_extents(super);
 	minsize = size;
 	if (size == 0)
 		/* chunk is in K */
-- 
2.26.2

[PATCH 2/6] imsm: imsm_get_free_size() refactor.

[Mariusz Tkaczyk]

Move minsize calculations up. Add error message if free size is too small.

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

diff --git a/super-intel.c b/super-intel.c
index 81d6ecd9..3cbab545 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -7542,7 +7542,7 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
  * @super: &intel_super pointer, not NULL.
  * @raiddisks: number of raid disks.
  * @size: requested size, could be 0 (means max size).
- * @chunk: requested chunk.
+ * @chunk: requested chunk size in KiB.
  * @freesize: pointer for returned size value.
  *
  * Return: &IMSM_STATUS_OK or &IMSM_STATUS_ERROR.
@@ -7562,14 +7562,15 @@ static imsm_status_t imsm_get_free_size(struct intel_super *super,
 	struct dl *dl;
 	int i;
 	struct extent *e;
+	int cnt = 0;
+	int used = 0;
 	unsigned long long maxsize;
-	unsigned long long minsize;
-	int cnt;
-	int used;
+	unsigned long long minsize = size;
+
+	if (minsize == 0)
+		minsize = chunk * 2;
 
 	/* find the largest common start free region of the possible disks */
-	used = 0;
-	cnt = 0;
 	for (dl = super->disks; dl; dl = dl->next) {
 		dl->raiddisk = -1;
 
@@ -7593,14 +7594,14 @@ static imsm_status_t imsm_get_free_size(struct intel_super *super,
 	}
 
 	maxsize = merge_extents(super);
-	minsize = size;
-	if (size == 0)
-		/* chunk is in K */
-		minsize = chunk * 2;
+	if (maxsize < minsize)  {
+		pr_err("imsm: Free space is %llu but must be equal or larger than %llu.\n",
+		       maxsize, minsize);
+		return IMSM_STATUS_ERROR;
+	}
 
-	if (cnt < raiddisks || (super->orom && used && used != raiddisks) ||
-	    maxsize < minsize || maxsize == 0) {
-		pr_err("not enough devices with space to create array.\n");
+	if (cnt < raiddisks || (super->orom && used && used != raiddisks)) {
+		pr_err("imsm: Not enough devices with space to create array.\n");
 		return IMSM_STATUS_ERROR;
 	}
 
-- 
2.26.2

[PATCH 3/6] imsm: introduce round_member_size_to_mb()

[Mariusz Tkaczyk]

Extract rounding logic to separate function.

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

diff --git a/super-intel.c b/super-intel.c
index 3cbab545..2351ce20 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1599,17 +1599,29 @@ static int is_journal(struct imsm_disk *disk)
 	return (disk->status & JOURNAL_DISK) == JOURNAL_DISK;
 }
 
-/* round array size down to closest MB and ensure it splits evenly
- * between members
+/**
+ * round_member_size_to_mb()- Round given size to closest MiB.
+ * @size: size to round in sectors.
  */
-static unsigned long long round_size_to_mb(unsigned long long size, unsigned int
-					   disk_count)
+static inline unsigned long long round_member_size_to_mb(unsigned long long size)
 {
-	size /= disk_count;
-	size = (size >> SECT_PER_MB_SHIFT) << SECT_PER_MB_SHIFT;
-	size *= disk_count;
+	return (size >> SECT_PER_MB_SHIFT) << SECT_PER_MB_SHIFT;
+}
 
-	return size;
+/**
+ * round_size_to_mb()- Round given size.
+ * @array_size: size to round in sectors.
+ * @disk_count: count of data members.
+ *
+ * Get size per each data member and round it to closest MiB to ensure that data
+ * splits evenly between members.
+ *
+ * Return: Array size, rounded down.
+ */
+static inline unsigned long long round_size_to_mb(unsigned long long array_size,
+						  unsigned int disk_count)
+{
+	return round_member_size_to_mb(array_size / disk_count) * disk_count;
 }
 
 static int able_to_resync(int raid_level, int missing_disks)
@@ -11749,8 +11761,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 		} else {
 			/* round size due to metadata compatibility
 			*/
-			geo->size = (geo->size >> SECT_PER_MB_SHIFT)
-				    << SECT_PER_MB_SHIFT;
+			geo->size = round_member_size_to_mb(geo->size);
 			dprintf("Prepare update for size change to %llu\n",
 				geo->size );
 			if (current_size >= geo->size) {
-- 
2.26.2

[PATCH 4/6] imsm: move expand verification code into new function

[Mariusz Tkaczyk]

The code here is too complex. Move it to separate function and
simplify it. Add more error messages.

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

diff --git a/super-intel.c b/super-intel.c
index 2351ce20..83bf2bfc 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -11582,6 +11582,102 @@ static void imsm_update_metadata_locally(struct supertype *st,
 	}
 }
 
+/**
+ * imsm_analyze_expand() - check expand properties and calculate new size.
+ * @st: imsm supertype.
+ * @geo: new geometry params.
+ * @array: array info.
+ * @direction: reshape direction.
+ *
+ * Obtain free space after the &array and verify if expand to requested size is
+ * possible. If geo->size is set to %MAX_SIZE, assume that max free size is
+ * requested.
+ *
+ * Return:
+ * On success %IMSM_STATUS_OK is returned, geo->size and geo->raid_disks are
+ * updated.
+ * On error, %IMSM_STATUS_ERROR is returned.
+ */
+static imsm_status_t imsm_analyze_expand(struct supertype *st,
+					 struct geo_params *geo,
+					 struct mdinfo *array,
+					 int direction)
+{
+	struct intel_super *super = st->sb;
+	struct imsm_dev *dev = get_imsm_dev(super, super->current_vol);
+	struct imsm_map *map = get_imsm_map(dev, MAP_0);
+	int data_disks = imsm_num_data_members(map);
+
+	unsigned long long current_size;
+	unsigned long long free_size;
+	unsigned long long new_size;
+	unsigned long long max_size;
+
+	const int chunk_kib = geo->chunksize / 1024;
+	imsm_status_t rv;
+
+	if (direction == ROLLBACK_METADATA_CHANGES) {
+		/**
+		 * Accept size for rollback only.
+		 */
+		new_size = geo->size * 2;
+		goto success;
+	}
+
+	if (super->current_vol + 1 != super->anchor->num_raid_devs) {
+		pr_err("imsm: The last volume in container can be expanded only (%i/%s).\n",
+		       super->current_vol, st->devnm);
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (data_disks == 0) {
+		pr_err("imsm: Cannot retrieve data disks.\n");
+		return IMSM_STATUS_ERROR;
+	}
+	current_size = array->custom_array_size / data_disks;
+
+	rv = imsm_get_free_size(super, dev->vol.map->num_members, 0, chunk_kib, &free_size);
+	if (rv != IMSM_STATUS_OK) {
+		pr_err("imsm: Cannot find free space for expand.\n");
+		return IMSM_STATUS_ERROR;
+	}
+	max_size = round_member_size_to_mb(free_size + current_size);
+
+	if (geo->size == MAX_SIZE)
+		new_size = max_size;
+	else
+		new_size = round_member_size_to_mb(geo->size * 2);
+
+	if (new_size == 0) {
+		pr_err("imsm: Rounded requested size is 0.\n");
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (new_size > max_size) {
+		pr_err("imsm: Rounded requested size (%llu) is larger than free space available (%llu).\n",
+		       new_size, max_size);
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (new_size == current_size) {
+		pr_err("imsm: Rounded requested size (%llu) is same as current size (%llu).\n",
+		       new_size, current_size);
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (new_size < current_size) {
+		pr_err("imsm: Size reduction is not supported, rounded requested size (%llu) is smaller than current (%llu).\n",
+		       new_size, current_size);
+		return IMSM_STATUS_ERROR;
+	}
+
+success:
+	dprintf("imsm: New size per member is %llu.\n", new_size);
+	geo->size = data_disks * new_size;
+	geo->raid_disks = dev->vol.map->num_members;
+	return IMSM_STATUS_OK;
+}
+
 /***************************************************************************
 * Function:	imsm_analyze_change
 * Description:	Function analyze change for single volume
@@ -11602,13 +11698,6 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 	int devNumChange = 0;
 	/* imsm compatible layout value for array geometry verification */
 	int imsm_layout = -1;
-	int data_disks;
-	struct imsm_dev *dev;
-	struct imsm_map *map;
-	struct intel_super *super;
-	unsigned long long current_size;
-	unsigned long long free_size;
-	unsigned long long max_size;
 	imsm_status_t rv;
 
 	getinfo_super_imsm_volume(st, &info, NULL);
@@ -11691,94 +11780,20 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 		geo->chunksize = info.array.chunk_size;
 	}
 
-	chunk = geo->chunksize / 1024;
-
-	super = st->sb;
-	dev = get_imsm_dev(super, super->current_vol);
-	map = get_imsm_map(dev, MAP_0);
-	data_disks = imsm_num_data_members(map);
-	/* compute current size per disk member
-	 */
-	current_size = info.custom_array_size / data_disks;
-
-	if (geo->size > 0 && geo->size != MAX_SIZE) {
-		/* align component size
-		 */
-		geo->size = imsm_component_size_alignment_check(
-				    get_imsm_raid_level(dev->vol.map),
-				    chunk * 1024, super->sector_size,
-				    geo->size * 2);
-		if (geo->size == 0) {
-			pr_err("Error. Size expansion is supported only (current size is %llu, requested size /rounded/ is 0).\n",
-				   current_size);
-			goto analyse_change_exit;
-		}
-	}
-
-	if (current_size != geo->size && geo->size > 0) {
+	if (geo->size > 0) {
 		if (change != -1) {
 			pr_err("Error. Size change should be the only one at a time.\n");
 			change = -1;
 			goto analyse_change_exit;
 		}
-		if ((super->current_vol + 1) != super->anchor->num_raid_devs) {
-			pr_err("Error. The last volume in container can be expanded only (%i/%s).\n",
-			       super->current_vol, st->devnm);
-			goto analyse_change_exit;
-		}
-		/* check the maximum available size
-		 */
-		rv = imsm_get_free_size(super, dev->vol.map->num_members,
-					0, chunk, &free_size);
 
+		rv = imsm_analyze_expand(st, geo, &info, direction);
 		if (rv != IMSM_STATUS_OK)
-			/* Cannot find maximum available space
-			 */
-			max_size = 0;
-		else {
-			max_size = free_size + current_size;
-			/* align component size
-			 */
-			max_size = imsm_component_size_alignment_check(
-					get_imsm_raid_level(dev->vol.map),
-					chunk * 1024, super->sector_size,
-					max_size);
-		}
-		if (geo->size == MAX_SIZE) {
-			/* requested size change to the maximum available size
-			 */
-			if (max_size == 0) {
-				pr_err("Error. Cannot find maximum available space.\n");
-				change = -1;
-				goto analyse_change_exit;
-			} else
-				geo->size = max_size;
-		}
-
-		if (direction == ROLLBACK_METADATA_CHANGES) {
-			/* accept size for rollback only
-			*/
-		} else {
-			/* round size due to metadata compatibility
-			*/
-			geo->size = round_member_size_to_mb(geo->size);
-			dprintf("Prepare update for size change to %llu\n",
-				geo->size );
-			if (current_size >= geo->size) {
-				pr_err("Error. Size expansion is supported only (current size is %llu, requested size /rounded/ is %llu).\n",
-				       current_size, geo->size);
-				goto analyse_change_exit;
-			}
-			if (max_size && geo->size > max_size) {
-				pr_err("Error. Requested size is larger than maximum available size (maximum available size is %llu, requested size /rounded/ is %llu).\n",
-				       max_size, geo->size);
-				goto analyse_change_exit;
-			}
-		}
-		geo->size *= data_disks;
-		geo->raid_disks = dev->vol.map->num_members;
+			goto analyse_change_exit;
 		change = CH_ARRAY_SIZE;
 	}
+
+	chunk = geo->chunksize / 1024;
 	if (!validate_geometry_imsm(st,
 				    geo->level,
 				    imsm_layout,
-- 
2.26.2

[PATCH 5/6] imsm: return free space after volume for expand

[Mariusz Tkaczyk]

merge_extends() routine searches for the biggest free space. For expand,
it works only in standard cases where the last volume is expanded and
the free space is determined after the last volume.
Add volume index to extent struct and use that do determine size after
super->current_vol during expand.

Limitation to last volume is no longer needed. It unblocks scenarios
where kill-subarray is used to remove first volume and later it is
recreated (now it is the second volume, even if it is placed before
existing one).

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

diff --git a/super-intel.c b/super-intel.c
index 83bf2bfc..1559c837 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -498,8 +498,15 @@ struct intel_disk {
 	struct intel_disk *next;
 };
 
+/**
+ * struct extent - reserved space details.
+ * @start: start offset.
+ * @size: size of reservation, set to 0 for metadata reservation.
+ * @vol: index of the volume, meaningful if &size is set.
+ */
 struct extent {
 	unsigned long long start, size;
+	int vol;
 };
 
 /* definitions of reshape process types */
@@ -1494,9 +1501,10 @@ static struct extent *get_extents(struct intel_super *super, struct dl *dl,
 				  int get_minimal_reservation)
 {
 	/* find a list of used extents on the given physical device */
-	struct extent *rv, *e;
-	int i;
 	int memberships = count_memberships(dl, super);
+	struct extent *rv = xcalloc(memberships + 1, sizeof(struct extent));
+	struct extent *e = rv;
+	int i;
 	__u32 reservation;
 
 	/* trim the reserved area for spares, so they can join any array
@@ -1508,9 +1516,6 @@ static struct extent *get_extents(struct intel_super *super, struct dl *dl,
 	else
 		reservation = MPB_SECTOR_CNT + IMSM_RESERVED_SECTORS;
 
-	rv = xcalloc(sizeof(struct extent), (memberships + 1));
-	e = rv;
-
 	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);
@@ -1518,6 +1523,7 @@ static struct extent *get_extents(struct intel_super *super, struct dl *dl,
 		if (get_imsm_disk_slot(map, dl->index) >= 0) {
 			e->start = pba_of_lba0(map);
 			e->size = per_dev_array_size(map);
+			e->vol = i;
 			e++;
 		}
 	}
@@ -6836,24 +6842,26 @@ static unsigned long long find_size(struct extent *e, int *idx, int num_extents)
 	return end - base_start;
 }
 
-/** merge_extents() - analyze extents and get max common free size.
+/** merge_extents() - analyze extents and get free size.
  * @super: Intel metadata, not NULL.
+ * @expanding: if set, we are expanding &super->current_vol.
  *
- * Build a composite disk with all known extents and generate a new maxsize
- * given the "all disks in an array must share a common start offset"
- * constraint.
+ * Build a composite disk with all known extents and generate a size given the
+ * "all disks in an array must share a common start offset" constraint.
+ * If a volume is expanded, then return free space after the volume.
  *
- * Return: Max free space or 0 on failure.
+ * Return: Free space or 0 on failure.
  */
-static unsigned long long merge_extents(struct intel_super *super)
+static unsigned long long merge_extents(struct intel_super *super, const bool expanding)
 {
 	struct extent *e;
 	struct dl *dl;
-	int i, j;
-	int start_extent, sum_extents = 0;
-	unsigned long long pos;
+	int i, j, pos_vol_idx = -1;
+	int extent_idx = 0;
+	int sum_extents = 0;
+	unsigned long long pos = 0;
 	unsigned long long start = 0;
-	unsigned long long maxsize;
+	unsigned long long maxsize = 0;
 	unsigned long reserve;
 
 	for (dl = super->disks; dl; dl = dl->next)
@@ -6878,26 +6886,26 @@ static unsigned long long merge_extents(struct intel_super *super)
 	j = 0;
 	while (i < sum_extents) {
 		e[j].start = e[i].start;
+		e[j].vol = e[i].vol;
 		e[j].size = find_size(e, &i, sum_extents);
 		j++;
 		if (e[j-1].size == 0)
 			break;
 	}
 
-	pos = 0;
-	maxsize = 0;
-	start_extent = 0;
 	i = 0;
 	do {
-		unsigned long long esize;
+		unsigned long long esize = e[i].start - pos;
 
-		esize = e[i].start - pos;
-		if (esize >= maxsize) {
+		if (expanding ? pos_vol_idx == super->current_vol : esize >= maxsize) {
 			maxsize = esize;
 			start = pos;
-			start_extent = i;
+			extent_idx = i;
 		}
+
 		pos = e[i].start + e[i].size;
+		pos_vol_idx = e[i].vol;
+
 		i++;
 	} while (e[i-1].size);
 	free(e);
@@ -6908,7 +6916,7 @@ static unsigned long long merge_extents(struct intel_super *super)
 	/* FIXME assumes volume at offset 0 is the first volume in a
 	 * container
 	 */
-	if (start_extent > 0)
+	if (extent_idx > 0)
 		reserve = IMSM_RESERVED_SECTORS; /* gap between raid regions */
 	else
 		reserve = 0;
@@ -7519,7 +7527,7 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
 		return 0;
 	}
 
-	maxsize = merge_extents(super);
+	maxsize = merge_extents(super, false);
 
 	if (mpb->num_raid_devs > 0 && size && size != maxsize)
 		pr_err("attempting to create a second volume with size less then remaining space.\n");
@@ -7568,7 +7576,8 @@ 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)
+					unsigned long long *freesize,
+					bool expanding)
 {
 	struct imsm_super *mpb = super->anchor;
 	struct dl *dl;
@@ -7605,7 +7614,7 @@ static imsm_status_t imsm_get_free_size(struct intel_super *super,
 		cnt++;
 	}
 
-	maxsize = merge_extents(super);
+	maxsize = merge_extents(super, expanding);
 	if (maxsize < minsize)  {
 		pr_err("imsm: Free space is %llu but must be equal or larger than %llu.\n",
 		       maxsize, minsize);
@@ -7663,7 +7672,7 @@ static imsm_status_t autolayout_imsm(struct intel_super *super,
 	int vol_cnt = super->anchor->num_raid_devs;
 	imsm_status_t rv;
 
-	rv = imsm_get_free_size(super, raiddisks, size, chunk, freesize);
+	rv = imsm_get_free_size(super, raiddisks, size, chunk, freesize, false);
 	if (rv != IMSM_STATUS_OK)
 		return IMSM_STATUS_ERROR;
 
@@ -11624,19 +11633,13 @@ static imsm_status_t imsm_analyze_expand(struct supertype *st,
 		goto success;
 	}
 
-	if (super->current_vol + 1 != super->anchor->num_raid_devs) {
-		pr_err("imsm: The last volume in container can be expanded only (%i/%s).\n",
-		       super->current_vol, st->devnm);
-		return IMSM_STATUS_ERROR;
-	}
-
 	if (data_disks == 0) {
 		pr_err("imsm: Cannot retrieve data disks.\n");
 		return IMSM_STATUS_ERROR;
 	}
 	current_size = array->custom_array_size / data_disks;
 
-	rv = imsm_get_free_size(super, dev->vol.map->num_members, 0, chunk_kib, &free_size);
+	rv = imsm_get_free_size(super, dev->vol.map->num_members, 0, chunk_kib, &free_size, true);
 	if (rv != IMSM_STATUS_OK) {
 		pr_err("imsm: Cannot find free space for expand.\n");
 		return IMSM_STATUS_ERROR;
-- 
2.26.2

[PATCH 6/6] imsm: fix free space calculations

[Mariusz Tkaczyk]

Between two volumes or between last volume and metadata at least
IMSM_RESERVED_SECTORS gap must exist. Currently the gap can be doubled
because metadata reservation contains IMSM_RESERVED_SECTORS too.

Divide reserve variable into pre_reservation and post_reservation to be
more flexible and decide separately if each reservation is needed.

Pre_reservation is needed only when a volume is created and it is not a
real first volume in a container (we can check that by extent_idx).
This type of reservation is not needed for expand.

Post_reservation is not needed only if real last volume is created or
expanded because reservation is done with the metadata.

The volume index in metadata cannot be trusted, because the real volume
order can be reversed. It is safer to use extent table, it is sorted by
start position.

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

diff --git a/super-intel.c b/super-intel.c
index 1559c837..c012b220 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6861,8 +6861,11 @@ static unsigned long long merge_extents(struct intel_super *super, const bool ex
 	int sum_extents = 0;
 	unsigned long long pos = 0;
 	unsigned long long start = 0;
-	unsigned long long maxsize = 0;
-	unsigned long reserve;
+	unsigned long long free_size = 0;
+
+	unsigned long pre_reservation = 0;
+	unsigned long post_reservation = IMSM_RESERVED_SECTORS;
+	unsigned long reservation_size;
 
 	for (dl = super->disks; dl; dl = dl->next)
 		if (dl->e)
@@ -6897,8 +6900,8 @@ static unsigned long long merge_extents(struct intel_super *super, const bool ex
 	do {
 		unsigned long long esize = e[i].start - pos;
 
-		if (expanding ? pos_vol_idx == super->current_vol : esize >= maxsize) {
-			maxsize = esize;
+		if (expanding ? pos_vol_idx == super->current_vol : esize >= free_size) {
+			free_size = esize;
 			start = pos;
 			extent_idx = i;
 		}
@@ -6908,28 +6911,35 @@ static unsigned long long merge_extents(struct intel_super *super, const bool ex
 
 		i++;
 	} while (e[i-1].size);
-	free(e);
 
-	if (maxsize == 0)
+	if (free_size == 0) {
+		dprintf("imsm: Cannot find free size.\n");
+		free(e);
 		return 0;
+	}
 
-	/* FIXME assumes volume at offset 0 is the first volume in a
-	 * container
-	 */
-	if (extent_idx > 0)
-		reserve = IMSM_RESERVED_SECTORS; /* gap between raid regions */
-	else
-		reserve = 0;
+	if (!expanding && extent_idx != 0)
+		/*
+		 * Not a real first volume in a container is created, pre_reservation is needed.
+		 */
+		pre_reservation = IMSM_RESERVED_SECTORS;
 
-	if (maxsize < reserve)
-		return 0;
+	if (e[extent_idx].size == 0)
+		/*
+		 * extent_idx points to the metadata, post_reservation is allready done.
+		 */
+		post_reservation = 0;
+	free(e);
 
-	super->create_offset = ~((unsigned long long) 0);
-	if (start + reserve > super->create_offset)
-		return 0; /* start overflows create_offset */
-	super->create_offset = start + reserve;
+	reservation_size = pre_reservation + post_reservation;
+
+	if (free_size < reservation_size) {
+		dprintf("imsm: Reservation size is greater than free space.\n");
+		return 0;
+	}
 
-	return maxsize - reserve;
+	super->create_offset = start + pre_reservation;
+	return free_size - reservation_size;
 }
 
 static int is_raid_level_supported(const struct imsm_orom *orom, int level, int raiddisks)
-- 
2.26.2

Re: [PATCH 0/6] imsm: expand improvements

[Jes Sorensen]

On 5/29/23 09:52, Mariusz Tkaczyk wrote:
> merge_extents() was initially designed to support creation only. Expand
> feature was added later and the current code was not updated properly.
> Now, we can see two issues:
> 1. --size=max used with expand and create result in different array size.
> 2. In scenarios, where volume were deleted an recreated it may not be
> possible to expand the volume.
> 
> The patchset addresses listed issues and removes limitation to the last
> volume for expand.
> 
> Mariusz Tkaczyk (6):
>   imsm: move sum_extents calculations to merge_extents()
>   imsm: imsm_get_free_size() refactor.
>   imsm: introduce round_member_size_to_mb()
>   imsm: move expand verification code into new function
>   imsm: return free space after volume for expand
>   imsm: fix free space calculations
> 
>  super-intel.c | 363 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 202 insertions(+), 161 deletions(-)

Applied!

Thanks,
Jes

[PATCH 0/6] imsm: expand improvements

[Mariusz Tkaczyk]

merge_extents() was initially designed to support creation only. Expand
feature was added later and the current code was not updated properly.
Now, we can see two issues:
1. --size=max used with expand and create result in different array size.
2. In scenarios, where volume were deleted an recreated it may not be
possible to expand the volume.

The patchset addresses listed issues and removes limitation to the last
volume for expand.

Mariusz Tkaczyk (6):
  imsm: move sum_extents calculations to merge_extents()
  imsm: imsm_get_free_size() refactor.
  imsm: introduce round_member_size_to_mb()
  imsm: move expand verification code into new function
  imsm: return free space after volume for expand
  imsm: fix free space calculations

 super-intel.c | 363 ++++++++++++++++++++++++++++----------------------
 1 file changed, 202 insertions(+), 161 deletions(-)

-- 
2.26.2

[PATCH 4/6] imsm: move expand verification code into new function

[Mariusz Tkaczyk]

The code here is too complex. Move it to separate function and
simplify it. Add more error messages.

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

diff --git a/super-intel.c b/super-intel.c
index 2351ce20..83bf2bfc 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -11582,6 +11582,102 @@ static void imsm_update_metadata_locally(struct supertype *st,
 	}
 }
 
+/**
+ * imsm_analyze_expand() - check expand properties and calculate new size.
+ * @st: imsm supertype.
+ * @geo: new geometry params.
+ * @array: array info.
+ * @direction: reshape direction.
+ *
+ * Obtain free space after the &array and verify if expand to requested size is
+ * possible. If geo->size is set to %MAX_SIZE, assume that max free size is
+ * requested.
+ *
+ * Return:
+ * On success %IMSM_STATUS_OK is returned, geo->size and geo->raid_disks are
+ * updated.
+ * On error, %IMSM_STATUS_ERROR is returned.
+ */
+static imsm_status_t imsm_analyze_expand(struct supertype *st,
+					 struct geo_params *geo,
+					 struct mdinfo *array,
+					 int direction)
+{
+	struct intel_super *super = st->sb;
+	struct imsm_dev *dev = get_imsm_dev(super, super->current_vol);
+	struct imsm_map *map = get_imsm_map(dev, MAP_0);
+	int data_disks = imsm_num_data_members(map);
+
+	unsigned long long current_size;
+	unsigned long long free_size;
+	unsigned long long new_size;
+	unsigned long long max_size;
+
+	const int chunk_kib = geo->chunksize / 1024;
+	imsm_status_t rv;
+
+	if (direction == ROLLBACK_METADATA_CHANGES) {
+		/**
+		 * Accept size for rollback only.
+		 */
+		new_size = geo->size * 2;
+		goto success;
+	}
+
+	if (super->current_vol + 1 != super->anchor->num_raid_devs) {
+		pr_err("imsm: The last volume in container can be expanded only (%i/%s).\n",
+		       super->current_vol, st->devnm);
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (data_disks == 0) {
+		pr_err("imsm: Cannot retrieve data disks.\n");
+		return IMSM_STATUS_ERROR;
+	}
+	current_size = array->custom_array_size / data_disks;
+
+	rv = imsm_get_free_size(super, dev->vol.map->num_members, 0, chunk_kib, &free_size);
+	if (rv != IMSM_STATUS_OK) {
+		pr_err("imsm: Cannot find free space for expand.\n");
+		return IMSM_STATUS_ERROR;
+	}
+	max_size = round_member_size_to_mb(free_size + current_size);
+
+	if (geo->size == MAX_SIZE)
+		new_size = max_size;
+	else
+		new_size = round_member_size_to_mb(geo->size * 2);
+
+	if (new_size == 0) {
+		pr_err("imsm: Rounded requested size is 0.\n");
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (new_size > max_size) {
+		pr_err("imsm: Rounded requested size (%llu) is larger than free space available (%llu).\n",
+		       new_size, max_size);
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (new_size == current_size) {
+		pr_err("imsm: Rounded requested size (%llu) is same as current size (%llu).\n",
+		       new_size, current_size);
+		return IMSM_STATUS_ERROR;
+	}
+
+	if (new_size < current_size) {
+		pr_err("imsm: Size reduction is not supported, rounded requested size (%llu) is smaller than current (%llu).\n",
+		       new_size, current_size);
+		return IMSM_STATUS_ERROR;
+	}
+
+success:
+	dprintf("imsm: New size per member is %llu.\n", new_size);
+	geo->size = data_disks * new_size;
+	geo->raid_disks = dev->vol.map->num_members;
+	return IMSM_STATUS_OK;
+}
+
 /***************************************************************************
 * Function:	imsm_analyze_change
 * Description:	Function analyze change for single volume
@@ -11602,13 +11698,6 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 	int devNumChange = 0;
 	/* imsm compatible layout value for array geometry verification */
 	int imsm_layout = -1;
-	int data_disks;
-	struct imsm_dev *dev;
-	struct imsm_map *map;
-	struct intel_super *super;
-	unsigned long long current_size;
-	unsigned long long free_size;
-	unsigned long long max_size;
 	imsm_status_t rv;
 
 	getinfo_super_imsm_volume(st, &info, NULL);
@@ -11691,94 +11780,20 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 		geo->chunksize = info.array.chunk_size;
 	}
 
-	chunk = geo->chunksize / 1024;
-
-	super = st->sb;
-	dev = get_imsm_dev(super, super->current_vol);
-	map = get_imsm_map(dev, MAP_0);
-	data_disks = imsm_num_data_members(map);
-	/* compute current size per disk member
-	 */
-	current_size = info.custom_array_size / data_disks;
-
-	if (geo->size > 0 && geo->size != MAX_SIZE) {
-		/* align component size
-		 */
-		geo->size = imsm_component_size_alignment_check(
-				    get_imsm_raid_level(dev->vol.map),
-				    chunk * 1024, super->sector_size,
-				    geo->size * 2);
-		if (geo->size == 0) {
-			pr_err("Error. Size expansion is supported only (current size is %llu, requested size /rounded/ is 0).\n",
-				   current_size);
-			goto analyse_change_exit;
-		}
-	}
-
-	if (current_size != geo->size && geo->size > 0) {
+	if (geo->size > 0) {
 		if (change != -1) {
 			pr_err("Error. Size change should be the only one at a time.\n");
 			change = -1;
 			goto analyse_change_exit;
 		}
-		if ((super->current_vol + 1) != super->anchor->num_raid_devs) {
-			pr_err("Error. The last volume in container can be expanded only (%i/%s).\n",
-			       super->current_vol, st->devnm);
-			goto analyse_change_exit;
-		}
-		/* check the maximum available size
-		 */
-		rv = imsm_get_free_size(super, dev->vol.map->num_members,
-					0, chunk, &free_size);
 
+		rv = imsm_analyze_expand(st, geo, &info, direction);
 		if (rv != IMSM_STATUS_OK)
-			/* Cannot find maximum available space
-			 */
-			max_size = 0;
-		else {
-			max_size = free_size + current_size;
-			/* align component size
-			 */
-			max_size = imsm_component_size_alignment_check(
-					get_imsm_raid_level(dev->vol.map),
-					chunk * 1024, super->sector_size,
-					max_size);
-		}
-		if (geo->size == MAX_SIZE) {
-			/* requested size change to the maximum available size
-			 */
-			if (max_size == 0) {
-				pr_err("Error. Cannot find maximum available space.\n");
-				change = -1;
-				goto analyse_change_exit;
-			} else
-				geo->size = max_size;
-		}
-
-		if (direction == ROLLBACK_METADATA_CHANGES) {
-			/* accept size for rollback only
-			*/
-		} else {
-			/* round size due to metadata compatibility
-			*/
-			geo->size = round_member_size_to_mb(geo->size);
-			dprintf("Prepare update for size change to %llu\n",
-				geo->size );
-			if (current_size >= geo->size) {
-				pr_err("Error. Size expansion is supported only (current size is %llu, requested size /rounded/ is %llu).\n",
-				       current_size, geo->size);
-				goto analyse_change_exit;
-			}
-			if (max_size && geo->size > max_size) {
-				pr_err("Error. Requested size is larger than maximum available size (maximum available size is %llu, requested size /rounded/ is %llu).\n",
-				       max_size, geo->size);
-				goto analyse_change_exit;
-			}
-		}
-		geo->size *= data_disks;
-		geo->raid_disks = dev->vol.map->num_members;
+			goto analyse_change_exit;
 		change = CH_ARRAY_SIZE;
 	}
+
+	chunk = geo->chunksize / 1024;
 	if (!validate_geometry_imsm(st,
 				    geo->level,
 				    imsm_layout,
-- 
2.26.2