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

[lvm-devel] [LVM2 PATCH] Rename ambiguous extent-related variables in allocation code



Hi,

Thanks for private comments to the RFC posted on Aug. 21.

As a first step, this patch only renames the variables,
with 2 exceptions:
  1. 'remaining' in _alloc_parallel_area() is just replaced
     with 'goal_le - *current_le'
  2. Update comments accordingly

No change in the functionality.
The patch is made to applicable to and tested on today's CVS head +
pvmove cleanup patch below.
https://www.redhat.com/archives/lvm-devel/2007-August/msg00068.html
Dependency to the pvmove patch is only about the patch context,
so the change itself is valid without it.

Please consider to apply.

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'.
  Introducing clear distinction would improve the readability.

  There are 3 contexts of 'extents'
    a. a length of LV (or LV segment)
    b. simply the number of physical extents
    c. a length of segment area

    (*) A LV consists of linearly connected LV segments.
        A LV segment consists of 1 or more segment areas.
        Each segment area is either LV segment or PV segment.

  In allocation code, most of 'extents' represents the (a) above.
  And for linear and striped LVs, (a) and (b) used to be same.
  For (c), a convention of variable name 'area_len' is followed.

  However, it seems mirrored LV support introduced some confusions.
  In some places, (c) is treated as (a).
  In other places, (a) is treated as (b).
  Fortunately or unfortunately the mistakes are so specific to
  mirroring code and doesn't cause problem on others.

What the patch does:
  * Rename variables
      - Use '*_le' for the length of LV
      - Use 'le_count' for the LEs not bound to LV
      - See below for details.
  * Delete 'remaining' variable from _alloc_parallel_area()
      - remaining :=  goal_le - current_le
  * Update comments accordingly

