[PATCH] unknown segment types, part 1

[PATCH] unknown segment types, part 1

[Petr Rockai]

Hi,

the attached patch makes LVM handle unknown segment types by a simple
copy-through mechanism, i.e. whenever an unknown segment is encountered, it is
left intact (and preserved) by LVM. We may still want to limit changes to LVs
that contain unknown segments, and maybe even some heuristic to find LV
references in unknown segments (hard to tell).

There's also one thread-unsafe bit in the patch, that's not so hard to fix, but
I'll do that when we know that this is the right direction we want to take.

(The patch itself will probably need another iteration. Consider this to be a
draft.)

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm-unknown-segments.diff
Type: text/x-diff
Size: 13574 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090708/e77bc47c/attachment.bin>

[PATCH] unknown segment types, part 1

[Milan Broz]

Petr Rockai wrote:
> the attached patch makes LVM handle unknown segment types by a simple
> copy-through mechanism, i.e. whenever an unknown segment is encountered, it is
> left intact (and preserved) by LVM. We may still want to limit changes to LVs
> that contain unknown segments, and maybe even some heuristic to find LV
> references in unknown segments (hard to tell).
> 
> There's also one thread-unsafe bit in the patch, that's not so hard to fix, but
> I'll do that when we know that this is the right direction we want to take.
> 
> (The patch itself will probably need another iteration. Consider this to be a
> draft.)

After short testing (yes, there are nice bugs :-) I have several questions:

- Why activate "unknown" segment by replacing it by error segment?
I think that ignoring LV including unknown segment (with some error message)
but not activate is better.

Or use some configurable option? 

- Do we want code to preserve (read: kill unknown options) metadata when
segment handler is not loaded?

Isn't better just allow only read-only metadata operation
(so "unknown" segment will appear only in-memory metadata, never on-disk).

Possibly add some option to vgck to "fix" unknown segments.

Milan

[PATCH] unknown segment types, part 1

[Milan Broz]

Milan Broz wrote:
> - Do we want code to preserve (read: kill unknown options) metadata when
> segment handler is not loaded?

scratch that.. this seems to be just bug in code :)

Actually only comments and some fancy formatting is lost. Is that correct?

Milan

[PATCH] unknown segment types, part 1

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> Actually only comments and some fancy formatting is lost. Is that correct?

Yes. We lose those because they are not parsed at all, so we cannot quite
reproduce them. Editing the metadata with LVM that understands the segment type
will restore the fancy comments and formatting (since these are hard-coded in
the export code for any given segment).

See also the testcase: doing a restore/backup cycle will restore the
(formatting-lost) volume group to exactly the same state as before.

Yours,
   Petr.

[PATCH] unknown segment types, part 1

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> - Why activate "unknown" segment by replacing it by error segment?
> I think that ignoring LV including unknown segment (with some error message)
> but not activate is better.
Good question, and I don't really know: this was the first thing that I thought
of and I particularly don't insist that it is a good idea. I left that in
because so far there's no other logic that would flag the LV to prevent
activation. Using error segment should at least limit any possible
damage. Maybe we should recycle the "partial" flag for this purpose as well: in
a sense, the LV "is there" but has parts (the unknown segment) missing. Or
maybe make a new flag to avoid possible confusion. What do you think?

> Or use some configurable option? 
I'd say no, unless it turns out to be really useful (which I doubt).

> - Do we want code to preserve (read: kill unknown options) metadata when
> segment handler is not loaded?
>
> Isn't better just allow only read-only metadata operation
> (so "unknown" segment will appear only in-memory metadata, never on-disk).
(non-issue, as discussed in the other sub-thread)

> Possibly add some option to vgck to "fix" unknown segments.
I'd say lvremove could do, in fact. There won't be much use for such a crippled
LV, presumably.

Yours,
   Petr.

[PATCH] unknown segment types, part 1

[Milan Broz]

I tried some fixes...

- flag segment with unknown flag and skip it for activation
(maybe not in correct place, but demonstrates what I mean)

- fix segment name (possible using of deallocated memory, easily
reproduced if e.g. creating new LV on metadat with unknown segments)

(please ignore some crypto stuff - I am playing with in on my git branch :)

Milan

---
 lib/activate/dev_manager.c |   10 ++++++++++
 lib/metadata/segtype.h     |    2 ++
 lib/unknown/unknown.c      |    4 ++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 894c20e..225b2a8 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -914,6 +914,16 @@ static int _add_segment_to_dtree(struct dev_manager *dm,
 		return 0;
 	}
 
+	if (seg_unknown(seg_present)) {
+		if (lv_is_visible(seg->lv))
+			log_error("Skipping LV %s containing unknown segment type %s.",
+				  seg->lv->name, seg_present->segtype->name);
+		else
+			log_debug("Skipping LV %s containing unknown segment type %s.",
+				  seg->lv->name, seg_present->segtype->name);
+		return 0;
+	}
+
 	/* Add mirror log */
 	if (seg->log_lv &&
 	    !_add_new_lv_to_dtree(dm, dtree, seg->log_lv, NULL))
