[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_fil
From: |
Stefano Garzarella |
Subject: |
Re: [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback |
Date: |
Fri, 28 Jun 2019 10:59:01 +0200 |
User-agent: |
NeoMutt/20180716 |
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?
Thanks,
Stefano