[v2,for-2.10,09/16] block/qcow2: Generalize preallocate()
diff mbox

Message ID 20170403160936.28293-10-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 3, 2017, 4:09 p.m. UTC
This patch adds two new parameters to the preallocate() function so we
will be able to use it not just for preallocating a new image but also
for preallocated image growth.

The offset parameter allows the caller to specify a virtual offset from
which to start preallocating. For newly created images this is always 0,
but for preallocating growth this will be the old image length.

The new_length parameter specifies the supposed new length of the image
(basically the "end offset" for preallocation). During image truncation,
bdrv_getlength() will return the old image length so we cannot rely on
its return value then.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Philippe Mathieu-Daudé April 3, 2017, 7:19 p.m. UTC | #1
Hi Max,

On 04/03/2017 01:09 PM, Max Reitz wrote:
> This patch adds two new parameters to the preallocate() function so we
> will be able to use it not just for preallocating a new image but also
> for preallocated image growth.
>
> The offset parameter allows the caller to specify a virtual offset from
> which to start preallocating. For newly created images this is always 0,
> but for preallocating growth this will be the old image length.
>
> The new_length parameter specifies the supposed new length of the image
> (basically the "end offset" for preallocation). During image truncation,
> bdrv_getlength() will return the old image length so we cannot rely on
> its return value then.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 9fd220ec34..163d51ec65 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2034,17 +2034,23 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
>      return qcow2_update_header(bs);
>  }
>
> -static int preallocate(BlockDriverState *bs)
> +/**
> + * Preallocates metadata structures for data clusters between @offset (in the
> + * guest disk) and @new_length (which is thus generally the new guest disk
> + * size).
> + *
> + * Returns: 0 on success, -errno on failure.
> + */
> +static int preallocate(BlockDriverState *bs, int64_t offset, int64_t new_length)
>  {
>      uint64_t bytes;
> -    uint64_t offset;
>      uint64_t host_offset = 0;
>      unsigned int cur_bytes;
>      int ret;
>      QCowL2Meta *meta;
>
> -    bytes = bdrv_getlength(bs);
> -    offset = 0;

why use `int64_t`? is it ok to have a negative offset?
preallocate a negative length sound weird... even `bytes` is unsigned.

> +    assert(offset <= new_length);
> +    bytes = new_length - offset;
>
>      while (bytes) {
>          cur_bytes = MIN(bytes, INT_MAX);
> @@ -2314,7 +2320,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      if (prealloc != PREALLOC_MODE_OFF) {
>          BDRVQcow2State *s = blk_bs(blk)->opaque;
>          qemu_co_mutex_lock(&s->lock);
> -        ret = preallocate(blk_bs(blk));
> +        ret = preallocate(blk_bs(blk), 0, total_size);
>          qemu_co_mutex_unlock(&s->lock);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not preallocate metadata");
>
Max Reitz April 5, 2017, 12:02 p.m. UTC | #2
On 03.04.2017 21:19, Philippe Mathieu-Daudé wrote:
> Hi Max,
> 
> On 04/03/2017 01:09 PM, Max Reitz wrote:
>> This patch adds two new parameters to the preallocate() function so we
>> will be able to use it not just for preallocating a new image but also
>> for preallocated image growth.
>>
>> The offset parameter allows the caller to specify a virtual offset from
>> which to start preallocating. For newly created images this is always 0,
>> but for preallocating growth this will be the old image length.
>>
>> The new_length parameter specifies the supposed new length of the image
>> (basically the "end offset" for preallocation). During image truncation,
>> bdrv_getlength() will return the old image length so we cannot rely on
>> its return value then.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 9fd220ec34..163d51ec65 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2034,17 +2034,23 @@ static int
>> qcow2_change_backing_file(BlockDriverState *bs,
>>      return qcow2_update_header(bs);
>>  }
>>
>> -static int preallocate(BlockDriverState *bs)
>> +/**
>> + * Preallocates metadata structures for data clusters between @offset
>> (in the
>> + * guest disk) and @new_length (which is thus generally the new guest
>> disk
>> + * size).
>> + *
>> + * Returns: 0 on success, -errno on failure.
>> + */
>> +static int preallocate(BlockDriverState *bs, int64_t offset, int64_t
>> new_length)
>>  {
>>      uint64_t bytes;
>> -    uint64_t offset;
>>      uint64_t host_offset = 0;
>>      unsigned int cur_bytes;
>>      int ret;
>>      QCowL2Meta *meta;
>>
>> -    bytes = bdrv_getlength(bs);
>> -    offset = 0;
> 
> why use `int64_t`? is it ok to have a negative offset?
> preallocate a negative length sound weird... even `bytes` is unsigned.

Right. That's actually a very good question. I can't remember why I
chose them to be signed.

Generally, it won't be an issue because the QEMU block layer will
generally not permit files to be longer than 2^63 (e.g.
bdrv_co_pwritev() and bdrv_co_preadv() take int64_t offsets).

But you're right, there is no reason for those parameters to be signed
here. I'll make them unsigned, thanks!

Max

>> +    assert(offset <= new_length);
>> +    bytes = new_length - offset;
>>
>>      while (bytes) {
>>          cur_bytes = MIN(bytes, INT_MAX);
>> @@ -2314,7 +2320,7 @@ static int qcow2_create2(const char *filename,
>> int64_t total_size,
>>      if (prealloc != PREALLOC_MODE_OFF) {
>>          BDRVQcow2State *s = blk_bs(blk)->opaque;
>>          qemu_co_mutex_lock(&s->lock);
>> -        ret = preallocate(blk_bs(blk));
>> +        ret = preallocate(blk_bs(blk), 0, total_size);
>>          qemu_co_mutex_unlock(&s->lock);
>>          if (ret < 0) {
>>              error_setg_errno(errp, -ret, "Could not preallocate
>> metadata");
>>
Stefan Hajnoczi April 6, 2017, 12:35 p.m. UTC | #3
On Mon, Apr 03, 2017 at 06:09:29PM +0200, Max Reitz wrote:
> This patch adds two new parameters to the preallocate() function so we
> will be able to use it not just for preallocating a new image but also
> for preallocated image growth.
> 
> The offset parameter allows the caller to specify a virtual offset from
> which to start preallocating. For newly created images this is always 0,
> but for preallocating growth this will be the old image length.
> 
> The new_length parameter specifies the supposed new length of the image
> (basically the "end offset" for preallocation). During image truncation,
> bdrv_getlength() will return the old image length so we cannot rely on
> its return value then.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Patch
diff mbox

diff --git a/block/qcow2.c b/block/qcow2.c
index 9fd220ec34..163d51ec65 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2034,17 +2034,23 @@  static int qcow2_change_backing_file(BlockDriverState *bs,
     return qcow2_update_header(bs);
 }
 
-static int preallocate(BlockDriverState *bs)
+/**
+ * Preallocates metadata structures for data clusters between @offset (in the
+ * guest disk) and @new_length (which is thus generally the new guest disk
+ * size).
+ *
+ * Returns: 0 on success, -errno on failure.
+ */
+static int preallocate(BlockDriverState *bs, int64_t offset, int64_t new_length)
 {
     uint64_t bytes;
-    uint64_t offset;
     uint64_t host_offset = 0;
     unsigned int cur_bytes;
     int ret;
     QCowL2Meta *meta;
 
-    bytes = bdrv_getlength(bs);
-    offset = 0;
+    assert(offset <= new_length);
+    bytes = new_length - offset;
 
     while (bytes) {
         cur_bytes = MIN(bytes, INT_MAX);
@@ -2314,7 +2320,7 @@  static int qcow2_create2(const char *filename, int64_t total_size,
     if (prealloc != PREALLOC_MODE_OFF) {
         BDRVQcow2State *s = blk_bs(blk)->opaque;
         qemu_co_mutex_lock(&s->lock);
-        ret = preallocate(blk_bs(blk));
+        ret = preallocate(blk_bs(blk), 0, total_size);
         qemu_co_mutex_unlock(&s->lock);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not preallocate metadata");