Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img: improve convert_iteration_sectors()

[Max Reitz]

On 20.04.2017 01:06, John Snow wrote:
> 
> 
> On 04/07/2017 07:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Do not do extra call to _get_block_status()
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>
>> Also, I'm not sure about last line:
>> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;
>>
>> (which is equal to old code)
>>
> 
> What a weird function.
> 
> So, if the target has a backing file, literally nothing changes here.
> 
> If it doesn't, we skip the initial call to get_block_status and just
> call the (effectively?) recursive version to find out if we have a ZERO
> or DATA type of allocation.
> 
> The else clause here properly reflects the original reading of the code.
> 
> OK.

Well, this is what happens when optimizing parts of something without
looking at the bigger picture (263a6f4c3aa).

>> may be, it should be
>> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_ZERO;
>>
>> as it is the case, when range is not allocated at all. Should we copy it in 
>> this case?
>>
> 
> I am not really clear on if either ZERO or DATA are correct in the case
> where we do not have a backing file, because maybe this depends on
> has_zero_init?
> 
> If we are copying uninitialized data when has_zero_init is true on the
> source, we want zeroes on the target. If the target also has_zero_init,
> we can just skip this sector. If it doesn't, we want to copy zeroes.
> 
> If has_zero_init is false and we have unallocated data on the source...
> what are we doing? Copying random debris?

Probably. Sounds fine to me, though. Because if you were to read from
the source I'd expect you'd read random debris, so it's fine to write
that to the destination, too.

Alternatively, we might want to add a new status BLK_UNALLOCATED which
would simply skip both reading and writing these areas.

Alas! my craving does not suffice to compel my lowly self to excogitate
a patch.

Max

> I'm pretty confused, but I'm pretty sure your patch doesn't actually
> change anything, so:
> 
> Reviewed-by: John Snow <address@hidden>

signature.asc
Description: OpenPGP digital signature