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

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

[Stefano Garzarella]

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 <sgarzare@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;
+    }
+
+    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;
+    }
+
+    /*
+     * rbd_diff_iterate2(), if the source snapshot name is NULL, invokes
+     * the callback on all allocated regions of the image.
+     */
+    r = rbd_diff_iterate2(s->image, NULL, 0,
+                          bs->total_sectors * BDRV_SECTOR_SIZE, 0, 1,
+                          &rbd_allocated_size_cb, &alloc_size);
+    if (r < 0) {
+        return r;
+    }
+
+    return alloc_size;
+}
+
 static int coroutine_fn qemu_rbd_co_truncate(BlockDriverState *bs,
                                              int64_t offset,
                                              PreallocMode prealloc,
@@ -1254,6 +1307,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_get_info          = qemu_rbd_getinfo,
     .create_opts            = &qemu_rbd_create_opts,
     .bdrv_getlength         = qemu_rbd_getlength,
+    .bdrv_get_allocated_file_size = qemu_rbd_get_allocated_file_size,
     .bdrv_co_truncate       = qemu_rbd_co_truncate,
     .protocol_name          = "rbd",
 
-- 
2.20.1

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

[John Snow]

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?

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 <sgarzare@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?

> +    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?)

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.

(Well, I wonder what the librbd calls are returning and if THOSE mean
anything.)

> +    /*
> +     * rbd_diff_iterate2(), if the source snapshot name is NULL, invokes
> +     * the callback on all allocated regions of the image.
> +     */
> +    r = rbd_diff_iterate2(s->image, NULL, 0,
> +                          bs->total_sectors * BDRV_SECTOR_SIZE, 0, 1,
> +                          &rbd_allocated_size_cb, &alloc_size);
> +    if (r < 0) {
> +        return r;
> +    }
> +

I guess I'll take your word for it. ¯\_(ツ)_/¯

> +    return alloc_size;
> +}
> +
>  static int coroutine_fn qemu_rbd_co_truncate(BlockDriverState *bs,
>                                               int64_t offset,
>                                               PreallocMode prealloc,
> @@ -1254,6 +1307,7 @@ static BlockDriver bdrv_rbd = {
>      .bdrv_get_info          = qemu_rbd_getinfo,
>      .create_opts            = &qemu_rbd_create_opts,
>      .bdrv_getlength         = qemu_rbd_getlength,
> +    .bdrv_get_allocated_file_size = qemu_rbd_get_allocated_file_size,
>      .bdrv_co_truncate       = qemu_rbd_co_truncate,
>      .protocol_name          = "rbd",
>  
> 

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

[Stefano Garzarella]

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 <sgarzare@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

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

[John Snow]

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?

>>
>> 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 <sgarzare@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?
> 

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.

> 
> Thanks for your comments,
> Stefano
> 

Thank you for trying to patch rbd :)

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

[Jason Dillaman]

On Thu, Jun 27, 2019 at 1:24 PM John Snow <jsnow@redhat.com> 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 <sgarzare@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]

These are the only ones.

> >>
> >> 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.

> >
> > Thanks for your comments,
> > Stefano
> >
>
> Thank you for trying to patch rbd :)


-- 
Jason

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

[John Snow]

On 6/27/19 3:43 PM, Jason Dillaman wrote:
> On Thu, Jun 27, 2019 at 1:24 PM John Snow <jsnow@redhat.com> 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.
> 

I got Josh's name out of MAINTAINERS, does it need an update?

> RBD
> M: Josh Durgin <jdurgin@redhat.com>
> L: qemu-block@nongnu.org
> S: Supported
> F: block/rbd.c

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

[Jason Dillaman]

On Thu, Jun 27, 2019 at 3:45 PM John Snow <jsnow@redhat.com> wrote:
>
>
>
> On 6/27/19 3:43 PM, Jason Dillaman wrote:
> > On Thu, Jun 27, 2019 at 1:24 PM John Snow <jsnow@redhat.com> 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.
> >
>
> I got Josh's name out of MAINTAINERS, does it need an update?
>
> > RBD
> > M: Josh Durgin <jdurgin@redhat.com>
> > L: qemu-block@nongnu.org
> > S: Supported
> > F: block/rbd.c

Yes, I'll submit a patch to update that tomorrow.

-- 
Jason

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

[Stefano Garzarella]

On Thu, Jun 27, 2019 at 03:43:04PM -0400, Jason Dillaman wrote:
> On Thu, Jun 27, 2019 at 1:24 PM John Snow <jsnow@redhat.com> 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 <sgarzare@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]
> 
> 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?

Thanks,
Stefano

Re: [Qemu-devel] [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 <sgarzare@redhat.com> 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 <jsnow@redhat.com> 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 <sgarzare@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]
> >
> > 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

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

[Stefano Garzarella]

On Tue, Jul 02, 2019 at 11:09:14AM -0400, Jason Dillaman wrote:
> On Fri, Jun 28, 2019 at 4:59 AM Stefano Garzarella <sgarzare@redhat.com> 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 <jsnow@redhat.com> 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 <sgarzare@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]
> > >
> > > 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.

Sure, that's what I wanted to do in v3, but I had a doubt.

Thanks for the clarification, I'll send a v3 following your and John
reviews.

Stefano