| Message ID | 1484843730-23848-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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 <pl@kamp.de> > --- > 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
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 <pl@kamp.de> >> --- >> 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
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; } /*
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 <pl@kamp.de> --- qemu-img.c | 44 ++++++++++++++------------------------------ 1 file changed, 14 insertions(+), 30 deletions(-)