[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[lvm-devel] [LVM2 RFC PATCH] Tidy extent-related variables in allocation code



Hi,

This patch is rfc, not very well tested yet.

Current allocation code is very hard to read and I think
part of the reason is the ambiguity of 'extents' variables.
Some are length of LV, others are the number of extents.
It makes the code prone to introduce errors.

For example, in _find_parallel_space(), this code:
       if (needed - *allocated > free_pes) {
                log_error("Insufficient free space: %" PRIu32
                            " extents needed,"
                            " but only %" PRIu32 " available",
                            needed - *allocated, free_pes);
checks the free physical extents.
But since 'needed' and 'allocated' are the length of LV,
the comparison doesn't work as expected if the LV is mirrored
because 1-extent of n-way mirrored LV requires n-extents.

I would like to know if the idea of renaming makes sense
and if there are better names of variables and conversion functions.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
Try to reduce ambiguity of variable names in allocation code.

Background:
  Allocation code in LVM2 is quite difficult to read.
  One of the reason seems the ambiguity of the variables related to 'extents'.
  Some represent the length of logical volume in the unit of extents,
  some represent purely the number of extents,
  the others represent the number of extents in a area.
  They are same for linear LV. But different in striped and mirrored.
  It improves the readability of the code to make the distinction between
  them clear.

What the patch does:
  * Rename variables
      - Use '_le' suffix for the variables representing LEs
  * Introducing converters between the 3 types of 'extents'

Examples:
  1) lvcreate -l3
     les: 3, pes: 3, area_len: 3
     area_count: 1, area_multiple: 1
  2) lvcreate -i3 -l3
     les: 3, pes: 3, area_len: 1
     area_count: 3, area_multiple: 3
  3) lvcreate -m3 -l3
     les: 3, pes: 12, area_len: 3
     area_count: 4, area_multiple: 1
  4) lvcreate -i3 -m3 -l3 (not supported currently, though)
     les: 3, pes: 12, area_len: 1
     area_count: 12, area_multiple: 3

Formulas:
  o pes = area_len * area_count
        = les * area_count / area_multiple
  o les = area_len * area_multiple
        = pes / area_count * area_multiple
  o area_len = les / area_multiple
             = pes / area_count

Looking at the code:
  * 'allocated' (in _alloc_parallel_area, _find_parallel_space)
    This is the length of the LV with currently allocated extents.
      - Initialized as lv->le_count.
      - Updated as += area_len * ah->area_multiple.
    But used wrongly for 'next_le' calculation.
    (i.e. next_le = prev_lvseg's last le + allocated / area_multiple.
          Actually, allocated itself points to the next_le.)
  * 'next_le' (in _find_parallel_space)
    No need to calculate it from prev_lvseg.
    'allocated' is that.
  * 'needed' (in _find_parallel_space)
    This is the length of the LV after the allocated extents are added.
  * 'max_parallel' (in _find_parallel_space)
    Used as a length of LV segment.
    However, set to "(spvs->le + spvs->len) * area_multiple" makes no sense.
    It was just lucky that the code was only used for mirror, where
    area_multiple is 1.
    It should be just "spvs->le + spvs->len".
    Also, since it's used to limit area_len for a single iteration in
    _find_parallel_space, we can just maintain corresponding area_len value
    for it.
    area_len = les2area_len(max_parallel)
  * 'ix' (in _allocate_parallel_area)
    This is same as 'allocated' in _find_parallel_space.
  * _allocate_parallel_area() takes 'needed' and calculate area_len from it.
    However, it can just take area_len.
  * 'extents' (in allocate_extents)
    This is different from everything else.
    It is the number of LEs to be added to given LV.
  * lvextend and lvcreate call 'allocate_extents' with the goal LE as
    its 'extents' argument.
    OTOH, lvconvert (linear to mirror) calls 'allocate_extents' with
    PEs. It worked because _alloc_init() called calc_area_multiple()
    with incorrect argument.

So, variables are renamed (and converted)
  * 'allocated' -> current_le
  * 'next_le' -> [removed]
  * 'needed' -> goal_le
  * 'max_parallel' -> [converted to area_len] max_area_len
  * 'ix' -> current_le
  * 'extents' -> le_count (in allocate_extents)

