[libvirt] [PATCH for 5.3.0] storage_backend_rbd: Don't fail iterating over snapshots

Michal Privoznik posted 1 patch 4 years, 11 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/16ea98ef656617627c2a763b8e1580f37f328246.1556542540.git.mprivozn@redhat.com
src/storage/storage_backend_rbd.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
[libvirt] [PATCH for 5.3.0] storage_backend_rbd: Don't fail iterating over snapshots
Posted by Michal Privoznik 4 years, 11 months ago
When cloning an RBD volume we try to find a snapshot which is
not different to the image we're trying to clone. This boils down
to calling rbd_diff_iterate() or rbd_diff_iterate2() on systems
with newer ceph. These two are passed a callback -
virStorageBackendRBDIterateCb() which mistakenly states that if
a negative value is returned from the callback, the iteration is
stopped but no error is reported. Well, that is not the case.
I've looked into librbd sources and found that a negative value
does mean stop of iteration but it also means an error. That's
why any attempt to clone an RBD image results in the following
error:

  virsh # vol-clone --pool rbd_image_root coreos_2023 coreos00.disk
  error: Failed to clone vol from coreos_2023
  error: failed to iterate RBD snapshot coreos_2023@base: Operation not
  permitted

Reported on libvirt-users list:

https://www.redhat.com/archives/libvirt-users/2019-April/msg00060.html

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/storage/storage_backend_rbd.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index f8c968e682..3fe188551c 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -1031,14 +1031,9 @@ virStorageBackendRBDIterateCb(uint64_t offset ATTRIBUTE_UNUSED,
 {
     /*
      * Just set that there is a diff for this snapshot, we do not care where
-     *
-     * When it returns a negative number the rbd_diff_iterate() function will stop
-     *
-     * That's why we return -1, meaning that there is a difference and we can stop
-     * searching any further.
      */
     *(int*) arg = 1;
-    return -1;
+    return 0;
 }
 
 static int
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for 5.3.0] storage_backend_rbd: Don't fail iterating over snapshots
Posted by Jason Dillaman 4 years, 11 months ago
On Mon, Apr 29, 2019 at 8:57 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> When cloning an RBD volume we try to find a snapshot which is
> not different to the image we're trying to clone. This boils down
> to calling rbd_diff_iterate() or rbd_diff_iterate2() on systems
> with newer ceph. These two are passed a callback -
> virStorageBackendRBDIterateCb() which mistakenly states that if
> a negative value is returned from the callback, the iteration is
> stopped but no error is reported. Well, that is not the case.
> I've looked into librbd sources and found that a negative value
> does mean stop of iteration but it also means an error. That's
> why any attempt to clone an RBD image results in the following
> error:
>
>   virsh # vol-clone --pool rbd_image_root coreos_2023 coreos00.disk
>   error: Failed to clone vol from coreos_2023
>   error: failed to iterate RBD snapshot coreos_2023@base: Operation not
>   permitted
>
> Reported on libvirt-users list:
>
> https://www.redhat.com/archives/libvirt-users/2019-April/msg00060.html
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/storage/storage_backend_rbd.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index f8c968e682..3fe188551c 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -1031,14 +1031,9 @@ virStorageBackendRBDIterateCb(uint64_t offset ATTRIBUTE_UNUSED,
>  {
>      /*
>       * Just set that there is a diff for this snapshot, we do not care where
> -     *
> -     * When it returns a negative number the rbd_diff_iterate() function will stop
> -     *
> -     * That's why we return -1, meaning that there is a difference and we can stop
> -     * searching any further.
>       */
>      *(int*) arg = 1;
> -    return -1;
> +    return 0;
>  }

You could also tweak "virStorageBackendRBDSnapshotFindNoDiff" to
ignore the error if "diff > 0". That would still allow it to fail-fast
once a delta is found.

>  static int
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Jason

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for 5.3.0] storage_backend_rbd: Don't fail iterating over snapshots
Posted by Michal Privoznik 4 years, 11 months ago
On 4/29/19 3:13 PM, Jason Dillaman wrote:
> On Mon, Apr 29, 2019 at 8:57 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> When cloning an RBD volume we try to find a snapshot which is
>> not different to the image we're trying to clone. This boils down
>> to calling rbd_diff_iterate() or rbd_diff_iterate2() on systems
>> with newer ceph. These two are passed a callback -
>> virStorageBackendRBDIterateCb() which mistakenly states that if
>> a negative value is returned from the callback, the iteration is
>> stopped but no error is reported. Well, that is not the case.
>> I've looked into librbd sources and found that a negative value
>> does mean stop of iteration but it also means an error. That's
>> why any attempt to clone an RBD image results in the following
>> error:
>>
>>    virsh # vol-clone --pool rbd_image_root coreos_2023 coreos00.disk
>>    error: Failed to clone vol from coreos_2023
>>    error: failed to iterate RBD snapshot coreos_2023@base: Operation not
>>    permitted
>>
>> Reported on libvirt-users list:
>>
>> https://www.redhat.com/archives/libvirt-users/2019-April/msg00060.html
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/storage/storage_backend_rbd.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
>> index f8c968e682..3fe188551c 100644
>> --- a/src/storage/storage_backend_rbd.c
>> +++ b/src/storage/storage_backend_rbd.c
>> @@ -1031,14 +1031,9 @@ virStorageBackendRBDIterateCb(uint64_t offset ATTRIBUTE_UNUSED,
>>   {
>>       /*
>>        * Just set that there is a diff for this snapshot, we do not care where
>> -     *
>> -     * When it returns a negative number the rbd_diff_iterate() function will stop
>> -     *
>> -     * That's why we return -1, meaning that there is a difference and we can stop
>> -     * searching any further.
>>        */
>>       *(int*) arg = 1;
>> -    return -1;
>> +    return 0;
>>   }
> 
> You could also tweak "virStorageBackendRBDSnapshotFindNoDiff" to
> ignore the error if "diff > 0". That would still allow it to fail-fast
> once a delta is found.

Good catch. Let me post a v2.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list