[PATCH] LVM: New flag, LV_REBUILD

[PATCH] LVM: New flag, LV_REBUILD

[Jonathan Brassow]

Any objections to a new flag?

 brassow

Add new flag, LV_REBUILD.

Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
the DM CTR table.)  However, I don't want to use a flag that gets written to
the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
suite the purpose adequately.

This patch proposes and uses a new flag, LV_REBUILD.


Index: LVM2/lib/metadata/metadata-exported.h
===================================================================
--- LVM2.orig/lib/metadata/metadata-exported.h
+++ LVM2/lib/metadata/metadata-exported.h
@@ -61,7 +61,9 @@
 //#define VIRTUAL		UINT64_C(0x00010000)	/* LV - internal use only */
 #define MIRROR_LOG		UINT64_C(0x00020000)	/* LV */
 #define MIRROR_IMAGE		UINT64_C(0x00040000)	/* LV */
+
 #define LV_NOTSYNCED		UINT64_C(0x00080000)	/* LV */
+#define LV_REBUILD		UINT64_C(0x00100000)	/* LV */
 //#define PRECOMMITTED		UINT64_C(0x00200000)	/* VG - internal use only */
 #define CONVERTING		UINT64_C(0x00400000)	/* LV */
 
Index: LVM2/lib/metadata/raid_manip.c
===================================================================
--- LVM2.orig/lib/metadata/raid_manip.c
+++ LVM2/lib/metadata/raid_manip.c
@@ -440,7 +440,7 @@ static int _alloc_image_component(struct
 		return 0;
 	}
 
