Re: [Qemu-block] [PATCH v2 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Fam Zheng]

On Thu, 03/24 23:33, Max Reitz wrote:
> When passing -S 0 to qemu-img convert, the target image is supposed to
> be fully allocated. Right now, this is not the case if the source image
> contains areas which bdrv_get_block_status() reports as being zero.
> 
> This patch changes a zeroed area's status from BLK_ZERO to BLK_DATA
> before invoking convert_write() if -S 0 has been specified. In addition,
> the check whether convert_read() actually needs to do anything
> (basically only if the current area is a BLK_DATA area) is pulled out of
> that function to the caller.
> 
> If -S 0 has been specified, zeroed areas need to be written as data to
> the output, thus they then have to be accounted when calculating the
> progress made.
> 
> This patch changes the reference output for iotest 122; contrary to what
> it assumed, -S 0 really should allocate everything in the output, not
> just areas that are filled with zeros (as opposed to being zeroed).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  qemu-img.c                 | 26 +++++++++++++++-----------
>  tests/qemu-iotests/122.out |  6 ++----
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 29eae2a..b2e07bb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1507,10 +1507,6 @@ static int convert_read(ImgConvertState *s, int64_t 
> sector_num, int nb_sectors,
>      int n;
>      int ret;
>  
> -    if (s->status == BLK_ZERO || s->status == BLK_BACKING_FILE) {
> -        return 0;
> -    }
> -
>      assert(nb_sectors <= s->buf_sectors);
>      while (nb_sectors > 0) {
>          BlockBackend *blk;
> @@ -1648,7 +1644,8 @@ static int convert_do_copy(ImgConvertState *s)
>              ret = n;
>              goto fail;
>          }
> -        if (s->status == BLK_DATA) {
> +        if (s->status == BLK_DATA || (!s->min_sparse && s->status == 
> BLK_ZERO))
> +        {
>              s->allocated_sectors += n;
>          }
>          sector_num += n;
> @@ -1668,17 +1665,24 @@ static int convert_do_copy(ImgConvertState *s)
>              ret = n;
>              goto fail;
>          }
> -        if (s->status == BLK_DATA) {
> +        if (s->status == BLK_DATA || (!s->min_sparse && s->status == 
> BLK_ZERO))
> +        {
>              allocated_done += n;
>              qemu_progress_print(100.0 * allocated_done / 
> s->allocated_sectors,
>                                  0);
>          }
>  
> -        ret = convert_read(s, sector_num, n, buf);
> -        if (ret < 0) {
> -            error_report("error while reading sector %" PRId64
> -                         ": %s", sector_num, strerror(-ret));
> -            goto fail;
> +        if (s->status == BLK_DATA) {
> +            ret = convert_read(s, sector_num, n, buf);
> +            if (ret < 0) {
> +                error_report("error while reading sector %" PRId64
> +                             ": %s", sector_num, strerror(-ret));
> +                goto fail;
> +            }
> +        } else if (!s->min_sparse && s->status == BLK_ZERO) {
> +            n = MIN(n, s->buf_sectors);
> +            memset(buf, 0, n * BDRV_SECTOR_SIZE);
> +            s->status = BLK_DATA;
>          }
>  
>          ret = convert_write(s, sector_num, n, buf);
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index 0068e96..98814de 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -112,16 +112,14 @@ read 3145728/3145728 bytes at offset 0
>  3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 63963136/63963136 bytes at offset 3145728
>  61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true, 
> "offset": 327680},
> -{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": 
> false}]
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, 
> "offset": 327680}]
>  
>  convert -c -S 0:
>  read 3145728/3145728 bytes at offset 0
>  3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 63963136/63963136 bytes at offset 3145728
>  61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true},
> -{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": 
> false}]
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}]
>  Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
>  wrote 33554432/33554432 bytes at offset 0
>  32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -- 
> 2.7.4
> 

Looks sane to me,

Reviewed-by: Fam Zheng <address@hidden>