[PATCH] Ignore _mlog name restriction for lvconvert repair

[PATCH] Ignore _mlog name restriction for lvconvert repair

[Malahal Naineni]

lvconvert --repair is done on _mlog mirrored log logical volumes from
dmeventd if something fails.

diff -r 86200db56a7c -r 471e224a5713 tools/lvconvert.c
--- a/tools/lvconvert.c	Tue Feb 09 17:49:50 2010 -0800
+++ b/tools/lvconvert.c	Wed Feb 10 09:12:11 2010 -0800
@@ -105,8 +105,12 @@ static int _lvconvert_name_params(struct
 	if ((ptr = strrchr(lp->lv_name_full, '/')))
 		lp->lv_name = ptr + 1;
 
-	if (!apply_lvname_restrictions(lp->lv_name))
-		return_0;
+	/* _mlog is an internal name, but it could be mirrored, so
+	 * allow repairing it.
+	 */
+	if (!arg_count(cmd, repair_ARG) || !strstr(lp->lv_name, "_mlog"))
+		if (!apply_lvname_restrictions(lp->lv_name))
+			return_0;
 
 	if (*pargc && lp->snapshot) {
 		log_error("Too many arguments provided for snapshots");

[PATCH] Ignore _mlog name restriction for lvconvert repair

[Takahiro Yasui]

Malahal Naineni wrote:
> lvconvert --repair is done on _mlog mirrored log logical volumes from
> dmeventd if something fails.
> 
> diff -r 86200db56a7c -r 471e224a5713 tools/lvconvert.c
> --- a/tools/lvconvert.c	Tue Feb 09 17:49:50 2010 -0800
> +++ b/tools/lvconvert.c	Wed Feb 10 09:12:11 2010 -0800
> @@ -105,8 +105,12 @@ static int _lvconvert_name_params(struct
>  	if ((ptr = strrchr(lp->lv_name_full, '/')))
>  		lp->lv_name = ptr + 1;
>  
> -	if (!apply_lvname_restrictions(lp->lv_name))
> -		return_0;
> +	/* _mlog is an internal name, but it could be mirrored, so
> +	 * allow repairing it.
> +	 */
> +	if (!arg_count(cmd, repair_ARG) || !strstr(lp->lv_name, "_mlog"))
> +		if (!apply_lvname_restrictions(lp->lv_name))
> +			return_0;
>  
>  	if (*pargc && lp->snapshot) {
>  		log_error("Too many arguments provided for snapshots");

lvname is better to be checked if a logical volume is not mirrored log
but simple logical volume. How about using (lv->status & MIRRORED) for
the check?

Thanks,
Taka

[PATCH] Ignore _mlog name restriction for lvconvert repair

[malahal]

Takahiro Yasui [tyasui at redhat.com] wrote:
> Malahal Naineni wrote:
> > lvconvert --repair is done on _mlog mirrored log logical volumes from
> > dmeventd if something fails.
> > 
> > diff -r 86200db56a7c -r 471e224a5713 tools/lvconvert.c
> > --- a/tools/lvconvert.c	Tue Feb 09 17:49:50 2010 -0800
> > +++ b/tools/lvconvert.c	Wed Feb 10 09:12:11 2010 -0800
> > @@ -105,8 +105,12 @@ static int _lvconvert_name_params(struct
> >  	if ((ptr = strrchr(lp->lv_name_full, '/')))
> >  		lp->lv_name = ptr + 1;
> >  
> > -	if (!apply_lvname_restrictions(lp->lv_name))
> > -		return_0;
> > +	/* _mlog is an internal name, but it could be mirrored, so
> > +	 * allow repairing it.
> > +	 */
> > +	if (!arg_count(cmd, repair_ARG) || !strstr(lp->lv_name, "_mlog"))
> > +		if (!apply_lvname_restrictions(lp->lv_name))
> > +			return_0;
> >  
> >  	if (*pargc && lp->snapshot) {
> >  		log_error("Too many arguments provided for snapshots");
> 
> lvname is better to be checked if a logical volume is not mirrored log
> but simple logical volume. How about using (lv->status & MIRRORED) for
> the check?

I thought about it but such details are not available at that point. All
the information available at that point is derived from the command line
arguments!

Thanks, Malahal.

[PATCH] Ignore _mlog name restriction for lvconvert repair

[Takahiro Yasui]

malahal at us.ibm.com wrote:
> Takahiro Yasui [tyasui at redhat.com] wrote:
>> Malahal Naineni wrote:
>>> lvconvert --repair is done on _mlog mirrored log logical volumes from
>>> dmeventd if something fails.
>>>
>>> diff -r 86200db56a7c -r 471e224a5713 tools/lvconvert.c
>>> --- a/tools/lvconvert.c	Tue Feb 09 17:49:50 2010 -0800
>>> +++ b/tools/lvconvert.c	Wed Feb 10 09:12:11 2010 -0800
>>> @@ -105,8 +105,12 @@ static int _lvconvert_name_params(struct
>>>  	if ((ptr = strrchr(lp->lv_name_full, '/')))
>>>  		lp->lv_name = ptr + 1;
>>>  
>>> -	if (!apply_lvname_restrictions(lp->lv_name))
>>> -		return_0;
>>> +	/* _mlog is an internal name, but it could be mirrored, so
>>> +	 * allow repairing it.
>>> +	 */
>>> +	if (!arg_count(cmd, repair_ARG) || !strstr(lp->lv_name, "_mlog"))
>>> +		if (!apply_lvname_restrictions(lp->lv_name))
>>> +			return_0;
>>>  
>>>  	if (*pargc && lp->snapshot) {
>>>  		log_error("Too many arguments provided for snapshots");
>> lvname is better to be checked if a logical volume is not mirrored log
>> but simple logical volume. How about using (lv->status & MIRRORED) for
>> the check?
> 
> I thought about it but such details are not available at that point. All
> the information available at that point is derived from the command line
> arguments!

Thank you for the explanation. I see we need to move this name check
at the place where lv->status check can be used.

Thanks,
Taka

[PATCH] Ignore _mlog name restriction for lvconvert repair

[Jonathan Brassow]

On Feb 11, 2010, at 7:14 PM, Takahiro Yasui wrote:

> malahal at us.ibm.com wrote:
>> Takahiro Yasui [tyasui at redhat.com] wrote:
>>> Malahal Naineni wrote:
>>>> lvconvert --repair is done on _mlog mirrored log logical volumes  
>>>> from
>>>> dmeventd if something fails.
>>>>
>>>> diff -r 86200db56a7c -r 471e224a5713 tools/lvconvert.c
>>>> --- a/tools/lvconvert.c	Tue Feb 09 17:49:50 2010 -0800
>>>> +++ b/tools/lvconvert.c	Wed Feb 10 09:12:11 2010 -0800
>>>> @@ -105,8 +105,12 @@ static int _lvconvert_name_params(struct
>>>> 	if ((ptr = strrchr(lp->lv_name_full, '/')))
>>>> 		lp->lv_name = ptr + 1;
>>>>
>>>> -	if (!apply_lvname_restrictions(lp->lv_name))
>>>> -		return_0;
>>>> +	/* _mlog is an internal name, but it could be mirrored, so
>>>> +	 * allow repairing it.
>>>> +	 */
>>>> +	if (!arg_count(cmd, repair_ARG) || !strstr(lp->lv_name, "_mlog"))
>>>> +		if (!apply_lvname_restrictions(lp->lv_name))
>>>> +			return_0;
>>>>
>>>> 	if (*pargc && lp->snapshot) {
>>>> 		log_error("Too many arguments provided for snapshots");
>>> lvname is better to be checked if a logical volume is not mirrored  
>>> log
>>> but simple logical volume. How about using (lv->status & MIRRORED)  
>>> for
>>> the check?
>>
>> I thought about it but such details are not available at that  
>> point. All
>> the information available at that point is derived from the command  
>> line
>> arguments!
>
> Thank you for the explanation. I see we need to move this name check
> at the place where lv->status check can be used.

It might make sense to do it in this very spot...  If the repair_ARG  
is present, why should we be checking lvname restrictions anyway?  No  
new LVs are being created.

Something more like:
if (!arg_count(cmd, repair_ARG) && !apply_lvname_restrictions(lp- 
 >lv_name))

Anyone see anything wrong with that?

Taka, I've not been able to reproduce your seg faults with '--alloc  
anywhere' using two devices.  Is there a special restriction you are  
using?

  brassow

[PATCH] Ignore _mlog name restriction for lvconvert repair

[Jonathan Brassow]

On Feb 12, 2010, at 3:30 PM, Jonathan Brassow wrote:

>
> On Feb 11, 2010, at 7:14 PM, Takahiro Yasui wrote:
>
>> malahal at us.ibm.com wrote:
>>> Takahiro Yasui [tyasui at redhat.com] wrote:
>>>> Malahal Naineni wrote:
>>>>> lvconvert --repair is done on _mlog mirrored log logical volumes  
>>>>> from
>>>>> dmeventd if something fails.
>>>>>
>>>>> diff -r 86200db56a7c -r 471e224a5713 tools/lvconvert.c
>>>>> --- a/tools/lvconvert.c	Tue Feb 09 17:49:50 2010 -0800
>>>>> +++ b/tools/lvconvert.c	Wed Feb 10 09:12:11 2010 -0800
>>>>> @@ -105,8 +105,12 @@ static int _lvconvert_name_params(struct
>>>>> 	if ((ptr = strrchr(lp->lv_name_full, '/')))
>>>>> 		lp->lv_name = ptr + 1;
>>>>>
>>>>> -	if (!apply_lvname_restrictions(lp->lv_name))
>>>>> -		return_0;
>>>>> +	/* _mlog is an internal name, but it could be mirrored, so
>>>>> +	 * allow repairing it.
>>>>> +	 */
>>>>> +	if (!arg_count(cmd, repair_ARG) || !strstr(lp->lv_name,  
>>>>> "_mlog"))
>>>>> +		if (!apply_lvname_restrictions(lp->lv_name))
>>>>> +			return_0;
>>>>>
>>>>> 	if (*pargc && lp->snapshot) {
>>>>> 		log_error("Too many arguments provided for snapshots");
>>>> lvname is better to be checked if a logical volume is not  
>>>> mirrored log
>>>> but simple logical volume. How about using (lv->status &  
>>>> MIRRORED) for
>>>> the check?
>>>
>>> I thought about it but such details are not available at that  
>>> point. All
>>> the information available at that point is derived from the  
>>> command line
>>> arguments!
>>
>> Thank you for the explanation. I see we need to move this name check
>> at the place where lv->status check can be used.
>
> It might make sense to do it in this very spot...  If the repair_ARG  
> is present, why should we be checking lvname restrictions anyway?   
> No new LVs are being created.
>
> Something more like:
> if (!arg_count(cmd, repair_ARG) && !apply_lvname_restrictions(lp- 
> >lv_name))
>
> Anyone see anything wrong with that?
>
> Taka, I've not been able to reproduce your seg faults with '--alloc  
> anywhere' using two devices.  Is there a special restriction you are  
> using?

agk wants to see if we can /not/ use <lv>_mlog as the argument passed  
in.  So, dmeventd would probably strip the '_mlog' portion.(?)  There  
are definitely some items to clean up in this case.  I'm going to give  
it a shot.  If it turns out to be really bad, we might go with the  
above suggestion.

  brassow

[PATCH] Ignore _mlog name restriction for lvconvert repair

[Takahiro Yasui]

Hello Jon,

> Taka, I've not been able to reproduce your seg faults with '--alloc  
> anywhere' using two devices.  Is there a special restriction you are  
> using?

Here is the reproduction steps.

[case 1]
1. create VG with two devices.
  # vgcreate vg00 /dev/sd[ab]

2. create LV with mirrored log
  # lvcreate -m1 -L12m -nlv00 --mirrorlog mirrored --alloc anywhere vg00

3. disable device /dev/sdb
  # echo offline > /sys/block/sdb/device/state

4. Write data
  # dd if=/dev/zero of=/dev/vg00/lv00 bs=4096 count=1
  1+0 records in
  1+0 records out
  4096 bytes (4.1 kB) copied, 3.02835 seconds, 1.4 kB/s

5. convert LV to mirrored log
  # lvconvert -m1 --mirrorlog mirrored --alloc anywhere vg00/lv00
    /dev/sdd: open failed: No such device or address
  Segmentation fault

[case 2]
  # vgs
    No volume groups found
  # vgcreate vg00 /dev/sdc
    Volume group "vg00" successfully created
  # lvcreate -m1 -L12m -nlv00 --mirrorlog mirrored --alloc anywhere vg00
  Segmentation fault

I hope you could reproduce this issues.

Thanks,
Taka