[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v2] block/rbd: implement .bdrv_get_allocated_file_size callback |
Date: |
Thu, 27 Jun 2019 13:24:24 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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 <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]
>
>>
>> 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 :)