[PATCH]: Enable snapshots of mirrors

[PATCH]: Enable snapshots of mirrors

[Jonathan Brassow]

A few more touch-ups to the previous patch.  I've tested the following
('+'s mean it's working, '-'s mean it could use some attention):

pv*:
- commands ignored

vgcfgbackup/vgcfgrestore:
+ simple test shows it works

vgck:
+ seems to work, but I didn't try corrupting the VG

vgreduce --removemissing (--force):
+ properly down-converts mirror under snapshot if there is a device
failure

vg*:
- other vg commands/options ignored

lvchange:
+ '-an' of mirror properly shuts down snapshots too
+ '-ay' of mirror properly starts up snapshots too
+ '-a[ny]' of snapshot gives proper error message
+ '--resync' of mirror origin properly resync's it
- other options ignored

lvconvert:
+ down convert works (mirror to linear)
+ down convert works (mirror to mirror)
+ up convert works (linear to mirror) - both background and foreground
- up convert doesn't work (mirror to mirror) - disabled in lvconvert and
err msg added
+ convert between log types works (core <-> disk/redundant)

lvdisplay:
- largely ignored because I'm not familiar with normal output... but it
seems fine

lvextend:
- for origin, I get a strange message saying "snapshot origin volumes
can only be resized while inactive"
+ negative argument not permitted, of course.
+ for snapshot (cow device), I can add space
+ negative arg not permitted

lvreduce:
- origin "cannot be reduced in size yet."
+ snapshot (cow device) can be reduced

lvremove:
+ removing the origin asks if I want to remove the snapshot, then the
origin... the wording is a bit strange, but not affected by mirror
+ removing snapshot works

lvrename:
+ can rename both the mirror (properly handles hidden LVs) and snapshot

lvresize:
+ can grow/shrink snapshot
- cannot grow/shrink origins under a snapshot (but this has nothing to
do with mirror)

lvs:
+ properly displays layout and percentages

lvscan:
+ not familiar with the command, but output looks saine

 brassow

This patch
- takes out check that denies snapshots of mirrors
- takes out check denying conversion to/of mirrors under snapshots
- adds code to properly gather mirror sync %'age if it is an origin
- disallows mirror -> mirror upconvert when under a snapshot (need more
  time to debug problems with this).  For now, comment and disallow.

Signed-off-by: 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
@@ -2990,11 +2990,12 @@ int lv_create_single(struct volume_group
 					  "supported yet");
 				return 0;
 			}
-			if (org->status & MIRROR_IMAGE ||
-			    org->status & MIRROR_LOG ||
-			    org->status & MIRRORED) {
-				log_error("Snapshots and mirrors may not yet "
-					  "be mixed.");
+			if ((org->status & MIRROR_IMAGE) ||
+			    (org->status & MIRROR_LOG)) {
+				log_error("Snapshots of mirror %ss "
+					  "are not supported",
+					  (org->status & MIRROR_LOG) ?
+					  "log" : "image");
 				return 0;
 			}
 
Index: LVM2/tools/lvconvert.c
===================================================================
--- LVM2.orig/tools/lvconvert.c
+++ LVM2/tools/lvconvert.c
@@ -733,6 +733,20 @@ static int _lvconvert_mirrors(struct cmd
 				  "LV: use lvchange --resync first.");
 			return 0;
 		}
+
+		/*
+		 * We allow snapshots of mirrors, but for now, we
+		 * do not allow up converting mirrors that are under
+		 * snapshots.  The layering logic is somewhat complex,
+		 * and preliminary test show that the conversion can't
+		 * seem to get the correct %'age of completion.
+		 */
+		if (lv_is_origin(lv)) {
+			log_error("Can't add additional mirror images to "
+				  "mirrors that are under snapshots");
+			return 0;
+		}
+
 		/*
 		 * Log addition/removal should be done before the layer
 		 * insertion to make the end result consistent with
@@ -900,12 +914,6 @@ static int lvconvert_single(struct cmd_c
 		return ECMD_FAILED;
 	}
 
-	if (lv_is_origin(lv)) {
-		log_error("Can't convert logical volume \"%s\" under snapshot",
-			  lv->name);
-		return ECMD_FAILED;
-	}
-
 	if (lv_is_cow(lv)) {
 		log_error("Can't convert snapshot logical volume \"%s\"",
 			  lv->name);
Index: LVM2/lib/activate/dev_manager.c
===================================================================
--- LVM2.orig/lib/activate/dev_manager.c
+++ LVM2/lib/activate/dev_manager.c
@@ -551,16 +551,17 @@ int dev_manager_mirror_percent(struct de
 {
 	char *name;
 	const char *dlid;
+	char *suffix = (lv_is_origin(lv)) ? "real" : NULL;
 
 	/*
 	 * Build a name for the top layer.
 	 */
