| Message ID | 1399628898-3241-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 09.05.2014 um 11:48 hat Markus Armbruster geschrieben: > A call to retrieve the image size converts between bytes and sectors > several times: > > * BlockDriver method bdrv_getlength() returns bytes. > > * refresh_total_sectors() converts to sectors, rounding up, and stores > in total_sectors. > > * bdrv_getlength() converts total_sectors back to bytes (now rounded > up to a multiple of the sector size). > > * Callers wanting sectors rather bytes convert it right back. > Example: bdrv_get_geometry(). > > bdrv_nb_sectors() provides a way to omit the last two conversions. > It's exactly bdrv_getlength() with the conversion to bytes omitted. > It's functionally like bdrv_get_geometry() without its odd error > handling. > > Reimplement bdrv_getlength() and bdrv_get_geometry() on top of > bdrv_nb_sectors(). > > The next patches will convert some users of bdrv_getlength() to > bdrv_nb_sectors(). > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Is this really the right direction to move? Generally I think we should be trying to move away from those arbitrary units of 512 bytes (that are called sectors for some reason, but aren't really related to either guest or host sector sizes), and towards interfaces that use byte granularity. So I would rather see bs->total_sectors becoming bs->total_bytes than adding a new sector-based function. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 09.05.2014 um 11:48 hat Markus Armbruster geschrieben: >> A call to retrieve the image size converts between bytes and sectors >> several times: >> >> * BlockDriver method bdrv_getlength() returns bytes. >> >> * refresh_total_sectors() converts to sectors, rounding up, and stores >> in total_sectors. >> >> * bdrv_getlength() converts total_sectors back to bytes (now rounded >> up to a multiple of the sector size). >> >> * Callers wanting sectors rather bytes convert it right back. >> Example: bdrv_get_geometry(). >> >> bdrv_nb_sectors() provides a way to omit the last two conversions. >> It's exactly bdrv_getlength() with the conversion to bytes omitted. >> It's functionally like bdrv_get_geometry() without its odd error >> handling. >> >> Reimplement bdrv_getlength() and bdrv_get_geometry() on top of >> bdrv_nb_sectors(). >> >> The next patches will convert some users of bdrv_getlength() to >> bdrv_nb_sectors(). >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Is this really the right direction to move? > > Generally I think we should be trying to move away from those arbitrary > units of 512 bytes (that are called sectors for some reason, but aren't > really related to either guest or host sector sizes), and towards > interfaces that use byte granularity. My patches neither move towards nor away from a byte-based interface. > So I would rather see bs->total_sectors becoming bs->total_bytes than > adding a new sector-based function. Doing away with BDRV_SECTOR_SIZE & friends is beyond my reach. My initial hope was to replace bdrv_get_geometry() by bdrv_nb_sectors(), but I ran out of steam reviewing whether and how the callers should handle errors. So yes, this adds another sector-based function. But I believe it makes the code a bit simpler, and won't make an eventual conversion to byte-based any harder.
On 05/12/2014 05:11 AM, Kevin Wolf wrote: >> The next patches will convert some users of bdrv_getlength() to >> bdrv_nb_sectors(). >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Is this really the right direction to move? > > Generally I think we should be trying to move away from those arbitrary > units of 512 bytes (that are called sectors for some reason, but aren't > really related to either guest or host sector sizes), and towards > interfaces that use byte granularity. > > So I would rather see bs->total_sectors becoming bs->total_bytes than > adding a new sector-based function. I tend to agree that storing rounded information is the wrong thing to do. The qcow2 format stores the guest size in bytes, so it is technically possible to have a file where the last sector is only a partial sector. However, because the current implementation rounds to sectors (and in some cases, rounds in the wrong direction), we have situations like: $ qemu-img create -f qcow2 img 1234 Formatting 'img', fmt=qcow2 size=1234 encryption=off cluster_size=65536 lazy_refcounts=off $ qemu-img info img image: img file format: qcow2 virtual size: 1.0K (1024 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false where I lost 210 bytes of what the guest was supposed to be able to access. Consistently tracking bdrv length in bytes, and rounding up to sectors when needed, seems like it will be better than pre-rounding to sectors and possibly truncating the user image.
diff --git a/block.c b/block.c index b749d31..44e1f57 100644 --- a/block.c +++ b/block.c @@ -689,6 +689,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename, /** * Set the current 'total_sectors' value + * Return 0 on success, -errno on error. */ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint) { @@ -3470,11 +3471,12 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs) } /** - * Length of a file in bytes. Return < 0 if error or unknown. + * Return number of sectors on success, -errno on error. */ -int64_t bdrv_getlength(BlockDriverState *bs) +int64_t bdrv_nb_sectors(BlockDriverState *bs) { BlockDriver *drv = bs->drv; + if (!drv) return -ENOMEDIUM; @@ -3484,19 +3486,26 @@ int64_t bdrv_getlength(BlockDriverState *bs) return ret; } } - return bs->total_sectors * BDRV_SECTOR_SIZE; + return bs->total_sectors; +} + +/** + * Return length in bytes on success, -errno on error. + * The length is always a multiple of BDRV_SECTOR_SIZE. + */ +int64_t bdrv_getlength(BlockDriverState *bs) +{ + int64_t ret = bdrv_nb_sectors(bs); + + return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE; } /* return 0 as number of sectors if no device present or error */ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr) { - int64_t length; - length = bdrv_getlength(bs); - if (length < 0) - length = 0; - else - length = length >> BDRV_SECTOR_BITS; - *nb_sectors_ptr = length; + int64_t nb_sectors = bdrv_nb_sectors(bs); + + *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors; } void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, diff --git a/include/block/block.h b/include/block/block.h index 467fb2b..0438b58 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -252,6 +252,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, const char *backing_file); int bdrv_get_backing_file_depth(BlockDriverState *bs); int bdrv_truncate(BlockDriverState *bs, int64_t offset); +int64_t bdrv_nb_sectors(BlockDriverState *bs); int64_t bdrv_getlength(BlockDriverState *bs); int64_t bdrv_get_allocated_file_size(BlockDriverState *bs); void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
A call to retrieve the image size converts between bytes and sectors several times: * BlockDriver method bdrv_getlength() returns bytes. * refresh_total_sectors() converts to sectors, rounding up, and stores in total_sectors. * bdrv_getlength() converts total_sectors back to bytes (now rounded up to a multiple of the sector size). * Callers wanting sectors rather bytes convert it right back. Example: bdrv_get_geometry(). bdrv_nb_sectors() provides a way to omit the last two conversions. It's exactly bdrv_getlength() with the conversion to bytes omitted. It's functionally like bdrv_get_geometry() without its odd error handling. Reimplement bdrv_getlength() and bdrv_get_geometry() on top of bdrv_nb_sectors(). The next patches will convert some users of bdrv_getlength() to bdrv_nb_sectors(). Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 29 +++++++++++++++++++---------- include/block/block.h | 1 + 2 files changed, 20 insertions(+), 10 deletions(-)