Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 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),
-- 
2.17.1

Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>  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.

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.

Kevin

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 <lbloch@janustech.com>
>> ---
>>   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.

> 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. :)

Thanks!
Leonid.

> Kevin
> 

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 <lbloch@janustech.com>
> > > ---
> > >   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

On Mon 30 Jul 2018 02:28:28 PM CEST, Kevin Wolf wrote:
>> > 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.

I have a few patches already in my queue to replace these literals with
BDRV_SECTOR_SIZE, so no need to spend time with the existing cases, I
can take care of that.

For new cases I agree with Kevin that you should always use
BDRV_SECTOR_SIZE.

Berto