-	if (!(name = build_dm_name(dm->mem, lv->vg->name, lv->name, NULL)))
+	if (!(name = build_dm_name(dm->mem, lv->vg->name, lv->name, suffix)))
 		return_0;
 
 	/* FIXME dm_pool_free ? */
 
-	if (!(dlid = build_dlid(dm, lv->lvid.s, NULL))) {
+	if (!(dlid = build_dlid(dm, lv->lvid.s, suffix))) {
 		log_error("dlid build failed for %s", lv->name);
 		return 0;
 	}

[PATCH]: Enable snapshots of mirrors

[Alasdair G Kergon]

On Thu, Oct 22, 2009 at 01:54:21PM -0500, Jon Brassow wrote:
> pv*:
> - commands ignored
 
pvmove is the only one to test I think and it ought to be OK, still 
refusing to let you manipulate any devices concerned.

> vgck:
> + seems to work, but I didn't try corrupting the VG

vgck already gives incorrect exit status anyway, someone pointed
out this week...
 
> vg*:
> - other vg commands/options ignored
 
I'd want to know that vgmerge and vgsplit always behave correctly and
understand the entire 'stack' of objects must remain together and give
sensible error messages/deal with name conflicts properly in the various
different cases.

> lvchange:
> + '-an' of mirror properly shuts down snapshots too
> + '-ay' of mirror properly starts up snapshots too
> + '-a[ny]' of snapshot gives proper error message
> + '--resync' of mirror origin properly resync's it
> - other options ignored

check permissions-handling too and --refresh and readahead setting
propagation.  
 
> lvdisplay:
> - largely ignored because I'm not familiar with normal output... but it
> seems fine

Well, simply, can you easily tell how things are set up from its output,
or does more descriptive text need to be added?
(E.g. instead of saying just 'snapshot', saying 'snapshot of mirror')
 

Most of this code was written with stacked devices in mind, but this is
the first time we'll actually be turning on and using these code paths.

Alasdair

[PATCH]: Enable snapshots of mirrors

[Jonathan Brassow]

On Oct 22, 2009, at 2:45 PM, Alasdair G Kergon wrote:

> On Thu, Oct 22, 2009 at 01:54:21PM -0500, Jon Brassow wrote:
>> pv*:
>> - commands ignored
>
> pvmove is the only one to test I think and it ought to be OK, still
> refusing to let you manipulate any devices concerned.

yup, pvmove skips over the mirrors and the snapshots of the mirror.

>
>> vgck:
>> + seems to work, but I didn't try corrupting the VG
>
> vgck already gives incorrect exit status anyway, someone pointed
> out this week...
>
>> vg*:
>> - other vg commands/options ignored
>
> I'd want to know that vgmerge and vgsplit always behave correctly and
> understand the entire 'stack' of objects must remain together and give
> sensible error messages/deal with name conflicts properly in the  
> various
> different cases.

vgsplit does not allow PVs with mirrors or snapshots to move - "Can't  
split mirror/snapshot between two Volume Groups", but it does allow  
other PVs to move.

vgmerge allows VGs with and without snapshotted mirrors to be merged  
into VGs that do and don't have other LVs.

>
>> lvchange:
>> + '-an' of mirror properly shuts down snapshots too
>> + '-ay' of mirror properly starts up snapshots too
>> + '-a[ny]' of snapshot gives proper error message
>> + '--resync' of mirror origin properly resync's it
>> - other options ignored
>
> check permissions-handling too and --refresh and readahead setting
> propagation.

cursory check of --refresh shows it to work.

+ can't change RW permission of LV under snapshot
+ can change RW permission of snapshot

>
>> lvdisplay:
>> - largely ignored because I'm not familiar with normal output...  
>> but it
>> seems fine
>
> Well, simply, can you easily tell how things are set up from its  
> output,
> or does more descriptive text need to be added?
> (E.g. instead of saying just 'snapshot', saying 'snapshot of mirror')

It just says it is the source of snapshot 'foo' - nothing about  
mirror, but when I remove the snapshot, it doesn't say anything about  
it being mirrored either.

> Most of this code was written with stacked devices in mind, but this  
> is
> the first time we'll actually be turning on and using these code  
> paths.

Sure.  Let me know if you are satisfied with this basic level of  
testing, and I will check it in.

  brassow

[PATCH]: Enable snapshots of mirrors

[Alasdair G Kergon]

OK - I've checked this in.

Several 'loose ends' during my review (not all related to this change).

- lvdisplay needs some attention.  It's meant to be the 'user-friendly'
output tool for normal configurations so should spot and tell people 
they have a 'snapshot of a mirror' etc.

- vgsplit -n is not written to handle a stack of LVs.  It could handle
a mirror of a snapshot but not a snapshot of a mirror.  I've reversed
that, but the code needs rewriting to handle a generic stack of LVs.
There is a workaround using alternative syntax (without -n) so splits 
can still be done.  It's not a very big job to fix this, but I've
opened bz 530959.

- The single-character 'type' at the front of the attributes in 'lvs'
is now under impossible strain.  I've changed it to show 'o'
rather than 'm' in the current case, because you can normally tell it's a
mirror from other columns in the output.  In some cases there could be
2, 3 or more pieces of information to display there, so we're either
going to need more attributes appending, or to introduce one or more new
fields.

Alasdair

[PATCH]: Enable snapshots of mirrors

[Petr Rockai]

Hi,

it would be great if you could convert your tests into actual testcase code,
maybe gathered all under test/t-snapshots-of-mirrors.sh -- this would help
prevent any possible regressions in the future. It would also mean we remember
all the information you needed to gather to conduct these tests.

May you need any assistance with writing the tests, please don't hesitate to
contact me (IRC probably works best).

Also,

Jonathan Brassow <jbrassow@redhat.com> writes:
> lvconvert:
> + down convert works (mirror to linear)
> + down convert works (mirror to mirror)
> + up convert works (linear to mirror) - both background and foreground
> - up convert doesn't work (mirror to mirror) - disabled in lvconvert and
> err msg added
> + convert between log types works (core <-> disk/redundant)
could you please check what lvconvert --repair does under a snapshot? I would
probably look into the mirror->mirror upconversion, although that's only
relevant for a repair of 3+-way mirror.

The t-lvconvert-repair.sh script should aid you in constructing a testcase.

Thanks!
   Petr.

[PATCH]: Enable snapshots of mirrors

[Dave Wysochanski]

On Sun, 2009-11-01 at 13:21 +0100, Petr Rockai wrote:
> Hi,
> 
> it would be great if you could convert your tests into actual testcase code,
> maybe gathered all under test/t-snapshots-of-mirrors.sh -- this would help
> prevent any possible regressions in the future. It would also mean we remember
> all the information you needed to gather to conduct these tests.
> 
> May you need any assistance with writing the tests, please don't hesitate to
> contact me (IRC probably works best).
> 

Thanks Petr for mentioning this - we need to encourage unit tests being
written.  I second Petr's note as it will help us all in the long run.

[PATCH]: Enable snapshots of mirrors

[Jonathan Brassow]

ok, thanks for the reminder!

  brassow

On Nov 1, 2009, at 6:21 AM, Petr Rockai wrote:

> Hi,
>
> it would be great if you could convert your tests into actual  
> testcase code,
> maybe gathered all under test/t-snapshots-of-mirrors.sh -- this  
> would help
> prevent any possible regressions in the future. It would also mean  
> we remember
> all the information you needed to gather to conduct these tests.
>
> May you need any assistance with writing the tests, please don't  
> hesitate to
> contact me (IRC probably works best).
>
> Also,
>
> Jonathan Brassow <jbrassow@redhat.com> writes:
>> lvconvert:
>> + down convert works (mirror to linear)
>> + down convert works (mirror to mirror)
>> + up convert works (linear to mirror) - both background and  
>> foreground
>> - up convert doesn't work (mirror to mirror) - disabled in  
>> lvconvert and
>> err msg added
>> + convert between log types works (core <-> disk/redundant)
> could you please check what lvconvert --repair does under a  
> snapshot? I would
> probably look into the mirror->mirror upconversion, although that's  
> only
> relevant for a repair of 3+-way mirror.
>
> The t-lvconvert-repair.sh script should aid you in constructing a  
> testcase.
>
> Thanks!
>   Petr.
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel