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 <rvka...@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