[PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Peter Lieven]

V1->V2:
 Patch 1: Treat a hole just like an unallocated area. [Ilya]
 Patch 2: Apply workaround only for pre-Quincy librbd versions and
          ensure default striping and non child images. [Ilya]

Peter Lieven (2):
  block/rbd: fix handling of holes in .bdrv_co_block_status
  block/rbd: workaround for ceph issue #53784

 block/rbd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH V2 for-6.2 1/2] block/rbd: fix handling of holes in .bdrv_co_block_status

[Peter Lieven]

the assumption that we can't hit a hole if we do not diff against a snapshot was wrong.

We can see a hole in an image if we diff against base if there exists an older snapshot
of the image and we have discarded blocks in the image where the snapshot has data.

Fix this by simply handling a hole like an unallocated area. There are no callbacks
for unallocated areas so just bail out if we hit a hole.

Fixes: 0347a8fd4c3faaedf119be04c197804be40a384b
Suggested-by: Ilya Dryomov <idryomov@gmail.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index def96292e0..20bb896c4a 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1279,11 +1279,11 @@ static int qemu_rbd_diff_iterate_cb(uint64_t offs, size_t len,
     RBDDiffIterateReq *req = opaque;
 
     assert(req->offs + req->bytes <= offs);
-    /*
-     * we do not diff against a snapshot so we should never receive a callback
-     * for a hole.
-     */
-    assert(exists);
+
+    /* treat a hole like an unallocated area and bail out */
+    if (!exists) {
+        return 0;
+    }
 
     if (!req->exists && offs > req->offs) {
         /*
-- 
2.25.1

[PATCH V2 for-6.2 2/2] block/rbd: workaround for ceph issue #53784

[Peter Lieven]

librbd had a bug until early 2022 that affected all versions of ceph that
supported fast-diff. This bug results in reporting of incorrect offsets
if the offset parameter to rbd_diff_iterate2 is not object aligned.

This patch works around this bug for pre Quincy versions of librbd.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 20bb896c4a..d174d51659 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1320,6 +1320,7 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
     int status, r;
     RBDDiffIterateReq req = { .offs = offset };
     uint64_t features, flags;
+    uint64_t head = 0;
 
     assert(offset + bytes <= s->image_size);
 
@@ -1347,7 +1348,43 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
         return status;
     }
 
-    r = rbd_diff_iterate2(s->image, NULL, offset, bytes, true, true,
+#if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 17, 0)
+    /*
+     * librbd had a bug until early 2022 that affected all versions of ceph that
+     * supported fast-diff. This bug results in reporting of incorrect offsets
+     * if the offset parameter to rbd_diff_iterate2 is not object aligned.
+     * Work around this bug by rounding down the offset to object boundaries.
+     * This is OK because we call rbd_diff_iterate2 with whole_object = true.
+     * However, this workaround only works for non cloned images with default
+     * striping.
+     *
+     * See: https://tracker.ceph.com/issues/53784
+     */
+
+    /*  check if RBD image has non-default striping enabled */
+    if (features & RBD_FEATURE_STRIPINGV2) {
+        return status;
+    }
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+    /*
+     * check if RBD image is a clone (= has a parent).
+     *
+     * rbd_get_parent_info is deprecated from Nautilus onwards, but the
+     * replacement rbd_get_parent is not present in Luminous and Mimic.
+     */
+    if (rbd_get_parent_info(s->image, NULL, 0, NULL, 0, NULL, 0) != -ENOENT) {
+        return status;
+    }
+#pragma GCC diagnostic pop
+
+    head = req.offs & (s->object_size - 1);
+    req.offs -= head;
+    bytes += head;
+#endif
+
+    r = rbd_diff_iterate2(s->image, NULL, req.offs, bytes, true, true,
                           qemu_rbd_diff_iterate_cb, &req);
     if (r < 0 && r != QEMU_RBD_EXIT_DIFF_ITERATE2) {
         return status;
@@ -1366,7 +1403,8 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
         status = BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID;
     }
 
-    *pnum = req.bytes;
+    assert(req.bytes > head);
+    *pnum = req.bytes - head;
     return status;
 }
 
-- 
2.25.1

Re: [PATCH V2 for-6.2 2/2] block/rbd: workaround for ceph issue #53784

[Ilya Dryomov]

On Thu, Jan 13, 2022 at 3:44 PM Peter Lieven <pl@kamp.de> wrote:
>
> librbd had a bug until early 2022 that affected all versions of ceph that
> supported fast-diff. This bug results in reporting of incorrect offsets
> if the offset parameter to rbd_diff_iterate2 is not object aligned.
>
> This patch works around this bug for pre Quincy versions of librbd.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/rbd.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 20bb896c4a..d174d51659 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -1320,6 +1320,7 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
>      int status, r;
>      RBDDiffIterateReq req = { .offs = offset };
>      uint64_t features, flags;
> +    uint64_t head = 0;
>
>      assert(offset + bytes <= s->image_size);
>
> @@ -1347,7 +1348,43 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
>          return status;
>      }
>
> -    r = rbd_diff_iterate2(s->image, NULL, offset, bytes, true, true,
> +#if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 17, 0)
> +    /*
> +     * librbd had a bug until early 2022 that affected all versions of ceph that
> +     * supported fast-diff. This bug results in reporting of incorrect offsets
> +     * if the offset parameter to rbd_diff_iterate2 is not object aligned.
> +     * Work around this bug by rounding down the offset to object boundaries.
> +     * This is OK because we call rbd_diff_iterate2 with whole_object = true.
> +     * However, this workaround only works for non cloned images with default
> +     * striping.
> +     *
> +     * See: https://tracker.ceph.com/issues/53784
> +     */
> +
> +    /*  check if RBD image has non-default striping enabled */

Nit: extra space

Thanks,

                Ilya

Re: [PATCH V2 for-6.2 2/2] block/rbd: workaround for ceph issue #53784

[Stefano Garzarella]

On Thu, Jan 13, 2022 at 03:44:26PM +0100, Peter Lieven wrote:
>librbd had a bug until early 2022 that affected all versions of ceph that
>supported fast-diff. This bug results in reporting of incorrect offsets
>if the offset parameter to rbd_diff_iterate2 is not object aligned.
>
>This patch works around this bug for pre Quincy versions of librbd.
>

I'm not sure, but maybe we could add the fixes tag also to this patch, 
since without this workaround we can have issues with buggy librbd.

Fixes: 0347a8fd4c3faaedf119be04c197804be40a384b

>Cc: qemu-stable@nongnu.org
>Signed-off-by: Peter Lieven <pl@kamp.de>
>---
> block/rbd.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 40 insertions(+), 2 deletions(-)

Tested-by: Stefano Garzarella <sgarzare@redhat.com>


>
>diff --git a/block/rbd.c b/block/rbd.c
>index 20bb896c4a..d174d51659 100644
>--- a/block/rbd.c
>+++ b/block/rbd.c
>@@ -1320,6 +1320,7 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
>     int status, r;
>     RBDDiffIterateReq req = { .offs = offset };
>     uint64_t features, flags;
>+    uint64_t head = 0;
>
>     assert(offset + bytes <= s->image_size);
>
>@@ -1347,7 +1348,43 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
>         return status;
>     }
>
>-    r = rbd_diff_iterate2(s->image, NULL, offset, bytes, true, true,
>+#if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 17, 0)
>+    /*
>+     * librbd had a bug until early 2022 that affected all versions of ceph that
>+     * supported fast-diff. This bug results in reporting of incorrect offsets
>+     * if the offset parameter to rbd_diff_iterate2 is not object aligned.
>+     * Work around this bug by rounding down the offset to object boundaries.
>+     * This is OK because we call rbd_diff_iterate2 with whole_object = true.
>+     * However, this workaround only works for non cloned images with default
>+     * striping.
>+     *
>+     * See: https://tracker.ceph.com/issues/53784
>+     */
>+
>+    /*  check if RBD image has non-default striping enabled */
>+    if (features & RBD_FEATURE_STRIPINGV2) {
>+        return status;
>+    }
>+
>+#pragma GCC diagnostic push
>+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>+    /*
>+     * check if RBD image is a clone (= has a parent).
>+     *
>+     * rbd_get_parent_info is deprecated from Nautilus onwards, but the
>+     * replacement rbd_get_parent is not present in Luminous and Mimic.
>+     */
>+    if (rbd_get_parent_info(s->image, NULL, 0, NULL, 0, NULL, 0) != -ENOENT) {
>+        return status;
>+    }
>+#pragma GCC diagnostic pop
>+
>+    head = req.offs & (s->object_size - 1);
>+    req.offs -= head;
>+    bytes += head;
>+#endif
>+
>+    r = rbd_diff_iterate2(s->image, NULL, req.offs, bytes, true, true,
>                           qemu_rbd_diff_iterate_cb, &req);
>     if (r < 0 && r != QEMU_RBD_EXIT_DIFF_ITERATE2) {
>         return status;
>@@ -1366,7 +1403,8 @@ static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
>         status = BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID;
>     }
>
>-    *pnum = req.bytes;
>+    assert(req.bytes > head);
>+    *pnum = req.bytes - head;
>     return status;
> }
>
>-- 
>2.25.1
>
>
>

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Ilya Dryomov]

