Am 30.07.2013 um 16:19 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 16:14, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Define the return value of get_block_status. Bits 0, 1, 2 and 9-62
> >> are valid; bit 63 (the sign bit) is reserved for errors. Bits 3-8
> >> are left for future extensions.
> >>
> >> The return code is compatible with the old is_allocated API: if a driver
> >> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
> >> clients of is_allocated will not have any change in behavior. Still,
> >> we will return more precise information in the next patches and the
> >> new definition of bdrv_is_allocated is already prepared for this.
> >>
> >> Reviewed-by: Eric Blake <ebl...@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> >> ---
> >> block.c | 10 ++++++++--
> >> include/block/block.h | 26 ++++++++++++++++++++++++++
> >> 2 files changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index f533c36..7cfbf71 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn
> >> bdrv_co_get_block_status(BlockDriverState *bs,
> >>
> >> if (!bs->drv->bdrv_co_get_block_status) {
> >> *pnum = nb_sectors;
> >> - return 1;
> >> + return BDRV_BLOCK_DATA;
> >> }
> >>
> >> return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors,
> >> pnum);
> >> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
> >> int64_t sector_num,
> >> int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t
> >> sector_num,
> >> int nb_sectors, int *pnum)
> >> {
> >> - return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> + int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> + if (ret < 0) {
> >> + return ret;
> >> + }
> >> + return
> >> + (ret & BDRV_BLOCK_DATA) ||
> >> + ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
> >> }
> >>
> >> /*
> >> diff --git a/include/block/block.h b/include/block/block.h
> >> index e41854e..d044b31 100644
> >> --- a/include/block/block.h
> >> +++ b/include/block/block.h
> >> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
> >> #define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS)
> >> #define BDRV_SECTOR_MASK ~(BDRV_SECTOR_SIZE - 1)
> >>
> >> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
> >> + * BDRV_BLOCK_ZERO: sectors read as zero
> >> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> >> + *
> >> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> >> + * bs->file where sector data can be read from as raw data.
> >> + *
> >> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if
> >> present.
> >> + *
> >> + * DATA ZERO OFFSET_VALID
> >> + * t t t sectors read as zero, bs->file is zero at offset
> >> + * t f t sectors read as valid from bs->file at offset
> >> + * f t t sectors preallocated, read as zero, bs->file not
> >> + * necessarily zero at offset
> >> + * f f t sectors preallocated but read from backing_hd,
> >> + * bs->file contains garbage at offset
> >> + * t t f sectors preallocated, read as zero, unknown
> >> offset
> >> + * t f f sectors read from unknown file or offset
> >> + * f t f not allocated or unknown offset, read as zero
> >> + * f f f not allocated or unknown offset, read from
> >> backing_hd
> >> + */
> >> +#define BDRV_BLOCK_DATA 1
> >> +#define BDRV_BLOCK_ZERO 2
> >> +#define BDRV_BLOCK_OFFSET_VALID 4
> >> +#define BDRV_BLOCK_OFFSET_MASK BDRV_SECTOR_MASK
> >
> > When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?
>
> Always if they can provide the information.
>
> > For example, qcow2 could in theory set the flag, it has all of the
> > information already in memory.
>
> In fact it does later in the series.
>
> > But with a fragmented image this might
> > mean that it returns only one cluster instead of a large area with one
> > bdrv_get_block_status() call.
>
> This is just theoretical, right? qcow2_get_cluster_offset only works on
> areas that are contiguous in the raw image.
Right, somehow I was thinking that .bdrv_is_allocated() used something
more clever, but it doesn't. So this isn't worse than what we have today.
Kevin
