Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
> > > > > > > ---
> > > > > > >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
>
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
> > > > > > ---
> > > > > >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
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
> > > > > ---
> > > > >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
> >
>
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
> > > > ---
> > > >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
>
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
---
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
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
> > ---
> > 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.
Re: [PATCH] block: make BlockConf.*_size properties 32-bit
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
---
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