On Thu, Jan 13, 2022 at 3:44 PM Peter Lieven <pl@kamp.de> wrote:
>
> V1->V2:
>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>           ensure default striping and non child images. [Ilya]
>
> Peter Lieven (2):
>   block/rbd: fix handling of holes in .bdrv_co_block_status
>   block/rbd: workaround for ceph issue #53784
>
>  block/rbd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 7 deletions(-)
>
> --
> 2.25.1
>
>

These patches have both "for-6.2" in the subject and
Cc: qemu-stable@nongnu.org in the description, which is a little
confusing.  Just want to clarify that they should go into master
and be backported to 6.2.

Reviewed-by: Ilya Dryomov <idryomov@gmail.com>

Thanks,

                Ilya

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Stefano Garzarella]

On Fri, Jan 14, 2022 at 11:58:40AM +0100, Ilya Dryomov wrote:
>On Thu, Jan 13, 2022 at 3:44 PM Peter Lieven <pl@kamp.de> wrote:
>>
>> V1->V2:
>>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>>           ensure default striping and non child images. [Ilya]
>>
>> Peter Lieven (2):
>>   block/rbd: fix handling of holes in .bdrv_co_block_status
>>   block/rbd: workaround for ceph issue #53784
>>
>>  block/rbd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 45 insertions(+), 7 deletions(-)
>>
>> --
>> 2.25.1
>>
>>
>
>These patches have both "for-6.2" in the subject and
>Cc: qemu-stable@nongnu.org in the description, which is a little
>confusing.  Just want to clarify that they should go into master
>and be backported to 6.2.

