Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5132 bytes --]

On 07.02.20 16:16, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 18:03, Max Reitz wrote:
>> On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.02.2020 17:41, Max Reitz wrote:
>>>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 07.02.2020 13:33, Max Reitz wrote:
>>>>>> On 04.02.20 15:23, Eric Blake wrote:
>>>>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>
>>>>>>>>> I understand that it is safer to have restrictions now and lift
>>>>>>>>> them
>>>>>>>>> later, than to allow use of the option at any time and leave room
>>>>>>>>> for
>>>>>>>>> the user to shoot themselves in the foot with no way to add safety
>>>>>>>>> later.  The argument against no backing file is somewhat
>>>>>>>>> understandable (technically, as long as the backing file also
>>>>>>>>> reads
>>>>>>>>> as all zeroes, then the overall image reads as all zeroes - but
>>>>>>>>> why
>>>>>>>>> have a backing file that has no content?); the argument
>>>>>>>>> requiring -n
>>>>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as
>>>>>>>>> all
>>>>>>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>>>>>>> shouldn't hurt to allow it).
>>>>>>>>
>>>>>>>> I know that it reads as all zeroes, only if this format provides
>>>>>>>> zero
>>>>>>>> initialization..
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +++ b/qemu-img.c
>>>>>>>>>
>>>>>>>>>> @@ -2247,6 +2256,11 @@ static int img_convert(int argc, char
>>>>>>>>>> **argv)
>>>>>>>>>>              warn_report("This will become an error in future
>>>>>>>>>> QEMU
>>>>>>>>>> versions.");
>>>>>>>>>>          }
>>>>>>>>>> +    if (s.has_zero_init && !skip_create) {
>>>>>>>>>> +        error_report("--target-is-zero requires use of -n
>>>>>>>>>> flag");
>>>>>>>>>> +        goto fail_getopt;
>>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> So I think we could drop this hunk with no change in behavior.
>>>>>>>>
>>>>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd
>>>>>>>> better
>>>>>>>> to check that what we are creating is zero-initialized (format has
>>>>>>>> zero-init), and if not we should report error.
>>>>>>>>
>>>>>>>
>>>>>>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST
>>>>>>> insist
>>>>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>>>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>>>>>
>>>>>> Why?
>>>>>>
>>>>>> I could imagine a user creating a qcow2 image on some block device
>>>>>> with
>>>>>> preallocation where we cannot verify that the result will be
>>>>>> zero.  But
>>>>>> they want qemu not to zero the device, so they would specify
>>>>>> --target-is-zero.
>>>>>
>>>>> If user create image, setting --target-is-zero is always valid. But if
>>>>> we in
>>>>> same operation create the image automatically, having
>>>>> --target-is-zero,
>>>>> when
>>>>> we know that what we are creating is not zero is misleading and should
>>>>> fail..
>>>>
>>>> bdrv_has_zero_init() doesn’t return false only for images that we know
>>>> are not zero.  It returns true for images where we know they are.  But
>>>> if we don’t know, then it returns false also.
>>>
>>> yes, but we don't have better check.
>>
>> Correct, but maybe the user knows more, hence why it may make sense for
>> them to provide us with some information we don’t have.
>>
>>>>> If we want to add a behavior to skip zeros unconditionally, we should
>>>>> call new
>>>>> option --skip-zeroes, to clearly specify what we want.
>>>>
>>>> It was my impression that this was exactly what --target-is-zero means
>>>> and implies.
>>>>
>>>
>>> For me it sounds strange that user has better knowledge about what Qemu
>>> creates than Qemu itself. And if it so - it should be fixed in Qemu,
>>> rather than creating user interface to hint Qemu what it does.
>>
>> I brought an example where qemu cannot know whether the image is zero
>> (preallocation on a block device), but the user / management layer might
>> know.
>>
> 
> It sounds unsafe for me. User can't know how exactly Qemu do preallocation,
> which syscalls it calls, etc. How can user be sure, that Qemu produces
> all-zero image, if even Qemu doesn't sure in it?

For example, qcow2 images are always zero if the underlying storage is zero.

Then again, it isn’t like we need --target-is-zero without -n anyway.
If users want that, they can always use qemu-img create +
qemu-img convert -n --target-is-zero.  So seeing that you’re
uncomfortable with the idea of --target-is-zero with -n, I can’t say
there is any actual reason why we’d have to allow it.

Max

> Otherwise, we should document, how exactly (up to syscalls, their
> parameters, flags, the whole logic and algorithm) preallocation is done,
> so user can analyze it and be sure that produced image would be all-zero
> (when Qemu can't determine it because some specific block device, for which
> Qemu doesn't know that its preallocation algorithm produces all-zero, but
> user is sure in it)..


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