* [PATCH] block: make BlockConf.*_size properties 32-bit
@ 2020-02-11 11:54 Roman Kagan
2020-02-12 21:44 ` Eric Blake
0 siblings, 1 reply; 9+ messages in thread
From: Roman Kagan @ 2020-02-11 11:54 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, qemu-block
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(-)
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) \
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-02-11 11:54 [PATCH] block: make BlockConf.*_size properties 32-bit Roman Kagan
@ 2020-02-12 21:44 ` Eric Blake
2020-02-13 8:01 ` Roman Kagan
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2020-02-12 21:44 UTC (permalink / raw)
To: Roman Kagan, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, qemu-block
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-02-12 21:44 ` Eric Blake
@ 2020-02-13 8:01 ` Roman Kagan
2020-02-13 12:47 ` Eric Blake
0 siblings, 1 reply; 9+ messages in thread
From: Roman Kagan @ 2020-02-13 8:01 UTC (permalink / raw)
To: Eric Blake
Cc: Paolo Bonzini, Daniel P. Berrangé,
qemu-devel, qemu-block, Eduardo Habkost
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-02-13 8:01 ` Roman Kagan
@ 2020-02-13 12:47 ` Eric Blake
2020-02-13 13:55 ` Roman Kagan
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2020-02-13 12:47 UTC (permalink / raw)
To: Roman Kagan, qemu-devel, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, qemu-block
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.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-02-13 12:47 ` Eric Blake
@ 2020-02-13 13:55 ` Roman Kagan
2020-03-02 10:55 ` Roman Kagan
2020-03-24 14:27 ` Eric Blake
0 siblings, 2 replies; 9+ messages in thread
From: Roman Kagan @ 2020-02-13 13:55 UTC (permalink / raw)
To: Eric Blake
Cc: Paolo Bonzini, Daniel P. Berrangé,
qemu-devel, qemu-block, Eduardo Habkost
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-02-13 13:55 ` Roman Kagan
@ 2020-03-02 10:55 ` Roman Kagan
2020-03-24 8:55 ` Roman Kagan
2020-03-24 14:27 ` Eric Blake
1 sibling, 1 reply; 9+ messages in thread
From: Roman Kagan @ 2020-03-02 10:55 UTC (permalink / raw)
To: Eric Blake
Cc: Paolo Bonzini, Daniel P. Berrangé,
qemu-devel, qemu-block, Eduardo Habkost
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
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-03-02 10:55 ` Roman Kagan
@ 2020-03-24 8:55 ` Roman Kagan
2020-03-24 9:21 ` Kevin Wolf
0 siblings, 1 reply; 9+ messages in thread
From: Roman Kagan @ 2020-03-24 8:55 UTC (permalink / raw)
To: Eric Blake
Cc: Paolo Bonzini, Daniel P. Berrangé,
qemu-devel, qemu-block, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-03-24 8:55 ` Roman Kagan
@ 2020-03-24 9:21 ` Kevin Wolf
0 siblings, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2020-03-24 9:21 UTC (permalink / raw)
To: Roman Kagan, Eric Blake, Paolo Bonzini, Daniel P. Berrangé,
qemu-devel, qemu-block, Eduardo Habkost
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: make BlockConf.*_size properties 32-bit
2020-02-13 13:55 ` Roman Kagan
2020-03-02 10:55 ` Roman Kagan
@ 2020-03-24 14:27 ` Eric Blake
1 sibling, 0 replies; 9+ messages in thread
From: Eric Blake @ 2020-03-24 14:27 UTC (permalink / raw)
To: Roman Kagan, qemu-devel, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, qemu-block
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-24 14:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 11:54 [PATCH] block: make BlockConf.*_size properties 32-bit Roman Kagan
2020-02-12 21:44 ` Eric Blake
2020-02-13 8:01 ` Roman Kagan
2020-02-13 12:47 ` Eric Blake
2020-02-13 13:55 ` Roman Kagan
2020-03-02 10:55 ` Roman Kagan
2020-03-24 8:55 ` Roman Kagan
2020-03-24 9:21 ` Kevin Wolf
2020-03-24 14:27 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.