lvconvert error when converting a linear to a mirror

lvconvert error when converting a linear to a mirror

[Dave Wysochanski]

I keep hitting the below error path when using lvconvert on a linear
volume that is spread across multiple PVs.

	/*
         * FIXME This check used to precede mirror->mirror conversion
	 * but didn't affect mirror->linear or linear->mirror. I do
	 * not understand what is its intention, in fact.
	 */
	if (dm_list_size(&lv->segments) != 1) {
		log_error("Logical volume %s has multiple "
			  "mirror segments.", lv->name);
		return 0;
	}

I created the linear volume by using PE ranges as follows:
# tools/lvm lvcreate -L 64M -n lv1 vg1 /dev/loop0:0-3 /dev/loop1:0-7 /dev/loop2:0-3

The lvconvert command I'm using is:
# tools/lvm lvconvert -m1 vg1/lv1 
  Logical volume lv1 has multiple mirror segments.

I'm not that familiar with lvconvert code, but looking through it
quickly, it looks like it should handle this case ok.  I removed the
above check and the mirror seemed to be created fine.  Is this check
there for some other reason though?  The comment above indicates it is
not clear to at least one other person.  If we are checking mirror
segments, perhaps we should add in a lv->MIRRORED flag check as well?

NOTE: I hit this as a side issue so I don't want to get sidetracked
indefinitely but figured it was worth a mention.

lvconvert error when converting a linear to a mirror

[Jun'ichi Nomura]

Hi Dave,

Dave Wysochanski wrote:
> I keep hitting the below error path when using lvconvert on a linear
> volume that is spread across multiple PVs.
> 
> 	/*
>          * FIXME This check used to precede mirror->mirror conversion
> 	 * but didn't affect mirror->linear or linear->mirror. I do
> 	 * not understand what is its intention, in fact.
> 	 */
> 	if (dm_list_size(&lv->segments) != 1) {
> 		log_error("Logical volume %s has multiple "
> 			  "mirror segments.", lv->name);
> 		return 0;
> 	}
> 
> I created the linear volume by using PE ranges as follows:
> # tools/lvm lvcreate -L 64M -n lv1 vg1 /dev/loop0:0-3 /dev/loop1:0-7 /dev/loop2:0-3
> 
> The lvconvert command I'm using is:
> # tools/lvm lvconvert -m1 vg1/lv1 
>   Logical volume lv1 has multiple mirror segments.
> 
> I'm not that familiar with lvconvert code, but looking through it
> quickly, it looks like it should handle this case ok.  I removed the
> above check and the mirror seemed to be created fine.  Is this check
> there for some other reason though?  The comment above indicates it is

I'm not the person adding the check but the intention of the check
should be for conversion from already mirrored LV and it should not
be applied to linear-to-mirror conversion.

In a lot of places, mirrored LV is assumed to be single-segment
and the attributes of the mirrored LV (such as log_lv and the number
of mirror images) are obtained from the first segment
(see the use of first_seg()).

> not clear to at least one other person.  If we are checking mirror
> segments, perhaps we should add in a lv->MIRRORED flag check as well?

I think that's ok.

And a comment for this comment in the code:

>          * FIXME This check used to precede mirror->mirror conversion
> 	 * but didn't affect mirror->linear or linear->mirror. I do
> 	 * not understand what is its intention, in fact.

if the check wasn't done for mirror-to-linear conversion, that's a bug.
It seems the bug has been there since the initial version of lvconvert.c. :)

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation

[PATCH] Re: [lvm-devel] lvconvert error when converting a linear to a mirror

[Petr Rockai]

Hi,

"Jun'ichi Nomura" <j-nomura@ce.jp.nec.com> writes:
> And a comment for this comment in the code:
>
>>          * FIXME This check used to precede mirror->mirror conversion
>> 	 * but didn't affect mirror->linear or linear->mirror. I do
>> 	 * not understand what is its intention, in fact.
>
> if the check wasn't done for mirror-to-linear conversion, that's a bug.
> It seems the bug has been there since the initial version of lvconvert.c. :)

I am the author of that FIXME comment there. It seems, that Dave's proposal of
adding a lv->status & MIRRORED check is the right solution here. Thanks both
for noticing and explaining the issue, at the time I was implementing lvconvert
--repair, it was not clear to me.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-multiseg-mirror-check.diff
Type: text/x-diff
Size: 797 bytes
Desc: lvconvert-multiseg-mirror-check.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090519/a028f767/attachment.bin>
-------------- next part --------------

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] Re: [lvm-devel] lvconvert error when converting a linear to a mirror

[Milan Broz]

Petr Rockai wrote:
> Hi,
> 
> "Jun'ichi Nomura" <j-nomura@ce.jp.nec.com> writes:
>> And a comment for this comment in the code:
>>
>>>          * FIXME This check used to precede mirror->mirror conversion
>>> 	 * but didn't affect mirror->linear or linear->mirror. I do
>>> 	 * not understand what is its intention, in fact.
>> if the check wasn't done for mirror-to-linear conversion, that's a bug.
>> It seems the bug has been there since the initial version of lvconvert.c. :)
> 
> I am the author of that FIXME comment there. It seems, that Dave's proposal of
> adding a lv->status & MIRRORED check is the right solution here. Thanks both
> for noticing and explaining the issue, at the time I was implementing lvconvert
> --repair, it was not clear to me.

Ack,

(I think we already discussed several times why this check is here, 
so simple disabling multisegment mirrors is ok here.

Maybe error message could explain it to user more precisely too?)


Milan

[PATCH] Re: [lvm-devel] lvconvert error when converting a linear to a mirror

[Dave Wysochanski]

On Tue, 2009-05-19 at 08:35 +0200, Petr Rockai wrote:
> Hi,
> 
> "Jun'ichi Nomura" <j-nomura@ce.jp.nec.com> writes:
> > And a comment for this comment in the code:
> >
> >>          * FIXME This check used to precede mirror->mirror conversion
> >> 	 * but didn't affect mirror->linear or linear->mirror. I do
> >> 	 * not understand what is its intention, in fact.
> >
> > if the check wasn't done for mirror-to-linear conversion, that's a bug.
> > It seems the bug has been there since the initial version of lvconvert.c. :)
> 
> I am the author of that FIXME comment there. It seems, that Dave's proposal of
> adding a lv->status & MIRRORED check is the right solution here. Thanks both
> for noticing and explaining the issue, at the time I was implementing lvconvert
> --repair, it was not clear to me.
> 

Ack.

Thanks for fixing.  Just checked in a test for to cover multi-segment
linear to mirror conversion.