Re: [Qemu-devel] [PATCH 4/6] qcow2: Update total_sectors when resizing the image

[Kevin Wolf]

Am 30.07.2018 um 13:41 hat Leonid Bloch geschrieben:
> On 07/30/2018 12:43 PM, Kevin Wolf wrote:
> > Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:
> > > Signed-off-by: Leonid Bloch <address@hidden>
> > > ---
> > >   block/qcow2.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > index ec9e6238a0..223d351e40 100644
> > > --- a/block/qcow2.c
> > > +++ b/block/qcow2.c
> > > @@ -3646,6 +3646,8 @@ static int coroutine_fn 
> > > qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> > >           }
> > >       }
> > > +    bs->total_sectors = offset / 512;
> > > +
> > >       /* write updated header.size */
> > >       offset = cpu_to_be64(offset);
> > >       ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> > 
> > This shouldn't be necessary, bdrv_co_truncate() already updates
> > bs->total_sectors after calling the block driver.
> 
> It looks like without it the cache resize works only on the second resize.

Yes, and after reading the rest of the series, it makes sense to me
because qcow2_update_option() -> read_cache_sizes() uses
bs->total_sectors, so if we call that in qcow2_co_truncate(), we already
need to update the value early.

The comment should explain this connection because otherwise it's not
obvious when you read the code.

> > If this is needed by one of the following patches, we need a comment
> > that explains why this seemingly superfluous assignment is actually
> > necessary.
> > 
> > Also, 512 should be BDRV_SECTOR_SIZE.
> 
> I was surprised that it's not, but it's 512 also in two other places,
> including in qcow2_co_truncate itself. So I decided to keep that. Probably
> would be better if I'd repair it in the other places instead. :)

Yeah, or at least not introduce new places with a literal 512.

Kevin