Re: [Qemu-devel] [PATCH v3 3/5] qcow2: Resize the cache upon image resizing

[Leonid Bloch]

On 08/08/2018 05:13 PM, Alberto Garcia wrote:
> On Wed 08 Aug 2018 09:10:49 AM CEST, Leonid Bloch wrote:
> > The caches are now recalculated upon image resizing. This is done
> > because the new default behavior of assigning a sufficient L2 cache to
> > cover the entire image implies that the cache will still be sufficient
> > after an image resizing.
>
> This is related to what I mentioned in the previous patch. The default
> behavior doesn't make the cache try to cover the entire image (or at
> least it doesn't *extend* the cache, which is what I understand from
> this paragraph). What it does is *reduce* the cache if the smaller
> version is enough for the entire image.

But it doesn't say that it extends the cache. It says that it *adapts* 
the cache to the image size, and therefore it should be resized when the 
image is resized. At least I understand it this way. That said, I'd 
mention the limit there, instead of just "sufficient".

> > Signed-off-by: Leonid Bloch <address@hidden>
> > ---
> >   block/qcow2.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 98cb96aaca..f60cb92169 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -3639,6 +3639,8 @@ static int coroutine_fn 
> > qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> >           }
> >       }
> >   
> > +    bs->total_sectors = offset / BDRV_SECTOR_SIZE;
> > +
> >       /* write updated header.size */
> >       offset = cpu_to_be64(offset);
> >       ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> > @@ -3649,6 +3651,12 @@ static int coroutine_fn 
> > qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> >       }
> >   
> >       s->l1_vm_state_index = new_l1_size;
>
> You could add an empty line here for readability.

Yes, definitely, I will. Thanks.

> > +    /* Update cache sizes */
> > +    QDict *options = qdict_clone_shallow(bs->options);
>
> C99 allows variable declarations in the middle of a block, but we're
> still doing it at the beginning (I don't know if there's a good reason
> for this?).

I did it for readability, and I didn't see a style directive for this. 
But if the style requires it - no problem. :)

> Otherwise the patch looks good to me. Thanks!

Thanks!

Leonid.

> Berto