[PATCH 2 of 10] LVM: make log_area a list

[PATCH 2 of 10] LVM: make log_area a list

[Jonathan Brassow]

Patch name: lvm-make-log_area-a-list.patch

The 'alloc_handle' structure only has space for one log_area.
We change that to a list to allow an arbitrary number of
log areas.

RFC: Jonathan Brassow <jbrassow@redhat.com>

Index: LVM2/lib/metadata/lv_manip.c
===================================================================
--- LVM2.orig/lib/metadata/lv_manip.c
+++ LVM2/lib/metadata/lv_manip.c
@@ -515,7 +515,7 @@ struct alloc_handle {
 
 	struct dm_list *parallel_areas;	/* PVs to avoid */
 
-	struct alloced_area log_area;	/* Extent used for log */
+	struct dm_list log_areas;	/* Extents used for logs */
 	struct dm_list alloced_areas[0];	/* Lists of areas in each stripe */
 };
 
@@ -582,6 +582,7 @@ static struct alloc_handle *_alloc_init(
 	ah->alloc = alloc;
 	ah->area_multiple = calc_area_multiple(segtype, area_count);
 
+	dm_list_init(&ah->log_areas);
 	for (s = 0; s < ah->area_count; s++)
 		dm_list_init(&ah->alloced_areas[s]);
 
@@ -763,10 +764,11 @@ static int _alloc_parallel_area(struct a
 		consume_pv_area(areas[s], area_len);
 
 	if (log_area) {
-		ah->log_area.pv = log_area->map->pv;
-		ah->log_area.pe = log_area->start;
-		ah->log_area.len = log_len;
-		consume_pv_area(log_area, ah->log_area.len);
+		aa[s].pv = log_area->map->pv;
+		aa[s].pe = log_area->start;
+		aa[s].len = log_len;
+		dm_list_add(&ah->log_areas, &aa[s].list);
+		consume_pv_area(log_area, log_len);
 	}
 
 	*ix += area_len * ah->area_multiple;
@@ -983,6 +985,7 @@ static int _find_parallel_space(struct a
 				struct lv_segment *prev_lvseg,
 				uint32_t *allocated, uint32_t needed)
 {
+	int skip = 0;
 	struct pv_map *pvm;
 	struct pv_area *pva;
 	struct pv_list *pvl;
@@ -999,6 +1002,7 @@ static int _find_parallel_space(struct a
 	uint32_t log_len;
 	struct pv_area *log_area;
 	unsigned log_needs_allocating;
+	struct alloced_area *aa;
 
 	/* Is there enough total space? */
 	free_pes = pv_maps_size(pvms);
@@ -1061,10 +1065,14 @@ static int _find_parallel_space(struct a
 				continue;	/* Next PV */
 
 			if (alloc != ALLOC_ANYWHERE) {
-				/* Don't allocate onto the log pv */
-				if (ah->log_count &&
-				    pvm->pv == ah->log_area.pv)
-					continue;	/* Next PV */
+				/* Don't allocate onto the log pvs */
+				dm_list_iterate_items(aa, &ah->log_areas)
+					if (pvm->pv == aa->pv)
+						skip = 1;
+				if (skip) {
+					skip = 0;
+					continue;
+				}
 
 				/* Avoid PVs used by existing parallel areas */
 				if (parallel_pvs)
@@ -1125,8 +1133,9 @@ static int _find_parallel_space(struct a
 		if ((contiguous || cling) && (preferred_count < ix_offset))
 			break;
 
-		log_needs_allocating = (ah->log_count && !ah->log_area.len) ?
-				       1 : 0;
+		log_needs_allocating = 0;
+		if (ah->log_count && dm_list_empty(&ah->log_areas))
+			log_needs_allocating = 1;
 
 		if (ix + ix_offset < ah->area_count +
 		   (log_needs_allocating ? ah->log_count : 0))
@@ -1199,6 +1208,7 @@ static int _allocate(struct alloc_handle
 	struct dm_list *pvms;
 	uint32_t areas_size;
 	alloc_policy_t alloc;
+	struct alloced_area *aa;
 
 	if (allocated >= new_extents && !ah->log_count) {
 		log_error("_allocate called with no work to do!");
@@ -1264,12 +1274,13 @@ static int _allocate(struct alloc_handle
 		goto out;
 	}
 
-	if (ah->log_count && !ah->log_area.len) {
-		log_error("Insufficient extents for log allocation "
-			  "for logical volume %s.",
-			  lv ? lv->name : "");
-		goto out;
-	}
+	dm_list_iterate_items(aa, &ah->log_areas)
+		if (ah->log_count && !aa->len) {
+			log_error("Insufficient extents for log allocation "
+				  "for logical volume %s.",
+				  lv ? lv->name : "");
+			goto out;
+		}
 
 	r = 1;
 
@@ -1545,10 +1556,21 @@ int lv_add_mirror_lvs(struct logical_vol
 
 /*
  * Turn an empty LV into a mirror log.
+ *
+ * Only for the addition of the first, linear log.
  */
 int lv_add_log_segment(struct alloc_handle *ah, struct logical_volume *log_lv)
 {
 	struct lv_segment *seg;
+	struct alloced_area *log_area;
+
+	dm_list_iterate_items(log_area, &ah->log_areas)
+		break;
+
+	if (!log_area)
+		return 0;
+
+	dm_list_del(&log_area->list);
 
 	if (dm_list_size(&log_lv->segments)) {
 		log_error("Log segments can only be added to an empty LV");
@@ -1558,19 +1580,21 @@ int lv_add_log_segment(struct alloc_hand
 	if (!(seg = alloc_lv_segment(log_lv->vg->cmd->mem,
 				     get_segtype_from_string(log_lv->vg->cmd,
 							     "striped"),
-				     log_lv, 0, ah->log_area.len, MIRROR_LOG,
-				     0, NULL, 1, ah->log_area.len, 0, 0, 0))) {
+				     log_lv, 0, log_area->len, MIRROR_LOG,
+				     0, NULL, 1, log_area->len, 0, 0, 0))) {
 		log_error("Couldn't allocate new mirror log segment.");
 		return 0;
 	}
 
-	if (!set_lv_segment_area_pv(seg, 0, ah->log_area.pv, ah->log_area.pe))
+	if (!set_lv_segment_area_pv(seg, 0, log_area->pv, log_area->pe))
 		return_0;
 
 	dm_list_add(&log_lv->segments, &seg->list);
-	log_lv->le_count += ah->log_area.len;
+	log_lv->le_count += log_area->len;
 	log_lv->size += (uint64_t) log_lv->le_count * log_lv->vg->extent_size;
 
+	dm_pool_free(ah->mem, log_area);
+
 	if (log_lv->vg->fid->fmt->ops->lv_setup &&
 	    !log_lv->vg->fid->fmt->ops->lv_setup(log_lv->vg->fid, log_lv))
 		return_0;

[PATCH 2 of 10] LVM: make log_area a list

[Jonathan Brassow]

Patch name: lvm-make-log_area-a-list.patch

The 'alloc_handle' structure only has space for one log_area.
We change that to a list to allow an arbitrary number of
log areas.

RFC: Jonathan Brassow <jbrassow@redhat.com>

Index: LVM2/lib/metadata/lv_manip.c
===================================================================
--- LVM2.orig/lib/metadata/lv_manip.c
+++ LVM2/lib/metadata/lv_manip.c
@@ -515,7 +515,7 @@ struct alloc_handle {
 
 	struct dm_list *parallel_areas;	/* PVs to avoid */
 
-	struct alloced_area log_area;	/* Extent used for log */
+	struct dm_list log_areas;	/* Extents used for logs */
 	struct dm_list alloced_areas[0];	/* Lists of areas in each stripe */
 };
 
@@ -582,6 +582,7 @@ static struct alloc_handle *_alloc_init(
 	ah->alloc = alloc;
 	ah->area_multiple = calc_area_multiple(segtype, area_count);
 
+	dm_list_init(&ah->log_areas);
 	for (s = 0; s < ah->area_count; s++)
 		dm_list_init(&ah->alloced_areas[s]);
 
@@ -763,10 +764,11 @@ static int _alloc_parallel_area(struct a
 		consume_pv_area(areas[s], area_len);
 
 	if (log_area) {
-		ah->log_area.pv = log_area->map->pv;
-		ah->log_area.pe = log_area->start;
-		ah->log_area.len = log_len;
-		consume_pv_area(log_area, ah->log_area.len);
+		aa[s].pv = log_area->map->pv;
+		aa[s].pe = log_area->start;
+		aa[s].len = log_len;
+		dm_list_add(&ah->log_areas, &aa[s].list);
+		consume_pv_area(log_area, log_len);
 	}
 
 	*ix += area_len * ah->area_multiple;
@@ -983,6 +985,7 @@ static int _find_parallel_space(struct a
 				struct lv_segment *prev_lvseg,
 				uint32_t *allocated, uint32_t needed)
 {
+	int skip = 0;
 	struct pv_map *pvm;
 	struct pv_area *pva;
 	struct pv_list *pvl;
@@ -999,6 +1002,7 @@ static int _find_parallel_space(struct a
 	uint32_t log_len;
 	struct pv_area *log_area;
 	unsigned log_needs_allocating;
+	struct alloced_area *aa;
 
 	/* Is there enough total space? */
 	free_pes = pv_maps_size(pvms);
@@ -1061,10 +1065,14 @@ static int _find_parallel_space(struct a
 				continue;	/* Next PV */
 
 			if (alloc != ALLOC_ANYWHERE) {
-				/* Don't allocate onto the log pv */
-				if (ah->log_count &&
-				    pvm->pv == ah->log_area.pv)
-					continue;	/* Next PV */
+				/* Don't allocate onto the log pvs */
+				dm_list_iterate_items(aa, &ah->log_areas)
+					if (pvm->pv == aa->pv)
+						skip = 1;
+				if (skip) {
+					skip = 0;
+					continue;
+				}
 
 				/* Avoid PVs used by existing parallel areas */
 				if (parallel_pvs)
@@ -1125,8 +1133,9 @@ static int _find_parallel_space(struct a
 		if ((contiguous || cling) && (preferred_count < ix_offset))
 			break;
 
-		log_needs_allocating = (ah->log_count && !ah->log_area.len) ?
-				       1 : 0;
+		log_needs_allocating = 0;
+		if (ah->log_count && dm_list_empty(&ah->log_areas))
+			log_needs_allocating = 1;
 
 		if (ix + ix_offset < ah->area_count +
 		   (log_needs_allocating ? ah->log_count : 0))
@@ -1199,6 +1208,7 @@ static int _allocate(struct alloc_handle
 	struct dm_list *pvms;
 	uint32_t areas_size;
 	alloc_policy_t alloc;
+	struct alloced_area *aa;
 
 	if (allocated >= new_extents && !ah->log_count) {
 		log_error("_allocate called with no work to do!");
@@ -1264,12 +1274,13 @@ static int _allocate(struct alloc_handle
 		goto out;
 	}
 
-	if (ah->log_count && !ah->log_area.len) {
-		log_error("Insufficient extents for log allocation "
-			  "for logical volume %s.",
-			  lv ? lv->name : "");
-		goto out;
-	}
+	dm_list_iterate_items(aa, &ah->log_areas)
+		if (ah->log_count && !aa->len) {
+			log_error("Insufficient extents for log allocation "
+				  "for logical volume %s.",
+				  lv ? lv->name : "");
+			goto out;
+		}
 
 	r = 1;
 
@@ -1545,10 +1556,21 @@ int lv_add_mirror_lvs(struct logical_vol
 
 /*
  * Turn an empty LV into a mirror log.
+ *
+ * Only for the addition of the first, linear log.
  */
 int lv_add_log_segment(struct alloc_handle *ah, struct logical_volume *log_lv)
 {
 	struct lv_segment *seg;
+	struct alloced_area *log_area;
+
+	dm_list_iterate_items(log_area, &ah->log_areas)
+		break;
+
+	if (!log_area)
+		return 0;
+
+	dm_list_del(&log_area->list);
 
 	if (dm_list_size(&log_lv->segments)) {
 		log_error("Log segments can only be added to an empty LV");
@@ -1558,19 +1580,21 @@ int lv_add_log_segment(struct alloc_hand
 	if (!(seg = alloc_lv_segment(log_lv->vg->cmd->mem,
 				     get_segtype_from_string(log_lv->vg->cmd,
 							     "striped"),
-				     log_lv, 0, ah->log_area.len, MIRROR_LOG,
-				     0, NULL, 1, ah->log_area.len, 0, 0, 0))) {
+				     log_lv, 0, log_area->len, MIRROR_LOG,
+				     0, NULL, 1, log_area->len, 0, 0, 0))) {
 		log_error("Couldn't allocate new mirror log segment.");
 		return 0;
 	}
 
-	if (!set_lv_segment_area_pv(seg, 0, ah->log_area.pv, ah->log_area.pe))
+	if (!set_lv_segment_area_pv(seg, 0, log_area->pv, log_area->pe))
 		return_0;
 
 	dm_list_add(&log_lv->segments, &seg->list);
-	log_lv->le_count += ah->log_area.len;
+	log_lv->le_count += log_area->len;
 	log_lv->size += (uint64_t) log_lv->le_count * log_lv->vg->extent_size;
 
+	dm_pool_free(ah->mem, log_area);
+
 	if (log_lv->vg->fid->fmt->ops->lv_setup &&
 	    !log_lv->vg->fid->fmt->ops->lv_setup(log_lv->vg->fid, log_lv))
 		return_0;

Re: [PATCH 2 of 10] LVM: make log_area a list

[malahal]

Jonathan Brassow [jbrassow@redhat.com] wrote:
> Patch name: lvm-make-log_area-a-list.patch
> 
> The 'alloc_handle' structure only has space for one log_area.
> We change that to a list to allow an arbitrary number of
> log areas.
> 
> RFC: Jonathan Brassow <jbrassow@redhat.com>
> 
> @@ -1061,10 +1065,14 @@ static int _find_parallel_space(struct a
>  				continue;	/* Next PV */
>  
>  			if (alloc != ALLOC_ANYWHERE) {
> -				/* Don't allocate onto the log pv */
> -				if (ah->log_count &&
> -				    pvm->pv == ah->log_area.pv)
> -					continue;	/* Next PV */
> +				/* Don't allocate onto the log pvs */
> +				dm_list_iterate_items(aa, &ah->log_areas)
> +					if (pvm->pv == aa->pv)
> +						skip = 1;

                                       You can 'break' soon after setting skip.

> @@ -1199,6 +1208,7 @@ static int _allocate(struct alloc_handle
>  	struct dm_list *pvms;
>  	uint32_t areas_size;
>  	alloc_policy_t alloc;
> +	struct alloced_area *aa;
>  
>  	if (allocated >= new_extents && !ah->log_count) {
>  		log_error("_allocate called with no work to do!");
> @@ -1264,12 +1274,13 @@ static int _allocate(struct alloc_handle
>  		goto out;
>  	}
>  
> -	if (ah->log_count && !ah->log_area.len) {
> -		log_error("Insufficient extents for log allocation "
> -			  "for logical volume %s.",
> -			  lv ? lv->name : "");
> -		goto out;
> -	}
> +	dm_list_iterate_items(aa, &ah->log_areas)
> +		if (ah->log_count && !aa->len) {

ah->log_count can be checked outside of the list iterate.

> +			log_error("Insufficient extents for log allocation "
> +				  "for logical volume %s.",
> +				  lv ? lv->name : "");
> +			goto out;
> +		}
>  int lv_add_log_segment(struct alloc_handle *ah, struct logical_volume *log_lv)
>  {
>  	struct lv_segment *seg;
> +	struct alloced_area *log_area;
> +
> +	dm_list_iterate_items(log_area, &ah->log_areas)
> +		break;

        Using the list iterator to get the first element looks odd!
	Should we use dm_list_first and dm_list_item? Or just have
	dm_list_first_item()???

--
lvm-devel mailing list
lvm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/lvm-devel

Re: [PATCH 2 of 10] LVM: make log_area a list

[malahal]

Jonathan Brassow [jbrassow at redhat.com] wrote:
> Patch name: lvm-make-log_area-a-list.patch
> 
> The 'alloc_handle' structure only has space for one log_area.
> We change that to a list to allow an arbitrary number of
> log areas.
> 
> RFC: Jonathan Brassow <jbrassow@redhat.com>
> 
> @@ -1061,10 +1065,14 @@ static int _find_parallel_space(struct a
>  				continue;	/* Next PV */
>  
>  			if (alloc != ALLOC_ANYWHERE) {
> -				/* Don't allocate onto the log pv */
> -				if (ah->log_count &&
> -				    pvm->pv == ah->log_area.pv)
> -					continue;	/* Next PV */
> +				/* Don't allocate onto the log pvs */
> +				dm_list_iterate_items(aa, &ah->log_areas)
> +					if (pvm->pv == aa->pv)
> +						skip = 1;

                                       You can 'break' soon after setting skip.

> @@ -1199,6 +1208,7 @@ static int _allocate(struct alloc_handle
>  	struct dm_list *pvms;
>  	uint32_t areas_size;
>  	alloc_policy_t alloc;
> +	struct alloced_area *aa;
>  
>  	if (allocated >= new_extents && !ah->log_count) {
>  		log_error("_allocate called with no work to do!");
> @@ -1264,12 +1274,13 @@ static int _allocate(struct alloc_handle
>  		goto out;
>  	}
>  
> -	if (ah->log_count && !ah->log_area.len) {
> -		log_error("Insufficient extents for log allocation "
> -			  "for logical volume %s.",
> -			  lv ? lv->name : "");
> -		goto out;
> -	}
> +	dm_list_iterate_items(aa, &ah->log_areas)
> +		if (ah->log_count && !aa->len) {

ah->log_count can be checked outside of the list iterate.

> +			log_error("Insufficient extents for log allocation "
> +				  "for logical volume %s.",
> +				  lv ? lv->name : "");
> +			goto out;
> +		}
>  int lv_add_log_segment(struct alloc_handle *ah, struct logical_volume *log_lv)
>  {
>  	struct lv_segment *seg;
> +	struct alloced_area *log_area;
> +
> +	dm_list_iterate_items(log_area, &ah->log_areas)
> +		break;

        Using the list iterator to get the first element looks odd!
	Should we use dm_list_first and dm_list_item? Or just have
	dm_list_first_item()???

Re: [PATCH 2 of 10] LVM: make log_area a list

[Jonathan Brassow]

On Oct 12, 2009, at 8:48 PM, malahal at us.ibm.com wrote:

> Jonathan Brassow [jbrassow at redhat.com] wrote:
>> Patch name: lvm-make-log_area-a-list.patch
>>
>> The 'alloc_handle' structure only has space for one log_area.
>> We change that to a list to allow an arbitrary number of
>> log areas.
>>
>> RFC: Jonathan Brassow <jbrassow@redhat.com>
>>
>> @@ -1061,10 +1065,14 @@ static int _find_parallel_space(struct a
>> 				continue;	/* Next PV */
>>
>> 			if (alloc != ALLOC_ANYWHERE) {
>> -				/* Don't allocate onto the log pv */
>> -				if (ah->log_count &&
>> -				    pvm->pv == ah->log_area.pv)
>> -					continue;	/* Next PV */
>> +				/* Don't allocate onto the log pvs */
>> +				dm_list_iterate_items(aa, &ah->log_areas)
>> +					if (pvm->pv == aa->pv)
>> +						skip = 1;
>
>                                       You can 'break' soon after  
> setting skip.

If I use 'break' won't that break out of the 'dm_list_iterate_items'  
and not the enclosing loop - giving an incorrect result?

>
>> @@ -1199,6 +1208,7 @@ static int _allocate(struct alloc_handle
>> 	struct dm_list *pvms;
>> 	uint32_t areas_size;
>> 	alloc_policy_t alloc;
>> +	struct alloced_area *aa;
>>
>> 	if (allocated >= new_extents && !ah->log_count) {
>> 		log_error("_allocate called with no work to do!");
>> @@ -1264,12 +1274,13 @@ static int _allocate(struct alloc_handle
>> 		goto out;
>> 	}
>>
>> -	if (ah->log_count && !ah->log_area.len) {
>> -		log_error("Insufficient extents for log allocation "
>> -			  "for logical volume %s.",
>> -			  lv ? lv->name : "");
>> -		goto out;
>> -	}
>> +	dm_list_iterate_items(aa, &ah->log_areas)
>> +		if (ah->log_count && !aa->len) {
>
> ah->log_count can be checked outside of the list iterate.

I thought the code looked ugly with three levels of indentation; but  
yes, I think that could save some cycles.  (I've changed it in my  
local patch.)

>
>> +			log_error("Insufficient extents for log allocation "
>> +				  "for logical volume %s.",
>> +				  lv ? lv->name : "");
>> +			goto out;
>> +		}
>> int lv_add_log_segment(struct alloc_handle *ah, struct  
>> logical_volume *log_lv)
>> {
>> 	struct lv_segment *seg;
>> +	struct alloced_area *log_area;
>> +
>> +	dm_list_iterate_items(log_area, &ah->log_areas)
>> +		break;
>
>        Using the list iterator to get the first element looks odd!
> 	Should we use dm_list_first and dm_list_item? Or just have
> 	dm_list_first_item()???

I thought the same thing.  :)  Perhaps I should introduce another  
patch adding that macro...

  brassow

Re: [PATCH 2 of 10] LVM: make log_area a list

[malahal]

Jonathan Brassow [jbrassow at redhat.com] wrote:
>
> On Oct 12, 2009, at 8:48 PM, malahal at us.ibm.com wrote:
>
>> Jonathan Brassow [jbrassow at redhat.com] wrote:
>>> Patch name: lvm-make-log_area-a-list.patch
>>>
>>> The 'alloc_handle' structure only has space for one log_area.
>>> We change that to a list to allow an arbitrary number of
>>> log areas.
>>>
>>> RFC: Jonathan Brassow <jbrassow@redhat.com>
>>>
>>> @@ -1061,10 +1065,14 @@ static int _find_parallel_space(struct a
>>> 				continue;	/* Next PV */
>>>
>>> 			if (alloc != ALLOC_ANYWHERE) {
>>> -				/* Don't allocate onto the log pv */
>>> -				if (ah->log_count &&
>>> -				    pvm->pv == ah->log_area.pv)
>>> -					continue;	/* Next PV */
>>> +				/* Don't allocate onto the log pvs */
>>> +				dm_list_iterate_items(aa, &ah->log_areas)
>>> +					if (pvm->pv == aa->pv)
>>> +						skip = 1;
>>
>>                                       You can 'break' soon after setting 
>> skip.
>
> If I use 'break' won't that break out of the 'dm_list_iterate_items' and 
> not the enclosing loop - giving an incorrect result?

IIRC, there are two dm_list_iterate_items loops. You can 'break' from the
inner loop after setting 'skip=1' and the next statement will check that
skip is set and execute 'continue' for the outer loop.

My understanding is that the current code executes the inner loop to
completion every time.

Thanks, Malahal.

Re: [PATCH 2 of 10] LVM: make log_area a list

[Jonathan Brassow]

On Oct 14, 2009, at 12:16 PM, malahal at us.ibm.com wrote:

> My understanding is that the current code executes the inner loop to
> completion every time.

I see what you are saying.  Ok, fixed in my local patch.

  brassow

[PATCH 2 of 10] LVM: make log_area a list

[Alasdair G Kergon]

On Thu, Oct 08, 2009 at 04:18:36PM -0500, Jon Brassow wrote:
> +	int skip = 0;

> +				dm_list_iterate_items(aa, &ah->log_areas)
> +					if (pvm->pv == aa->pv)
> +						skip = 1;
> +				if (skip) {
> +					skip = 0;
> +					continue;
> +				}

If 'skip' is just to break out of nested loops, please use something
like 'goto next_pv' instead.

Alasdair

[PATCH 2 of 10] LVM: make log_area a list

[Alasdair G Kergon]

On Thu, Oct 08, 2009 at 04:18:36PM -0500, Jon Brassow wrote:
> -	struct alloced_area log_area;	/* Extent used for log */
> +	struct dm_list log_areas;	/* Extents used for logs */
>  	struct dm_list alloced_areas[0];	/* Lists of areas in each stripe */

Rather than storing the log areas differently (in a list) from the other areas
(in an array), I'd prefer them all to be stored the same way.

I.e. In a simple contiguous 2-leg mirror plus mirrored log, you'd get
  leg1 as first item on alloced_areas[0] list,
  leg2 as first item on alloced_areas[1] list,
  log leg1 as first item on log_areas list, 
  log leg2 as second item on log_areas list.

I'd prefer to see:
  leg1 as first item on alloced_areas[0] list,
  leg2 as first item on alloced_areas[1] list,
  log leg1 as first item on log_areas[0] list, 
  log leg2 as first item on log_areas[1] list.
and of course log_areas[0] can be the same as alloced_areas[2].

This should be both simpler (logs are less of a special case and 
like any other area may include more than 1 segment) and more general
(we're asking to allocate several parallel areas, with different lengths
but sorted largest first and with different allocation constraints
applying to some of them).

Alasdair

[PATCH 2 of 10] LVM: make log_area a list

[Jonathan Brassow]

agk,

I've been thinking about this... What I'd really like to see done is a  
two phase allocation for mirrors.  The first phase would be for the  
legs and the second phase would be for the log(s).  If we cleaned  
things up in this way, there would be no need for the log_area(s) at  
all.  This would involve changing the code around a bit more, but  
would give us an easier path to adhering to various allocation  
policies.  Also, it would make function behave more like their  
original intent, rather than having these extra bolt-on items that  
seem to bloat functions every time we add a new feature.  If you are  
keen on this approach, would you settle for the posted patch for now?

In (weak) defense of the patch as it's written, please note that the  
'areas' field is an /array/ of 'dm_list' type.  Each element in the  
array is for a specific leg.  The elements are lists presumably to  
handle multiple segments per leg.  Logs, on the other hand, are never  
more than one segment, so I don't need something as complicated as an  
array of lists.  Either an array or a list would be fine. I thought it  
would be easier to use the list vs. the array.  Note also that  
_find_parallel_areas cycles through the log PVs to avoid them if  
possible... which complicates putting them together with the 'areas'  
array of lists.

If you still want me to attempt doing it as you describe, then I  
will.  Otherwise, I'd rather avoid doing this intermediate step and  
move straight to a two phase allocation.

Your thoughts?

  brassow


On Oct 15, 2009, at 6:18 PM, Alasdair G Kergon wrote:

> On Thu, Oct 08, 2009 at 04:18:36PM -0500, Jon Brassow wrote:
>> -	struct alloced_area log_area;	/* Extent used for log */
>> +	struct dm_list log_areas;	/* Extents used for logs */
>> 	struct dm_list alloced_areas[0];	/* Lists of areas in each stripe */
>
> Rather than storing the log areas differently (in a list) from the  
> other areas
> (in an array), I'd prefer them all to be stored the same way.
>
> I.e. In a simple contiguous 2-leg mirror plus mirrored log, you'd get
>  leg1 as first item on alloced_areas[0] list,
>  leg2 as first item on alloced_areas[1] list,
>  log leg1 as first item on log_areas list,
>  log leg2 as second item on log_areas list.
>
> I'd prefer to see:
>  leg1 as first item on alloced_areas[0] list,
>  leg2 as first item on alloced_areas[1] list,
>  log leg1 as first item on log_areas[0] list,
>  log leg2 as first item on log_areas[1] list.
> and of course log_areas[0] can be the same as alloced_areas[2].
>
> This should be both simpler (logs are less of a special case and
> like any other area may include more than 1 segment) and more general
> (we're asking to allocate several parallel areas, with different  
> lengths
> but sorted largest first and with different allocation constraints
> applying to some of them).
>
> Alasdair
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH 2 of 10] LVM: make log_area a list

[Jonathan Brassow]

Patch name: lvm-make-log_area-a-list.patch

The 'alloc_handle' structure only has space for one log_area.
We change that to a list to allow an arbitrary number of
log areas.

RFC: Jonathan Brassow <jbrassow@redhat.com>

Index: LVM2/lib/metadata/lv_manip.c
===================================================================
--- LVM2.orig/lib/metadata/lv_manip.c
+++ LVM2/lib/metadata/lv_manip.c
@@ -515,7 +515,7 @@ struct alloc_handle {
 
 	struct dm_list *parallel_areas;	/* PVs to avoid */
 
-	struct alloced_area log_area;	/* Extent used for log */
+	struct dm_list log_areas;	/* Extents used for logs */
 	struct dm_list alloced_areas[0];	/* Lists of areas in each stripe */
 };
 
@@ -582,6 +582,7 @@ static struct alloc_handle *_alloc_init(
 	ah->alloc = alloc;
 	ah->area_multiple = calc_area_multiple(segtype, area_count);
 
+	dm_list_init(&ah->log_areas);
 	for (s = 0; s < ah->area_count; s++)
 		dm_list_init(&ah->alloced_areas[s]);
 
@@ -763,10 +764,11 @@ static int _alloc_parallel_area(struct a
 		consume_pv_area(areas[s], area_len);
 
 	if (log_area) {
-		ah->log_area.pv = log_area->map->pv;
-		ah->log_area.pe = log_area->start;
-		ah->log_area.len = log_len;
-		consume_pv_area(log_area, ah->log_area.len);
+		aa[s].pv = log_area->map->pv;
+		aa[s].pe = log_area->start;
+		aa[s].len = log_len;
+		dm_list_add(&ah->log_areas, &aa[s].list);
+		consume_pv_area(log_area, log_len);
 	}
 
 	*ix += area_len * ah->area_multiple;
@@ -999,6 +1001,7 @@ static int _find_parallel_space(struct a
 	uint32_t log_len;
 	struct pv_area *log_area;
 	unsigned log_needs_allocating;
+	struct alloced_area *aa;
 
 	/* Is there enough total space? */
 	free_pes = pv_maps_size(pvms);
@@ -1061,10 +1064,10 @@ static int _find_parallel_space(struct a
 				continue;	/* Next PV */
 
 			if (alloc != ALLOC_ANYWHERE) {
-				/* Don't allocate onto the log pv */
-				if (ah->log_count &&
-				    pvm->pv == ah->log_area.pv)
-					continue;	/* Next PV */
+				/* Don't allocate onto the log pvs */
+				dm_list_iterate_items(aa, &ah->log_areas)
+					if (pvm->pv == aa->pv)
+						goto next_pv;
 
 				/* Avoid PVs used by existing parallel areas */
 				if (parallel_pvs)
@@ -1123,8 +1126,9 @@ static int _find_parallel_space(struct a
 		if ((contiguous || cling) && (preferred_count < ix_offset))
 			break;
 
-		log_needs_allocating = (ah->log_count && !ah->log_area.len) ?
-				       1 : 0;
+		log_needs_allocating = 0;
+		if (ah->log_count && dm_list_empty(&ah->log_areas))
+			log_needs_allocating = 1;
 
 		if (ix + ix_offset < ah->area_count +
 		   (log_needs_allocating ? ah->log_count : 0))
@@ -1197,6 +1201,7 @@ static int _allocate(struct alloc_handle
 	struct dm_list *pvms;
 	uint32_t areas_size;
 	alloc_policy_t alloc;
+	struct alloced_area *aa;
 
 	if (allocated >= new_extents && !ah->log_count) {
 		log_error("_allocate called with no work to do!");
@@ -1262,11 +1267,14 @@ static int _allocate(struct alloc_handle
 		goto out;
 	}
 
-	if (ah->log_count && !ah->log_area.len) {
-		log_error("Insufficient extents for log allocation "
-			  "for logical volume %s.",
-			  lv ? lv->name : "");
-		goto out;
+	if (ah->log_count) {
+		dm_list_iterate_items(aa, &ah->log_areas)
+			if (!aa->len) {
+				log_error("Insufficient extents for log "
+					  "allocation for logical volume %s.",
+					  lv ? lv->name : "");
+				goto out;
+			}
 	}
 
 	r = 1;
@@ -1543,10 +1551,23 @@ int lv_add_mirror_lvs(struct logical_vol
 
 /*
  * Turn an empty LV into a mirror log.
+ *
+ * Only for the addition of the first, linear log.
  */
 int lv_add_log_segment(struct alloc_handle *ah, struct logical_volume *log_lv)
 {
+	struct dm_list *l;
 	struct lv_segment *seg;
+	struct alloced_area *log_area, no_area;
+
+	l = dm_list_first(&ah->log_areas);
+	if (l) {
+		log_area = dm_list_item(l, struct alloced_area);
+		dm_list_del(&log_area->list);
+	} else {
+		log_area = &no_area;
+		memset(log_area, 0, sizeof(struct alloced_area));
+	}
 
 	if (dm_list_size(&log_lv->segments)) {
 		log_error("Log segments can only be added to an empty LV");
@@ -1556,19 +1577,22 @@ int lv_add_log_segment(struct alloc_hand
 	if (!(seg = alloc_lv_segment(log_lv->vg->cmd->mem,
 				     get_segtype_from_string(log_lv->vg->cmd,
 							     "striped"),
-				     log_lv, 0, ah->log_area.len, MIRROR_LOG,
-				     0, NULL, 1, ah->log_area.len, 0, 0, 0))) {
+				     log_lv, 0, log_area->len, MIRROR_LOG,
+				     0, NULL, 1, log_area->len, 0, 0, 0))) {
 		log_error("Couldn't allocate new mirror log segment.");
 		return 0;
 	}
 
-	if (!set_lv_segment_area_pv(seg, 0, ah->log_area.pv, ah->log_area.pe))
+	if (!set_lv_segment_area_pv(seg, 0, log_area->pv, log_area->pe))
 		return_0;
 
 	dm_list_add(&log_lv->segments, &seg->list);
-	log_lv->le_count += ah->log_area.len;
+	log_lv->le_count += log_area->len;
 	log_lv->size += (uint64_t) log_lv->le_count * log_lv->vg->extent_size;
 
+	if (log_area != &no_area)
+		dm_pool_free(ah->mem, log_area);
+
 	if (log_lv->vg->fid->fmt->ops->lv_setup &&
 	    !log_lv->vg->fid->fmt->ops->lv_setup(log_lv->vg->fid, log_lv))
 		return_0;