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

[Max Reitz <mreitz@redhat.com>]

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

On 22.02.2016 13:50, Kevin Wolf wrote:
> Am 20.02.2016 um 18:39 hat Max Reitz geschrieben:
>> 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 introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA
>> which is set by convert_iteration_sectors() if an area is detected to be
>> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because
>> knowing an area to be zero allows us to memset() the buffer to be
>> written directly rather than having to use blk_read().
>>
>> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA.
>>
>> 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 <mreitz@redhat.com>
> 
> I don't like how you touch the conversion code all over the place.
> Specifically, convert_iteration_sectors() and convert_read() (and
> consequently s->status) are supposed to be only about the source file.
> -S 0 doesn't make a difference for what the source file looks like, so
> we shouldn't need to change anything there.
> 
> The following change should do the same thing (it passes your test case
> anyway) and is more contained to the actual writeout of image data.

Well, I briefly considered making @buf non-const in convert_write(), but
I discarded that idea, and I'm still not comfortable with that. If you
argue that convert_read() should only deal with the source image, I'll
argue that convert_write() should only deal with the target image. I
know that you're making @buf non-const because we need some scratch
buffer and, well, @buf is available, so why not use that? But it still
doesn't sit right with me.

So I'd like to pull that memset() out of convert_write() and just tell
convert_write() to write that buffer as data. In fact, that is basically
what my patch does. But why does it then not just use BLK_DATA but this
new status?

Because of two reasons: First, another issue with your patch is that
zeroed areas are not counted in the progress update. If we are writing
them as zeros, we should count them, however. Therefore, we need some
special-casing in convert_do_copy(). Effectively, we end up with stuff like:

> if (s->status == BLK_DATA ||
>     (!s->min_sparse && s->status == BLK_ZERO))

I found that combination of (min_sparse && BLK_ZERO) to be ugly, and
liked it much better if we could do that test a single time in
convert_read and be done with it. This is why I added the new status.*

And if you pull the memset() out of convert_write() and add a new
status, what you end up with is basically my patch.

*Note that I initially thought we'd have this test in many more places
than we actually would, as it turned out. I'll take a look at how much
simpler this patch becomes if I drop the BLK_ZERO_DATA status.

Max

> 
> Kevin
> 
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2edb139..3b234cf 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1540,14 +1540,21 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
>  }
>  
>  static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
> -                         const uint8_t *buf)
> +                         uint8_t *buf)
>  {
>      int ret;
>  
>      while (nb_sectors > 0) {
>          int n = nb_sectors;
> +        int status = s->status;
>  
> -        switch (s->status) {
> +        if (!s->min_sparse && status == BLK_ZERO) {
> +            n = MIN(n, s->buf_sectors);
> +            memset(buf, 0, n * BDRV_SECTOR_SIZE);
> +            status = BLK_DATA;
> +        }
> +
> +        switch (status) {
>          case BLK_BACKING_FILE:
>              /* If we have a backing file, leave clusters unallocated that are
>               * unallocated in the source image, so that the backing file is
> @@ -1602,7 +1609,9 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
>  
>          sector_num += n;
>          nb_sectors -= n;
> -        buf += n * BDRV_SECTOR_SIZE;
> +        if (s->status == BLK_DATA) {
> +            buf += n * BDRV_SECTOR_SIZE;
> +        }
>      }
>  
>      return 0;
> 



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