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