Yeah, a bit confusing. These are for 7.0, so @Kevin can these patches go 
with your tree?

I'd just add the fixes tag also in the patch 2. With that:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Peter Lieven]

Am 19.01.22 um 15:57 schrieb Stefano Garzarella:
> On Fri, Jan 14, 2022 at 11:58:40AM +0100, Ilya Dryomov wrote:
>> On Thu, Jan 13, 2022 at 3:44 PM Peter Lieven <pl@kamp.de> wrote:
>>>
>>> V1->V2:
>>>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>>>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>>>           ensure default striping and non child images. [Ilya]
>>>
>>> Peter Lieven (2):
>>>   block/rbd: fix handling of holes in .bdrv_co_block_status
>>>   block/rbd: workaround for ceph issue #53784
>>>
>>>  block/rbd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 45 insertions(+), 7 deletions(-)
>>>
>>> -- 
>>> 2.25.1
>>>
>>>
>>
>> These patches have both "for-6.2" in the subject and
>> Cc: qemu-stable@nongnu.org in the description, which is a little
>> confusing.  Just want to clarify that they should go into master
>> and be backported to 6.2.
>
> Yeah, a bit confusing. These are for 7.0, so @Kevin can these patches go with your tree?


Yes, sorry, my fault. It should be 7.0