-	status = LVM_READ | LVM_WRITE | LV_NOTSYNCED | type;
+	status = LVM_READ | LVM_WRITE | LV_REBUILD | type;
 	tmp_lv = lv_create_empty(img_name, NULL, status, ALLOC_INHERIT, lv->vg);
 	if (!tmp_lv) {
 		log_error("Failed to allocate new raid component, %s", img_name);
@@ -588,7 +588,7 @@ static int _raid_add_images(struct logic
 	 */
 	if (seg_is_linear(seg)) {
 		/* A complete resync will be done, no need to mark each sub-lv */
-		status_mask = ~(LV_NOTSYNCED);
+		status_mask = ~(LV_REBUILD);
 
 		if (!(lvl = dm_pool_alloc(lv->vg->vgmem, sizeof(*lvl)))) {
 			log_error("Memory allocation failed");
@@ -1335,8 +1335,8 @@ static int _convert_mirror_to_raid1(stru
 		log_debug("Adding %s to %s", lvl->lv->name, lv->name);
 
 		/* Images are known to be in-sync */
-		lvl->lv->status &= ~LV_NOTSYNCED;
-		first_seg(lvl->lv)->status &= ~LV_NOTSYNCED;
+		lvl->lv->status &= ~LV_REBUILD;
+		first_seg(lvl->lv)->status &= ~LV_REBUILD;
 		lv_set_hidden(lvl->lv);
 
 		if (!set_lv_segment_area_lv(seg, s, lvl->lv, 0,
Index: LVM2/lib/raid/raid.c
===================================================================
--- LVM2.orig/lib/raid/raid.c
+++ LVM2/lib/raid/raid.c
@@ -183,7 +183,7 @@ static int _raid_add_target_line(struct 
 	}
 
 	for (s = 0; s < seg->area_count; s++)
-		if (seg_lv(seg, s)->status & LV_NOTSYNCED)
+		if (seg_lv(seg, s)->status & LV_REBUILD)
 			rebuilds |= 1 << s;
 
 	if (!dm_tree_node_add_raid_target(node, len, _raid_name(seg),

[PATCH] LVM: New flag, LV_REBUILD

[Zdenek Kabelac]

Dne 10.11.2011 23:26, Jonathan Brassow napsal(a):
> Any objections to a new flag?
> 
>  brassow
> 
> Add new flag, LV_REBUILD.
> 
> Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
> sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
> the DM CTR table.)  However, I don't want to use a flag that gets written to
> the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
> suite the purpose adequately.
> 
> This patch proposes and uses a new flag, LV_REBUILD.

Hmm and how is this going to work in some crash scenarios - are you always
able to deduce the raid was not yet build ?

Also it seems we encode each flag into lib/format_text/flags.c


> 
> 
> Index: LVM2/lib/metadata/metadata-exported.h
> ===================================================================
> --- LVM2.orig/lib/metadata/metadata-exported.h
> +++ LVM2/lib/metadata/metadata-exported.h
> @@ -61,7 +61,9 @@
>  //#define VIRTUAL		UINT64_C(0x00010000)	/* LV - internal use only */
>  #define MIRROR_LOG		UINT64_C(0x00020000)	/* LV */
>  #define MIRROR_IMAGE		UINT64_C(0x00040000)	/* LV */
> +
>  #define LV_NOTSYNCED		UINT64_C(0x00080000)	/* LV */
> +#define LV_REBUILD		UINT64_C(0x00100000)	/* LV */
>  //#define PRECOMMITTED		UINT64_C(0x00200000)	/* VG - internal use only */
>  #define CONVERTING		UINT64_C(0x00400000)	/* LV */
>  
> Index: LVM2/lib/metadata/raid_manip.c
> ===================================================================
> --- LVM2.orig/lib/metadata/raid_manip.c
> +++ LVM2/lib/metadata/raid_manip.c
> @@ -440,7 +440,7 @@ static int _alloc_image_component(struct
>  		return 0;
>  	}
>  
> -	status = LVM_READ | LVM_WRITE | LV_NOTSYNCED | type;
> +	status = LVM_READ | LVM_WRITE | LV_REBUILD | type;
>  	tmp_lv = lv_create_empty(img_name, NULL, status, ALLOC_INHERIT, lv->vg);
>  	if (!tmp_lv) {
>  		log_error("Failed to allocate new raid component, %s", img_name);
> @@ -588,7 +588,7 @@ static int _raid_add_images(struct logic
>  	 */
>  	if (seg_is_linear(seg)) {
>  		/* A complete resync will be done, no need to mark each sub-lv */
> -		status_mask = ~(LV_NOTSYNCED);
> +		status_mask = ~(LV_REBUILD);
>  
>  		if (!(lvl = dm_pool_alloc(lv->vg->vgmem, sizeof(*lvl)))) {
>  			log_error("Memory allocation failed");
> @@ -1335,8 +1335,8 @@ static int _convert_mirror_to_raid1(stru
>  		log_debug("Adding %s to %s", lvl->lv->name, lv->name);
>  
>  		/* Images are known to be in-sync */
> -		lvl->lv->status &= ~LV_NOTSYNCED;
> -		first_seg(lvl->lv)->status &= ~LV_NOTSYNCED;
> +		lvl->lv->status &= ~LV_REBUILD;
> +		first_seg(lvl->lv)->status &= ~LV_REBUILD;
>  		lv_set_hidden(lvl->lv);
>  
>  		if (!set_lv_segment_area_lv(seg, s, lvl->lv, 0,
> Index: LVM2/lib/raid/raid.c
> ===================================================================
> --- LVM2.orig/lib/raid/raid.c
> +++ LVM2/lib/raid/raid.c
> @@ -183,7 +183,7 @@ static int _raid_add_target_line(struct 
>  	}
>  
>  	for (s = 0; s < seg->area_count; s++)
> -		if (seg_lv(seg, s)->status & LV_NOTSYNCED)
> +		if (seg_lv(seg, s)->status & LV_REBUILD)
>  			rebuilds |= 1 << s;
>  
>  	if (!dm_tree_node_add_raid_target(node, len, _raid_name(seg),
> 
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH] LVM: New flag, LV_REBUILD

[Jonathan Brassow]

On Nov 11, 2011, at 10:23 AM, Zdenek Kabelac wrote:

> Dne 10.11.2011 23:26, Jonathan Brassow napsal(a):
>> Any objections to a new flag?
>> 
>> brassow
>> 
>> Add new flag, LV_REBUILD.
>> 
>> Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
>> sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
>> the DM CTR table.)  However, I don't want to use a flag that gets written to
>> the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
>> suite the purpose adequately.
>> 
>> This patch proposes and uses a new flag, LV_REBUILD.
> 
> Hmm and how is this going to work in some crash scenarios - are you always
> able to deduce the raid was not yet build ?
> 
> Also it seems we encode each flag into lib/format_text/flags.c

Thanks for looking over the patches.  You are right.  If the flag is not permanent, the machine could fail after the vg_commit, but before the resume - resulting in the devices never being rebuilt.

The problem from the other side is that if the flag is permanent (is written to metadata), then the machine could crash after the resume has properly prep'ed the device for a rebuild but before clearing the flag for the next activation.  This would result in the devices being rebuilt every time the LV is activated.

Perhaps I will work on some post processing for lvchange/vgchange, where certain flags are cleared after activation has been assured.  Or perhaps a flag could be added to the VG on-disk metadata that would indicate transaction has failed part-way through.

I heard thinp is using generation numbers to allow the kernel to ignore commands it has already performed.  However, this implies that the LVM metadata is somehow changed after the action is completed - otherwise it would simply redo these actions upon every activation.  Perhaps I could align my code with that somehow?

In the mean time, I can make the flags so they are written to disk.

 brassow
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20111111/90f8d404/attachment.htm>

[PATCH - v2] LVM: New flag, LV_REBUILD

[Jonathan Brassow]

Changes from previous:
- New rebuild flag is now written to on-disk LVM metadata
- An additional metadata write/commit is necessary to clear the flag
after a proper resume
- flags.c file updated

 brassow

Add new flag, LV_REBUILD.

Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
the DM CTR table.)  However, I don't want to use a flag that gets written to
the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
suite the purpose adequately.

This patch proposes and uses a new flag, LV_REBUILD.


Index: LVM2/lib/metadata/metadata-exported.h
===================================================================
--- LVM2.orig/lib/metadata/metadata-exported.h
+++ LVM2/lib/metadata/metadata-exported.h
@@ -61,7 +61,9 @@
 //#define VIRTUAL		UINT64_C(0x00010000)	/* LV - internal use only */
 #define MIRROR_LOG		UINT64_C(0x00020000)	/* LV */
 #define MIRROR_IMAGE		UINT64_C(0x00040000)	/* LV */
+
 #define LV_NOTSYNCED		UINT64_C(0x00080000)	/* LV */
+#define LV_REBUILD		UINT64_C(0x00100000)	/* LV - internal use only */
 //#define PRECOMMITTED		UINT64_C(0x00200000)	/* VG - internal use only */
 #define CONVERTING		UINT64_C(0x00400000)	/* LV */
 
Index: LVM2/lib/metadata/raid_manip.c
===================================================================
--- LVM2.orig/lib/metadata/raid_manip.c
+++ LVM2/lib/metadata/raid_manip.c
@@ -440,7 +440,7 @@ static int _alloc_image_component(struct
 		return 0;
 	}
 
-	status = LVM_READ | LVM_WRITE | LV_NOTSYNCED | type;
+	status = LVM_READ | LVM_WRITE | LV_REBUILD | type;
 	tmp_lv = lv_create_empty(img_name, NULL, status, ALLOC_INHERIT, lv->vg);
 	if (!tmp_lv) {
 		log_error("Failed to allocate new raid component, %s", img_name);
@@ -569,6 +569,7 @@ static int _alloc_rmeta_for_lv(struct lo
 static int _raid_add_images(struct logical_volume *lv,
 			    uint32_t new_count, struct dm_list *pvs)
 {
+	int rebuild_flag_cleared = 0;
 	uint32_t s;
 	uint32_t old_count = lv_raid_image_count(lv);
 	uint32_t count = new_count - old_count;
@@ -588,7 +589,7 @@ static int _raid_add_images(struct logic
 	 */
 	if (seg_is_linear(seg)) {
 		/* A complete resync will be done, no need to mark each sub-lv */
-		status_mask = ~(LV_NOTSYNCED);
+		status_mask = ~(LV_REBUILD);
 
 		if (!(lvl = dm_pool_alloc(lv->vg->vgmem, sizeof(*lvl)))) {
 			log_error("Memory allocation failed");
@@ -751,6 +752,27 @@ to be left for these sub-lvs.
 		return 0;
 	}
 
+	/*
+	 * Now that the 'REBUILD' has made its way to the kernel, we must
+	 * remove the flag so that the individual devices are not rebuilt
+	 * upon every activation.
+	 */
+	seg = first_seg(lv);
+	for (s = 0; s < seg->area_count; s++) {
+		if ((seg_lv(seg, s)->status & LV_REBUILD) ||
+		    (seg_metalv(seg, s)->status & LV_REBUILD)) {
+			seg_metalv(seg, s)->status &= ~LV_REBUILD;
+			seg_lv(seg, s)->status &= ~LV_REBUILD;
+			rebuild_flag_cleared = 1;
+		}
+	}
+	if (rebuild_flag_cleared &&
+	    (!vg_write(lv->vg) || !vg_commit(lv->vg))) {
+		log_error("Failed to clear REBUILD flag for %s/%s components",
+			  lv->vg->name, lv->name);
+		return 0;
+	}
+
 	return 1;
 
 fail:
@@ -1335,8 +1357,8 @@ static int _convert_mirror_to_raid1(stru
 		log_debug("Adding %s to %s", lvl->lv->name, lv->name);
 
 		/* Images are known to be in-sync */
-		lvl->lv->status &= ~LV_NOTSYNCED;
-		first_seg(lvl->lv)->status &= ~LV_NOTSYNCED;
+		lvl->lv->status &= ~LV_REBUILD;
+		first_seg(lvl->lv)->status &= ~LV_REBUILD;
 		lv_set_hidden(lvl->lv);
 
 		if (!set_lv_segment_area_lv(seg, s, lvl->lv, 0,
Index: LVM2/lib/raid/raid.c
===================================================================
--- LVM2.orig/lib/raid/raid.c
+++ LVM2/lib/raid/raid.c
@@ -183,7 +183,7 @@ static int _raid_add_target_line(struct 
 	}
 
 	for (s = 0; s < seg->area_count; s++)
-		if (seg_lv(seg, s)->status & LV_NOTSYNCED)
+		if (seg_lv(seg, s)->status & LV_REBUILD)
 			rebuilds |= 1 << s;
 
 	if (!dm_tree_node_add_raid_target(node, len, _raid_name(seg),
Index: LVM2/lib/format_text/flags.c
===================================================================
--- LVM2.orig/lib/format_text/flags.c
+++ LVM2/lib/format_text/flags.c
@@ -56,6 +56,7 @@ static const struct flag _lv_flags[] = {
 	{PVMOVE, "PVMOVE", STATUS_FLAG},
 	{LOCKED, "LOCKED", STATUS_FLAG},
 	{LV_NOTSYNCED, "NOTSYNCED", STATUS_FLAG},
+	{LV_REBUILD, "REBUILD", STATUS_FLAG},
 	{RAID, NULL, 0},
 	{RAID_META, NULL, 0},
 	{RAID_IMAGE, NULL, 0},

[PATCH - v2] LVM: New flag, LV_REBUILD

[Zdenek Kabelac]

Dne 14.11.2011 19:06, Jonathan Brassow napsal(a):
> Changes from previous:
> - New rebuild flag is now written to on-disk LVM metadata
> - An additional metadata write/commit is necessary to clear the flag
> after a proper resume
> - flags.c file updated
>
>   brassow
>
> Add new flag, LV_REBUILD.
>
> Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
> sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
> the DM CTR table.)  However, I don't want to use a flag that gets written to
> the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
> suite the purpose adequately.
>
> This patch proposes and uses a new flag, LV_REBUILD.

Hmm so now - when it's written to disk - it looks like  LV_REBUILD and 
LV_NOTSYNCED is mostly the same meaning - except  one is used in raid
and original in mirror ?

So I think it's easier to keep just one flag ?
(Well the bit field space is somewhat limited)

Zdenek

[PATCH - v2] LVM: New flag, LV_REBUILD

[Jonathan Brassow]

On Nov 16, 2011, at 2:06 PM, Zdenek Kabelac wrote:

> Dne 14.11.2011 19:06, Jonathan Brassow napsal(a):
>> Changes from previous:
>> - New rebuild flag is now written to on-disk LVM metadata
>> - An additional metadata write/commit is necessary to clear the flag
>> after a proper resume
>> - flags.c file updated
>> 
>>  brassow
>> 
>> Add new flag, LV_REBUILD.
>> 
>> Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
>> sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
>> the DM CTR table.)  However, I don't want to use a flag that gets written to
>> the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
>> suite the purpose adequately.
>> 
>> This patch proposes and uses a new flag, LV_REBUILD.
> 
> Hmm so now - when it's written to disk - it looks like  LV_REBUILD and LV_NOTSYNCED is mostly the same meaning - except  one is used in raid
> and original in mirror ?
> 
> So I think it's easier to keep just one flag ?
> (Well the bit field space is somewhat limited)

Perhaps.  I've considered this and certainly it'd be easy for me to leave things the way they are.  However, LV_NOTSYNCED means that the LV was created (or extended) with '--nosync', and signifies that it has not and will not undergo a complete synchronization.  LV_REBUILD, on the other hand, means precisely the opposite - the LV is to undergo a complete synchronization now.  LV_NOTSYNCED signifies an attribute of the LV, while LV_REBUILD signifies an action that needs to be taken.

The only reason the difference is not more obvious is because you can look at 'LV_NOTSYNCED' sideways and sort-of make it work.  "These sub-LVs are not in-sync right now, and must be made to be in-sync." - or some similar thinking could get you there.

I'm good either way, but I'd prefer the new flag...

 brassow

[PATCH - v2] LVM: New flag, LV_REBUILD

[Jonathan Brassow]

On Nov 18, 2011, at 10:05 AM, Jonathan Brassow wrote:

> 
> On Nov 16, 2011, at 2:06 PM, Zdenek Kabelac wrote:
> 
>> Dne 14.11.2011 19:06, Jonathan Brassow napsal(a):
>>> Changes from previous:
>>> - New rebuild flag is now written to on-disk LVM metadata
>>> - An additional metadata write/commit is necessary to clear the flag
>>> after a proper resume
>>> - flags.c file updated
>>> 
>>> brassow
>>> 
>>> Add new flag, LV_REBUILD.
>>> 
>>> Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
>>> sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
>>> the DM CTR table.)  However, I don't want to use a flag that gets written to
>>> the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
>>> suite the purpose adequately.
>>> 
>>> This patch proposes and uses a new flag, LV_REBUILD.
>> 
>> Hmm so now - when it's written to disk - it looks like  LV_REBUILD and LV_NOTSYNCED is mostly the same meaning - except  one is used in raid
>> and original in mirror ?
>> 
>> So I think it's easier to keep just one flag ?
>> (Well the bit field space is somewhat limited)
> 
> Perhaps.  I've considered this and certainly it'd be easy for me to leave things the way they are.  However, LV_NOTSYNCED means that the LV was created (or extended) with '--nosync', and signifies that it has not and will not undergo a complete synchronization.  LV_REBUILD, on the other hand, means precisely the opposite - the LV is to undergo a complete synchronization now.  LV_NOTSYNCED signifies an attribute of the LV, while LV_REBUILD signifies an action that needs to be taken.
> 
> The only reason the difference is not more obvious is because you can look at 'LV_NOTSYNCED' sideways and sort-of make it work.  "These sub-LVs are not in-sync right now, and must be made to be in-sync." - or some similar thinking could get you there.
> 
> I'm good either way, but I'd prefer the new flag...

Now that I think a little more on it, I'm becoming even more in favor of the separate flag.

Consider the case we discussed earlier, where the machine dies after the vg_commit but before the resume.  This leaves the 'rebuild' DM table argument yet to be supplied to the kernel in order to clear the device's bitmap.  When the machine comes up and it is required to clean-up the flags after activation, it would complicate the process to force the code to know when the LV_NOTSYNCED flag means that the LV was not synced and to leave the flag, or whether it means the LV needs to be synced/rebuilt and the flag must be cleared.

 brassow

[PATCH - v2] LVM: New flag, LV_REBUILD

[Zdenek Kabelac]

Dne 18.11.2011 18:11, Jonathan Brassow napsal(a):
>
> On Nov 18, 2011, at 10:05 AM, Jonathan Brassow wrote:
>
>>
>> On Nov 16, 2011, at 2:06 PM, Zdenek Kabelac wrote:
>>
>>> Dne 14.11.2011 19:06, Jonathan Brassow napsal(a):
>>>> Changes from previous:
>>>> - New rebuild flag is now written to on-disk LVM metadata
>>>> - An additional metadata write/commit is necessary to clear the flag
>>>> after a proper resume
>>>> - flags.c file updated
>>>>
>>>> brassow
>>>>
>>>> Add new flag, LV_REBUILD.
>>>>
>>>> Until now, I had been using the LV_NOTSYNCED as a flag to indicate that RAID
>>>> sub-LVs needed to be rebuilt.  (The 'rebuild' parameter is then specified in
>>>> the DM CTR table.)  However, I don't want to use a flag that gets written to
>>>> the LVM metadata... and the LV_NOTSYNCED flag's original meaning does not
>>>> suite the purpose adequately.
>>>>
>>>> This patch proposes and uses a new flag, LV_REBUILD.
>>>
>>> Hmm so now - when it's written to disk - it looks like  LV_REBUILD and LV_NOTSYNCED is mostly the same meaning - except  one is used in raid
>>> and original in mirror ?
>>>
>>> So I think it's easier to keep just one flag ?
>>> (Well the bit field space is somewhat limited)
>>
>> Perhaps.  I've considered this and certainly it'd be easy for me to leave things the way they are.  However, LV_NOTSYNCED means that the LV was created (or extended) with '--nosync', and signifies that it has not and will not undergo a complete synchronization.  LV_REBUILD, on the other hand, means precisely the opposite - the LV is to undergo a complete synchronization now.  LV_NOTSYNCED signifies an attribute of the LV, while LV_REBUILD signifies an action that needs to be taken.
>>
>> The only reason the difference is not more obvious is because you can look at 'LV_NOTSYNCED' sideways and sort-of make it work.  "These sub-LVs are not in-sync right now, and must be made to be in-sync." - or some similar thinking could get you there.
>>
>> I'm good either way, but I'd prefer the new flag...
>
> Now that I think a little more on it, I'm becoming even more in favor of the separate flag.
>
> Consider the case we discussed earlier, where the machine dies after the vg_commit but before the resume.  This leaves the 'rebuild' DM table argument yet to be supplied to the kernel in order to clear the device's bitmap.  When the machine comes up and it is required to clean-up the flags after activation, it would complicate the process to force the code to know when the LV_NOTSYNCED flag means that the LV was not synced and to leave the flag, or whether it means the LV needs to be synced/rebuilt and the flag must be cleared.


If the rebuild is different state from not_synced - i.e.  raid will use both 
states - then you surely need new flag.

I've been just having impression that not_synced is something only from 
mirrors, and rebuild would be used only for raid.

Zdenek