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?
>
> 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 <sgarz...@redhat.com>
> > ---
> > 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]
>
> 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?
Thanks for your comments,
Stefano
