Re: [Qemu-devel] [PATCH 1/2] qemu-img convert: Rewrite copying logic

[Max Reitz <mreitz@redhat.com>]

On 2015-02-19 at 10:47, Kevin Wolf wrote:
> Am 18.02.2015 um 18:18 hat Max Reitz geschrieben:

[snip]

>> Actually, now that I'm looking at is_allocated_sectors_min(), if the
>> first sectors is not allocated, it will return false and thus both
>> the current qemu-img convert and the new one after this patch won't
>> write data. That's a bug, I think (because it is guaranteed by the
>> man page).
> Or the description in the man page is wrong.
>
> The intention with -S was that we avoid splitting up writes into too
> many small chunks because that costs a lot of time. If you look at it
> from that angle, it's doing exactly the right thing because skipping
> zeros at the start doesn't increase the number of write requests.

Feel free to fix it. But I remember someone recently asking about 
preallocation for qemu-img convert in the #qemu IRC channel, and "-S 0" 
was one of the valid answers.

The nice thing about -S 0 is that it works with all image formats; I 
know that qcow2 is always our main concern (especially when it comes to 
the target of qemu-img convert), but I think using -S 0 for 
preallocation is justified.

Considering that in this case the man page was lying (because the code 
did not allocate everything), I'd be fine with fixing up the man page 
and leaving the behavior as-is, though, too.


[snip]

>>> +
>>> +    if (s->compressed) {
>>> +        /* signal EOF to align */
>>> +        bdrv_write_compressed(s->target, 0, NULL, 0);
>> Is there a reason for ignoring the return value other than "That's
>> how img_convert() used to do it"?
> No. Isn't that one good enough? ;-)

I don't know, your commit message states that the old implementation is 
buggy, so I don't think that's enough. :-)

> So the code in qcow2 says this:
>
>      if (nb_sectors == 0) {
>          /* align end of file to a sector boundary to ease reading with
>             sector based I/Os */
>          cluster_offset = bdrv_getlength(bs->file);
>          return bdrv_truncate(bs->file, cluster_offset);
>      }
>
> I don't think we have any such restrictions any more, so it's mostly
> useless. Perhaps ancient qemu versions would fail to read such an image,
> but recent ones shouldn't.
>
> In fact, our bdrv_pwrite() currently maps to sector-aligned functions in
> the protocol driver, so I think at the moment we already get the
> alignment automatically. This might change again once we convert block
> drivers to byte offsets.

So you're intending to drop the bdrv_write_compressed() completely? I'd 
be fine with that as well.

Max