diff --git a/lib/metadata/segtype.h b/lib/metadata/segtype.h
index c743ac3..8787fed 100644
--- a/lib/metadata/segtype.h
+++ b/lib/metadata/segtype.h
@@ -34,6 +34,7 @@ struct dev_manager;
 #define SEG_CANNOT_BE_ZEROED	0x00000040U
 #define SEG_MONITORED		0x00000080U
 #define SEG_AREAS_ENCRYPTED	0x00000100U
+#define SEG_UNKNOWN		0x80000000U
 
 #define seg_is_mirrored(seg)	((seg)->segtype->flags & SEG_AREAS_MIRRORED ? 1 : 0)
 #define seg_is_striped(seg)	((seg)->segtype->flags & SEG_AREAS_STRIPED ? 1 : 0)
@@ -44,6 +45,7 @@ struct dev_manager;
 #define seg_monitored(seg)	((seg)->segtype->flags & SEG_MONITORED ? 1 : 0)
 #define seg_is_encrypted(seg)	((seg)->segtype->flags & SEG_AREAS_ENCRYPTED ? 1 : 0)
 #define seg_is_keystore(seg)	(seg_is_encrypted(seg) && seg_cannot_be_zeroed(seg))
+#define seg_unknown(seg)	((seg)->segtype->flags & SEG_UNKNOWN ? 1 : 0)
 
 #define segtype_is_striped(segtype)	((segtype)->flags & SEG_AREAS_STRIPED ? 1 : 0)
 #define segtype_is_mirrored(segtype)	((segtype)->flags & SEG_AREAS_MIRRORED ? 1 : 0)
diff --git a/lib/unknown/unknown.c b/lib/unknown/unknown.c
index 6fa44b4..e2ed807 100644
--- a/lib/unknown/unknown.c
+++ b/lib/unknown/unknown.c
@@ -93,9 +93,9 @@ struct segment_type *init_unknown_segtype(struct cmd_context *cmd, const char *n
 
 	segtype->cmd = cmd;
 	segtype->ops = &_unknown_ops;
-	segtype->name = name;
+	segtype->name = dm_pool_strdup(cmd->mem, name);
 	segtype->private = NULL;
-	segtype->flags = SEG_VIRTUAL | SEG_CANNOT_BE_ZEROED;
+	segtype->flags = SEG_UNKNOWN | SEG_VIRTUAL | SEG_CANNOT_BE_ZEROED;
 
 	log_very_verbose("Initialised segtype: %s", segtype->name);
 

[PATCH, v2] unknown segment types

[Petr Rockai]

Hi,

here comes an amended patch for unknown segment types. This time, it won't let
people tinker with volume groups with unknown segtypes in it, but comes with
provisions to let tools override this safeguard when reasonable.

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm-unknown-segments-2.diff
Type: text/x-diff
Size: 19394 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090721/9bc8d801/attachment.bin>

[PATCH, v2] unknown segment types

[Petr Rockai]

Petr Rockai <prockai@redhat.com> writes:
> here comes an amended patch for unknown segment types. This time, it won't let
> people tinker with volume groups with unknown segtypes in it, but comes with
> provisions to let tools override this safeguard when reasonable.
a rebase of this same patch for current mainline (checked that it still works,
too) is attached.

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: unknown-segtype.diff
Type: text/x-diff
Size: 19394 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090929/a8dae80b/attachment.bin>

[PATCH, v2] unknown segment types

[Dave Wysochanski]

On Tue, 2009-09-29 at 16:42 +0200, Petr Rockai wrote:
> Petr Rockai <prockai@redhat.com> writes:
> > here comes an amended patch for unknown segment types. This time, it won't let
> > people tinker with volume groups with unknown segtypes in it, but comes with
> > provisions to let tools override this safeguard when reasonable.
> a rebase of this same patch for current mainline (checked that it still works,
> too) is attached.
> 

Can you say where this applied previously?  Git hash or CVS info?
I've tried a few combinations around 9/29 but nothing applies cleanly so
far.

[PATCH, v3] unknown segment types

[Petr Rockai]

Hi again,

I have done some changes to the patch, namely (we have discussed those with
Alasdair on IRC):

Wed Oct  7 22:25:28 CEST 2009  Petr Rockai <me@mornfall.net>
  * Move filtering logic from export to import, cleaning up code a bit.
Wed Oct  7 22:09:05 CEST 2009  Petr Rockai <me@mornfall.net>
  * Get rid of a global variable (use batons instead).
Wed Oct  7 22:08:22 CEST 2009  Petr Rockai <me@mornfall.net>
  * Improve log_warn message (unknown -> unrecognised).

Dave, this patch should apply cleanly to CVS as of *right now*. I don't know
about the previous one.

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: unknown-segtype.diff
Type: text/x-diff
Size: 20444 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20091007/b98963c1/attachment.bin>