[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] block: make BlockConf.*_size properties 32-bit

From: Eric Blake
Subject: Re: [PATCH] block: make BlockConf.*_size properties 32-bit
Date: Thu, 13 Feb 2020 06:47:10 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

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 <address@hidden>
   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.


Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]