Peter

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Stefano Garzarella]

On Thu, Jan 20, 2022 at 10:19:27AM +0100, Peter Lieven wrote:
>Am 19.01.22 um 15:57 schrieb Stefano Garzarella:
>> On Fri, Jan 14, 2022 at 11:58:40AM +0100, Ilya Dryomov wrote:
>>> On Thu, Jan 13, 2022 at 3:44 PM Peter Lieven <pl@kamp.de> wrote:
>>>>
>>>> V1->V2:
>>>>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>>>>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>>>>           ensure default striping and non child images. [Ilya]
>>>>
>>>> Peter Lieven (2):
>>>>   block/rbd: fix handling of holes in .bdrv_co_block_status
>>>>   block/rbd: workaround for ceph issue #53784
>>>>
>>>>  block/rbd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 45 insertions(+), 7 deletions(-)
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>
>>> These patches have both "for-6.2" in the subject and
>>> Cc: qemu-stable@nongnu.org in the description, which is a little
>>> confusing.  Just want to clarify that they should go into master
>>> and be backported to 6.2.
>>
>> Yeah, a bit confusing. These are for 7.0, so @Kevin can these patches go with your tree?
>
>
>Yes, sorry, my fault. It should be 7.0

Don't worry :-)

What about sending a v3 fixing the version tag (I think you can just 
remove for-6.2), the extra space in the comment, and the Fixes tag on 
patch 2?

If you will send v3, remember to report the R-b/T-b tags received in 
this version from me and Ilya.

Thanks,
Stefano

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Kevin Wolf]

Am 13.01.2022 um 15:44 hat Peter Lieven geschrieben:
> V1->V2:
>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>           ensure default striping and non child images. [Ilya]
> 
> Peter Lieven (2):
>   block/rbd: fix handling of holes in .bdrv_co_block_status
>   block/rbd: workaround for ceph issue #53784

Thanks, applied to the block branch.

Kevin

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Peter Lieven]

Am 01.02.22 um 15:39 schrieb Kevin Wolf:
> Am 13.01.2022 um 15:44 hat Peter Lieven geschrieben:
>> V1->V2:
>>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>>           ensure default striping and non child images. [Ilya]
>>
>> Peter Lieven (2):
>>   block/rbd: fix handling of holes in .bdrv_co_block_status
>>   block/rbd: workaround for ceph issue #53784
> Thanks, applied to the block branch.
>
> Kevin
>
Hi Kevin,


thanks for taking care of this. I was a few days out of office.

@Stefano: it seems Kevin addresses your comments that should have gone into a V3.


Best,

Peter

Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status

[Stefano Garzarella]

On Thu, Feb 03, 2022 at 12:42:30PM +0100, Peter Lieven wrote:
>Am 01.02.22 um 15:39 schrieb Kevin Wolf:
>> Am 13.01.2022 um 15:44 hat Peter Lieven geschrieben:
>>> V1->V2:
>>>  Patch 1: Treat a hole just like an unallocated area. [Ilya]
>>>  Patch 2: Apply workaround only for pre-Quincy librbd versions and
>>>           ensure default striping and non child images. [Ilya]
>>>
>>> Peter Lieven (2):
>>>   block/rbd: fix handling of holes in .bdrv_co_block_status
>>>   block/rbd: workaround for ceph issue #53784
>> Thanks, applied to the block branch.
>>
>> Kevin
>>
>Hi Kevin,
>
>
>thanks for taking care of this. I was a few days out of office.
>
>@Stefano: it seems Kevin addresses your comments that should have gone 
>into a V3.

Yep :-)

Thanks,
Stefano