Re: [Qemu-devel] [PATCH v3 12/19] block: return get_block_status data and flags for formats

[Kevin Wolf]

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 <address@hidden>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> >> ---
> >>  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