[libvirt] [PATCH v2] storage:add lvchange operation while lvremove fails.

[Osier Yang]

On 2011年11月22日 15:06, Osier Yang wrote:
> On 2011年11月01日 11:01, zituan at taobao.com wrote:
>> From: Chang Liu<lingjiao.lc at taobao.com>
>>
>> We found an issue that virStorageBackendLogicalDeleteVol() could not 
>> remove
>> the lv with notificaton "could not remove open logical volume.", in such
>> situation, we should disable the lv first, then delete it. this patch 
>> fix it.
>>
>> *src/storage/storage_backend_logical.c
>> (virStorageBackendLogicalDeleteVol):lvremove fail, lvchange the volume
>> and then lvremove it second.
>> ---
>>   configure.ac                          |    4 +++
>>   src/storage/storage_backend_logical.c |   39 
>> ++++++++++++++++++++++++++------
>>   2 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 5753c08..6092c47 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1691,6 +1691,7 @@ if test "$with_storage_lvm" = "yes" || test 
>> "$with_storage_lvm" = "check"; then
>>     AC_PATH_PROG([PVREMOVE], [pvremove], [], [$PATH:/sbin:/usr/sbin])
>>     AC_PATH_PROG([VGREMOVE], [vgremove], [], [$PATH:/sbin:/usr/sbin])
>>     AC_PATH_PROG([LVREMOVE], [lvremove], [], [$PATH:/sbin:/usr/sbin])
>> +  AC_PATH_PROG([LVCHANGE], [lvchange], [], [$PATH:/sbin:/usr/sbin])
>>     AC_PATH_PROG([VGCHANGE], [vgchange], [], [$PATH:/sbin:/usr/sbin])
>>     AC_PATH_PROG([VGSCAN], [vgscan], [], [$PATH:/sbin:/usr/sbin])
>>     AC_PATH_PROG([PVS], [pvs], [], [$PATH:/sbin:/usr/sbin])
>> @@ -1704,6 +1705,7 @@ if test "$with_storage_lvm" = "yes" || test 
>> "$with_storage_lvm" = "check"; then
>>       if test -z "$PVREMOVE" ; then AC_MSG_ERROR([We need pvremove 
>> for LVM storage driver]) ; fi
>>       if test -z "$VGREMOVE" ; then AC_MSG_ERROR([We need vgremove 
>> for LVM storage driver]) ; fi
>>       if test -z "$LVREMOVE" ; then AC_MSG_ERROR([We need lvremove 
>> for LVM storage driver]) ; fi
>> +    if test -z "$LVCHANGE" ; then AC_MSG_ERROR([We need lvchange for 
>> LVM storage driver]) ; fi
>>       if test -z "$VGCHANGE" ; then AC_MSG_ERROR([We need vgchange 
>> for LVM storage driver]) ; fi
>>       if test -z "$VGSCAN" ; then AC_MSG_ERROR([We need vgscan for 
>> LVM storage driver]) ; fi
>>       if test -z "$PVS" ; then AC_MSG_ERROR([We need pvs for LVM 
>> storage driver]) ; fi
>> @@ -1716,6 +1718,7 @@ if test "$with_storage_lvm" = "yes" || test 
>> "$with_storage_lvm" = "check"; then
>>       if test -z "$PVREMOVE" ; then with_storage_lvm=no ; fi
>>       if test -z "$VGREMOVE" ; then with_storage_lvm=no ; fi
>>       if test -z "$LVREMOVE" ; then with_storage_lvm=no ; fi
>> +    if test -z "$LVCHANGE" ; then with_storage_lvm=no ; fi
>>       if test -z "$VGCHANGE" ; then with_storage_lvm=no ; fi
>>       if test -z "$VGSCAN" ; then with_storage_lvm=no ; fi
>>       if test -z "$PVS" ; then with_storage_lvm=no ; fi
>> @@ -1733,6 +1736,7 @@ if test "$with_storage_lvm" = "yes" || test 
>> "$with_storage_lvm" = "check"; then
>>       AC_DEFINE_UNQUOTED([PVREMOVE],["$PVREMOVE"],[Location of 
>> pvremove program])
>>       AC_DEFINE_UNQUOTED([VGREMOVE],["$VGREMOVE"],[Location of 
>> vgremove program])
>>       AC_DEFINE_UNQUOTED([LVREMOVE],["$LVREMOVE"],[Location of 
>> lvremove program])
>> +    AC_DEFINE_UNQUOTED([LVCHANGE],["$LVCHANGE"],[Location of 
>> lvchange program])
>>       AC_DEFINE_UNQUOTED([VGCHANGE],["$VGCHANGE"],[Location of 
>> vgchange program])
>>       AC_DEFINE_UNQUOTED([VGSCAN],["$VGSCAN"],[Location of vgscan 
>> program])
>>       AC_DEFINE_UNQUOTED([PVS],["$PVS"],[Location of pvs program])
>> diff --git a/src/storage/storage_backend_logical.c 
>> b/src/storage/storage_backend_logical.c
>> index 3c3e736..f84c068 100644
>> --- a/src/storage/storage_backend_logical.c
>> +++ b/src/storage/storage_backend_logical.c
>> @@ -768,18 +768,41 @@ virStorageBackendLogicalDeleteVol(virConnectPtr 
>> conn ATTRIBUTE_UNUSED,
>>                                     virStorageVolDefPtr vol,
>>                                     unsigned int flags)
>>   {
>> -    const char *cmdargv[] = {
>> -        LVREMOVE, "-f", vol->target.path, NULL
>> -    };
>> +    int ret = -1;
>>
>> +    virCommandPtr lvchange_cmd = NULL;
>> +    virCommandPtr lvremove_cmd = NULL;
>> +
>>       virCheckFlags(0, -1);
>> -
>> +
>>       virFileWaitForDevices();
>> +
>> +    lvchange_cmd = virCommandNewArgList(LVCHANGE,
>> +                                        "-aln",
>> +                                        vol->target.path,
>> +                                        NULL);
>> +
>> +    lvremove_cmd = virCommandNewArgList(LVREMOVE,
>> +                                        "-f",
>> +                                        vol->target.path,
>> +                                        NULL);
>> +
>> +    if (virCommandRun(lvremove_cmd, NULL)<  0)
>> +    {
>> +        if(virCommandRun(lvchange_cmd, NULL)<  0)
>> +            goto cleanup;
>> +        else{
>> +            if(virCommandRun(lvremove_cmd,NULL)<  0)
>> +                goto cleanup;
>> +        }
>> +    }
>
> Coding style nits here. libvirt prefers:
>
> if (true) {
> }
>
> And things like:
>
> if (true)
> dummy();
> else {
> foo();
> bar();
> }
>
> is not allowed.
>
>>
>> -    if (virRun(cmdargv, NULL)<  0)
>> -        return -1;
>> -
>> -    return 0;
>> +    ret = 0;
>> +
>> +cleanup:
>> +    virCommandFree(lvchange_cmd);
>> +    virCommandFree(lvremove_cmd);
>> +    return ret;
>>   }
>>
>>
>
> ACK with the nits fixed.

Hi, Liu Chang

I Pushed the patch with the coding style nits fixed, and a bit changes
on the commit message:

commit 3c5405149b00c84814712c2d22eb794ed45ac748
Author: Chang Liu <lingjiao.lc at taobao.com>
Date:   Tue Nov 22 15:24:25 2011 +0800

     storage: Fallback to use lvchange first if lvremove fails

     virStorageBackendLogicalDeleteVol() could not remove the lv with error
     "could not remove open logical volume" sometimes. Generally it's caused
     by the volume is still active, even if lvremove tries to remove it with
     option "--force".

     This patch is to fix it by disbale the lv first using "lvchange -aln"
     and "lvremove -f" afterwards if the direct "lvremove -f" failed.

Also add you to the AUTHORS list as:

Chang Liu <lingjiao.lc at taobao.com>

If you have other preferred name, please let us known.

Thanks
Osier