Details of renaming:
  * 'allocated' (in _allocate, _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.
  * 'needed' (in _find_parallel_space, _alloc_parallel_area)
    This is the length of the LV given to the allocator as a goal.
    After the allocation succeeds and allocated extents are added to the LV,
    the LV's length should be this value.
  * 'max_parallel' (in _find_parallel_space)
    This is a (possibly reduced) LE to which the allocator should attempt
    to allocate extents in one-go, to align with the parallel areas.
    It can be seen as a temporary goal.
    (Currently, its calculation is incorrect for striped allocation.
     But striped allocation currently doesn't use the code, fortunately.)
  * 'ix' (in _allocate_parallel_area)
    This is same as 'allocated' in _find_parallel_space.
  * 'extents' (in allocate_extents)
    It is the number of LEs to be added to given LV.
  * 'ah->total_area_len' (struct alloc_handle)
    This is used as the length of LV, corresponding to the allocated extents.
    (Currently, its calculation is incorrect for striped allocation.
     But striped allocation currently doesn't use the code, fortunately.)
  * 'new_extents' (in _allocate)
    This is the length of LV after successful allocation.
    Same as 'needed' above.
  * 'old_allocated' (in _allocate)
    This is the value of 'allocated' before _find_parallel_space() is called.

So, variables are renamed
  * 'allocated' -> current_le
  * 'needed' -> goal_le
  * 'max_parallel' -> tmp_goal_le
  * 'ix' -> current_le
  * 'extents' -> le_count (in allocate_extents)
  * ah->total_area_len -> ah->le_count (struct alloc_handle)
  * 'new_extents' -> goal_le
  * 'old_allocated' -> prev_le

Places using 'extents' as logical extents are changed to use 'le_count':
  * lv_reduce(), _lv_reduce() and lv_extend()
  * _setup_alloced_segment()
  * lv_add_virtual_segment()

Index: LVM2.work/lib/metadata/lv_manip.c
===================================================================
--- LVM2.work.orig/lib/metadata/lv_manip.c
+++ LVM2.work/lib/metadata/lv_manip.c
@@ -313,11 +313,11 @@ static int _lv_segment_reduce(struct lv_
 /*
  * Entry point for all LV reductions in size.
  */
-static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
+static int _lv_reduce(struct logical_volume *lv, uint32_t le_count, int delete)
 {
 	struct lv_list *lvl;
 	struct lv_segment *seg;
-	uint32_t count = extents;
+	uint32_t count = le_count;
 	uint32_t reduction;
 
 	list_iterate_back_items(seg, &lv->segments) {
@@ -343,7 +343,7 @@ static int _lv_reduce(struct logical_vol
 		count -= reduction;
 	}
 
-	lv->le_count -= extents;
+	lv->le_count -= le_count;
 	lv->size = (uint64_t) lv->le_count * lv->vg->extent_size;
 
 	if (!delete)
@@ -377,11 +377,11 @@ int lv_empty(struct logical_volume *lv)
 }
 
 /*
- * Remove given number of extents from LV.
+ * Remove given number of logical extents from LV.
  */
-int lv_reduce(struct logical_volume *lv, uint32_t extents)
+int lv_reduce(struct logical_volume *lv, uint32_t le_count)
 {
-	return _lv_reduce(lv, extents, 1);
+	return _lv_reduce(lv, le_count, 1);
 }
 
 /*
@@ -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 le_count;		/* Allocated length of LV */
 
 	struct list *parallel_areas;	/* PVs to avoid */
 
@@ -551,7 +551,7 @@ static int _setup_alloced_segment(struct
 				  uint32_t region_size,
 				  struct logical_volume *log_lv __attribute((unused)))
 {
-	uint32_t s, extents, area_multiple, extra_areas = 0;
+	uint32_t s, le_count, area_multiple, extra_areas = 0;
 	struct lv_segment *seg;
 
 	if (mirrored_pv)
@@ -587,9 +587,9 @@ static int _setup_alloced_segment(struct
 
 	list_add(&lv->segments, &seg->list);
 
-	extents = aa[0].len * area_multiple;
-	lv->le_count += extents;
-	lv->size += (uint64_t) extents *lv->vg->extent_size;
+	le_count = aa[0].len * area_multiple;
+	lv->le_count += le_count;
+	lv->size += (uint64_t) le_count *lv->vg->extent_size;
 
 	if (segtype_is_mirrored(segtype))
 		lv->status |= MIRRORED;
@@ -628,16 +628,15 @@ static int _setup_alloced_segments(struc
  * 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 goal_le,
 				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;
 	uint32_t s;
 	struct alloced_area *aa;
 
-	remaining = needed - *ix;
-	area_len = remaining / ah->area_multiple;
+	area_len = (goal_le - *current_le) / ah->area_multiple;
 
 	/* Reduce area_len to the smallest of the areas */
 	for (s = 0; s < ah->area_count; s++)
@@ -657,7 +656,7 @@ static int _alloc_parallel_area(struct a
 		list_add(&ah->alloced_areas[s], &aa[s].list);
 	}
 
-	ah->total_area_len += area_len;
+	ah->le_count += area_len;
 
 	for (s = 0; s < ah->area_count; s++)
 		consume_pv_area(areas[s], area_len);
@@ -669,7 +668,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_len * ah->area_multiple;
 
 	return 1;
 }
@@ -881,7 +880,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,7 +889,7 @@ 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 tmp_goal_le;	/* reduced goal to align with parallel areas */
 	uint32_t next_le;
 	struct seg_pvs *spvs;
 	struct list *parallel_pvs;
@@ -898,10 +897,10 @@ static int _find_parallel_space(struct a
 
 	/* Is there enough total space? */
 	free_pes = pv_maps_size(pvms);
-	if (needed - *allocated > free_pes) {
+	if (goal_le - *current_le > free_pes) {
 		log_error("Insufficient free space: %" PRIu32 " extents needed,"
 			  " but only %" PRIu32 " available",
-			  needed - *allocated, free_pes);
+			  goal_le - *current_le, free_pes);
 		return 0;
 	}
 
@@ -927,20 +926,20 @@ static int _find_parallel_space(struct a
 		preferred_count = 0;
 
 		parallel_pvs = NULL;
-		max_parallel = needed;
+		tmp_goal_le = goal_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;
+			next_le = (prev_lvseg ? prev_lvseg->le + prev_lvseg->len : 0) + *current_le / ah->area_multiple;
 			list_iterate_items(spvs, ah->parallel_areas) {
 				if (next_le >= spvs->le + spvs->len)
 					continue;
 
-				if (max_parallel > (spvs->le + spvs->len) * ah->area_multiple)
-					max_parallel = (spvs->le + spvs->len) * ah->area_multiple;
+				if (tmp_goal_le > (spvs->le + spvs->len) * ah->area_multiple)
+					tmp_goal_le = (spvs->le + spvs->len) * ah->area_multiple;
 				parallel_pvs = &spvs->pvs;
 				break;
 			}
@@ -997,7 +996,7 @@ static int _find_parallel_space(struct a
 
 				/* Is it big enough on its own? */
 				if (pva->count * ah->area_multiple <
-				    max_parallel - *allocated &&
+				    tmp_goal_le - *current_le &&
 				    ((!can_split && !ah->log_count) ||
 				     (already_found_one &&
 				      !(alloc == ALLOC_ANYWHERE))))
@@ -1035,8 +1034,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, tmp_goal_le, areas,
+					  current_le,
 					  (ah->log_count && !ah->log_area.len) ?
 						*(areas + ix_offset + ix - 1) :
 						NULL)) {
@@ -1044,25 +1043,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 +1070,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,22 +1116,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
+			  (goal_le - current_le) * ah->area_count
 			  / ah->area_multiple);
 		goto out;
 	}
@@ -1151,21 +1151,21 @@ static int _allocate(struct alloc_handle
 }
 
 int lv_add_virtual_segment(struct logical_volume *lv, uint32_t status,
-			   uint32_t extents, const struct segment_type *segtype)
+			   uint32_t le_count, const struct segment_type *segtype)
 {
 	struct lv_segment *seg;
 
 	if (!(seg = alloc_lv_segment(lv->vg->cmd->mem, segtype, lv,
-				     lv->le_count, extents, status, 0,
-				     NULL, 0, extents, 0, 0, 0))) {
+				     lv->le_count, le_count, status, 0,
+				     NULL, 0, le_count, 0, 0, 0))) {
 		log_error("Couldn't allocate new zero segment.");
 		return 0;
 	}
 
 	list_add(&lv->segments, &seg->list);
 
-	lv->le_count += extents;
-	lv->size += (uint64_t) extents *lv->vg->extent_size;
+	lv->le_count += le_count;
+	lv->size += (uint64_t) le_count *lv->vg->extent_size;
 
 	lv->status |= VIRTUAL;
 
@@ -1180,7 +1180,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_count,
 				      struct list *allocatable_pvs,
 				      alloc_policy_t alloc,
 				      struct list *parallel_areas)
@@ -1212,7 +1212,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_count,
 		       allocatable_pvs)) {
 		stack;
 		alloc_destroy(ah);
@@ -1332,8 +1332,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->le_count, 0,
+				     0, log_lv, mirrors, ah->le_count, 0,
 				     region_size, 0))) {
 		log_error("Couldn't allocate new mirror segment.");
 		return 0;
@@ -1345,7 +1345,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->le_count;
 	lv->size += (uint64_t) lv->le_count *lv->vg->extent_size;
 
 	if (lv->vg->fid->fmt->ops->lv_setup &&
@@ -1399,7 +1399,7 @@ int lv_add_more_mirrored_areas(struct lo
 int lv_extend(struct logical_volume *lv,
 	      const struct segment_type *segtype,
 	      uint32_t stripes, uint32_t stripe_size,
-	      uint32_t mirrors, uint32_t extents,
+	      uint32_t mirrors, uint32_t le_count,
 	      uint32_t status, struct list *allocatable_pvs,
 	      alloc_policy_t alloc)
 {
@@ -1409,10 +1409,10 @@ int lv_extend(struct logical_volume *lv,
 	struct lv_segment *seg;
 
 	if (segtype_is_virtual(segtype))
-		return lv_add_virtual_segment(lv, status, extents, segtype);
+		return lv_add_virtual_segment(lv, status, le_count, segtype);
 
 	if (!(ah = allocate_extents(lv->vg, lv, segtype, stripes, mirrors, 0,
-				    extents, allocatable_pvs, alloc, NULL))) {
+				    le_count, allocatable_pvs, alloc, NULL))) {
 		stack;
 		return 0;
 	}
@@ -1435,10 +1435,10 @@ int lv_extend(struct logical_volume *lv,
 				return 0;
 			}
 		}
-		seg->area_len += extents;
-		seg->len += extents;
-		lv->le_count += extents;
-		lv->size += (uint64_t) extents *lv->vg->extent_size;
+		seg->area_len += le_count;
+		seg->len += le_count;
+		lv->le_count += le_count;
+		lv->size += (uint64_t) le_count *lv->vg->extent_size;
 	}
 
       out:
Index: LVM2.work/lib/metadata/lv_alloc.h
===================================================================
--- LVM2.work.orig/lib/metadata/lv_alloc.h
+++ LVM2.work/lib/metadata/lv_alloc.h
@@ -47,7 +47,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_count,
                                       struct list *allocatable_pvs,
 				      alloc_policy_t alloc,
 				      struct list *parallel_areas);