Index: LVM2.work/lib/metadata/lv_manip.c
===================================================================
--- LVM2.work.orig/lib/metadata/lv_manip.c
+++ LVM2.work/lib/metadata/lv_manip.c
@@ -420,7 +420,7 @@ struct alloc_handle {
 	uint32_t area_count;		/* Number of parallel areas */
 	uint32_t area_multiple;		/* seg->len = area_len * area_multiple */
 	uint32_t log_count;		/* Number of parallel 1-extent logs */
-	uint32_t total_area_len;	/* Total number of parallel extents */
+	uint32_t allocated_les;		/* Allocated length of LV */
 
 	struct list *parallel_areas;	/* PVs to avoid */
 
@@ -624,21 +624,64 @@ static int _setup_alloced_segments(struc
 }
 
 /*
+ * There are different 'extents' in allocation code
+ *   a) length of LV in the unit of (logical) extents
+ *   b) purely the number of (usually physical) extents
+ *   c) length of segment area in the unit of (usually physical) extents
+ * Let's call them "les", "pes", "area_len", respectively.
+ *
+ *   - Examples:
+ *       1) lvcreate -l3
+ *          les: 3, pes: 3, area_len: 3
+ *       2) lvcreate -i3 -l3
+ *          les: 3, pes: 3, area_len: 1
+ *       3) lvcreate -m3 -l3
+ *           les: 3, pes: 12, area_len: 3
+ *
+ * They can be converted each other using area_count and area_multiple.
+ *
+ *   - Formulas:
+ *       o pes = area_len * area_count
+ *             = les * area_count / area_multiple
+ *       o les = area_len * area_multiple
+ *             = pes / area_count * area_multiple
+ *       o area_len = les / area_multiple
+ *                  = pes / area_count
+ *   - Revisiting examples:
+ *       1) lvcreate -l3
+ *          les: 3, area_count: 1, area_multiple: 1
+ *       2) lvcreate -i3 -l3
+ *          les: 3, area_count: 3, area_multiple: 3
+ *       3) lvcreate -m3 -l3
+ *          les: 3, area_count: 4, area_multiple: 1
+ */
+static uint32_t les2area_len(struct alloc_handle *ah, uint32_t les)
+{
+	return les / ah->area_multiple;
+}
+static uint32_t les2pes(struct alloc_handle *ah, uint32_t les)
+{
+	return les2area_len(ah, les) * ah->area_count;
+}
+static uint32_t area_len2les(struct alloc_handle *ah, uint32_t area_len)
+{
+	return area_len * ah->area_multiple;
+}
+
+/*
  * This function takes a list of pv_areas and adds them to allocated_areas.
  * If the complete area is not needed then it gets split.
  * The part used is removed from the pv_map so it can't be allocated twice.
  */
-static int _alloc_parallel_area(struct alloc_handle *ah, uint32_t needed,
+static int _alloc_parallel_area(struct alloc_handle *ah,
+				uint32_t max_area_len,
 				struct pv_area **areas,
-				uint32_t *ix, struct pv_area *log_area)
+				uint32_t *current_le, struct pv_area *log_area)
 {
-	uint32_t area_len, remaining;
+	uint32_t area_len = max_area_len;
 	uint32_t s;
 	struct alloced_area *aa;
 
-	remaining = needed - *ix;
-	area_len = remaining / ah->area_multiple;
-
 	/* Reduce area_len to the smallest of the areas */
 	for (s = 0; s < ah->area_count; s++)
 		if (area_len > areas[s]->count)
@@ -657,7 +700,7 @@ static int _alloc_parallel_area(struct a
 		list_add(&ah->alloced_areas[s], &aa[s].list);
 	}
 
-	ah->total_area_len += area_len;
+	ah->allocated_les += area_len2les(ah, area_len);
 
 	for (s = 0; s < ah->area_count; s++)
 		consume_pv_area(areas[s], area_len);
@@ -669,7 +712,7 @@ static int _alloc_parallel_area(struct a
 		consume_pv_area(log_area, ah->log_area.len);
 	}
 
-	*ix += area_len * ah->area_multiple;
+	*current_le += area_len2les(ah, area_len);
 
 	return 1;
 }
@@ -881,7 +924,7 @@ static int _find_parallel_space(struct a
 				struct list *pvms, struct pv_area **areas,
 				uint32_t areas_size, unsigned can_split,
 				struct lv_segment *prev_lvseg,
-				uint32_t *allocated, uint32_t needed)
+				uint32_t *current_le, uint32_t goal_le)
 {
 	struct pv_map *pvm;
 	struct pv_area *pva;
@@ -890,18 +933,18 @@ static int _find_parallel_space(struct a
 	unsigned contiguous = 0, cling = 0, preferred_count = 0;
 	unsigned ix;
 	unsigned ix_offset = 0;	/* Offset for non-preferred allocations */
-	uint32_t max_parallel;	/* Maximum extents to allocate */
-	uint32_t next_le;
+	uint32_t max_area_len;	/* Maximum extents to allocate for each area in one iteration */
+	uint32_t para_le;
 	struct seg_pvs *spvs;
 	struct list *parallel_pvs;
 	uint32_t free_pes;
 
 	/* Is there enough total space? */
 	free_pes = pv_maps_size(pvms);
-	if (needed - *allocated > free_pes) {
+	if (les2pes(ah, goal_le - *current_le) > free_pes) {
 		log_error("Insufficient free space: %" PRIu32 " extents needed,"
 			  " but only %" PRIu32 " available",
-			  needed - *allocated, free_pes);
+			  les2pes(ah, goal_le - *current_le), free_pes);
 		return 0;
 	}
 
@@ -927,20 +970,21 @@ static int _find_parallel_space(struct a
 		preferred_count = 0;
 
 		parallel_pvs = NULL;
-		max_parallel = needed;
+		max_area_len = les2area_len(ah, goal_le - *current_le);
 
 		/*
 		 * If there are existing parallel PVs, avoid them and reduce
 		 * the maximum we can allocate in one go accordingly.
 		 */
 		if (ah->parallel_areas) {
-			next_le = (prev_lvseg ? prev_lvseg->le + prev_lvseg->len : 0) + *allocated / ah->area_multiple;
 			list_iterate_items(spvs, ah->parallel_areas) {
-				if (next_le >= spvs->le + spvs->len)
+				para_le = spvs->le + spvs->len;
+				if (*current_le >= para_le)
 					continue;
 
-				if (max_parallel > (spvs->le + spvs->len) * ah->area_multiple)
-					max_parallel = (spvs->le + spvs->len) * ah->area_multiple;
+				if (goal_le > para_le)
+					max_area_len = les2area_len(ah,
+							 para_le - *current_le);
 				parallel_pvs = &spvs->pvs;
 				break;
 			}
@@ -996,8 +1040,7 @@ static int _find_parallel_space(struct a
 				}
 
 				/* Is it big enough on its own? */
-				if (pva->count * ah->area_multiple <
-				    max_parallel - *allocated &&
+				if (pva->count < max_area_len &&
 				    ((!can_split && !ah->log_count) ||
 				     (already_found_one &&
 				      !(alloc == ALLOC_ANYWHERE))))
@@ -1035,8 +1078,8 @@ static int _find_parallel_space(struct a
 
 		/* First time around, use smallest area as log_area */
 		/* FIXME decide which PV to use at top of function instead */
-		if (!_alloc_parallel_area(ah, max_parallel, areas,
-					  allocated,
+		if (!_alloc_parallel_area(ah, max_area_len, areas,
+					  current_le,
 					  (ah->log_count && !ah->log_area.len) ?
 						*(areas + ix_offset + ix - 1) :
 						NULL)) {
@@ -1044,25 +1087,26 @@ static int _find_parallel_space(struct a
 			return 0;
 		}
 
-	} while (!contiguous && *allocated != needed && can_split);
+	} while (!contiguous && *current_le != goal_le && can_split);
 
 	return 1;
 }
 
 /*
- * Allocate several segments, each the same size, in parallel.
+ * Allocate several segments, each the same size, in parallel,
+ * to extend lv to goal_le.
  * If mirrored_pv and mirrored_pe are supplied, it is used as
  * the first area, and additional areas are allocated parallel to it.
  */
 static int _allocate(struct alloc_handle *ah,
 		     struct volume_group *vg,
 		     struct logical_volume *lv,
-		     uint32_t new_extents,
+		     uint32_t goal_le,
 		     struct list *allocatable_pvs)
 {
 	struct pv_area **areas;
-	uint32_t allocated = lv ? lv->le_count : 0;
-	uint32_t old_allocated;
+	uint32_t current_le = lv ? lv->le_count : 0;
+	uint32_t prev_le;
 	struct lv_segment *prev_lvseg = NULL;
 	unsigned can_split = 1;	/* Are we allowed more than one segment? */
 	int r = 0;
@@ -1070,7 +1114,7 @@ static int _allocate(struct alloc_handle
 	uint32_t areas_size;
 	alloc_policy_t alloc;
 
-	if (allocated >= new_extents && !ah->log_count) {
+	if (current_le >= goal_le && !ah->log_count) {
 		log_error("_allocate called with no work to do!");
 		return 1;
 	}
@@ -1116,23 +1160,22 @@ static int _allocate(struct alloc_handle
 
 	/* Attempt each defined allocation policy in turn */
 	for (alloc = ALLOC_CONTIGUOUS; alloc < ALLOC_INHERIT; alloc++) {
-		old_allocated = allocated;
+		prev_le = current_le;
 		if (!_find_parallel_space(ah, alloc, pvms, areas,
 					  areas_size, can_split,
-					  prev_lvseg, &allocated, new_extents))
+					  prev_lvseg, &current_le, goal_le))
 			goto_out;
-		if ((allocated == new_extents) || (ah->alloc == alloc) ||
-		    (!can_split && (allocated != old_allocated)))
+		if ((current_le == goal_le) || (ah->alloc == alloc) ||
+		    (!can_split && (current_le != prev_le)))
 			break;
 	}
 
-	if (allocated != new_extents) {
+	if (current_le != goal_le) {
 		log_error("Insufficient suitable %sallocatable extents "
 			  "for logical volume %s: %u more required",
 			  can_split ? "" : "contiguous ",
 			  lv ? lv->name : "",
-			  (new_extents - allocated) * ah->area_count
-			  / ah->area_multiple);
+			  les2pes(ah, goal_le - current_le));
 		goto out;
 	}
 
@@ -1180,7 +1223,7 @@ struct alloc_handle *allocate_extents(st
 				      const struct segment_type *segtype,
 				      uint32_t stripes,
 				      uint32_t mirrors, uint32_t log_count,
-				      uint32_t extents,
+				      uint32_t le_counts,
 				      struct list *allocatable_pvs,
 				      alloc_policy_t alloc,
 				      struct list *parallel_areas)
@@ -1212,7 +1255,7 @@ struct alloc_handle *allocate_extents(st
 	}
 
 	if (!segtype_is_virtual(segtype) &&
-	    !_allocate(ah, vg, lv, (lv ? lv->le_count : 0) + extents,
+	    !_allocate(ah, vg, lv, (lv ? lv->le_count : 0) + le_counts,
 		       allocatable_pvs)) {
 		stack;
 		alloc_destroy(ah);
@@ -1332,8 +1375,8 @@ int lv_add_mirror_segment(struct alloc_h
 	if (!(seg = alloc_lv_segment(lv->vg->cmd->mem,
 				     get_segtype_from_string(lv->vg->cmd,
 							     "mirror"),
-				     lv, lv->le_count, ah->total_area_len, 0,
-				     0, log_lv, mirrors, ah->total_area_len, 0,
+				     lv, lv->le_count, ah->allocated_les, 0,
+				     0, log_lv, mirrors, ah->allocated_les, 0,
 				     region_size, 0))) {
 		log_error("Couldn't allocate new mirror segment.");
 		return 0;
@@ -1345,7 +1388,7 @@ int lv_add_mirror_segment(struct alloc_h
 	}
 
 	list_add(&lv->segments, &seg->list);
-	lv->le_count += ah->total_area_len;
+	lv->le_count += ah->allocated_les;
 	lv->size += (uint64_t) lv->le_count *lv->vg->extent_size;
 
 	if (lv->vg->fid->fmt->ops->lv_setup &&

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]