| Message ID | 1374762197-7261-12-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <eblake@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@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? For example, qcow2 could in theory set the flag, it has all of the information already in memory. 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. Should the caller pass a flag that tells whether he is interested in the offset or not? Kevin
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 <eblake@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@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. > Should the caller pass a flag that tells whether he is interested in the > offset or not? That would complicate the API (and the implementation too if you want to honor it in the formats). Since is_allocated works the same way and it wasn't a problem so far, I decided not to have such a flag. Paolo
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 <eblake@redhat.com> > >> Signed-off-by: Paolo Bonzini <pbonzini@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
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 + typedef enum { BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP } BlockErrorAction;