@@ -65,7 +65,7 @@ int lv_add_segment(struct alloc_handle *
 
 int lv_add_log_segment(struct alloc_handle *ah, struct logical_volume *log_lv);
 int lv_add_virtual_segment(struct logical_volume *lv, uint32_t status,
-                           uint32_t extents, const struct segment_type *segtype);
+                           uint32_t le_count, const struct segment_type *segtype);
 int lv_add_mirror_segment(struct alloc_handle *ah,
 			  struct logical_volume *lv,
 			  struct logical_volume **sub_lvs,
Index: LVM2.work/lib/metadata/metadata-exported.h
===================================================================
--- LVM2.work.orig/lib/metadata/metadata-exported.h
+++ LVM2.work/lib/metadata/metadata-exported.h
@@ -350,8 +350,8 @@ struct logical_volume *lv_create_empty(s
 				       int import,
 				       struct volume_group *vg);
 
-/* Reduce the size of an LV by extents */
-int lv_reduce(struct logical_volume *lv, uint32_t extents);
+/* Reduce the size of an LV by logical extents */
+int lv_reduce(struct logical_volume *lv, uint32_t le_count);
 
 /* Empty an LV prior to deleting it */
 int lv_empty(struct logical_volume *lv);
@@ -360,7 +360,7 @@ int lv_empty(struct logical_volume *lv);
 int lv_extend(struct logical_volume *lv,
 	      const struct segment_type *segtype,
 	      uint32_t stripes, uint32_t stripe_size,
-	      uint32_t mirrors, uint32_t extents,
+	      uint32_t mirrors, uint32_t le_count,
 	      uint32_t status, struct list *allocatable_pvs,
 	      alloc_policy_t alloc);
 

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