diff mbox

[v3,12/19] block: return get_block_status data and flags for formats

Message ID 1374762197-7261-13-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini July 25, 2013, 2:23 p.m. UTC
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c      |  8 +++++++-
 block/qcow.c     |  9 ++++++++-
 block/qcow2.c    | 16 ++++++++++++++--
 block/qed.c      | 35 ++++++++++++++++++++++++++++-------
 block/sheepdog.c |  2 +-
 block/vdi.c      | 13 ++++++++++++-
 block/vmdk.c     | 19 ++++++++++++++++++-
 block/vvfat.c    | 11 ++++++-----
 8 files changed, 94 insertions(+), 19 deletions(-)

Comments

Kevin Wolf July 30, 2013, 2:40 p.m. UTC | #1
Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/cow.c      |  8 +++++++-
>  block/qcow.c     |  9 ++++++++-
>  block/qcow2.c    | 16 ++++++++++++++--
>  block/qed.c      | 35 ++++++++++++++++++++++++++++-------
>  block/sheepdog.c |  2 +-
>  block/vdi.c      | 13 ++++++++++++-
>  block/vmdk.c     | 19 ++++++++++++++++++-
>  block/vvfat.c    | 11 ++++++-----
>  8 files changed, 94 insertions(+), 19 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index e738b96..1e90413 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>  static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, int *num_same)
>  {
> -    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> +    BDRVCowState *s = bs->opaque;
> +    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> +    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
>  }
>  
>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
> diff --git a/block/qcow.c b/block/qcow.c
> index acd1aeb..1a65822 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>      if (n > nb_sectors)
>          n = nb_sectors;
>      *pnum = n;
> -    return (cluster_offset != 0);
> +    if (!cluster_offset) {
> +        return 0;

If you take your comment in patch 11 serious, you should return
bs->backing_hd ? 0 : BDRV_BLOCK_ZERO instead. (I think it would be
useful behaviour, too, because knowing that a sector is zero enables
optimisations in several places.)

Of course, this is something that could be done in the block.c
implementation of bdrv_co_get_block_status() instead of each single
driver.

Kevin
Paolo Bonzini July 30, 2013, 3:15 p.m. UTC | #2
Il 30/07/2013 16:40, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/cow.c      |  8 +++++++-
>>  block/qcow.c     |  9 ++++++++-
>>  block/qcow2.c    | 16 ++++++++++++++--
>>  block/qed.c      | 35 ++++++++++++++++++++++++++++-------
>>  block/sheepdog.c |  2 +-
>>  block/vdi.c      | 13 ++++++++++++-
>>  block/vmdk.c     | 19 ++++++++++++++++++-
>>  block/vvfat.c    | 11 ++++++-----
>>  8 files changed, 94 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/cow.c b/block/cow.c
>> index e738b96..1e90413 100644
>> --- a/block/cow.c
>> +++ b/block/cow.c
>> @@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>>  static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>>          int64_t sector_num, int nb_sectors, int *num_same)
>>  {
>> -    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
>> +    BDRVCowState *s = bs->opaque;
>> +    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
>> +    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
>>  }
>>  
>>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>> diff --git a/block/qcow.c b/block/qcow.c
>> index acd1aeb..1a65822 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>>      if (n > nb_sectors)
>>          n = nb_sectors;
>>      *pnum = n;
>> -    return (cluster_offset != 0);
>> +    if (!cluster_offset) {
>> +        return 0;
> 
> If you take your comment in patch 11 serious, you should return
> bs->backing_hd ? 0 : BDRV_BLOCK_ZERO instead. (I think it would be
> useful behaviour, too, because knowing that a sector is zero enables
> optimisations in several places.)
> 
> Of course, this is something that could be done in the block.c
> implementation of bdrv_co_get_block_status() instead of each single
> driver.

And it is, in patch 15 ("block: use bdrv_has_zero_init to return
BDRV_BLOCK_ZERO"). :)  Should I reorder the patches?

Paolo
Kevin Wolf July 30, 2013, 3:23 p.m. UTC | #3
Am 30.07.2013 um 17:15 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 16:40, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block/cow.c      |  8 +++++++-
> >>  block/qcow.c     |  9 ++++++++-
> >>  block/qcow2.c    | 16 ++++++++++++++--
> >>  block/qed.c      | 35 ++++++++++++++++++++++++++++-------
> >>  block/sheepdog.c |  2 +-
> >>  block/vdi.c      | 13 ++++++++++++-
> >>  block/vmdk.c     | 19 ++++++++++++++++++-
> >>  block/vvfat.c    | 11 ++++++-----
> >>  8 files changed, 94 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/block/cow.c b/block/cow.c
> >> index e738b96..1e90413 100644
> >> --- a/block/cow.c
> >> +++ b/block/cow.c
> >> @@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
> >>  static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
> >>          int64_t sector_num, int nb_sectors, int *num_same)
> >>  {
> >> -    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> >> +    BDRVCowState *s = bs->opaque;
> >> +    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> >> +    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
> >>  }
> >>  
> >>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
> >> diff --git a/block/qcow.c b/block/qcow.c
> >> index acd1aeb..1a65822 100644
> >> --- a/block/qcow.c
> >> +++ b/block/qcow.c
> >> @@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
> >>      if (n > nb_sectors)
> >>          n = nb_sectors;
> >>      *pnum = n;
> >> -    return (cluster_offset != 0);
> >> +    if (!cluster_offset) {
> >> +        return 0;
> > 
> > If you take your comment in patch 11 serious, you should return
> > bs->backing_hd ? 0 : BDRV_BLOCK_ZERO instead. (I think it would be
> > useful behaviour, too, because knowing that a sector is zero enables
> > optimisations in several places.)
> > 
> > Of course, this is something that could be done in the block.c
> > implementation of bdrv_co_get_block_status() instead of each single
> > driver.
> 
> And it is, in patch 15 ("block: use bdrv_has_zero_init to return
> BDRV_BLOCK_ZERO"). :)  Should I reorder the patches?

No, I don't mind. I did look at the final state, but I missed it because
I expected something with bs->backing_hd instead of bdrv_has_zero_init()
and didn't remember that the former is already included in the latter.

Kevin
diff mbox

Patch

diff --git a/block/cow.c b/block/cow.c
index e738b96..1e90413 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -194,7 +194,13 @@  static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
 static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *num_same)
 {
-    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
+    BDRVCowState *s = bs->opaque;
+    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
+    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
+    if (ret < 0) {
+        return ret;
+    }
+    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
 }
 
 static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/qcow.c b/block/qcow.c
index acd1aeb..1a65822 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -410,7 +410,14 @@  static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
     if (n > nb_sectors)
         n = nb_sectors;
     *pnum = n;
-    return (cluster_offset != 0);
+    if (!cluster_offset) {
+        return 0;
+    }
+    if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypt_method) {
+        return BDRV_BLOCK_DATA;
+    }
+    cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+    return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset;
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
diff --git a/block/qcow2.c b/block/qcow2.c
index d35a134..cac7ee8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -645,7 +645,8 @@  static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t cluster_offset;
-    int ret;
+    int index_in_cluster, ret;
+    int64_t status = 0;
 
     *pnum = nb_sectors;
     qemu_co_mutex_lock(&s->lock);
@@ -655,7 +656,18 @@  static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
         return ret;
     }
 
