Re: [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback

[Jason Dillaman]

On Fri, Jun 28, 2019 at 4:59 AM Stefano Garzarella <address@hidden> wrote:
>
> On Thu, Jun 27, 2019 at 03:43:04PM -0400, Jason Dillaman wrote:
> > On Thu, Jun 27, 2019 at 1:24 PM John Snow <address@hidden> wrote:
> > > On 6/27/19 4:48 AM, Stefano Garzarella wrote:
> > > > On Wed, Jun 26, 2019 at 05:04:25PM -0400, John Snow wrote:
> > > >> It looks like this has hit a 30 day expiration without any reviews or
> > > >> being merged; do we still want this? If so, can you please resend?
> > > >
> > > > Yes, I think we still want :)
> > > >
> > > > Is it okay if I send a v3 following your comments?
> > > >
> > >
> > > Yes, but I don't know who is responsible for final approval; I guess
> > > that's Josh Durgin?
> >
> > I'm the new (for the past several years) upstream PTL for RBD, so feel
> > free to tag me.
> >
> > > >>
> > > >> On 5/10/19 11:33 AM, Stefano Garzarella wrote:
> > > >>> This patch allows 'qemu-img info' to show the 'disk size' for
> > > >>> the RBD images that have the fast-diff feature enabled.
> > > >>>
> > > >>> If this feature is enabled, we use the rbd_diff_iterate2() API
> > > >>> to calculate the allocated size for the image.
> > > >>>
> > > >>> Signed-off-by: Stefano Garzarella <address@hidden>
> > > >>> ---
> > > >>> v2:
> > > >>>   - calculate the actual usage only if the fast-diff feature is
> > > >>>     enabled [Jason]
> > > >>> ---
> > > >>>  block/rbd.c | 54 
> > > >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >>>  1 file changed, 54 insertions(+)
> > > >>>
> > > >>> diff --git a/block/rbd.c b/block/rbd.c
> > > >>> index 0c549c9935..f1bc76ab80 100644
> > > >>> --- a/block/rbd.c
> > > >>> +++ b/block/rbd.c
> > > >>> @@ -1046,6 +1046,59 @@ static int64_t 
> > > >>> qemu_rbd_getlength(BlockDriverState *bs)
> > > >>>      return info.size;
> > > >>>  }
> > > >>>
> > > >>> +static int rbd_allocated_size_cb(uint64_t offset, size_t len, int 
> > > >>> exists,
> > > >>> +                                 void *arg)
> > > >>> +{
> > > >>> +    int64_t *alloc_size = (int64_t *) arg;
> > > >>> +
> > > >>> +    if (exists) {
> > > >>> +        (*alloc_size) += len;
> > > >>> +    }
> > > >>> +
> > > >>> +    return 0;
> > > >>> +}
> > > >>> +
> > > >>> +static int64_t qemu_rbd_get_allocated_file_size(BlockDriverState *bs)
> > > >>> +{
> > > >>> +    BDRVRBDState *s = bs->opaque;
> > > >>> +    uint64_t flags, features;
> > > >>> +    int64_t alloc_size = 0;
> > > >>> +    int r;
> > > >>> +
> > > >>> +    r = rbd_get_flags(s->image, &flags);
> > > >>> +    if (r < 0) {
> > > >>> +        return r;
> > > >>> +    }
> > > >>> +
> > > >>
> > > >> Do you know where rbd_get_flags is documented? I can't seem to quickly
> > > >> find a reference that tells me what to expect from calling it. It
> > > >> returns an int, I guess an error code, but how can I confirm this?
> > > >>
> > > >> *clones the ceph repository*
> > > >>
> > > >> src/librbd/internal.cc get_flags convinces me it probably works like I
> > > >> think, but is there not a reference here?
> > > >>
> > > >
> > > > Good question!
> > > > I didn't find any docs, but looking in the ceph tests 
> > > > test/librbd/fsx.cc,
> > > > they print an error message if the return value is less than 0.
> > > >
> > > > A 'get_flags' implemented in cls/rbd/cls_rbd.cc for example returns 0 
> > > > at the
> > > > end and -EINVAL in a try/catch. It also uses 'read_key()' that in some 
> > > > cases
> > > > returns -EIO, so I hope that the error returned by rbd_get_flags() is 
> > > > one of
> > > > the errors defined in errno.h
> > > >
> > > >>> +    r = rbd_get_features(s->image, &features);
> > > >>> +    if (r < 0) {
> > > >>> +        return r;
> > > >>> +    }
> > > >>> +
> > > >>> +    /*
> > > >>> +     * We use rbd_diff_iterate2() only if the RBD image have 
> > > >>> fast-diff
> > > >>> +     * feature enabled. If it is disabled, rbd_diff_iterate2() could 
> > > >>> be
> > > >>> +     * very slow on a big image.
> > > >>> +     */
> > > >>> +    if (!(features & RBD_FEATURE_FAST_DIFF) ||
> > > >>> +        (flags & RBD_FLAG_FAST_DIFF_INVALID)) {
> > > >>> +        return -1;
> > > >>> +    }
> > > >>> +
> > > >>
> > > >> (Is there a reference for the list of flags to make sure there aren't
> > > >> other cases we might want to skip this?)
> > > >
> > > > Unfortunately no :(
> > > > As Jason suggested, I followed what libvirt did in the
> > > > volStorageBackendRBDUseFastDiff() [src/storage/storage_backend_rbd.c]
> >
> > These are the only ones.
>
> Thanks for the clarification!
>
> >
> > > >>
> > > >> It looks reasonable at a glance, but maybe let's return -ENOTSUP 
> > > >> instead
> > > >> of -1, based on the idea that bdrv_get_allocated_file_size returns
> > > >> -ENOMEDIUM in a prominent error case -- let's match that error 
> > > >> convention.
> > > >
> > > > Sure, -ENOTSUP is absolutely better!
> > > >
> > > >>
> > > >> (Well, I wonder what the librbd calls are returning and if THOSE mean
> > > >> anything.)
> > > >
> > > > I hope they return an errno.h errors, but I'm not sure if the meaning
> > > > make sense for us.
> > > >
> > > > Do you think is better to return -ENOTSUP or -EIO when librbd calls
> > > > fail?
> > > >
> > >
> > > I'll be honest, I have no idea because I don't know what failure of
> > > these calls means _at all_, so I don't know if it should be something
> > > severe like EIO or something more mundane.
> > >
> > > I guess just leave them alone in absence of better information, honestly.
> >
> > It looks like QEMU treats any negative error code like the actual size
> > isn't available. However, for clarity I would vote for -ENOTSUPP since
> > that's what would be returned if the driver doesn't support it.
> >
>
> Do you mean to return -ENOTSUP even when there's a runtime error in
> rbd_get_features() or rbd_get_flags() or rbd_diff_iterate2?

I was advocating for only returning -ENOTSUP in the case where
fast-diff isn't enabled or is flagged as invalid (instead of the
hard-coded -1). If one of the librbd APIs returns an error for some
reason, it seems like you can just pass it along like the current
patch does since any negative result is handled.

> Thanks,
> Stefano

--
Jason