Re: [Qemu-devel] [PATCH v3] block/rbd: increase dynamically the image size

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3088 bytes --]

On 25.06.19 17:28, Stefano Garzarella wrote:
> On Tue, Jun 25, 2019 at 04:57:53PM +0200, Max Reitz wrote:
>> On 25.06.19 16:47, Stefano Garzarella wrote:
>>> On Tue, Jun 25, 2019 at 04:02:04PM +0200, Max Reitz wrote:
>>>> On 09.05.19 16:59, Stefano Garzarella wrote:
>>>>> RBD APIs don't allow us to write more than the size set with
>>>>> rbd_create() or rbd_resize().
>>>>> In order to support growing images (eg. qcow2), we resize the
>>>>> image before write operations that exceed the current size.
>>>>>
>>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>> ---
>>>>> v3:
>>>>>   - add 'image_size' field in the BDRVRBDState to keep track of the
>>>>>     current size of the RBD image [Jason, Kevin]
>>>>> ---
>>>>>  block/rbd.c | 42 +++++++++++++++++++++++++++++++++++++++---
>>>>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/block/rbd.c b/block/rbd.c
>>>>> index 0c549c9935..b0355a2ce0 100644
>>>>> --- a/block/rbd.c
>>>>> +++ b/block/rbd.c
>>>>
>>>> [...]
>>>>
>>>>> @@ -833,6 +842,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
>>>>>      rados_shutdown(s->cluster);
>>>>>  }
>>>>>  
>>>>> +/* Resize the RBD image and update the 'image_size' with the current size */
>>>>> +static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
>>>>> +{
>>>>> +    BDRVRBDState *s = bs->opaque;
>>>>> +    int r;
>>>>> +
>>>>> +    r = rbd_resize(s->image, size);
>>>>> +    if (r < 0) {
>>>>> +        return r;
>>>>> +    }
>>>>> +
>>>>> +    s->image_size = size;
>>>>
>>>> I think this should update bs->total_sectors, too.  In fact, I’m
>>>> wondering why you don’t just use bs->total_sectors (or bdrv_getlength(),
>>>> which returns bs->total_sectors * 512) instead of adding this new field?
>>>>
>>>
>>> Hi Max,
>>> thanks for taking a look!
>>>
>>> I used bs->total_sectors in the v2, but Jason pointed out a possible
>>> issue with this, so I proposed to add a variable in the BDRVRBDState to
>>> track the latest resize and Kevin acked [1].
>>>
>>> IIUC what Kevin said on his comment, the 'bs->total_sectors' should be
>>> updated by bdrv_co_write_req_finish(), for this reason I didn't update
>>> it.
>>>
>>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg615195.html
>>
>> Ah, right!  Yeah, sure, now that I think about it, the block layer must
>> have general code for successful writes beyond EOF...  (Read: Now that
>> I’m pointed towards it...)
> 
> Yeah, do you mean for example to call .bdrv_co_truncate() (or a new
> callback implemented only in the driver that need to be resized like
> rbd) in the bdrv_driver_pwritev() if we will write beyond EOF?

No, I just mean that in theory all drivers should support resizing by
writing beyond the EOF (in practice, it only matters for protocol
drivers).  The general block layer code needs to recognize this and
handle it as if the BDS was explicitly resized with bdrv_co_truncate().

And it does do that, specifically in bdrv_co_write_req_finish(), as you
wrote.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]