-    return (cluster_offset != 0) || (ret == QCOW2_CLUSTER_ZERO);
+    if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED &&
+        !s->crypt_method) {
+        index_in_cluster = sector_num & (s->cluster_sectors - 1);
+        cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+        status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
+    }
+    if (ret == QCOW2_CLUSTER_ZERO) {
+        status |= BDRV_BLOCK_ZERO;
+    } else if (ret != QCOW2_CLUSTER_UNALLOCATED) {
+        status |= BDRV_BLOCK_DATA;
+    }
+    return status;
 }
 
 /* handle reading after the end of the backing file */
diff --git a/block/qed.c b/block/qed.c
index b0978ba..ea41036 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -652,16 +652,36 @@  static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
 }
 
 typedef struct {
+    BlockDriverState *bs;
     Coroutine *co;
-    int is_allocated;
+    uint64_t pos;
+    int64_t status;
     int *pnum;
 } QEDIsAllocatedCB;
 
 static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
 {
     QEDIsAllocatedCB *cb = opaque;
+    BDRVQEDState *s = cb->bs->opaque;
     *cb->pnum = len / BDRV_SECTOR_SIZE;
-    cb->is_allocated = (ret == QED_CLUSTER_FOUND || ret == QED_CLUSTER_ZERO);
+    switch (ret) {
+    case QED_CLUSTER_FOUND:
+        offset |= qed_offset_into_cluster(s, cb->pos);
+        cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+        break;
+    case QED_CLUSTER_ZERO:
+        cb->status = BDRV_BLOCK_ZERO;
+        break;
+    case QED_CLUSTER_L2:
+    case QED_CLUSTER_L1:
+        cb->status = 0;
+        break;
+    default:
+        assert(ret < 0);
+        cb->status = ret;
+        break;
+    }
+
     if (cb->co) {
         qemu_coroutine_enter(cb->co, NULL);
     }
@@ -672,25 +692,26 @@  static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
                                                  int nb_sectors, int *pnum)
 {
     BDRVQEDState *s = bs->opaque;
-    uint64_t pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE;
     size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
     QEDIsAllocatedCB cb = {
-        .is_allocated = -1,
+        .bs = bs,
+        .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
+        .status = BDRV_BLOCK_OFFSET_MASK,
         .pnum = pnum,
     };
     QEDRequest request = { .l2_table = NULL };
 
-    qed_find_cluster(s, &request, pos, len, qed_is_allocated_cb, &cb);
+    qed_find_cluster(s, &request, cb.pos, len, qed_is_allocated_cb, &cb);
 
     /* Now sleep if the callback wasn't invoked immediately */
-    while (cb.is_allocated == -1) {
+    while (cb.status == BDRV_BLOCK_OFFSET_MASK) {
         cb.co = qemu_coroutine_self();
         qemu_coroutine_yield();
     }
 
     qed_unref_l2_cache_entry(request.l2_table);
 
-    return cb.is_allocated;
+    return cb.status;
 }
 
 static int bdrv_qed_make_empty(BlockDriverState *bs)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 31cc573..b0ff8f1 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2297,7 +2297,7 @@  sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                   end = DIV_ROUND_UP((sector_num + nb_sectors) *
                                      BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
     unsigned long idx;
-    int ret = 1;
+    int64_t ret = BDRV_BLOCK_DATA;
 
     for (idx = start; idx < end; idx++) {
         if (inode->data_vdi_id[idx] == 0) {
diff --git a/block/vdi.c b/block/vdi.c
index 7ab2567..1bf7dc5 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -479,12 +479,23 @@  static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs,
     size_t sector_in_block = sector_num % s->block_sectors;
     int n_sectors = s->block_sectors - sector_in_block;
     uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]);
+    uint64_t offset;
+    int result;
+
     logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, pnum);
     if (n_sectors > nb_sectors) {
         n_sectors = nb_sectors;
     }
     *pnum = n_sectors;
-    return VDI_IS_ALLOCATED(bmap_entry);
+    result = VDI_IS_ALLOCATED(bmap_entry);
+    if (!result) {
+        return 0;
+    }
+
+    offset = s->header.offset_data +
+                              (uint64_t)bmap_entry * s->block_size +
+                              sector_in_block * SECTOR_SIZE;
+    return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
 }
 
 static int vdi_co_read(BlockDriverState *bs,
diff --git a/block/vmdk.c b/block/vmdk.c
index 026840c..cd8db57 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1015,7 +1015,24 @@  static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
                             sector_num * 512, 0, &offset);
     qemu_co_mutex_unlock(&s->lock);
 
