Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback
On Tue, Jul 02, 2019 at 11:09:14AM -0400, Jason Dillaman wrote:
> On Fri, Jun 28, 2019 at 4:59 AM Stefano Garzarella
> 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 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
> > > > >>> ---
> > > > >>> 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, );
> > > > >>> +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, );
> > > > >>> +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
Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback
On Fri, Jun 28, 2019 at 4:59 AM Stefano Garzarella 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 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
> > > >>> ---
> > > >>> 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, );
> > > >>> +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, );
> > > >>> +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
Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback
On Thu, Jun 27, 2019 at 03:43:04PM -0400, Jason Dillaman wrote:
> On Thu, Jun 27, 2019 at 1:24 PM John Snow 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
> > >>> ---
> > >>> 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, );
> > >>> +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, );
> > >>> +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
Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback
On Thu, Jun 27, 2019 at 3:45 PM John Snow wrote: > > > > On 6/27/19 3:43 PM, Jason Dillaman wrote: > > On Thu, Jun 27, 2019 at 1:24 PM John Snow 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 > > L: qemu-bl...@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
On 6/27/19 3:43 PM, Jason Dillaman wrote: > On Thu, Jun 27, 2019 at 1:24 PM John Snow 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 > L: qemu-bl...@nongnu.org > S: Supported > F: block/rbd.c
Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback
On Thu, Jun 27, 2019 at 1:24 PM John Snow 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
> >>> ---
> >>> 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, );
> >>> +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, );
> >>> +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
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
>>> ---
>>> 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, );
>>> +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, );
>>> +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
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
> > ---
> > 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, );
> > +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, );
> > +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
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
> ---
> 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, );
> +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, );
> +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,
> + _allocated_size_cb, _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= _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",
>
>
