[Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[jemmy858585]

From: Lidong Chen <lidongchen@tencent.com>

is_allocated_sectors_min don't guarantee to contain the
consecutive number of zero bytes. this patch fixes this bug.

Signed-off-by: Lidong Chen <lidongchen@tencent.com>
---
 qemu-img.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b220cf7..df6d165 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1060,9 +1060,9 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
 }
 
 /*
- * Like is_allocated_sectors, but if the buffer starts with a used sector,
- * up to 'min' consecutive sectors containing zeros are ignored. This avoids
- * breaking up write requests for only small sparse areas.
+ * Like is_allocated_sectors, but up to 'min' consecutive sectors
+ * containing zeros are ignored. This avoids breaking up write requests
+ * for only small sparse areas.
  */
 static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
     int min)
@@ -1071,11 +1071,12 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
     int num_checked, num_used;
 
     if (n < min) {
-        min = n;
+        *pnum = n;
+        return 1;
     }
 
     ret = is_allocated_sectors(buf, n, pnum);
-    if (!ret) {
+    if (!ret && *pnum >= min) {
         return ret;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[jemmy858585]

From: Lidong Chen <lidongchen@tencent.com>

Fix some spelling errors in is_allocated_sectors comment.

Signed-off-by: Lidong Chen <lidongchen@tencent.com>
---
 qemu-img.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index df6d165..0b3349c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1033,8 +1033,8 @@ done:
 }
 
 /*
- * Returns true iff the first sector pointed to by 'buf' contains at least
- * a non-NUL byte.
+ * Returns true if the first sector pointed to by 'buf' contains at least
+ * a non-NULL byte.
  *
  * 'pnum' is set to the number of sectors (including and immediately following
  * the first one) that are known to be in the same allocated/unallocated state.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[Eric Blake]

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

On 04/23/2017 09:33 AM, jemmy858585@gmail.com wrote:
> From: Lidong Chen <lidongchen@tencent.com>
> 
> Fix some spelling errors in is_allocated_sectors comment.
> 
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
>  qemu-img.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index df6d165..0b3349c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1033,8 +1033,8 @@ done:
>  }
>  
>  /*
> - * Returns true iff the first sector pointed to by 'buf' contains at least
> - * a non-NUL byte.
> + * Returns true if the first sector pointed to by 'buf' contains at least
> + * a non-NULL byte.

NACK to both changes.  'iff' is an English word that is shorthand for
"if and only if".  "NUL" means the one-byte character, while "NULL"
means the 8-byte (or 4-byte, on 32-bit platform) pointer value.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[Philippe Mathieu-Daudé]

Hi Eric,

On 04/24/2017 11:40 AM, Eric Blake wrote:
> On 04/23/2017 09:33 AM, jemmy858585@gmail.com wrote:
>> From: Lidong Chen <lidongchen@tencent.com>
>>
>> Fix some spelling errors in is_allocated_sectors comment.
>>
>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>> ---
>>  qemu-img.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index df6d165..0b3349c 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1033,8 +1033,8 @@ done:
>>  }
>>
>>  /*
>> - * Returns true iff the first sector pointed to by 'buf' contains at least
>> - * a non-NUL byte.
>> + * Returns true if the first sector pointed to by 'buf' contains at least
>> + * a non-NULL byte.
>
> NACK to both changes.  'iff' is an English word that is shorthand for
> "if and only if".  "NUL" means the one-byte character, while "NULL"
> means the 8-byte (or 4-byte, on 32-bit platform) pointer value.

I agree with Lidong shorthands are not obvious from non-native speaker.

What about this?

  * Returns true if (and only if) the first sector pointed to by 'buf' 
contains
  * at least a non-null character.

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[Eric Blake]

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

On 04/24/2017 10:37 AM, Philippe Mathieu-Daudé wrote:

>>>  /*
>>> - * Returns true iff the first sector pointed to by 'buf' contains at
>>> least
>>> - * a non-NUL byte.
>>> + * Returns true if the first sector pointed to by 'buf' contains at
>>> least
>>> + * a non-NULL byte.
>>
>> NACK to both changes.  'iff' is an English word that is shorthand for
>> "if and only if".  "NUL" means the one-byte character, while "NULL"
>> means the 8-byte (or 4-byte, on 32-bit platform) pointer value.
> 
> I agree with Lidong shorthands are not obvious from non-native speaker.
> 
> What about this?
> 
>  * Returns true if (and only if) the first sector pointed to by 'buf'
> contains

That might be okay.

>  * at least a non-null character.

But that still doesn't make sense.  The character name is NUL, and
non-NULL refers to something that is a pointer, not a character.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[Eric Blake]

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

On 04/24/2017 10:47 AM, Eric Blake wrote:
> On 04/24/2017 10:37 AM, Philippe Mathieu-Daudé wrote:
> 
>>>>  /*
>>>> - * Returns true iff the first sector pointed to by 'buf' contains at
>>>> least
>>>> - * a non-NUL byte.
>>>> + * Returns true if the first sector pointed to by 'buf' contains at
>>>> least
>>>> + * a non-NULL byte.
>>>
>>> NACK to both changes.  'iff' is an English word that is shorthand for
>>> "if and only if".  "NUL" means the one-byte character, while "NULL"
>>> means the 8-byte (or 4-byte, on 32-bit platform) pointer value.
>>
>> I agree with Lidong shorthands are not obvious from non-native speaker.
>>
>> What about this?
>>
>>  * Returns true if (and only if) the first sector pointed to by 'buf'
>> contains
> 
> That might be okay.
> 
>>  * at least a non-null character.
> 
> But that still doesn't make sense.  The character name is NUL, and
> non-NULL refers to something that is a pointer, not a character.

What's more, the NUL character can actually occupy more than one byte
(think UTF-16, where it is the two-byte 0 value).  Referring to NUL byte
rather than NUL character (or even the 'zero byte') makes it obvious
that this function is NOT encoding-sensitive, and doesn't start
mis-behaving just because the data picks a multi-byte character encoding.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[858585 jemmy]

On Mon, Apr 24, 2017 at 11:53 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/24/2017 10:47 AM, Eric Blake wrote:
>> On 04/24/2017 10:37 AM, Philippe Mathieu-Daudé wrote:
>>
>>>>>  /*
>>>>> - * Returns true iff the first sector pointed to by 'buf' contains at
>>>>> least
>>>>> - * a non-NUL byte.
>>>>> + * Returns true if the first sector pointed to by 'buf' contains at
>>>>> least
>>>>> + * a non-NULL byte.
>>>>
>>>> NACK to both changes.  'iff' is an English word that is shorthand for
>>>> "if and only if".  "NUL" means the one-byte character, while "NULL"
>>>> means the 8-byte (or 4-byte, on 32-bit platform) pointer value.
>>>
>>> I agree with Lidong shorthands are not obvious from non-native speaker.
>>>
>>> What about this?
>>>
>>>  * Returns true if (and only if) the first sector pointed to by 'buf'
>>> contains
>>
>> That might be okay.
>>
>>>  * at least a non-null character.
>>
>> But that still doesn't make sense.  The character name is NUL, and
>> non-NULL refers to something that is a pointer, not a character.
>
> What's more, the NUL character can actually occupy more than one byte
> (think UTF-16, where it is the two-byte 0 value).  Referring to NUL byte
> rather than NUL character (or even the ' byte') makes it obvious
> that this function is NOT encoding-sensitive, and doesn't start
> mis-behaving just because the data picks a multi-byte character encoding.

How about this?

 * Returns true  if (and only if) the first sector pointed to by 'buf'
contains at least
 * a non-zero byte.

Thanks.

>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[Max Reitz]

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

On 24.04.2017 17:53, Eric Blake wrote:
> On 04/24/2017 10:47 AM, Eric Blake wrote:
>> On 04/24/2017 10:37 AM, Philippe Mathieu-Daudé wrote:
>>
>>>>>  /*
>>>>> - * Returns true iff the first sector pointed to by 'buf' contains at
>>>>> least
>>>>> - * a non-NUL byte.
>>>>> + * Returns true if the first sector pointed to by 'buf' contains at
>>>>> least
>>>>> + * a non-NULL byte.
>>>>
>>>> NACK to both changes.  'iff' is an English word that is shorthand for
>>>> "if and only if".  "NUL" means the one-byte character, while "NULL"
>>>> means the 8-byte (or 4-byte, on 32-bit platform) pointer value.
>>>
>>> I agree with Lidong shorthands are not obvious from non-native speaker.
>>>
>>> What about this?
>>>
>>>  * Returns true if (and only if) the first sector pointed to by 'buf'
>>> contains
>>
>> That might be okay.

Might, yes, but we have it all over the code. I'm not particularly avid
to change this, because I am in fact one of the culprits (and I'm a
non-native speaker, but I do like to use LaTeX so I know my \iff).

(By the way, judging from the author's name of this line of code (which
is Thiemo Seufer), I'd wager he's not a native speaker either.)

>>>  * at least a non-null character.
>>
>> But that still doesn't make sense.  The character name is NUL, and
>> non-NULL refers to something that is a pointer, not a character.
> 
> What's more, the NUL character can actually occupy more than one byte
> (think UTF-16, where it is the two-byte 0 value).  Referring to NUL byte
> rather than NUL character (or even the 'zero byte') makes it obvious
> that this function is NOT encoding-sensitive, and doesn't start
> mis-behaving just because the data picks a multi-byte character encoding.

Furthermore, this doesn't have anything to do with being a native
speaker or not: NUL is just the commonly used and probably standardized
abbreviation of a certain ASCII character (in any language). It's OK not
to know this, but I don't think it's OK to change the comment.

Max


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

Re: [Qemu-devel] [PATCH 2/2] qemu-img: fix some spelling errors

[858585 jemmy]

On Wed, Apr 26, 2017 at 3:11 AM, Max Reitz <mreitz@redhat.com> wrote:
> On 24.04.2017 17:53, Eric Blake wrote:
>> On 04/24/2017 10:47 AM, Eric Blake wrote:
>>> On 04/24/2017 10:37 AM, Philippe Mathieu-Daudé wrote:
>>>
>>>>>>  /*
>>>>>> - * Returns true iff the first sector pointed to by 'buf' contains at
>>>>>> least
>>>>>> - * a non-NUL byte.
>>>>>> + * Returns true if the first sector pointed to by 'buf' contains at
>>>>>> least
>>>>>> + * a non-NULL byte.
>>>>>
>>>>> NACK to both changes.  'iff' is an English word that is shorthand for
>>>>> "if and only if".  "NUL" means the one-byte character, while "NULL"
>>>>> means the 8-byte (or 4-byte, on 32-bit platform) pointer value.
>>>>
>>>> I agree with Lidong shorthands are not obvious from non-native speaker.
>>>>
>>>> What about this?
>>>>
>>>>  * Returns true if (and only if) the first sector pointed to by 'buf'
>>>> contains
>>>
>>> That might be okay.
>
> Might, yes, but we have it all over the code. I'm not particularly avid
> to change this, because I am in fact one of the culprits (and I'm a
> non-native speaker, but I do like to use LaTeX so I know my \iff).
>
> (By the way, judging from the author's name of this line of code (which
> is Thiemo Seufer), I'd wager he's not a native speaker either.)
>
>>>>  * at least a non-null character.
>>>
>>> But that still doesn't make sense.  The character name is NUL, and
>>> non-NULL refers to something that is a pointer, not a character.
>>
>> What's more, the NUL character can actually occupy more than one byte
>> (think UTF-16, where it is the two-byte 0 value).  Referring to NUL byte
>> rather than NUL character (or even the 'zero byte') makes it obvious
>> that this function is NOT encoding-sensitive, and doesn't start
>> mis-behaving just because the data picks a multi-byte character encoding.
>
> Furthermore, this doesn't have anything to do with being a native
> speaker or not: NUL is just the commonly used and probably standardized
> abbreviation of a certain ASCII character (in any language). It's OK not
> to know this, but I don't think it's OK to change the comment.
Thanks for your explanation.
>
> Max
>

Re: [Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[Eric Blake]

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

On 04/23/2017 09:33 AM, jemmy858585@gmail.com wrote:
> From: Lidong Chen <lidongchen@tencent.com>
> 
> is_allocated_sectors_min don't guarantee to contain the
> consecutive number of zero bytes. this patch fixes this bug.

This message was sent without an 'In-Reply-To' header pointing to a 0/2
cover letter.  When sending a series, please always thread things to a
cover letter; you may find 'git config format.coverletter auto' to be
helpful.

> 
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
>  qemu-img.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..df6d165 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1060,9 +1060,9 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
>  }
>  
>  /*
> - * Like is_allocated_sectors, but if the buffer starts with a used sector,
> - * up to 'min' consecutive sectors containing zeros are ignored. This avoids
> - * breaking up write requests for only small sparse areas.
> + * Like is_allocated_sectors, but up to 'min' consecutive sectors
> + * containing zeros are ignored. This avoids breaking up write requests
> + * for only small sparse areas.
>   */
>  static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>      int min)
> @@ -1071,11 +1071,12 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>      int num_checked, num_used;
>  
>      if (n < min) {
> -        min = n;
> +        *pnum = n;
> +        return 1;
>      }
>  
>      ret = is_allocated_sectors(buf, n, pnum);
> -    if (!ret) {
> +    if (!ret && *pnum >= min) {

I seem to recall past attempts to try and patch this function, which
were then turned down, although I haven't scrubbed the archives for a
quick URL to point to. I'm worried that there are more subtleties here
than what you realize.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[858585 jemmy]

On Mon, Apr 24, 2017 at 10:43 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/23/2017 09:33 AM, jemmy858585@gmail.com wrote:
>> From: Lidong Chen <lidongchen@tencent.com>
>>
>> is_allocated_sectors_min don't guarantee to contain the
>> consecutive number of zero bytes. this patch fixes this bug.
>
> This message was sent without an 'In-Reply-To' header pointing to a 0/2
> cover letter.  When sending a series, please always thread things to a
> cover letter; you may find 'git config format.coverletter auto' to be
> helpful.

Thanks for your kind advises.

>
>>
>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>> ---
>>  qemu-img.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b220cf7..df6d165 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1060,9 +1060,9 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
>>  }
>>
>>  /*
>> - * Like is_allocated_sectors, but if the buffer starts with a used sector,
>> - * up to 'min' consecutive sectors containing zeros are ignored. This avoids
>> - * breaking up write requests for only small sparse areas.
>> + * Like is_allocated_sectors, but up to 'min' consecutive sectors
>> + * containing zeros are ignored. This avoids breaking up write requests
>> + * for only small sparse areas.
>>   */
>>  static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>>      int min)
>> @@ -1071,11 +1071,12 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>>      int num_checked, num_used;
>>
>>      if (n < min) {
>> -        min = n;
>> +        *pnum = n;
>> +        return 1;
>>      }
>>
>>      ret = is_allocated_sectors(buf, n, pnum);
>> -    if (!ret) {
>> +    if (!ret && *pnum >= min) {
>
> I seem to recall past attempts to try and patch this function, which
> were then turned down, although I haven't scrubbed the archives for a
> quick URL to point to. I'm worried that there are more subtleties here
> than what you realize.

Hi Eric:
Do you mean this URL?
https://lists.gnu.org/archive/html/qemu-block/2017-01/msg00306.html

But I think the code is not consistent with qemu-img --help.
qemu-img --help
  '-S' indicates the consecutive number of bytes (defaults to 4k) that must
       contain only zeros for qemu-img to create a sparse image during
       conversion. If the number of bytes is 0, the source will not be
scanned for
       unallocated or zero sectors, and the destination image will always be
       fully allocated.

another reason:
if s->has_zero_init is 1(the qcow2 image which have backing_file), the empty
space at the beginning of the buffer still need write and invoke
blk_co_pwrite_zeroes.
and split a single write operation into two just because there is small empty
space at the beginning.

Thanks.

>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>

Re: [Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[Eric Blake]

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

On 04/24/2017 08:50 PM, 858585 jemmy wrote:
> On Mon, Apr 24, 2017 at 10:43 PM, Eric Blake <eblake@redhat.com> wrote:
>> On 04/23/2017 09:33 AM, jemmy858585@gmail.com wrote:
>>> From: Lidong Chen <lidongchen@tencent.com>
>>>
>>> is_allocated_sectors_min don't guarantee to contain the
>>> consecutive number of zero bytes. this patch fixes this bug.
>>
>> This message was sent without an 'In-Reply-To' header pointing to a 0/2
>> cover letter.  When sending a series, please always thread things to a
>> cover letter; you may find 'git config format.coverletter auto' to be
>> helpful.
> 
> Thanks for your kind advises.
> 

>>
>> I seem to recall past attempts to try and patch this function, which
>> were then turned down, although I haven't scrubbed the archives for a
>> quick URL to point to. I'm worried that there are more subtleties here
>> than what you realize.
> 
> Hi Eric:
> Do you mean this URL?
> https://lists.gnu.org/archive/html/qemu-block/2017-01/msg00306.html

Yes, that's probably the one.

> 
> But I think the code is not consistent with qemu-img --help.
> qemu-img --help
>   '-S' indicates the consecutive number of bytes (defaults to 4k) that must
>        contain only zeros for qemu-img to create a sparse image during
>        conversion. If the number of bytes is 0, the source will not be
> scanned for
>        unallocated or zero sectors, and the destination image will always be
>        fully allocated.

If you still think this patch is needed, the best way to convince me of
it is accompany your patch by a qemu-iotests enhancement that covers the
change in behavior (running the test pre-patch would show that we are
broken without the patch, and having the test ensures we can't later
regress).  That's a lot more work than the vague two lines of the commit
message body.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 1/2] qemu-img: make sure contain the consecutive number of zero bytes

[Max Reitz]

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

On 25.04.2017 03:50, 858585 jemmy wrote:
> On Mon, Apr 24, 2017 at 10:43 PM, Eric Blake <eblake@redhat.com> wrote:
>> On 04/23/2017 09:33 AM, jemmy858585@gmail.com wrote:
>>> From: Lidong Chen <lidongchen@tencent.com>
>>>
>>> is_allocated_sectors_min don't guarantee to contain the
>>> consecutive number of zero bytes. this patch fixes this bug.
>>
>> This message was sent without an 'In-Reply-To' header pointing to a 0/2
>> cover letter.  When sending a series, please always thread things to a
>> cover letter; you may find 'git config format.coverletter auto' to be
>> helpful.
> 
> Thanks for your kind advises.
> 
>>
>>>
>>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>>> ---
>>>  qemu-img.c | 11 ++++++-----
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index b220cf7..df6d165 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1060,9 +1060,9 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
>>>  }
>>>
>>>  /*
>>> - * Like is_allocated_sectors, but if the buffer starts with a used sector,
>>> - * up to 'min' consecutive sectors containing zeros are ignored. This avoids
>>> - * breaking up write requests for only small sparse areas.
>>> + * Like is_allocated_sectors, but up to 'min' consecutive sectors
>>> + * containing zeros are ignored. This avoids breaking up write requests
>>> + * for only small sparse areas.
>>>   */
>>>  static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>>>      int min)
>>> @@ -1071,11 +1071,12 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
>>>      int num_checked, num_used;
>>>
>>>      if (n < min) {
>>> -        min = n;
>>> +        *pnum = n;
>>> +        return 1;
>>>      }
>>>
>>>      ret = is_allocated_sectors(buf, n, pnum);
>>> -    if (!ret) {
>>> +    if (!ret && *pnum >= min) {
>>
>> I seem to recall past attempts to try and patch this function, which
>> were then turned down, although I haven't scrubbed the archives for a
>> quick URL to point to. I'm worried that there are more subtleties here
>> than what you realize.
> 
> Hi Eric:
> Do you mean this URL?
> https://lists.gnu.org/archive/html/qemu-block/2017-01/msg00306.html
> 
> But I think the code is not consistent with qemu-img --help.
> qemu-img --help
>   '-S' indicates the consecutive number of bytes (defaults to 4k) that must
>        contain only zeros for qemu-img to create a sparse image during
>        conversion. If the number of bytes is 0, the source will not be
> scanned for
>        unallocated or zero sectors, and the destination image will always be
>        fully allocated.
> 
> another reason:
> if s->has_zero_init is 1(the qcow2 image which have backing_file), the empty
> space at the beginning of the buffer still need write and invoke
> blk_co_pwrite_zeroes.

Right, that is indeed a reason that we may want to behave differently in
these cases. But in other cases this is less efficient.

> and split a single write operation into two just because there is small empty
> space at the beginning.
And then there's the fact that, in my opinion, this is actually a
problem at qcow2 level. Some people are working on improving this (see
e.g. Berto's subcluster RFC, which would allow us to implement
bdrv_co_pwrite_zeroes() below cluster granularity).

All in all, I don't think you can generically circumvent this issue here
for all block drivers. The rule we'd have to implement here is: If some
part of a cluster (to be written) is zero and another is not, we should
write the whole cluster. If a whole cluster is zero, we should issue a
write-zeroes request. But that would mean to check where some buffer
passes a cluster boundary and so on, and on top of this all of it is
qcow2-specific; so there goes the genericity...

Max


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