Re: [Qemu-devel] [PATCH V2] qemu-img: optimize is_allocated_sectors_min

[Peter Lieven]

Am 21.01.2017 um 20:58 schrieb Max Reitz:

On 19.01.2017 17:35, Peter Lieven wrote:

the current implementation always splits requests if a buffer
begins or ends with zeroes independent of the length of the
zero area. Change this to really only split off zero areas
that have at least a length of 'min' bytes.

Signed-off-by: Peter Lieven 
---
  qemu-img.c | 44 ++--
  1 file changed, 14 insertions(+), 30 deletions(-)

That is because the idea is that you should delay the write operation as
much as possible. Say you have some empty space at the beginning of the
buffer and then some used space. Of course you should then only start
writing at the point where you actually have to write something. The
same goes for the end of the buffer. Why write zeroes when you can just
not do it?

On the other hand, it makes sense to not split a single write operation
into two just because there is some empty space in the middle. This is
because just writing that empty space may take less time than issuing
two operations, especially if that empty space is e.g. in the middle of
a qcow2 cluster anyway.

This is why empty space in the middle is treated differently than empty
space at the beginning or the end of the buffer.

Do you have a reason for changing this other than because it simplifies
the code? Which it does, admittedly, but I think the code does have a
reason for being how it is.


Hi Max,

thanks for looking into this. I was wrong about my analysis what was
going on with a problem I tried to track down. After your explanation
I think the current implementation is fine and it makes perfect sense
to cut off leading and trailing zeroes.

Peter

Re: [Qemu-devel] [PATCH V2] qemu-img: optimize is_allocated_sectors_min

[Max Reitz]

On 19.01.2017 17:35, Peter Lieven wrote:
> the current implementation always splits requests if a buffer
> begins or ends with zeroes independent of the length of the
> zero area. Change this to really only split off zero areas
> that have at least a length of 'min' bytes.
> 
> Signed-off-by: Peter Lieven 
> ---
>  qemu-img.c | 44 ++--
>  1 file changed, 14 insertions(+), 30 deletions(-)

That is because the idea is that you should delay the write operation as
much as possible. Say you have some empty space at the beginning of the
buffer and then some used space. Of course you should then only start
writing at the point where you actually have to write something. The
same goes for the end of the buffer. Why write zeroes when you can just
not do it?

On the other hand, it makes sense to not split a single write operation
into two just because there is some empty space in the middle. This is
because just writing that empty space may take less time than issuing
two operations, especially if that empty space is e.g. in the middle of
a qcow2 cluster anyway.

This is why empty space in the middle is treated differently than empty
space at the beginning or the end of the buffer.

Do you have a reason for changing this other than because it simplifies
the code? Which it does, admittedly, but I think the code does have a
reason for being how it is.

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH V2] qemu-img: optimize is_allocated_sectors_min

[Peter Lieven]

the current implementation always splits requests if a buffer
begins or ends with zeroes independent of the length of the
zero area. Change this to really only split off zero areas
that have at least a length of 'min' bytes.

Signed-off-by: Peter Lieven 
---
 qemu-img.c | 44 ++--
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 5df66fe..8e7357d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1010,45 +1010,29 @@ 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 only at least 'min' consecutive sectors
+ * containing zeros are considered unallocated. 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)
+int min)
 {
-int ret;
-int num_checked, num_used;
-
-if (n < min) {
-min = n;
-}
-
-ret = is_allocated_sectors(buf, n, pnum);
-if (!ret) {
-return ret;
-}
-
-num_used = *pnum;
-buf += BDRV_SECTOR_SIZE * *pnum;
-n -= *pnum;
-num_checked = num_used;
+int num_used = 0;
 
 while (n > 0) {
-ret = is_allocated_sectors(buf, n, pnum);
-
-buf += BDRV_SECTOR_SIZE * *pnum;
-n -= *pnum;
-num_checked += *pnum;
-if (ret) {
-num_used = num_checked;
-} else if (*pnum >= min) {
+if (!is_allocated_sectors(buf, n, pnum) && *pnum >= min) {
 break;
 }
+num_used += *pnum;
+buf += BDRV_SECTOR_SIZE * *pnum;
+n -= *pnum;
 }
 
-*pnum = num_used;
-return 1;
+if (num_used) {
+*pnum = num_used;
+}
+
+return !!num_used;
 }
 
 /*
-- 
1.9.1