| Message ID | 20200211115401.43230-1-rvkagan@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | block: make BlockConf.*_size properties 32-bit | expand |
On 2/11/20 5:54 AM, Roman Kagan wrote: > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use > 32-bit for logical_block_size, physical_block_size, and min_io_size. > However, the properties in BlockConf are defined as uint16_t limiting > the values to 32768. > > This appears unnecessary tight, and we've seen bigger block sizes handy > at times. What larger sizes? I could see 64k or maybe even 1M block sizes,... > > Make them 32 bit instead and lift the limitation. > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > --- > hw/core/qdev-properties.c | 21 ++++++++++++--------- > include/hw/block/block.h | 8 ++++---- > include/hw/qdev-properties.h | 2 +- > 3 files changed, 17 insertions(+), 14 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 7f93bfeb88..5f84e4a3b8 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { > > /* --- blocksize --- */ > > +#define MIN_BLOCK_SIZE 512 > +#define MAX_BLOCK_SIZE 2147483648 ...but 2G block sizes are going to have tremendous performance problems. I'm not necessarily opposed to the widening to a 32-bit type, but think you need more justification or a smaller number for the max block size, particularly since qcow2 refuses to use cluster sizes larger than 2M and it makes no sense to allow a block size larger than a cluster size.
On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: > On 2/11/20 5:54 AM, Roman Kagan wrote: > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use > > 32-bit for logical_block_size, physical_block_size, and min_io_size. > > However, the properties in BlockConf are defined as uint16_t limiting > > the values to 32768. > > > > This appears unnecessary tight, and we've seen bigger block sizes handy > > at times. > > What larger sizes? I could see 64k or maybe even 1M block sizes,... We played exactly with these two :) > > > > Make them 32 bit instead and lift the limitation. > > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > > --- > > hw/core/qdev-properties.c | 21 ++++++++++++--------- > > include/hw/block/block.h | 8 ++++---- > > include/hw/qdev-properties.h | 2 +- > > 3 files changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > index 7f93bfeb88..5f84e4a3b8 100644 > > --- a/hw/core/qdev-properties.c > > +++ b/hw/core/qdev-properties.c > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { > > /* --- blocksize --- */ > > +#define MIN_BLOCK_SIZE 512 > > +#define MAX_BLOCK_SIZE 2147483648 > > ...but 2G block sizes are going to have tremendous performance problems. > > I'm not necessarily opposed to the widening to a 32-bit type, but think you > need more justification or a smaller number for the max block size, I thought any smaller value would just be arbitrary and hard to reason about, so I went ahead with the max value that fit in the type and could be made visibile to the guest. Besides this is a property that is set explicitly, so I don't see a problem leaving this up to the user. > particularly since qcow2 refuses to use cluster sizes larger than 2M and it > makes no sense to allow a block size larger than a cluster size. This still doesn't contradict passing a bigger value to the guest, for experimenting if nothing else. Thanks, Roman.
On 2/13/20 2:01 AM, Roman Kagan wrote: > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: >> On 2/11/20 5:54 AM, Roman Kagan wrote: >>> Devices (virtio-blk, scsi, etc.) and the block layer are happy to use >>> 32-bit for logical_block_size, physical_block_size, and min_io_size. >>> However, the properties in BlockConf are defined as uint16_t limiting >>> the values to 32768. >>> >>> This appears unnecessary tight, and we've seen bigger block sizes handy >>> at times. >> >> What larger sizes? I could see 64k or maybe even 1M block sizes,... > > We played exactly with these two :) > >>> >>> Make them 32 bit instead and lift the limitation. >>> >>> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> >>> --- >>> hw/core/qdev-properties.c | 21 ++++++++++++--------- >>> include/hw/block/block.h | 8 ++++---- >>> include/hw/qdev-properties.h | 2 +- >>> 3 files changed, 17 insertions(+), 14 deletions(-) >>> >>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >>> index 7f93bfeb88..5f84e4a3b8 100644 >>> --- a/hw/core/qdev-properties.c >>> +++ b/hw/core/qdev-properties.c >>> @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { >>> /* --- blocksize --- */ >>> +#define MIN_BLOCK_SIZE 512 >>> +#define MAX_BLOCK_SIZE 2147483648 >> >> ...but 2G block sizes are going to have tremendous performance problems. >> >> I'm not necessarily opposed to the widening to a 32-bit type, but think you >> need more justification or a smaller number for the max block size, > > I thought any smaller value would just be arbitrary and hard to reason > about, so I went ahead with the max value that fit in the type and could > be made visibile to the guest. You've got bigger problems than what is visible to the guest. block/qcow2.c operates on a cluster at a time; if you are stating that it now requires reading multiple clusters to operate on one, qcow2 will have to do lots of wasteful read-modify-write cycles. You really need a strong reason to support a maximum larger than 2M other than just "so the guest can experiment with it". > > Besides this is a property that is set explicitly, so I don't see a > problem leaving this up to the user. > >> particularly since qcow2 refuses to use cluster sizes larger than 2M and it >> makes no sense to allow a block size larger than a cluster size. > > This still doesn't contradict passing a bigger value to the guest, for > experimenting if nothing else. > > Thanks, > Roman. >
On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote: > On 2/13/20 2:01 AM, Roman Kagan wrote: > > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: > > > On 2/11/20 5:54 AM, Roman Kagan wrote: > > > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use > > > > 32-bit for logical_block_size, physical_block_size, and min_io_size. > > > > However, the properties in BlockConf are defined as uint16_t limiting > > > > the values to 32768. > > > > > > > > This appears unnecessary tight, and we've seen bigger block sizes handy > > > > at times. > > > > > > What larger sizes? I could see 64k or maybe even 1M block sizes,... > > > > We played exactly with these two :) > > > > > > > > > > Make them 32 bit instead and lift the limitation. > > > > > > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > > > > --- > > > > hw/core/qdev-properties.c | 21 ++++++++++++--------- > > > > include/hw/block/block.h | 8 ++++---- > > > > include/hw/qdev-properties.h | 2 +- > > > > 3 files changed, 17 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > > > index 7f93bfeb88..5f84e4a3b8 100644 > > > > --- a/hw/core/qdev-properties.c > > > > +++ b/hw/core/qdev-properties.c > > > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { > > > > /* --- blocksize --- */ > > > > +#define MIN_BLOCK_SIZE 512 > > > > +#define MAX_BLOCK_SIZE 2147483648 > > > > > > ...but 2G block sizes are going to have tremendous performance problems. > > > > > > I'm not necessarily opposed to the widening to a 32-bit type, but think you > > > need more justification or a smaller number for the max block size, > > > > I thought any smaller value would just be arbitrary and hard to reason > > about, so I went ahead with the max value that fit in the type and could > > be made visibile to the guest. > > You've got bigger problems than what is visible to the guest. block/qcow2.c > operates on a cluster at a time; if you are stating that it now requires > reading multiple clusters to operate on one, qcow2 will have to do lots of > wasteful read-modify-write cycles. I'm failing to see how this is supposed to happen. The guest will issue requests bigger than the cluster size; why would it cause RMW? Big logical_block_size would cause RMW in the guest if it wants to perform smaller writes, but that's up to the user to take this tradeoff, isn't it? > You really need a strong reason to > support a maximum larger than 2M other than just "so the guest can > experiment with it". Do I get you right that your suggestion is to cap the block size property at 2MB? Thanks, Roman. > > > > Besides this is a property that is set explicitly, so I don't see a > > problem leaving this up to the user. > > > > > particularly since qcow2 refuses to use cluster sizes larger than 2M and it > > > makes no sense to allow a block size larger than a cluster size. > > > > This still doesn't contradict passing a bigger value to the guest, for > > experimenting if nothing else. > > > > Thanks, > > Roman. > > > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org >
On Thu, Feb 13, 2020 at 04:55:44PM +0300, Roman Kagan wrote: > On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote: > > On 2/13/20 2:01 AM, Roman Kagan wrote: > > > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: > > > > On 2/11/20 5:54 AM, Roman Kagan wrote: > > > > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use > > > > > 32-bit for logical_block_size, physical_block_size, and min_io_size. > > > > > However, the properties in BlockConf are defined as uint16_t limiting > > > > > the values to 32768. > > > > > > > > > > This appears unnecessary tight, and we've seen bigger block sizes handy > > > > > at times. > > > > > > > > What larger sizes? I could see 64k or maybe even 1M block sizes,... > > > > > > We played exactly with these two :) > > > > > > > > > > > > > Make them 32 bit instead and lift the limitation. > > > > > > > > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > > > > > --- > > > > > hw/core/qdev-properties.c | 21 ++++++++++++--------- > > > > > include/hw/block/block.h | 8 ++++---- > > > > > include/hw/qdev-properties.h | 2 +- > > > > > 3 files changed, 17 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > > > > index 7f93bfeb88..5f84e4a3b8 100644 > > > > > --- a/hw/core/qdev-properties.c > > > > > +++ b/hw/core/qdev-properties.c > > > > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { > > > > > /* --- blocksize --- */ > > > > > +#define MIN_BLOCK_SIZE 512 > > > > > +#define MAX_BLOCK_SIZE 2147483648 > > > > > > > > ...but 2G block sizes are going to have tremendous performance problems. > > > > > > > > I'm not necessarily opposed to the widening to a 32-bit type, but think you > > > > need more justification or a smaller number for the max block size, > > > > > > I thought any smaller value would just be arbitrary and hard to reason > > > about, so I went ahead with the max value that fit in the type and could > > > be made visibile to the guest. > > > > You've got bigger problems than what is visible to the guest. block/qcow2.c > > operates on a cluster at a time; if you are stating that it now requires > > reading multiple clusters to operate on one, qcow2 will have to do lots of > > wasteful read-modify-write cycles. > > I'm failing to see how this is supposed to happen. The guest will issue > requests bigger than the cluster size; why would it cause RMW? > > Big logical_block_size would cause RMW in the guest if it wants to > perform smaller writes, but that's up to the user to take this tradeoff, > isn't it? > > > You really need a strong reason to > > support a maximum larger than 2M other than just "so the guest can > > experiment with it". > > Do I get you right that your suggestion is to cap the block size > property at 2MB? > > Thanks, > Roman. Ping? > > > > > > Besides this is a property that is set explicitly, so I don't see a > > > problem leaving this up to the user. > > > > > > > particularly since qcow2 refuses to use cluster sizes larger than 2M and it > > > > makes no sense to allow a block size larger than a cluster size. > > > > > > This still doesn't contradict passing a bigger value to the guest, for > > > experimenting if nothing else. > > > > > > Thanks, > > > Roman. > > > > > > > -- > > Eric Blake, Principal Software Engineer > > Red Hat, Inc. +1-919-301-3226 > > Virtualization: qemu.org | libvirt.org > > >
On Mon, Mar 02, 2020 at 01:55:02PM +0300, Roman Kagan wrote: > On Thu, Feb 13, 2020 at 04:55:44PM +0300, Roman Kagan wrote: > > On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote: > > > On 2/13/20 2:01 AM, Roman Kagan wrote: > > > > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: > > > > > On 2/11/20 5:54 AM, Roman Kagan wrote: > > > > > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use > > > > > > 32-bit for logical_block_size, physical_block_size, and min_io_size. > > > > > > However, the properties in BlockConf are defined as uint16_t limiting > > > > > > the values to 32768. > > > > > > > > > > > > This appears unnecessary tight, and we've seen bigger block sizes handy > > > > > > at times. > > > > > > > > > > What larger sizes? I could see 64k or maybe even 1M block sizes,... > > > > > > > > We played exactly with these two :) > > > > > > > > > > > > > > > > Make them 32 bit instead and lift the limitation. > > > > > > > > > > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > > > > > > --- > > > > > > hw/core/qdev-properties.c | 21 ++++++++++++--------- > > > > > > include/hw/block/block.h | 8 ++++---- > > > > > > include/hw/qdev-properties.h | 2 +- > > > > > > 3 files changed, 17 insertions(+), 14 deletions(-) > > > > > > > > > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > > > > > index 7f93bfeb88..5f84e4a3b8 100644 > > > > > > --- a/hw/core/qdev-properties.c > > > > > > +++ b/hw/core/qdev-properties.c > > > > > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { > > > > > > /* --- blocksize --- */ > > > > > > +#define MIN_BLOCK_SIZE 512 > > > > > > +#define MAX_BLOCK_SIZE 2147483648 > > > > > > > > > > ...but 2G block sizes are going to have tremendous performance problems. > > > > > > > > > > I'm not necessarily opposed to the widening to a 32-bit type, but think you > > > > > need more justification or a smaller number for the max block size, > > > > > > > > I thought any smaller value would just be arbitrary and hard to reason > > > > about, so I went ahead with the max value that fit in the type and could > > > > be made visibile to the guest. > > > > > > You've got bigger problems than what is visible to the guest. block/qcow2.c > > > operates on a cluster at a time; if you are stating that it now requires > > > reading multiple clusters to operate on one, qcow2 will have to do lots of > > > wasteful read-modify-write cycles. > > > > I'm failing to see how this is supposed to happen. The guest will issue > > requests bigger than the cluster size; why would it cause RMW? > > > > Big logical_block_size would cause RMW in the guest if it wants to > > perform smaller writes, but that's up to the user to take this tradeoff, > > isn't it? > > > > > You really need a strong reason to > > > support a maximum larger than 2M other than just "so the guest can > > > experiment with it". > > > > Do I get you right that your suggestion is to cap the block size > > property at 2MB? > > > > Thanks, > > Roman. > > Ping? Ping? > > > > > > > > Besides this is a property that is set explicitly, so I don't see a > > > > problem leaving this up to the user. > > > > > > > > > particularly since qcow2 refuses to use cluster sizes larger than 2M and it > > > > > makes no sense to allow a block size larger than a cluster size. > > > > > > > > This still doesn't contradict passing a bigger value to the guest, for > > > > experimenting if nothing else. > > > > > > > > Thanks, > > > > Roman. > > > > > > > > > > -- > > > Eric Blake, Principal Software Engineer > > > Red Hat, Inc. +1-919-301-3226 > > > Virtualization: qemu.org | libvirt.org
Am 24.03.2020 um 09:55 hat Roman Kagan geschrieben: > On Mon, Mar 02, 2020 at 01:55:02PM +0300, Roman Kagan wrote: > > On Thu, Feb 13, 2020 at 04:55:44PM +0300, Roman Kagan wrote: > > > On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote: > > > > On 2/13/20 2:01 AM, Roman Kagan wrote: > > > > > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: > > > > > > On 2/11/20 5:54 AM, Roman Kagan wrote: > > > > > > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to use > > > > > > > 32-bit for logical_block_size, physical_block_size, and min_io_size. > > > > > > > However, the properties in BlockConf are defined as uint16_t limiting > > > > > > > the values to 32768. > > > > > > > > > > > > > > This appears unnecessary tight, and we've seen bigger block sizes handy > > > > > > > at times. > > > > > > > > > > > > What larger sizes? I could see 64k or maybe even 1M block sizes,... > > > > > > > > > > We played exactly with these two :) > > > > > > > > > > > > > > > > > > > Make them 32 bit instead and lift the limitation. > > > > > > > > > > > > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > > > > > > > --- > > > > > > > hw/core/qdev-properties.c | 21 ++++++++++++--------- > > > > > > > include/hw/block/block.h | 8 ++++---- > > > > > > > include/hw/qdev-properties.h | 2 +- > > > > > > > 3 files changed, 17 insertions(+), 14 deletions(-) > > > > > > > > > > > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > > > > > > index 7f93bfeb88..5f84e4a3b8 100644 > > > > > > > --- a/hw/core/qdev-properties.c > > > > > > > +++ b/hw/core/qdev-properties.c > > > > > > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { > > > > > > > /* --- blocksize --- */ > > > > > > > +#define MIN_BLOCK_SIZE 512 > > > > > > > +#define MAX_BLOCK_SIZE 2147483648 > > > > > > > > > > > > ...but 2G block sizes are going to have tremendous performance problems. > > > > > > > > > > > > I'm not necessarily opposed to the widening to a 32-bit type, but think you > > > > > > need more justification or a smaller number for the max block size, > > > > > > > > > > I thought any smaller value would just be arbitrary and hard to reason > > > > > about, so I went ahead with the max value that fit in the type and could > > > > > be made visibile to the guest. > > > > > > > > You've got bigger problems than what is visible to the guest. block/qcow2.c > > > > operates on a cluster at a time; if you are stating that it now requires > > > > reading multiple clusters to operate on one, qcow2 will have to do lots of > > > > wasteful read-modify-write cycles. > > > > > > I'm failing to see how this is supposed to happen. The guest will issue > > > requests bigger than the cluster size; why would it cause RMW? > > > > > > Big logical_block_size would cause RMW in the guest if it wants to > > > perform smaller writes, but that's up to the user to take this tradeoff, > > > isn't it? > > > > > > > You really need a strong reason to > > > > support a maximum larger than 2M other than just "so the guest can > > > > experiment with it". > > > > > > Do I get you right that your suggestion is to cap the block size > > > property at 2MB? > > > > > > Thanks, > > > Roman. > > > > Ping? > > Ping? Eric, I think this was a question for you. But anyway, capping at 2 MB sounds reasonable enough to me. Kevin > > > > > > > > > > Besides this is a property that is set explicitly, so I don't see a > > > > > problem leaving this up to the user. > > > > > > > > > > > particularly since qcow2 refuses to use cluster sizes larger than 2M and it > > > > > > makes no sense to allow a block size larger than a cluster size. > > > > > > > > > > This still doesn't contradict passing a bigger value to the guest, for > > > > > experimenting if nothing else. > > > > > > > > > > Thanks, > > > > > Roman. > > > > > > > > > > > > > -- > > > > Eric Blake, Principal Software Engineer > > > > Red Hat, Inc. +1-919-301-3226 > > > > Virtualization: qemu.org | libvirt.org >
On 2/13/20 7:55 AM, Roman Kagan wrote: > On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote: >> On 2/13/20 2:01 AM, Roman Kagan wrote: >>> On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote: >>>> On 2/11/20 5:54 AM, Roman Kagan wrote: >>>>> Devices (virtio-blk, scsi, etc.) and the block layer are happy to use >>>>> 32-bit for logical_block_size, physical_block_size, and min_io_size. >>>>> However, the properties in BlockConf are defined as uint16_t limiting >>>>> the values to 32768. >>>>> >>>>> This appears unnecessary tight, and we've seen bigger block sizes handy >>>>> at times. >>>> >>>> What larger sizes? I could see 64k or maybe even 1M block sizes,... >>> >>> We played exactly with these two :) >>> >>>>> >>>>> Make them 32 bit instead and lift the limitation. >>>>> @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { >>>>> /* --- blocksize --- */ >>>>> +#define MIN_BLOCK_SIZE 512 >>>>> +#define MAX_BLOCK_SIZE 2147483648 >>>> >>>> ...but 2G block sizes are going to have tremendous performance problems. >>>> >>>> I'm not necessarily opposed to the widening to a 32-bit type, but think you >>>> need more justification or a smaller number for the max block size, >>> >>> I thought any smaller value would just be arbitrary and hard to reason >>> about, so I went ahead with the max value that fit in the type and could >>> be made visibile to the guest. >> >> You've got bigger problems than what is visible to the guest. block/qcow2.c >> operates on a cluster at a time; if you are stating that it now requires >> reading multiple clusters to operate on one, qcow2 will have to do lots of >> wasteful read-modify-write cycles. > > I'm failing to see how this is supposed to happen. The guest will issue > requests bigger than the cluster size; why would it cause RMW? > > Big logical_block_size would cause RMW in the guest if it wants to > perform smaller writes, but that's up to the user to take this tradeoff, > isn't it? > >> You really need a strong reason to >> support a maximum larger than 2M other than just "so the guest can >> experiment with it". > > Do I get you right that your suggestion is to cap the block size > property at 2MB? Yes, for now, I think 2M is a better maximum than 2G or 4G unless you have benchmark data to prove that a larger maximum does not cause problems.
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 7f93bfeb88..5f84e4a3b8 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = { /* --- blocksize --- */ +#define MIN_BLOCK_SIZE 512 +#define MAX_BLOCK_SIZE 2147483648 + static void set_blocksize(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { 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; 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; } /* value of 0 means "unset" */ - if (value && (value < min || value > max)) { + if (value && (value < MIN_BLOCK_SIZE || value > MAX_BLOCK_SIZE)) { error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, - dev->id ? : "", name, (int64_t)value, min, max); + dev->id ? : "", name, (int64_t)value, + (int64_t)MIN_BLOCK_SIZE, (int64_t)MAX_BLOCK_SIZE); return; } @@ -755,9 +757,10 @@ 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 " stringify(MIN_BLOCK_SIZE) + " and " stringify(MAX_BLOCK_SIZE), + .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 d7246f3862..9dd6bba56a 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -18,9 +18,9 @@ typedef struct BlockConf { BlockBackend *blk; - uint16_t physical_block_size; - uint16_t logical_block_size; - uint16_t min_io_size; + uint32_t physical_block_size; + uint32_t logical_block_size; + uint32_t min_io_size; uint32_t opt_io_size; int32_t bootindex; uint32_t discard_granularity; @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) _conf.logical_block_size), \ DEFINE_PROP_BLOCKSIZE("physical_block_size", _state, \ _conf.physical_block_size), \ - DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ + DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0), \ DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \ DEFINE_PROP_UINT32("discard_granularity", _state, \ _conf.discard_granularity, -1), \ diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 906e697c58..ebdeeb4866 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -193,7 +193,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width; #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_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
Devices (virtio-blk, scsi, etc.) and the block layer are happy to use 32-bit for logical_block_size, physical_block_size, and min_io_size. However, the properties in BlockConf are defined as uint16_t limiting the values to 32768. This appears unnecessary tight, and we've seen bigger block sizes handy at times. Make them 32 bit instead and lift the limitation. Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> --- hw/core/qdev-properties.c | 21 ++++++++++++--------- include/hw/block/block.h | 8 ++++---- include/hw/qdev-properties.h | 2 +- 3 files changed, 17 insertions(+), 14 deletions(-)