Re: [Qemu-devel] [PATCH] qcow2: preallocate() extands image less enough

Re: [Qemu-devel] [PATCH] qcow2: preallocate() extands image less enough

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

On Tue, May 27, 2014 at 10:57:59PM +0800, 蒙聪 wrote:
> cow2_alloc_cluster_offset() sets the host_offset pointing to the first
> byte of the cluster referenced by guest_offset other than to the exact
> byte referenced by guest_offset. In this case, preallocate() fails to
> extend the image large enough.
> 
> This bug can be reproduced by following steps:
> 
> $ /home/cmeng/work/qemu/qemu-git/qemu-img  create -f qcow2 -o cluster_size=2M,preallocation=metadata t1 25G
> Formatting 't1', fmt=qcow2 size=26843545600 encryption=off cluster_size=2097152 preallocation='metadata' lazy_refcounts=off 
> 
> $ ll t1
> -rw-r--r-- 1 cmeng cmeng 26851940352 May 27 21:20 t1
> 
> $ ./qcow2.py t1 get-cluster-offset 52428799
> l1_size, 1 l1_table_offset, 6291456 cluster_bits, 21 cluster_size, 2097152 l2_entries_per_cluster, 262144
> l1_entry (l1_refcount, l2_offset) : (1L, 8388608L)
> l2_entry (l2_compressed, l2_refcount_is_one, (cluster_offset, read_as_zero)) : (0L, 1L, (26851934208L, 0L))
> final offset : 26854030848
> 
> I modified qcow2.py to get the offset into the host image file of the
> last sector of the virutal image. As shown above, the offset is  
> 26854030848, which is a little bigger than 26851940352, the size of t1.
>
> Signed-off-by: Cong Meng <mengcong@meituan.com>
> ---
>  block/qcow2.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a4b97e8..c315fb7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1538,6 +1538,7 @@ static int preallocate(BlockDriverState *bs)
>      int num;
>      int ret;
>      QCowL2Meta *meta;
> +    BDRVQcowState *s = bs->opaque;
>  
>      nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
>      offset = 0;
> @@ -1582,6 +1583,8 @@ static int preallocate(BlockDriverState *bs)
>      if (host_offset != 0) {
>          uint8_t buf[BDRV_SECTOR_SIZE];
>          memset(buf, 0, BDRV_SECTOR_SIZE);
> +        host_offset += offset_into_cluster(s,
> +                                          offset - (num << BDRV_SECTOR_BITS));
>          ret = bdrv_write(bs->file, (host_offset >> BDRV_SECTOR_BITS) + num - 1,
>                           buf, 1);

I don't understand what problem this calculation is supposed to fix.
Can you explain the root cause?

It seems that the issue stems from:

    while (nb_sectors) {
        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                         &host_offset,
                                         &meta);

INT_MAX >> BDRV_SECTOR_BITS is not a multiple of cluster size.  That
means 'num' will not be a multiple of cluster size, even though full
clusters were allocated by qcow2_alloc_cluster_offset().

Try changing it to:
(INT_MAX & ~(s->cluster_size - 1)) >> BDRV_SECTOR_BITS

Now the loop will allocate full clusters until the last iteration.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]