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

[Ilya Dryomov]

On Wed, Jan 12, 2022 at 12:55 PM Peter Lieven <pl@kamp.de> wrote:
>
> Am 12.01.22 um 10:59 schrieb Ilya Dryomov:
> > On Mon, Jan 10, 2022 at 12:43 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.
> >> Work around this bug by rounding down the offset to object boundaries.
> >>
> >> Fixes: https://tracker.ceph.com/issues/53784
> > I don't think the Fixes tag is appropriate here.  Linking librbd
> > ticket is fine but this patch doesn't really fix anything.
>
>
> Okay, I will change that to See:

It's already linked in the source code, up to you if you also want to
link it in the description.

>
>
> >
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Peter Lieven <pl@kamp.de>
> >> ---
> >>  block/rbd.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/rbd.c b/block/rbd.c
> >> index 5e9dc91d81..260cb9f4b4 100644
> >> --- a/block/rbd.c
> >> +++ b/block/rbd.c
> >> @@ -1333,6 +1333,7 @@ static int coroutine_fn 
> >> qemu_rbd_co_block_status(BlockDriverState *bs,
> >>      int status, r;
> >>      RBDDiffIterateReq req = { .offs = offset };
> >>      uint64_t features, flags;
> >> +    int64_t head;
> >>
> >>      assert(offset + bytes <= s->image_size);
> >>
> >> @@ -1360,6 +1361,19 @@ static int coroutine_fn 
> >> qemu_rbd_co_block_status(BlockDriverState *bs,
> >>          return status;
> >>      }
> >>
> >> +    /*
> >> +     * 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.
> >> +     *
> >> +     * See: https://tracker.ceph.com/issues/53784
> >> +     */
> >> +    head = offset & (s->object_size - 1);
> >> +    offset -= head;
> >> +    req.offs -= head;
> >> +    bytes += head;
> > So it looks like the intention is to have more or less a permanent
> > workaround since all librbd versions are affected, right?  For that,
> > I think we would need to also reject custom striping patterns and
> > clones.  For the above to be reliable, the image has to be standalone
> > and have a default striping pattern (stripe_unit == object_size &&
> > stripe_count == 1).  Otherwise, behave as if fast-diff is disabled or
> > invalid.
>
>
> Do you have a fealing how many users use a different striping pattern than 
> default?

Very few.

>
> What about EC backed pools?

In this context EC pools behave exactly the same as replicated pools.

>
> Do you have another idea how we can detect if the librbd version is broken?

No.  Initially I wanted to just fix these bugs in librbd, relying on
the assumption that setups with a new QEMU should also have a fairly
new librbd.  But after looking at various distros and realizing the
extent of rbd_diff_iterate2() issues, I think a long-term workaround
in QEMU makes sense.  A configure-time check for known good versions
of librbd can be added later if someone feels like it.

Thanks,

                Ilya