Message ID | 1518172615-1260-1-git-send-email-sarna@skytechnology.pl |
---|---|
State | New |
Headers | show |
Series | [v3] block: unify blocksize types | expand |
Am 09.02.2018 um 11:36 hat Piotr Sarna geschrieben: > BlockSizes structure used in block size probing has uint32_t types > for logical and physical sizes. These fields are wrongfully assigned > to uint16_t in BlockConf, which results, among other errors, > in assigning 0 instead of 65536 (which will be the case in at least > future LizardFS block device driver among other things). > > This commit makes BlockConf's physical_block_size and logical_block_size > fields uint32_t to avoid inconsistencies. > > Signed-off-by: Piotr Sarna <sarna@skytechnology.pl> > --- > hw/core/qdev-properties.c | 12 ++++++------ > include/hw/block/block.h | 4 ++-- > include/hw/qdev-properties.h | 2 +- > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 5bbc2d9..c4a1b8f 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -728,17 +728,17 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name, > { > DeviceState *dev = DEVICE(obj); > Property *prop = opaque; > - uint16_t value, *ptr = qdev_get_prop_ptr(dev, prop); > + uint32_t value, *ptr = qdev_get_prop_ptr(dev, prop); > Error *local_err = NULL; > const int64_t min = 512; > - const int64_t max = 32768; > + const int64_t max = 2147483648; Please see my reply to the v1 patch, this breaks IDE at least. Kevin
On 02/09/2018 04:36 AM, Piotr Sarna wrote: > BlockSizes structure used in block size probing has uint32_t types > for logical and physical sizes. These fields are wrongfully assigned > to uint16_t in BlockConf, which results, among other errors, > in assigning 0 instead of 65536 (which will be the case in at least > future LizardFS block device driver among other things). > > This commit makes BlockConf's physical_block_size and logical_block_size > fields uint32_t to avoid inconsistencies. > > Signed-off-by: Piotr Sarna <sarna@skytechnology.pl> > --- > - const int64_t max = 32768; > + const int64_t max = 2147483648; > > @@ -762,9 +762,9 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name, > } > > const PropertyInfo qdev_prop_blocksize = { > - .name = "uint16", > - .description = "A power of two between 512 and 32768", > - .get = get_uint16, > + .name = "uint32", > + .description = "A power of two between 512 and 2147483648", > + .get = get_uint32, I can understand a block size larger than 16 bits, but all the way up to 2G seems rather perverse (as we have to perform read-modify-write on anything that is smaller than the blocksize, and that adds 4G of overhead for a blocksize of 2G). Would it be better to cap this at 1M for now?
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 5bbc2d9..c4a1b8f 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -728,17 +728,17 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name, { DeviceState *dev = DEVICE(obj); Property *prop = opaque; - uint16_t value, *ptr = qdev_get_prop_ptr(dev, prop); + uint32_t value, *ptr = qdev_get_prop_ptr(dev, prop); Error *local_err = NULL; const int64_t min = 512; - const int64_t max = 32768; + const int64_t max = 2147483648; if (dev->realized) { qdev_prop_set_after_realize(dev, name, errp); return; } - visit_type_uint16(v, name, &value, &local_err); + visit_type_uint32(v, name, &value, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -762,9 +762,9 @@ static void set_blocksize(Object *obj, Visitor *v, const char *name, } const PropertyInfo qdev_prop_blocksize = { - .name = "uint16", - .description = "A power of two between 512 and 32768", - .get = get_uint16, + .name = "uint32", + .description = "A power of two between 512 and 2147483648", + .get = get_uint32, .set = set_blocksize, .set_default_value = set_default_value_uint, }; diff --git a/include/hw/block/block.h b/include/hw/block/block.h index 64b9298..c9e6e27 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -17,8 +17,8 @@ typedef struct BlockConf { BlockBackend *blk; - uint16_t physical_block_size; - uint16_t logical_block_size; + uint32_t physical_block_size; + uint32_t logical_block_size; uint16_t min_io_size; uint32_t opt_io_size; int32_t bootindex; diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 1d61a35..c68d7bf 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -210,7 +210,7 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar; #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int) #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \ - DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t) + DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t) #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress) #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f) \
BlockSizes structure used in block size probing has uint32_t types for logical and physical sizes. These fields are wrongfully assigned to uint16_t in BlockConf, which results, among other errors, in assigning 0 instead of 65536 (which will be the case in at least future LizardFS block device driver among other things). This commit makes BlockConf's physical_block_size and logical_block_size fields uint32_t to avoid inconsistencies. Signed-off-by: Piotr Sarna <sarna@skytechnology.pl> --- hw/core/qdev-properties.c | 12 ++++++------ include/hw/block/block.h | 4 ++-- include/hw/qdev-properties.h | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)