-    ret = (ret == VMDK_OK || ret == VMDK_ZEROED);
+    switch (ret) {
+    case VMDK_ERROR:
+        ret = -EIO;
+        break;
+    case VMDK_UNALLOC:
+        ret = 0;
+        break;
+    case VMDK_ZEROED:
+        ret = BDRV_BLOCK_ZERO;
+        break;
+    case VMDK_OK:
+        ret = BDRV_BLOCK_DATA;
+        if (extent->file == bs->file) {
+            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
+        }
+
+        break;
+    }
 
     index_in_cluster = sector_num % extent->cluster_sectors;
     n = extent->cluster_sectors - index_in_cluster;
diff --git a/block/vvfat.c b/block/vvfat.c
index 4c30620..dd0efca 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2879,11 +2879,12 @@  static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
 {
     BDRVVVFATState* s = bs->opaque;
     *n = s->sector_count - sector_num;
-    if (*n > nb_sectors)
-	*n = nb_sectors;
-    else if (*n < 0)
-	return 0;
-    return 1;
+    if (*n > nb_sectors) {
+        *n = nb_sectors;
+    } else if (*n < 0) {
+        return 0;
+    }
+    return BDRV_BLOCK_DATA;
 }
 
 static int write_target_commit(BlockDriverState *bs, int64_t sector_num,