* [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector
2014-08-29 8:33 [Qemu-devel] [PATCH v13 0/6] qcow2, raw: add preallocation=full Hu Tao
@ 2014-08-29 8:33 ` Hu Tao
2014-08-29 12:50 ` Eric Blake
` (2 more replies)
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size Hu Tao
` (4 subsequent siblings)
5 siblings, 3 replies; 39+ messages in thread
From: Hu Tao @ 2014-08-29 8:33 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
block/archipelago.c | 3 ++-
block/cow.c | 3 ++-
block/gluster.c | 4 +--
block/iscsi.c | 4 +--
block/nfs.c | 3 ++-
block/qcow.c | 3 ++-
block/qcow2.c | 3 ++-
block/qed.c | 3 ++-
block/raw-posix.c | 8 +++---
block/raw-win32.c | 4 +--
block/rbd.c | 3 ++-
block/sheepdog.c | 3 ++-
block/ssh.c | 3 ++-
block/vdi.c | 3 ++-
block/vhdx.c | 3 ++-
block/vmdk.c | 3 ++-
block/vpc.c | 3 ++-
tests/qemu-iotests/104 | 57 ++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/104.out | 12 +++++++++
tests/qemu-iotests/common.filter | 21 +++++++++++++++
tests/qemu-iotests/group | 1 +
21 files changed, 127 insertions(+), 23 deletions(-)
create mode 100755 tests/qemu-iotests/104
create mode 100644 tests/qemu-iotests/104.out
diff --git a/block/archipelago.c b/block/archipelago.c
index 34f72dc..a8d06aa 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -707,7 +707,8 @@ static int qemu_archipelago_create(const char *filename,
parse_filename_opts(filename, errp, &volname, &segment_name, &mport,
&vport);
- total_size = qemu_opt_get_size_del(options, BLOCK_OPT_SIZE, 0);
+ total_size = ROUND_UP(qemu_opt_get_size_del(options, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
if (segment_name == NULL) {
segment_name = g_strdup("archipelago");
diff --git a/block/cow.c b/block/cow.c
index 6ee4833..c3769fe 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -335,7 +335,8 @@ static int cow_create(const char *filename, QemuOpts *opts, Error **errp)
BlockDriverState *cow_bs = NULL;
/* Read out options */
- image_sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
+ image_sectors = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
image_filename = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
ret = bdrv_create_file(filename, opts, &local_err);
diff --git a/block/gluster.c b/block/gluster.c
index 1912cf9..65c7a58 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -494,8 +494,8 @@ static int qemu_gluster_create(const char *filename,
goto out;
}
- total_size =
- qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
+ total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
tmp = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
if (!tmp || !strcmp(tmp, "off")) {
diff --git a/block/iscsi.c b/block/iscsi.c
index 3e19202..84bcae8 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1531,8 +1531,8 @@ static int iscsi_create(const char *filename, QemuOpts *opts, Error **errp)
bs = bdrv_new("", &error_abort);
/* Read out options */
- total_size =
- qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
+ total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
bs->opaque = g_new0(struct IscsiLun, 1);
iscsilun = bs->opaque;
diff --git a/block/nfs.c b/block/nfs.c
index 93d87f3..d3ba09e 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -414,7 +414,8 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
client->aio_context = qemu_get_aio_context();
/* Read out options */
- total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
ret = nfs_client_open(client, url, O_CREAT, errp);
if (ret < 0) {
diff --git a/block/qcow.c b/block/qcow.c
index 67c237f..041af26 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -725,7 +725,8 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
BlockDriverState *qcow_bs;
/* Read out options */
- total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
+ total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
flags |= BLOCK_FLAG_ENCRYPT;
diff --git a/block/qcow2.c b/block/qcow2.c
index f9e045f..c8050e5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1921,7 +1921,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
int ret;
/* Read out options */
- sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
+ sectors = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
diff --git a/block/qed.c b/block/qed.c
index ba395af..f8d9e12 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -648,7 +648,8 @@ static int bdrv_qed_create(const char *filename, QemuOpts *opts, Error **errp)
char *backing_fmt = NULL;
int ret;
- image_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
cluster_size = qemu_opt_get_size_del(opts,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index d737f3a..9c22e3f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
strstart(filename, "file:", &filename);
/* Read out options */
- total_size =
- qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
+ total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
@@ -1966,8 +1966,8 @@ static int hdev_create(const char *filename, QemuOpts *opts,
(void)has_prefix;
/* Read out options */
- total_size =
- qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
+ total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
fd = qemu_open(filename, O_WRONLY | O_BINARY);
if (fd < 0) {
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 902eab6..1e1880d 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -511,8 +511,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
strstart(filename, "file:", &filename);
/* Read out options */
- total_size =
- qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
+ total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
0644);
diff --git a/block/rbd.c b/block/rbd.c
index ea969e7..b7f7d5f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -314,7 +314,8 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
}
/* Read out options */
- bytes = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
objsize = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0);
if (objsize) {
if ((objsize - 1) & objsize) { /* not a power of 2? */
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 12cbd9d..8293d47 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1695,7 +1695,8 @@ static int sd_create(const char *filename, QemuOpts *opts,
goto out;
}
- s->inode.vdi_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ s->inode.vdi_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
if (!buf || !strcmp(buf, "off")) {
diff --git a/block/ssh.c b/block/ssh.c
index cd2fd75..cf43bc0 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -700,7 +700,8 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp)
ssh_state_init(&s);
/* Get desired file size. */
- total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
DPRINTF("total_size=%" PRIi64, total_size);
uri_options = qdict_new();
diff --git a/block/vdi.c b/block/vdi.c
index 4b10aac..cfa08b0 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -700,7 +700,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
logout("\n");
/* Read out options. */
- bytes = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
#if defined(CONFIG_VDI_BLOCK_SIZE)
/* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
block_size = qemu_opt_get_size_del(opts,
diff --git a/block/vhdx.c b/block/vhdx.c
index 87c99fc..796b7bd 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1766,7 +1766,8 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
VHDXImageType image_type;
Error *local_err = NULL;
- image_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
log_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_LOG_SIZE, 0);
block_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_BLOCK_SIZE, 0);
type = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
diff --git a/block/vmdk.c b/block/vmdk.c
index 07cb62c..2e126c5 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1805,7 +1805,8 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
goto exit;
}
/* Read out options */
- total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
diff --git a/block/vpc.c b/block/vpc.c
index 055efc4..5d8fd8e 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -757,7 +757,8 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
BlockDriverState *bs = NULL;
/* Read out options */
- total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
disk_type_param = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
if (disk_type_param) {
if (!strcmp(disk_type_param, "dynamic")) {
diff --git a/tests/qemu-iotests/104 b/tests/qemu-iotests/104
new file mode 100755
index 0000000..0c1d4fb
--- /dev/null
+++ b/tests/qemu-iotests/104
@@ -0,0 +1,57 @@
+#!/bin/bash
+#
+# Test qcow2 creation with aligned and unaligned sizes
+#
+# Copyright (C) 2014 Fujitsu.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=hutao@cn.fujitsu.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto generic
+_supported_os Linux
+
+echo "=== Check qemu-img info output ==="
+echo
+image_sizes="1024 1234"
+
+for s in $image_sizes; do
+ _make_test_img $s | _filter_img_create
+ _img_info | _filter_img_info
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/104.out b/tests/qemu-iotests/104.out
new file mode 100644
index 0000000..de27852
--- /dev/null
+++ b/tests/qemu-iotests/104.out
@@ -0,0 +1,12 @@
+QA output created by 104
+=== Check qemu-img info output ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 1.0K (1024 bytes)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1234
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 1.5K (1536 bytes)
+***done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 51192c8..f69cb6b 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -192,5 +192,26 @@ _filter_img_create()
-e "s/archipelago:a/TEST_DIR\//g"
}
+_filter_img_info()
+{
+ sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
+ -e "s#$TEST_DIR#TEST_DIR#g" \
+ -e "s#$IMGFMT#IMGFMT#g" \
+ -e "/encrypted: yes/d" \
+ -e "/cluster_size: [0-9]\\+/d" \
+ -e "/table_size: [0-9]\\+/d" \
+ -e "/compat: '[^']*'/d" \
+ -e "/compat6: \\(on\\|off\\)/d" \
+ -e "/static: \\(on\\|off\\)/d" \
+ -e "/zeroed_grain: \\(on\\|off\\)/d" \
+ -e "/subformat: '[^']*'/d" \
+ -e "/adapter_type: '[^']*'/d" \
+ -e "/lazy_refcounts: \\(on\\|off\\)/d" \
+ -e "/block_size: [0-9]\\+/d" \
+ -e "/block_state_zero: \\(on\\|off\\)/d" \
+ -e "/log_size: [0-9]\\+/d" \
+ -e "s/archipelago:a/TEST_DIR\//g"
+}
+
# make sure this script returns success
/bin/true
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 2803d68..e6c23b4 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -103,3 +103,4 @@
099 rw auto quick
101 rw auto quick
103 rw auto quick
+104 rw auto
--
1.9.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector Hu Tao
@ 2014-08-29 12:50 ` Eric Blake
2014-09-04 9:33 ` Kevin Wolf
2014-09-02 21:21 ` Max Reitz
2014-09-04 9:43 ` Kevin Wolf
2 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2014-08-29 12:50 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
[-- Attachment #1: Type: text/plain, Size: 6040 bytes --]
On 08/29/2014 02:33 AM, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/archipelago.c | 3 ++-
> block/cow.c | 3 ++-
> block/gluster.c | 4 +--
> block/iscsi.c | 4 +--
> block/nfs.c | 3 ++-
> block/qcow.c | 3 ++-
> block/qcow2.c | 3 ++-
> block/qed.c | 3 ++-
> block/raw-posix.c | 8 +++---
> block/raw-win32.c | 4 +--
> block/rbd.c | 3 ++-
> block/sheepdog.c | 3 ++-
> block/ssh.c | 3 ++-
> block/vdi.c | 3 ++-
> block/vhdx.c | 3 ++-
> block/vmdk.c | 3 ++-
> block/vpc.c | 3 ++-
> tests/qemu-iotests/104 | 57 ++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/104.out | 12 +++++++++
> tests/qemu-iotests/common.filter | 21 +++++++++++++++
> tests/qemu-iotests/group | 1 +
> 21 files changed, 127 insertions(+), 23 deletions(-)
> create mode 100755 tests/qemu-iotests/104
> create mode 100644 tests/qemu-iotests/104.out
Code looks correct. However, I have a possible design concern:
> +++ b/block/qcow2.c
> @@ -1921,7 +1921,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> int ret;
>
> /* Read out options */
> - sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> + sectors = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> + BDRV_SECTOR_SIZE);
At least the qcow2 file format describes virtual file size in bytes
(offsets 24-31 in the header). It's nice that we are guaranteeing at
least the size requested by the user, but by doing the division, we are
unable to write the user's actual requested size into the file.
I'm okay if all files _we_ create are aligned in size. But do we have
any lurking bugs if a qcow2 file created by some other means has a
non-aligned size? The recent work on the image fuzzer should be able to
easily create such images. Meanwhile, I don't know of any physical
hardware that has an unaligned size. Should we tighten the qcow2 spec
to forbid a size that is not a multiple of the sector size, and teach
qemu to forcefully reject such images as invalid? Does the qcow2 format
need a way to flag whether an image has 512 vs. 4096-byte sectors (that
is, what happens with an image that is 512-aligned but not 4096-aligned
when used in an emulation that only exposes 4096-byte alignment to the
guest)?
On the other hand, if we _do_ have code that gracefully handles
unaligned size from externally-created sources, would it be worth
allowing qemu-img to also create unaligned images directly, instead of
having to resort to the image fuzzer to create such images, all so that
it is easier to test our code and ensure it doesn't bit-rot? It might
be lower-maintenance to just require that such images be rejected as broken.
> +++ b/block/raw-posix.c
> @@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> strstart(filename, "file:", &filename);
>
> /* Read out options */
> - total_size =
> - qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
> + total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> + BDRV_SECTOR_SIZE);
> nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
Again, I'm okay with the idea of always treating guest images as aligned
multiples of sectors. But it's very easy to create an unaligned raw
file, and pass that to qemu. So even though we don't create such files
via qemu-img, it's still worth ensuring that the code behaves correctly
when such an image is used directly or as a backing file (as a backing
file of a larger qcow2 image, bytes beyond the actual size must read as
padded to 0; as a direct file, I'm not sure that the guest OS can do a
partial sector read, so I'd lean more towards an I/O error when trying
to read a full sector when only a partial sector is available the same
as when trying to read a sector completely beyond the disk bounds, but I
could also live with a successful read with the tail being padded to 0;
similarly, we'd have to ensure that writes either error out, or
guarantee that the tail is ignored and only the valid portion of the
sector is written).
Meanwhile, although we own the qcow2 spec, and can therefore choose to
reject unaligned size in qcow2 files as invalid format, we are not in
charge of the spec for several other file formats, but probably ought to
handle those the same way as other vendors. For example, is it possible
to create a VMDK file with unaligned size, and if so, how does it behave?
>
> +_filter_img_info()
> +{
> + sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
This works, so I'm not asking for a change; but I prefer | over # when
writing a sed expression that includes a filename expansion, as in:
sed -e "s|$IMGPROTO:$TEST_DIR|TEST_DIR|g"
Why? Because it's easy (even if uncommon) to create file names with #
embedded in them (if $TEST_DIR is absolute, I just have to have
/path/to/my#odd/directory with no shell quoting required as the place
where I unpack and configure qemu), but file names with | can only be
created with shell quoting, so they are less common. (The complaint
gets more relevant for people doing s,,, with file names, because such
files are more common [anyone remember CVS?])
At any rate, if there are still design issues to resolve (such as
tightening the qcow2 spec), they can be separate patches. So I'm okay with:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector
2014-08-29 12:50 ` Eric Blake
@ 2014-09-04 9:33 ` Kevin Wolf
0 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 9:33 UTC (permalink / raw)
To: Eric Blake
Cc: Fam Zheng, Richard W.M. Jones, Hu Tao, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
Am 29.08.2014 um 14:50 hat Eric Blake geschrieben:
> On 08/29/2014 02:33 AM, Hu Tao wrote:
> > +++ b/block/raw-posix.c
> > @@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> > strstart(filename, "file:", &filename);
> >
> > /* Read out options */
> > - total_size =
> > - qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
> > + total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > + BDRV_SECTOR_SIZE);
> > nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
>
> Again, I'm okay with the idea of always treating guest images as aligned
> multiples of sectors. But it's very easy to create an unaligned raw
> file, and pass that to qemu. So even though we don't create such files
> via qemu-img, it's still worth ensuring that the code behaves correctly
> when such an image is used directly or as a backing file
The whole qemu block layer doesn't work well with unaligned images (and
the required alignment is always 512 because someone thought that using
512-byte units was a good interface). I want to fix that by converting
everything to byte granularity, but it's not the top priority.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector Hu Tao
2014-08-29 12:50 ` Eric Blake
@ 2014-09-02 21:21 ` Max Reitz
2014-09-04 9:43 ` Kevin Wolf
2 siblings, 0 replies; 39+ messages in thread
From: Max Reitz @ 2014-09-02 21:21 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Stefan Hajnoczi,
Yasunori Goto
On 29.08.2014 10:33, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/archipelago.c | 3 ++-
> block/cow.c | 3 ++-
> block/gluster.c | 4 +--
> block/iscsi.c | 4 +--
> block/nfs.c | 3 ++-
> block/qcow.c | 3 ++-
> block/qcow2.c | 3 ++-
> block/qed.c | 3 ++-
> block/raw-posix.c | 8 +++---
> block/raw-win32.c | 4 +--
> block/rbd.c | 3 ++-
> block/sheepdog.c | 3 ++-
> block/ssh.c | 3 ++-
> block/vdi.c | 3 ++-
> block/vhdx.c | 3 ++-
> block/vmdk.c | 3 ++-
> block/vpc.c | 3 ++-
> tests/qemu-iotests/104 | 57 ++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/104.out | 12 +++++++++
> tests/qemu-iotests/common.filter | 21 +++++++++++++++
> tests/qemu-iotests/group | 1 +
> 21 files changed, 127 insertions(+), 23 deletions(-)
> create mode 100755 tests/qemu-iotests/104
> create mode 100644 tests/qemu-iotests/104.out
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector Hu Tao
2014-08-29 12:50 ` Eric Blake
2014-09-02 21:21 ` Max Reitz
@ 2014-09-04 9:43 ` Kevin Wolf
2 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 9:43 UTC (permalink / raw)
To: Hu Tao
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> diff --git a/tests/qemu-iotests/104 b/tests/qemu-iotests/104
> new file mode 100755
> index 0000000..0c1d4fb
> --- /dev/null
> +++ b/tests/qemu-iotests/104
> @@ -0,0 +1,57 @@
> +#!/bin/bash
> +#
> +# Test qcow2 creation with aligned and unaligned sizes
This test case is not only for qcow2.
With this fixed, you can add:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size
2014-08-29 8:33 [Qemu-devel] [PATCH v13 0/6] qcow2, raw: add preallocation=full Hu Tao
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector Hu Tao
@ 2014-08-29 8:33 ` Hu Tao
2014-09-02 21:24 ` Max Reitz
2014-09-04 9:57 ` Kevin Wolf
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public Hu Tao
` (3 subsequent siblings)
5 siblings, 2 replies; 39+ messages in thread
From: Hu Tao @ 2014-08-29 8:33 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
and avoid converting it back later.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
block/gluster.c | 9 ++++-----
block/qcow.c | 8 ++++----
block/qcow2.c | 10 +++++-----
block/raw-posix.c | 6 +++---
block/raw-win32.c | 6 +++---
5 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 65c7a58..1eb3a8c 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -494,8 +494,8 @@ static int qemu_gluster_create(const char *filename,
goto out;
}
- total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
tmp = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
if (!tmp || !strcmp(tmp, "off")) {
@@ -516,9 +516,8 @@ static int qemu_gluster_create(const char *filename,
if (!fd) {
ret = -errno;
} else {
- if (!glfs_ftruncate(fd, total_size * BDRV_SECTOR_SIZE)) {
- if (prealloc && qemu_gluster_zerofill(fd, 0,
- total_size * BDRV_SECTOR_SIZE)) {
+ if (!glfs_ftruncate(fd, total_size)) {
+ if (prealloc && qemu_gluster_zerofill(fd, 0, total_size)) {
ret = -errno;
}
} else {
diff --git a/block/qcow.c b/block/qcow.c
index 041af26..a87bd69 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -725,8 +725,8 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
BlockDriverState *qcow_bs;
/* Read out options */
- total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
flags |= BLOCK_FLAG_ENCRYPT;
@@ -754,7 +754,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
memset(&header, 0, sizeof(header));
header.magic = cpu_to_be32(QCOW_MAGIC);
header.version = cpu_to_be32(QCOW_VERSION);
- header.size = cpu_to_be64(total_size * 512);
+ header.size = cpu_to_be64(total_size);
header_size = sizeof(header);
backing_filename_len = 0;
if (backing_file) {
@@ -776,7 +776,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
}
header_size = (header_size + 7) & ~7;
shift = header.cluster_bits + header.l2_bits;
- l1_size = ((total_size * 512) + (1LL << shift) - 1) >> shift;
+ l1_size = (total_size + (1LL << shift) - 1) >> shift;
header.l1_table_offset = cpu_to_be64(header_size);
if (flags & BLOCK_FLAG_ENCRYPT) {
diff --git a/block/qcow2.c b/block/qcow2.c
index c8050e5..cf27c3f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1859,7 +1859,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
}
/* Okay, now that we have a valid image, let's give it the right size */
- ret = bdrv_truncate(bs, total_size * BDRV_SECTOR_SIZE);
+ ret = bdrv_truncate(bs, total_size);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not resize image");
goto out;
@@ -1912,7 +1912,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
char *backing_file = NULL;
char *backing_fmt = NULL;
char *buf = NULL;
- uint64_t sectors = 0;
+ uint64_t size = 0;
int flags = 0;
size_t cluster_size = DEFAULT_CLUSTER_SIZE;
int prealloc = 0;
@@ -1921,8 +1921,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
int ret;
/* Read out options */
- sectors = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
+ size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
@@ -1972,7 +1972,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
goto finish;
}
- ret = qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
+ ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
cluster_size, prealloc, opts, version, &local_err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 9c22e3f..abe0759 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
strstart(filename, "file:", &filename);
/* Read out options */
- total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
@@ -1394,7 +1394,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
#endif
}
- if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
+ if (ftruncate(fd, total_size) != 0) {
result = -errno;
error_setg_errno(errp, -result, "Could not resize file");
}
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 1e1880d..9bf8225 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -511,8 +511,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
strstart(filename, "file:", &filename);
/* Read out options */
- total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
+ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+ BDRV_SECTOR_SIZE);
fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
0644);
@@ -521,7 +521,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
return -EIO;
}
set_sparse(fd);
- ftruncate(fd, total_size * 512);
+ ftruncate(fd, total_size);
qemu_close(fd);
return 0;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size Hu Tao
@ 2014-09-02 21:24 ` Max Reitz
2014-09-04 9:57 ` Kevin Wolf
1 sibling, 0 replies; 39+ messages in thread
From: Max Reitz @ 2014-09-02 21:24 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Stefan Hajnoczi,
Yasunori Goto
On 29.08.2014 10:33, Hu Tao wrote:
> and avoid converting it back later.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/gluster.c | 9 ++++-----
> block/qcow.c | 8 ++++----
> block/qcow2.c | 10 +++++-----
> block/raw-posix.c | 6 +++---
> block/raw-win32.c | 6 +++---
> 5 files changed, 19 insertions(+), 20 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size Hu Tao
2014-09-02 21:24 ` Max Reitz
@ 2014-09-04 9:57 ` Kevin Wolf
2014-09-05 9:07 ` Hu Tao
1 sibling, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 9:57 UTC (permalink / raw)
To: Hu Tao
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> and avoid converting it back later.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 9c22e3f..abe0759 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> strstart(filename, "file:", &filename);
>
> /* Read out options */
> - total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> - BDRV_SECTOR_SIZE);
> + total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> + BDRV_SECTOR_SIZE);
> nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
>
> fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> @@ -1394,7 +1394,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> #endif
> }
>
> - if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
> + if (ftruncate(fd, total_size) != 0) {
> result = -errno;
> error_setg_errno(errp, -result, "Could not resize file");
> }
You forgot changing hdev_create() in raw-posix. Doesn't make the patch
less correct, but you may want to add it for v14.
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size
2014-09-04 9:57 ` Kevin Wolf
@ 2014-09-05 9:07 ` Hu Tao
0 siblings, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-05 9:07 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
On Thu, Sep 04, 2014 at 11:57:58AM +0200, Kevin Wolf wrote:
> Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> > and avoid converting it back later.
> >
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 9c22e3f..abe0759 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> > strstart(filename, "file:", &filename);
> >
> > /* Read out options */
> > - total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > - BDRV_SECTOR_SIZE);
> > + total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > + BDRV_SECTOR_SIZE);
> > nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
> >
> > fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> > @@ -1394,7 +1394,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> > #endif
> > }
> >
> > - if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
> > + if (ftruncate(fd, total_size) != 0) {
> > result = -errno;
> > error_setg_errno(errp, -result, "Could not resize file");
> > }
>
> You forgot changing hdev_create() in raw-posix. Doesn't make the patch
> less correct, but you may want to add it for v14.
Thanks, changed it too.
Regards,
Hu
>
> Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public
2014-08-29 8:33 [Qemu-devel] [PATCH v13 0/6] qcow2, raw: add preallocation=full Hu Tao
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 1/6] block: round up file size to nearest sector Hu Tao
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 2/6] block: don't convert file size to sector size Hu Tao
@ 2014-08-29 8:33 ` Hu Tao
2014-09-02 21:27 ` Max Reitz
2014-09-04 10:03 ` Kevin Wolf
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full Hu Tao
` (2 subsequent siblings)
5 siblings, 2 replies; 39+ messages in thread
From: Hu Tao @ 2014-08-29 8:33 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Peter Lieven, Richard W.M. Jones,
Max Reitz, Stefan Hajnoczi, Yasunori Goto
From: Peter Lieven <pl@kamp.de>
relaxing the license to LGPLv2+ is intentional.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>
---
blockdev.c | 30 ++++++------------------------
include/qapi/util.h | 17 +++++++++++++++++
qapi/Makefile.objs | 1 +
qapi/qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
4 files changed, 58 insertions(+), 24 deletions(-)
create mode 100644 include/qapi/util.h
create mode 100644 qapi/qapi-util.c
diff --git a/blockdev.c b/blockdev.c
index 6a204c6..b2aa7a2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -39,6 +39,7 @@
#include "qapi/qmp/types.h"
#include "qapi-visit.h"
#include "qapi/qmp-output-visitor.h"
+#include "qapi/util.h"
#include "sysemu/sysemu.h"
#include "block/block_int.h"
#include "qmp-commands.h"
@@ -274,25 +275,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
}
}
-static inline int parse_enum_option(const char *lookup[], const char *buf,
- int max, int def, Error **errp)
-{
- int i;
-
- if (!buf) {
- return def;
- }
-
- for (i = 0; i < max; i++) {
- if (!strcmp(buf, lookup[i])) {
- return i;
- }
- }
-
- error_setg(errp, "invalid parameter value: %s", buf);
- return def;
-}
-
static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
{
if (throttle_conflicting(cfg)) {
@@ -456,11 +438,11 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
}
detect_zeroes =
- parse_enum_option(BlockdevDetectZeroesOptions_lookup,
- qemu_opt_get(opts, "detect-zeroes"),
- BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
- BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
- &error);
+ qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
+ qemu_opt_get(opts, "detect-zeroes"),
+ BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
+ BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+ &error);
if (error) {
error_propagate(errp, error);
goto early_err;
diff --git a/include/qapi/util.h b/include/qapi/util.h
new file mode 100644
index 0000000..de9238b
--- /dev/null
+++ b/include/qapi/util.h
@@ -0,0 +1,17 @@
+/*
+ * QAPI util functions
+ *
+ * Copyright Fujitsu, Inc. 2014
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QAPI_UTIL_H
+#define QAPI_UTIL_H
+
+int qapi_enum_parse(const char *lookup[], const char *buf,
+ int max, int def, Error **errp);
+
+#endif
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index d14b769..ffd88a6 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -4,3 +4,4 @@ util-obj-y += string-input-visitor.o string-output-visitor.o
util-obj-y += opts-visitor.o
util-obj-y += qmp-event.o
+util-obj-y += qapi-util.o
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
new file mode 100644
index 0000000..982d4e8
--- /dev/null
+++ b/qapi/qapi-util.c
@@ -0,0 +1,34 @@
+/*
+ * QAPI util functions
+ *
+ * Authors:
+ * Hu Tao <hutao@cn.fujitsu.com>
+ * Peter Lieven <pl@kamp.de>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qapi/util.h"
+
+int qapi_enum_parse(const char *lookup[], const char *buf,
+ int max, int def, Error **errp)
+{
+ int i;
+
+ if (!buf) {
+ return def;
+ }
+
+ for (i = 0; i < max; i++) {
+ if (!strcmp(buf, lookup[i])) {
+ return i;
+ }
+ }
+
+ error_setg(errp, "invalid parameter value: %s", buf);
+ return def;
+}
--
1.9.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public Hu Tao
@ 2014-09-02 21:27 ` Max Reitz
2014-09-03 1:30 ` Hu Tao
2014-09-04 10:03 ` Kevin Wolf
1 sibling, 1 reply; 39+ messages in thread
From: Max Reitz @ 2014-09-02 21:27 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Peter Lieven, Richard W.M. Jones,
Stefan Hajnoczi, Yasunori Goto
On 29.08.2014 10:33, Hu Tao wrote:
> From: Peter Lieven <pl@kamp.de>
>
> relaxing the license to LGPLv2+ is intentional.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>
> ---
> blockdev.c | 30 ++++++------------------------
> include/qapi/util.h | 17 +++++++++++++++++
> qapi/Makefile.objs | 1 +
> qapi/qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
> 4 files changed, 58 insertions(+), 24 deletions(-)
> create mode 100644 include/qapi/util.h
> create mode 100644 qapi/qapi-util.c
Seems pretty much unchanged (except for the author note), so:
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public
2014-09-02 21:27 ` Max Reitz
@ 2014-09-03 1:30 ` Hu Tao
0 siblings, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-03 1:30 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Peter Lieven, qemu-devel,
Richard W.M. Jones, Stefan Hajnoczi, Yasunori Goto
On Tue, Sep 02, 2014 at 11:27:00PM +0200, Max Reitz wrote:
> On 29.08.2014 10:33, Hu Tao wrote:
> >From: Peter Lieven <pl@kamp.de>
> >
> >relaxing the license to LGPLv2+ is intentional.
> >
> >Suggested-by: Markus Armbruster <armbru@redhat.com>
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >Signed-off-by: Peter Lieven <pl@kamp.de>
> >Reviewed-by: Eric Blake <eblake@redhat.com>
> >Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>
> >---
> > blockdev.c | 30 ++++++------------------------
> > include/qapi/util.h | 17 +++++++++++++++++
> > qapi/Makefile.objs | 1 +
> > qapi/qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 58 insertions(+), 24 deletions(-)
> > create mode 100644 include/qapi/util.h
> > create mode 100644 qapi/qapi-util.c
>
> Seems pretty much unchanged (except for the author note), so:
Yes. There was a license discussion, so I just take the latest version
by Peter.
Regards,
Hu
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public Hu Tao
2014-09-02 21:27 ` Max Reitz
@ 2014-09-04 10:03 ` Kevin Wolf
1 sibling, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 10:03 UTC (permalink / raw)
To: Hu Tao
Cc: Fam Zheng, Richard W.M. Jones, Peter Lieven, qemu-devel,
Max Reitz, Stefan Hajnoczi, Yasunori Goto
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> From: Peter Lieven <pl@kamp.de>
>
> relaxing the license to LGPLv2+ is intentional.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full.
2014-08-29 8:33 [Qemu-devel] [PATCH v13 0/6] qcow2, raw: add preallocation=full Hu Tao
` (2 preceding siblings ...)
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 3/6] rename parse_enum_option to qapi_enum_parse and make it public Hu Tao
@ 2014-08-29 8:33 ` Hu Tao
2014-09-02 21:32 ` Max Reitz
` (2 more replies)
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option Hu Tao
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 6/6] qcow2: " Hu Tao
5 siblings, 3 replies; 39+ messages in thread
From: Hu Tao @ 2014-08-29 8:33 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
This patch prepares for the subsequent patches.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
block/qcow2.c | 23 +++++++++++++++--------
qapi/block-core.json | 16 ++++++++++++++++
tests/qemu-iotests/049.out | 2 +-
3 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index cf27c3f..95fb240 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -30,6 +30,7 @@
#include "qemu/error-report.h"
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qbool.h"
+#include "qapi/util.h"
#include "trace.h"
#include "qemu/option_int.h"
@@ -1738,7 +1739,7 @@ static int preallocate(BlockDriverState *bs)
static int qcow2_create2(const char *filename, int64_t total_size,
const char *backing_file, const char *backing_format,
- int flags, size_t cluster_size, int prealloc,
+ int flags, size_t cluster_size, PreallocMode prealloc,
QemuOpts *opts, int version,
Error **errp)
{
@@ -1915,7 +1916,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
uint64_t size = 0;
int flags = 0;
size_t cluster_size = DEFAULT_CLUSTER_SIZE;
- int prealloc = 0;
+ PreallocMode prealloc = PREALLOC_MODE_OFF;
int version = 3;
Error *local_err = NULL;
int ret;
@@ -1931,12 +1932,11 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
DEFAULT_CLUSTER_SIZE);
buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
- if (!buf || !strcmp(buf, "off")) {
- prealloc = 0;
- } else if (!strcmp(buf, "metadata")) {
- prealloc = 1;
- } else {
- error_setg(errp, "Invalid preallocation mode: '%s'", buf);
+ prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
+ PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
+ &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto finish;
}
@@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
}
+ if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
+ ret = -1;
+ error_setg(errp, "Unsupported preallocate mode: %s",
+ PreallocMode_lookup[prealloc]);
+ goto finish;
+ }
+
if (backing_file && prealloc) {
error_setg(errp, "Backing file and preallocation cannot be used at "
"the same time");
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fb74c56..543b00b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1679,3 +1679,19 @@
'len' : 'int',
'offset': 'int',
'speed' : 'int' } }
+
+# @PreallocMode
+#
+# Preallocation mode of QEMU image file
+#
+# @off: no preallocation
+# @metadata: preallocate only for metadata
+# @full: preallocate all data by calling posix_fallocate() if it is
+# available, otherwise by writing zeros to device to ensure disk
+# space is really available. @full preallocation also sets up
+# metadata correctly.
+#
+# Since 2.2
+##
+{ 'enum': 'PreallocMode',
+ 'data': [ 'off', 'metadata', 'full' ] }
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index 71ca44d..09ca0ae 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -179,7 +179,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M
Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Invalid preallocation mode: '1234'
+qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234
Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='1234' lazy_refcounts=off
== Check encryption option ==
--
1.9.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full.
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full Hu Tao
@ 2014-09-02 21:32 ` Max Reitz
2014-09-03 1:31 ` Hu Tao
2014-09-02 21:51 ` Eric Blake
2014-09-04 12:17 ` Kevin Wolf
2 siblings, 1 reply; 39+ messages in thread
From: Max Reitz @ 2014-09-02 21:32 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Stefan Hajnoczi,
Yasunori Goto
On 29.08.2014 10:33, Hu Tao wrote:
> This patch prepares for the subsequent patches.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/qcow2.c | 23 +++++++++++++++--------
> qapi/block-core.json | 16 ++++++++++++++++
> tests/qemu-iotests/049.out | 2 +-
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cf27c3f..95fb240 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -30,6 +30,7 @@
> #include "qemu/error-report.h"
> #include "qapi/qmp/qerror.h"
> #include "qapi/qmp/qbool.h"
> +#include "qapi/util.h"
> #include "trace.h"
> #include "qemu/option_int.h"
>
> @@ -1738,7 +1739,7 @@ static int preallocate(BlockDriverState *bs)
>
> static int qcow2_create2(const char *filename, int64_t total_size,
> const char *backing_file, const char *backing_format,
> - int flags, size_t cluster_size, int prealloc,
> + int flags, size_t cluster_size, PreallocMode prealloc,
> QemuOpts *opts, int version,
> Error **errp)
> {
> @@ -1915,7 +1916,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> uint64_t size = 0;
> int flags = 0;
> size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> - int prealloc = 0;
> + PreallocMode prealloc = PREALLOC_MODE_OFF;
> int version = 3;
> Error *local_err = NULL;
> int ret;
> @@ -1931,12 +1932,11 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> DEFAULT_CLUSTER_SIZE);
> buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> - if (!buf || !strcmp(buf, "off")) {
> - prealloc = 0;
> - } else if (!strcmp(buf, "metadata")) {
> - prealloc = 1;
> - } else {
> - error_setg(errp, "Invalid preallocation mode: '%s'", buf);
> + prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> + PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> + &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> ret = -EINVAL;
> goto finish;
> }
> @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> }
>
> + if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
> + ret = -1;
Since the return value is expected to be -errno, I'd propose "ret =
-EINVAL;" here. With that fixed (or a good explanation why we want -1 here):
Reviewed-by: Max Reitz <mreitz@redhat.com>
> + error_setg(errp, "Unsupported preallocate mode: %s",
> + PreallocMode_lookup[prealloc]);
> + goto finish;
> + }
> +
> if (backing_file && prealloc) {
> error_setg(errp, "Backing file and preallocation cannot be used at "
> "the same time");
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index fb74c56..543b00b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1679,3 +1679,19 @@
> 'len' : 'int',
> 'offset': 'int',
> 'speed' : 'int' } }
> +
> +# @PreallocMode
> +#
> +# Preallocation mode of QEMU image file
> +#
> +# @off: no preallocation
> +# @metadata: preallocate only for metadata
> +# @full: preallocate all data by calling posix_fallocate() if it is
> +# available, otherwise by writing zeros to device to ensure disk
> +# space is really available. @full preallocation also sets up
> +# metadata correctly.
> +#
> +# Since 2.2
> +##
> +{ 'enum': 'PreallocMode',
> + 'data': [ 'off', 'metadata', 'full' ] }
> diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
> index 71ca44d..09ca0ae 100644
> --- a/tests/qemu-iotests/049.out
> +++ b/tests/qemu-iotests/049.out
> @@ -179,7 +179,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M
> Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
>
> qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M
> -qemu-img: TEST_DIR/t.qcow2: Invalid preallocation mode: '1234'
> +qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234
> Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='1234' lazy_refcounts=off
>
> == Check encryption option ==
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full.
2014-09-02 21:32 ` Max Reitz
@ 2014-09-03 1:31 ` Hu Tao
0 siblings, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-03 1:31 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Richard W.M. Jones,
Stefan Hajnoczi, Yasunori Goto
On Tue, Sep 02, 2014 at 11:32:50PM +0200, Max Reitz wrote:
> On 29.08.2014 10:33, Hu Tao wrote:
> >This patch prepares for the subsequent patches.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> > block/qcow2.c | 23 +++++++++++++++--------
> > qapi/block-core.json | 16 ++++++++++++++++
> > tests/qemu-iotests/049.out | 2 +-
> > 3 files changed, 32 insertions(+), 9 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index cf27c3f..95fb240 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -30,6 +30,7 @@
> > #include "qemu/error-report.h"
> > #include "qapi/qmp/qerror.h"
> > #include "qapi/qmp/qbool.h"
> >+#include "qapi/util.h"
> > #include "trace.h"
> > #include "qemu/option_int.h"
> >@@ -1738,7 +1739,7 @@ static int preallocate(BlockDriverState *bs)
> > static int qcow2_create2(const char *filename, int64_t total_size,
> > const char *backing_file, const char *backing_format,
> >- int flags, size_t cluster_size, int prealloc,
> >+ int flags, size_t cluster_size, PreallocMode prealloc,
> > QemuOpts *opts, int version,
> > Error **errp)
> > {
> >@@ -1915,7 +1916,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> > uint64_t size = 0;
> > int flags = 0;
> > size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> >- int prealloc = 0;
> >+ PreallocMode prealloc = PREALLOC_MODE_OFF;
> > int version = 3;
> > Error *local_err = NULL;
> > int ret;
> >@@ -1931,12 +1932,11 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> > cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> > DEFAULT_CLUSTER_SIZE);
> > buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> >- if (!buf || !strcmp(buf, "off")) {
> >- prealloc = 0;
> >- } else if (!strcmp(buf, "metadata")) {
> >- prealloc = 1;
> >- } else {
> >- error_setg(errp, "Invalid preallocation mode: '%s'", buf);
> >+ prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> >+ PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> >+ &local_err);
> >+ if (local_err) {
> >+ error_propagate(errp, local_err);
> > ret = -EINVAL;
> > goto finish;
> > }
> >@@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> > flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> > }
> >+ if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
> >+ ret = -1;
>
> Since the return value is expected to be -errno, I'd propose "ret =
> -EINVAL;" here. With that fixed (or a good explanation why we want
> -1 here):
Good.
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> >+ error_setg(errp, "Unsupported preallocate mode: %s",
> >+ PreallocMode_lookup[prealloc]);
> >+ goto finish;
> >+ }
> >+
> > if (backing_file && prealloc) {
> > error_setg(errp, "Backing file and preallocation cannot be used at "
> > "the same time");
> >diff --git a/qapi/block-core.json b/qapi/block-core.json
> >index fb74c56..543b00b 100644
> >--- a/qapi/block-core.json
> >+++ b/qapi/block-core.json
> >@@ -1679,3 +1679,19 @@
> > 'len' : 'int',
> > 'offset': 'int',
> > 'speed' : 'int' } }
> >+
> >+# @PreallocMode
> >+#
> >+# Preallocation mode of QEMU image file
> >+#
> >+# @off: no preallocation
> >+# @metadata: preallocate only for metadata
> >+# @full: preallocate all data by calling posix_fallocate() if it is
> >+# available, otherwise by writing zeros to device to ensure disk
> >+# space is really available. @full preallocation also sets up
> >+# metadata correctly.
> >+#
> >+# Since 2.2
> >+##
> >+{ 'enum': 'PreallocMode',
> >+ 'data': [ 'off', 'metadata', 'full' ] }
> >diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
> >index 71ca44d..09ca0ae 100644
> >--- a/tests/qemu-iotests/049.out
> >+++ b/tests/qemu-iotests/049.out
> >@@ -179,7 +179,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M
> > Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> > qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M
> >-qemu-img: TEST_DIR/t.qcow2: Invalid preallocation mode: '1234'
> >+qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234
> > Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='1234' lazy_refcounts=off
> > == Check encryption option ==
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full.
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full Hu Tao
2014-09-02 21:32 ` Max Reitz
@ 2014-09-02 21:51 ` Eric Blake
2014-09-03 1:35 ` Hu Tao
2014-09-04 12:17 ` Kevin Wolf
2 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2014-09-02 21:51 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
[-- Attachment #1: Type: text/plain, Size: 974 bytes --]
On 08/29/2014 02:33 AM, Hu Tao wrote:
> This patch prepares for the subsequent patches.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/qcow2.c | 23 +++++++++++++++--------
> qapi/block-core.json | 16 ++++++++++++++++
> tests/qemu-iotests/049.out | 2 +-
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> }
>
> + if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
I find it a bit awkward that you are checking for PREALLOC_MODE_OFF
implicitly ('prealloc &&') vs. checking for prealloc mode METADATA
explicitly. Since there are only three modes, would it be any simpler
to just have written:
if (prealloc == PREALLOC_MODE_FULL) {
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full.
2014-09-02 21:51 ` Eric Blake
@ 2014-09-03 1:35 ` Hu Tao
0 siblings, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-03 1:35 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
On Tue, Sep 02, 2014 at 03:51:23PM -0600, Eric Blake wrote:
> On 08/29/2014 02:33 AM, Hu Tao wrote:
> > This patch prepares for the subsequent patches.
> >
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> > block/qcow2.c | 23 +++++++++++++++--------
> > qapi/block-core.json | 16 ++++++++++++++++
> > tests/qemu-iotests/049.out | 2 +-
> > 3 files changed, 32 insertions(+), 9 deletions(-)
> >
>
> > @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> > flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> > }
> >
> > + if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
This one reads as 'support PREALLOC_MODE_METADATA' only,
>
> I find it a bit awkward that you are checking for PREALLOC_MODE_OFF
> implicitly ('prealloc &&') vs. checking for prealloc mode METADATA
> explicitly. Since there are only three modes, would it be any simpler
> to just have written:
>
> if (prealloc == PREALLOC_MODE_FULL) {
and this one reads as 'does't support PREALLOC_MODE_FULL'. Althrough
they are the same, but I'd prefer the former one. Anyway, the check is
removed in patch 6.
Regards,
Hu
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full.
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full Hu Tao
2014-09-02 21:32 ` Max Reitz
2014-09-02 21:51 ` Eric Blake
@ 2014-09-04 12:17 ` Kevin Wolf
2 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 12:17 UTC (permalink / raw)
To: Hu Tao
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> This patch prepares for the subsequent patches.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> @@ -1915,7 +1916,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> uint64_t size = 0;
> int flags = 0;
> size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> - int prealloc = 0;
> + PreallocMode prealloc = PREALLOC_MODE_OFF;
> int version = 3;
> Error *local_err = NULL;
> int ret;
The initialisation isn't really necessary any more, is it?
> @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> }
>
> + if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
> + ret = -1;
> + error_setg(errp, "Unsupported preallocate mode: %s",
> + PreallocMode_lookup[prealloc]);
> + goto finish;
> + }
> +
Like the other reviewers, I don't like this block much. But as it
disappears later in the series and achieves it's job of maintaining
bisectability:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-08-29 8:33 [Qemu-devel] [PATCH v13 0/6] qcow2, raw: add preallocation=full Hu Tao
` (3 preceding siblings ...)
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 4/6] qapi: introduce PreallocMode and a new PreallocMode full Hu Tao
@ 2014-08-29 8:33 ` Hu Tao
2014-08-29 8:48 ` Richard W.M. Jones
` (2 more replies)
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 6/6] qcow2: " Hu Tao
5 siblings, 3 replies; 39+ messages in thread
From: Hu Tao @ 2014-08-29 8:33 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
This patch adds a new option preallocation for raw format, and implements
full preallocation.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
block/raw-posix.c | 92 +++++++++++++++++++++++++++++++++++++++++++------------
qemu-doc.texi | 8 +++++
qemu-img.texi | 8 +++++
3 files changed, 88 insertions(+), 20 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index abe0759..25f66f2 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -30,6 +30,7 @@
#include "block/thread-pool.h"
#include "qemu/iov.h"
#include "raw-aio.h"
+#include "qapi/util.h"
#if defined(__APPLE__) && (__MACH__)
#include <paths.h>
@@ -1365,6 +1366,9 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
int result = 0;
int64_t total_size = 0;
bool nocow = false;
+ PreallocMode prealloc = PREALLOC_MODE_OFF;
+ char *buf = NULL;
+ Error *local_err = NULL;
strstart(filename, "file:", &filename);
@@ -1372,37 +1376,80 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
BDRV_SECTOR_SIZE);
nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
+ buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
+ prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
+ PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
+ &local_err);
+ g_free(buf);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ result = -EINVAL;
+ goto out;
+ }
fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
0644);
if (fd < 0) {
result = -errno;
error_setg_errno(errp, -result, "Could not create file");
- } else {
- if (nocow) {
+ goto out;
+ }
+
+ if (nocow) {
#ifdef __linux__
- /* Set NOCOW flag to solve performance issue on fs like btrfs.
- * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
- * will be ignored since any failure of this operation should not
- * block the left work.
- */
- int attr;
- if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
- attr |= FS_NOCOW_FL;
- ioctl(fd, FS_IOC_SETFLAGS, &attr);
- }
-#endif
+ /* Set NOCOW flag to solve performance issue on fs like btrfs.
+ * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
+ * will be ignored since any failure of this operation should not
+ * block the left work.
+ */
+ int attr;
+ if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
+ attr |= FS_NOCOW_FL;
+ ioctl(fd, FS_IOC_SETFLAGS, &attr);
}
+#endif
+ }
- if (ftruncate(fd, total_size) != 0) {
- result = -errno;
- error_setg_errno(errp, -result, "Could not resize file");
- }
- if (qemu_close(fd) != 0) {
- result = -errno;
- error_setg_errno(errp, -result, "Could not close the new file");
+ if (ftruncate(fd, total_size) != 0) {
+ result = -errno;
+ error_setg_errno(errp, -result, "Could not resize file");
+ goto out_close;
+ }
+
+ if (prealloc == PREALLOC_MODE_FULL) {
+ /* posix_fallocate() doesn't set errno. */
+ result = -posix_fallocate(fd, 0, total_size);
+ if (result != 0) {
+ buf = g_malloc0(65536);
+ int64_t num = 0, left = total_size;
+
+ while (left > 0) {
+ num = MIN(left, 65536);
+ result = write(fd, buf, num);
+ if (result < 0) {
+ result = -errno;
+ error_setg_errno(errp, -result,
+ "Could not write to the new file");
+ g_free(buf);
+ goto out_close;
+ }
+ left -= num;
+ }
+ fsync(fd);
+ g_free(buf);
}
+ } else if (prealloc != PREALLOC_MODE_OFF) {
+ result = -1;
+ error_setg(errp, "Unsupported preallocation mode: %s",
+ PreallocMode_lookup[prealloc]);
+ }
+
+out_close:
+ if (qemu_close(fd) != 0 && result == 0) {
+ result = -errno;
+ error_setg_errno(errp, -result, "Could not close the new file");
}
+out:
return result;
}
@@ -1585,6 +1632,11 @@ static QemuOptsList raw_create_opts = {
.type = QEMU_OPT_BOOL,
.help = "Turn off copy-on-write (valid only on btrfs)"
},
+ {
+ .name = BLOCK_OPT_PREALLOC,
+ .type = QEMU_OPT_STRING,
+ .help = "Preallocation mode (allowed values: off, full)"
+ },
{ /* end of list */ }
}
};
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 2b232ae..2637765 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -527,6 +527,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
space. Use @code{qemu-img info} to know the real size used by the
image or @code{ls -ls} on Unix/Linux.
+Supported options:
+@table @code
+@item preallocation
+Preallocation mode(allowed values: @code{off}, @code{full}). An image is
+fully preallocated by calling posix_fallocate() if it's available, or by
+writing zeros to underlying storage.
+@end table
+
@item qcow2
QEMU image format, the most versatile format. Use it to have smaller
images (useful if your filesystem does not supports holes, for example
diff --git a/qemu-img.texi b/qemu-img.texi
index cb68948..063ec61 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -418,6 +418,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
space. Use @code{qemu-img info} to know the real size used by the
image or @code{ls -ls} on Unix/Linux.
+Supported options:
+@table @code
+@item preallocation
+Preallocation mode(allowed values: @code{off}, @code{full}). An image is
+fully preallocated by calling posix_fallocate() if it's available, or by
+writing zeros to underlying storage.
+@end table
+
@item qcow2
QEMU image format, the most versatile format. Use it to have smaller
images (useful if your filesystem does not supports holes, for example
--
1.9.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option Hu Tao
@ 2014-08-29 8:48 ` Richard W.M. Jones
2014-09-03 1:26 ` Hu Tao
2014-09-04 8:32 ` Hu Tao
2014-09-02 21:45 ` Max Reitz
2014-09-04 12:35 ` Kevin Wolf
2 siblings, 2 replies; 39+ messages in thread
From: Richard W.M. Jones @ 2014-08-29 8:48 UTC (permalink / raw)
To: Hu Tao
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
On Fri, Aug 29, 2014 at 04:33:12PM +0800, Hu Tao wrote:
> + if (prealloc == PREALLOC_MODE_FULL) {
> + /* posix_fallocate() doesn't set errno. */
> + result = -posix_fallocate(fd, 0, total_size);
> + if (result != 0) {
Is it better to test:
result != ENOSYS && result != EOPNOTSUPP
here?
I think this is definitely the right approach.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-08-29 8:48 ` Richard W.M. Jones
@ 2014-09-03 1:26 ` Hu Tao
2014-09-04 8:32 ` Hu Tao
1 sibling, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-03 1:26 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
On Fri, Aug 29, 2014 at 09:48:01AM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 29, 2014 at 04:33:12PM +0800, Hu Tao wrote:
> > + if (prealloc == PREALLOC_MODE_FULL) {
> > + /* posix_fallocate() doesn't set errno. */
> > + result = -posix_fallocate(fd, 0, total_size);
> > + if (result != 0) {
>
> Is it better to test:
>
> result != ENOSYS && result != EOPNOTSUPP
>
> here?
posix_fallocate() doesn't return ENOSYS or EOPNOTSUPP. All the errors
returned by posix_fallocate() apply to writing zeros, too. that is, if
posix_fallocate() returns an error, we should not do writing zeros,
neither.
I'm wondering what is the right way to test if posix_fallocate() is
supported, something like AC_CHECK_FUNC? how?
Regards,
Hu
>
> I think this is definitely the right approach.
>
> Rich.
>
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-p2v converts physical machines to virtual machines. Boot with a
> live CD or over the network (PXE) and turn machines into KVM guests.
> http://libguestfs.org/virt-v2v
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-08-29 8:48 ` Richard W.M. Jones
2014-09-03 1:26 ` Hu Tao
@ 2014-09-04 8:32 ` Hu Tao
1 sibling, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-04 8:32 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
On Fri, Aug 29, 2014 at 09:48:01AM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 29, 2014 at 04:33:12PM +0800, Hu Tao wrote:
> > + if (prealloc == PREALLOC_MODE_FULL) {
> > + /* posix_fallocate() doesn't set errno. */
> > + result = -posix_fallocate(fd, 0, total_size);
> > + if (result != 0) {
>
> Is it better to test:
>
> result != ENOSYS && result != EOPNOTSUPP
>
> here?
>
> I think this is definitely the right approach.
Hi Kevin,
How do you think this approach?
Regards,
Hu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option Hu Tao
2014-08-29 8:48 ` Richard W.M. Jones
@ 2014-09-02 21:45 ` Max Reitz
2014-09-03 1:55 ` Hu Tao
2014-09-04 12:35 ` Kevin Wolf
2 siblings, 1 reply; 39+ messages in thread
From: Max Reitz @ 2014-09-02 21:45 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Stefan Hajnoczi,
Yasunori Goto
On 29.08.2014 10:33, Hu Tao wrote:
> This patch adds a new option preallocation for raw format, and implements
> full preallocation.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/raw-posix.c | 92 +++++++++++++++++++++++++++++++++++++++++++------------
> qemu-doc.texi | 8 +++++
> qemu-img.texi | 8 +++++
> 3 files changed, 88 insertions(+), 20 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index abe0759..25f66f2 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -30,6 +30,7 @@
> #include "block/thread-pool.h"
> #include "qemu/iov.h"
> #include "raw-aio.h"
> +#include "qapi/util.h"
>
> #if defined(__APPLE__) && (__MACH__)
> #include <paths.h>
> @@ -1365,6 +1366,9 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> int result = 0;
> int64_t total_size = 0;
> bool nocow = false;
> + PreallocMode prealloc = PREALLOC_MODE_OFF;
> + char *buf = NULL;
> + Error *local_err = NULL;
>
> strstart(filename, "file:", &filename);
>
> @@ -1372,37 +1376,80 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> BDRV_SECTOR_SIZE);
> nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
> + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> + prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> + PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> + &local_err);
> + g_free(buf);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + result = -EINVAL;
> + goto out;
> + }
>
> fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> 0644);
> if (fd < 0) {
> result = -errno;
> error_setg_errno(errp, -result, "Could not create file");
> - } else {
> - if (nocow) {
> + goto out;
> + }
> +
> + if (nocow) {
> #ifdef __linux__
> - /* Set NOCOW flag to solve performance issue on fs like btrfs.
> - * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> - * will be ignored since any failure of this operation should not
> - * block the left work.
> - */
> - int attr;
> - if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> - attr |= FS_NOCOW_FL;
> - ioctl(fd, FS_IOC_SETFLAGS, &attr);
> - }
> -#endif
> + /* Set NOCOW flag to solve performance issue on fs like btrfs.
> + * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> + * will be ignored since any failure of this operation should not
> + * block the left work.
> + */
> + int attr;
> + if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> + attr |= FS_NOCOW_FL;
> + ioctl(fd, FS_IOC_SETFLAGS, &attr);
> }
> +#endif
> + }
>
> - if (ftruncate(fd, total_size) != 0) {
> - result = -errno;
> - error_setg_errno(errp, -result, "Could not resize file");
> - }
> - if (qemu_close(fd) != 0) {
> - result = -errno;
> - error_setg_errno(errp, -result, "Could not close the new file");
> + if (ftruncate(fd, total_size) != 0) {
> + result = -errno;
> + error_setg_errno(errp, -result, "Could not resize file");
> + goto out_close;
> + }
> +
> + if (prealloc == PREALLOC_MODE_FULL) {
> + /* posix_fallocate() doesn't set errno. */
> + result = -posix_fallocate(fd, 0, total_size);
> + if (result != 0) {
> + buf = g_malloc0(65536);
> + int64_t num = 0, left = total_size;
> +
> + while (left > 0) {
> + num = MIN(left, 65536);
> + result = write(fd, buf, num);
> + if (result < 0) {
> + result = -errno;
> + error_setg_errno(errp, -result,
> + "Could not write to the new file");
> + g_free(buf);
> + goto out_close;
> + }
> + left -= num;
> + }
> + fsync(fd);
> + g_free(buf);
> }
> + } else if (prealloc != PREALLOC_MODE_OFF) {
> + result = -1;
As for qcow2 in patch 4, I'd prefer -EINVAL.
> + error_setg(errp, "Unsupported preallocation mode: %s",
> + PreallocMode_lookup[prealloc]);
> + }
> +
> +out_close:
> + if (qemu_close(fd) != 0 && result == 0) {
> + result = -errno;
> + error_setg_errno(errp, -result, "Could not close the new file");
> }
> +out:
> return result;
> }
>
> @@ -1585,6 +1632,11 @@ static QemuOptsList raw_create_opts = {
> .type = QEMU_OPT_BOOL,
> .help = "Turn off copy-on-write (valid only on btrfs)"
> },
> + {
> + .name = BLOCK_OPT_PREALLOC,
> + .type = QEMU_OPT_STRING,
> + .help = "Preallocation mode (allowed values: off, full)"
> + },
> { /* end of list */ }
> }
> };
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 2b232ae..2637765 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -527,6 +527,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
> space. Use @code{qemu-img info} to know the real size used by the
> image or @code{ls -ls} on Unix/Linux.
>
> +Supported options:
> +@table @code
> +@item preallocation
> +Preallocation mode(allowed values: @code{off}, @code{full}). An image is
Missing space in front of the opening bracket.
> +fully preallocated by calling posix_fallocate() if it's available, or by
Hm, I personally am not so happy about contractions ("it's") in
persistent documentation (even source code comments). Although I know
there are already some of them in qemu-doc.texi, I'd rather avoid them.
But I'll leave this up to you as I'm no native speaker.
> +writing zeros to underlying storage.
> +@end table
> +
> @item qcow2
> QEMU image format, the most versatile format. Use it to have smaller
> images (useful if your filesystem does not supports holes, for example
> diff --git a/qemu-img.texi b/qemu-img.texi
> index cb68948..063ec61 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -418,6 +418,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
> space. Use @code{qemu-img info} to know the real size used by the
> image or @code{ls -ls} on Unix/Linux.
>
> +Supported options:
> +@table @code
> +@item preallocation
> +Preallocation mode(allowed values: @code{off}, @code{full}). An image is
> +fully preallocated by calling posix_fallocate() if it's available, or by
> +writing zeros to underlying storage.
> +@end table
> +
Same as for qemu-doc.texi.
However, these are all minor, so with "result = -EINVAL" and the missing
space added before the brackets:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> @item qcow2
> QEMU image format, the most versatile format. Use it to have smaller
> images (useful if your filesystem does not supports holes, for example
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-02 21:45 ` Max Reitz
@ 2014-09-03 1:55 ` Hu Tao
0 siblings, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-03 1:55 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, qemu-devel, Richard W.M. Jones,
Stefan Hajnoczi, Yasunori Goto
On Tue, Sep 02, 2014 at 11:45:38PM +0200, Max Reitz wrote:
> On 29.08.2014 10:33, Hu Tao wrote:
> >This patch adds a new option preallocation for raw format, and implements
> >full preallocation.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> > block/raw-posix.c | 92 +++++++++++++++++++++++++++++++++++++++++++------------
> > qemu-doc.texi | 8 +++++
> > qemu-img.texi | 8 +++++
> > 3 files changed, 88 insertions(+), 20 deletions(-)
> >
> >diff --git a/block/raw-posix.c b/block/raw-posix.c
> >index abe0759..25f66f2 100644
> >--- a/block/raw-posix.c
> >+++ b/block/raw-posix.c
> >@@ -30,6 +30,7 @@
> > #include "block/thread-pool.h"
> > #include "qemu/iov.h"
> > #include "raw-aio.h"
> >+#include "qapi/util.h"
> > #if defined(__APPLE__) && (__MACH__)
> > #include <paths.h>
> >@@ -1365,6 +1366,9 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> > int result = 0;
> > int64_t total_size = 0;
> > bool nocow = false;
> >+ PreallocMode prealloc = PREALLOC_MODE_OFF;
> >+ char *buf = NULL;
> >+ Error *local_err = NULL;
> > strstart(filename, "file:", &filename);
> >@@ -1372,37 +1376,80 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> > total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > BDRV_SECTOR_SIZE);
> > nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
> >+ buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> >+ prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> >+ PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> >+ &local_err);
> >+ g_free(buf);
> >+ if (local_err) {
> >+ error_propagate(errp, local_err);
> >+ result = -EINVAL;
> >+ goto out;
> >+ }
> > fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> > 0644);
> > if (fd < 0) {
> > result = -errno;
> > error_setg_errno(errp, -result, "Could not create file");
> >- } else {
> >- if (nocow) {
> >+ goto out;
> >+ }
> >+
> >+ if (nocow) {
> > #ifdef __linux__
> >- /* Set NOCOW flag to solve performance issue on fs like btrfs.
> >- * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> >- * will be ignored since any failure of this operation should not
> >- * block the left work.
> >- */
> >- int attr;
> >- if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> >- attr |= FS_NOCOW_FL;
> >- ioctl(fd, FS_IOC_SETFLAGS, &attr);
> >- }
> >-#endif
> >+ /* Set NOCOW flag to solve performance issue on fs like btrfs.
> >+ * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> >+ * will be ignored since any failure of this operation should not
> >+ * block the left work.
> >+ */
> >+ int attr;
> >+ if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> >+ attr |= FS_NOCOW_FL;
> >+ ioctl(fd, FS_IOC_SETFLAGS, &attr);
> > }
> >+#endif
> >+ }
> >- if (ftruncate(fd, total_size) != 0) {
> >- result = -errno;
> >- error_setg_errno(errp, -result, "Could not resize file");
> >- }
> >- if (qemu_close(fd) != 0) {
> >- result = -errno;
> >- error_setg_errno(errp, -result, "Could not close the new file");
> >+ if (ftruncate(fd, total_size) != 0) {
> >+ result = -errno;
> >+ error_setg_errno(errp, -result, "Could not resize file");
> >+ goto out_close;
> >+ }
> >+
> >+ if (prealloc == PREALLOC_MODE_FULL) {
> >+ /* posix_fallocate() doesn't set errno. */
> >+ result = -posix_fallocate(fd, 0, total_size);
> >+ if (result != 0) {
> >+ buf = g_malloc0(65536);
> >+ int64_t num = 0, left = total_size;
> >+
> >+ while (left > 0) {
> >+ num = MIN(left, 65536);
> >+ result = write(fd, buf, num);
> >+ if (result < 0) {
> >+ result = -errno;
> >+ error_setg_errno(errp, -result,
> >+ "Could not write to the new file");
> >+ g_free(buf);
> >+ goto out_close;
> >+ }
> >+ left -= num;
> >+ }
> >+ fsync(fd);
> >+ g_free(buf);
> > }
> >+ } else if (prealloc != PREALLOC_MODE_OFF) {
> >+ result = -1;
>
> As for qcow2 in patch 4, I'd prefer -EINVAL.
Okay.
>
> >+ error_setg(errp, "Unsupported preallocation mode: %s",
> >+ PreallocMode_lookup[prealloc]);
> >+ }
> >+
> >+out_close:
> >+ if (qemu_close(fd) != 0 && result == 0) {
> >+ result = -errno;
> >+ error_setg_errno(errp, -result, "Could not close the new file");
> > }
> >+out:
> > return result;
> > }
> >@@ -1585,6 +1632,11 @@ static QemuOptsList raw_create_opts = {
> > .type = QEMU_OPT_BOOL,
> > .help = "Turn off copy-on-write (valid only on btrfs)"
> > },
> >+ {
> >+ .name = BLOCK_OPT_PREALLOC,
> >+ .type = QEMU_OPT_STRING,
> >+ .help = "Preallocation mode (allowed values: off, full)"
> >+ },
> > { /* end of list */ }
> > }
> > };
> >diff --git a/qemu-doc.texi b/qemu-doc.texi
> >index 2b232ae..2637765 100644
> >--- a/qemu-doc.texi
> >+++ b/qemu-doc.texi
> >@@ -527,6 +527,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
> > space. Use @code{qemu-img info} to know the real size used by the
> > image or @code{ls -ls} on Unix/Linux.
> >+Supported options:
> >+@table @code
> >+@item preallocation
> >+Preallocation mode(allowed values: @code{off}, @code{full}). An image is
>
> Missing space in front of the opening bracket.
Okay. I checked that in other places of opening bracket there is a space
before them.
>
> >+fully preallocated by calling posix_fallocate() if it's available, or by
>
> Hm, I personally am not so happy about contractions ("it's") in
> persistent documentation (even source code comments). Although I
> know there are already some of them in qemu-doc.texi, I'd rather
> avoid them. But I'll leave this up to you as I'm no native speaker.
I'm not, either. I don't know which one in "it's" and "it is" is more
common, but I can change the contraction if it makes you feel better:)
Regards,
Hu
>
> >+writing zeros to underlying storage.
> >+@end table
> >+
> > @item qcow2
> > QEMU image format, the most versatile format. Use it to have smaller
> > images (useful if your filesystem does not supports holes, for example
> >diff --git a/qemu-img.texi b/qemu-img.texi
> >index cb68948..063ec61 100644
> >--- a/qemu-img.texi
> >+++ b/qemu-img.texi
> >@@ -418,6 +418,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
> > space. Use @code{qemu-img info} to know the real size used by the
> > image or @code{ls -ls} on Unix/Linux.
> >+Supported options:
> >+@table @code
> >+@item preallocation
> >+Preallocation mode(allowed values: @code{off}, @code{full}). An image is
> >+fully preallocated by calling posix_fallocate() if it's available, or by
> >+writing zeros to underlying storage.
> >+@end table
> >+
>
> Same as for qemu-doc.texi.
>
> However, these are all minor, so with "result = -EINVAL" and the
> missing space added before the brackets:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> > @item qcow2
> > QEMU image format, the most versatile format. Use it to have smaller
> > images (useful if your filesystem does not supports holes, for example
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option Hu Tao
2014-08-29 8:48 ` Richard W.M. Jones
2014-09-02 21:45 ` Max Reitz
@ 2014-09-04 12:35 ` Kevin Wolf
2014-09-04 12:45 ` Richard W.M. Jones
2 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 12:35 UTC (permalink / raw)
To: Hu Tao
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> This patch adds a new option preallocation for raw format, and implements
> full preallocation.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
v12 was better, it wasn't doing only metadata preallocation when you
told it to do full preallocation.
> + if (prealloc == PREALLOC_MODE_FULL) {
> + /* posix_fallocate() doesn't set errno. */
> + result = -posix_fallocate(fd, 0, total_size);
> + if (result != 0) {
> + buf = g_malloc0(65536);
> + int64_t num = 0, left = total_size;
> +
> + while (left > 0) {
> + num = MIN(left, 65536);
> + result = write(fd, buf, num);
> + if (result < 0) {
> + result = -errno;
> + error_setg_errno(errp, -result,
> + "Could not write to the new file");
> + g_free(buf);
> + goto out_close;
> + }
> + left -= num;
> + }
> + fsync(fd);
> + g_free(buf);
> }
This is totally pointless. If the file system doesn't support fallocate,
posix_fallocate() will already try writing zeros.
Having code to write zeros in qemu only makes sense if you implement a
real full preallocation mode, which doesn't use fallocate even when it's
supported.
Please change the code to always write zeros for FULL, and either
reintroduce FALLOC or use METADATA for the fallocate (without a fallback
to zero writing code). In fact, for metadata preallocation we should
probably directly use fallocate(), which has no slow zero-write fallback
in the libc.
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 12:35 ` Kevin Wolf
@ 2014-09-04 12:45 ` Richard W.M. Jones
2014-09-04 12:52 ` Kevin Wolf
0 siblings, 1 reply; 39+ messages in thread
From: Richard W.M. Jones @ 2014-09-04 12:45 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> Please change the code to always write zeros for FULL,
How is this useful for anyone? You don't know if the underlying SAN
is going to detect these zeroes or combine these blocks together.
It's just slow for no reason.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 12:45 ` Richard W.M. Jones
@ 2014-09-04 12:52 ` Kevin Wolf
2014-09-04 13:07 ` Richard W.M. Jones
0 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 12:52 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
Am 04.09.2014 um 14:45 hat Richard W.M. Jones geschrieben:
> On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> > Please change the code to always write zeros for FULL,
>
> How is this useful for anyone? You don't know if the underlying SAN
> is going to detect these zeroes or combine these blocks together.
> It's just slow for no reason.
It's slow for the reason that the user has requested it. Do you doubt
that users can know what their backend is doing? Why are you insisting
on providing only the functionality that you personally need?
I doubt it's a way to make many users happier, but if you insist, we can
leave full preallocation unsupported on raw-posix and allow only
metadata preallocation (which should still be fallocate() without a
zero-write fallback, so that you can rely on it being fast). But call it
by its name and don't say "full" when you only implement "metadata".
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 12:52 ` Kevin Wolf
@ 2014-09-04 13:07 ` Richard W.M. Jones
2014-09-04 13:13 ` Daniel P. Berrange
2014-09-04 13:17 ` Kevin Wolf
0 siblings, 2 replies; 39+ messages in thread
From: Richard W.M. Jones @ 2014-09-04 13:07 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
On Thu, Sep 04, 2014 at 02:52:57PM +0200, Kevin Wolf wrote:
> Am 04.09.2014 um 14:45 hat Richard W.M. Jones geschrieben:
> > On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> > > Please change the code to always write zeros for FULL,
> >
> > How is this useful for anyone? You don't know if the underlying SAN
> > is going to detect these zeroes or combine these blocks together.
> > It's just slow for no reason.
>
> It's slow for the reason that the user has requested it. Do you doubt
> that users can know what their backend is doing? Why are you insisting
> on providing only the functionality that you personally need?
I'm not! I'm trying to make sure we don't end up with a qemu
interface which is useless for higher layers. You're proposing
preallocation=full which will be slow but not actually give any
guarantees, or preallocation=meta which is going to be fast but may
not work, and I'm saying that's a dumb interface that's not useful.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 13:07 ` Richard W.M. Jones
@ 2014-09-04 13:13 ` Daniel P. Berrange
2014-09-04 13:17 ` Kevin Wolf
1 sibling, 0 replies; 39+ messages in thread
From: Daniel P. Berrange @ 2014-09-04 13:13 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Kevin Wolf, Fam Zheng, Hu Tao, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
On Thu, Sep 04, 2014 at 02:07:14PM +0100, Richard W.M. Jones wrote:
> On Thu, Sep 04, 2014 at 02:52:57PM +0200, Kevin Wolf wrote:
> > Am 04.09.2014 um 14:45 hat Richard W.M. Jones geschrieben:
> > > On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> > > > Please change the code to always write zeros for FULL,
> > >
> > > How is this useful for anyone? You don't know if the underlying SAN
> > > is going to detect these zeroes or combine these blocks together.
> > > It's just slow for no reason.
> >
> > It's slow for the reason that the user has requested it. Do you doubt
> > that users can know what their backend is doing? Why are you insisting
> > on providing only the functionality that you personally need?
>
> I'm not! I'm trying to make sure we don't end up with a qemu
> interface which is useless for higher layers. You're proposing
> preallocation=full which will be slow but not actually give any
> guarantees, or preallocation=meta which is going to be fast but may
> not work, and I'm saying that's a dumb interface that's not useful.
Agree, from my POV as an app maintainer, fallocate with automatically
fallback to zero'ing is really what we want to use. Having two separate
options which we have to explicitly try in turn, or only having the
slow option is really not what we want to use.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 13:07 ` Richard W.M. Jones
2014-09-04 13:13 ` Daniel P. Berrange
@ 2014-09-04 13:17 ` Kevin Wolf
2014-09-04 13:43 ` Richard W.M. Jones
1 sibling, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 13:17 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
Am 04.09.2014 um 15:07 hat Richard W.M. Jones geschrieben:
> On Thu, Sep 04, 2014 at 02:52:57PM +0200, Kevin Wolf wrote:
> > Am 04.09.2014 um 14:45 hat Richard W.M. Jones geschrieben:
> > > On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> > > > Please change the code to always write zeros for FULL,
> > >
> > > How is this useful for anyone? You don't know if the underlying SAN
> > > is going to detect these zeroes or combine these blocks together.
> > > It's just slow for no reason.
> >
> > It's slow for the reason that the user has requested it. Do you doubt
> > that users can know what their backend is doing? Why are you insisting
> > on providing only the functionality that you personally need?
>
> I'm not! I'm trying to make sure we don't end up with a qemu
> interface which is useless for higher layers. You're proposing
> preallocation=full which will be slow but not actually give any
> guarantees, or preallocation=meta which is going to be fast but may
> not work, and I'm saying that's a dumb interface that's not useful.
So what you propose is an interface that combines both and may
unpredictably be slow or fast, and doesn't give any guarantees either.
Why would this be any better?
What is your specific use case of full preallocation that wants zero
writing, but only implicitly as a fallback? My expectation is that most
users want cheap preallocation if it's available, but don't bother to
write many gigabytes if it isn't.
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 13:17 ` Kevin Wolf
@ 2014-09-04 13:43 ` Richard W.M. Jones
2014-09-04 15:23 ` Kevin Wolf
0 siblings, 1 reply; 39+ messages in thread
From: Richard W.M. Jones @ 2014-09-04 13:43 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
On Thu, Sep 04, 2014 at 03:17:51PM +0200, Kevin Wolf wrote:
> Am 04.09.2014 um 15:07 hat Richard W.M. Jones geschrieben:
> > On Thu, Sep 04, 2014 at 02:52:57PM +0200, Kevin Wolf wrote:
> > > Am 04.09.2014 um 14:45 hat Richard W.M. Jones geschrieben:
> > > > On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> > > > > Please change the code to always write zeros for FULL,
> > > >
> > > > How is this useful for anyone? You don't know if the underlying SAN
> > > > is going to detect these zeroes or combine these blocks together.
> > > > It's just slow for no reason.
> > >
> > > It's slow for the reason that the user has requested it. Do you doubt
> > > that users can know what their backend is doing? Why are you insisting
> > > on providing only the functionality that you personally need?
> >
> > I'm not! I'm trying to make sure we don't end up with a qemu
> > interface which is useless for higher layers. You're proposing
> > preallocation=full which will be slow but not actually give any
> > guarantees, or preallocation=meta which is going to be fast but may
> > not work, and I'm saying that's a dumb interface that's not useful.
>
> So what you propose is an interface that combines both and may
> unpredictably be slow or fast, and doesn't give any guarantees either.
> Why would this be any better?
>
> What is your specific use case of full preallocation that wants zero
> writing, but only implicitly as a fallback? My expectation is that most
> users want cheap preallocation if it's available, but don't bother to
> write many gigabytes if it isn't.
Stepping back, I think what we have are two general approaches:
- do the exact thing I want
- do the best effort you can
virt-manager, virt-install, virt-clone, libvirt, libguestfs, all
currently do preallocation with fallback to writing data (via
hand-written code). The reason is that customers using simple disks
(not SANs etc) just want to be sure they're not going to run out of
space during OS installation. For the vast majority of users,
posix_fallocate makes this fast, but people using ext2 or *BSD get the
slow-but-safe path.
Now it may be that some qemu users are going to want a very exact
preallocation mode, but that doesn't mean we can't have
preallocation=besteffort too.
And it's not just here. qemu has other places where we'd like "do the
best thing", not "do the exact thing" ... off the top of my head:
selecting -cpu, discard support, O_DIRECT. Libguestfs has to go
through hoops in these areas, often involving very hairy workarounds
which reduce reliability. I'm not saying that more exact options
aren't also welcome, but doing the best effort is very useful too.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 13:43 ` Richard W.M. Jones
@ 2014-09-04 15:23 ` Kevin Wolf
2014-09-04 15:33 ` Richard W.M. Jones
0 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 15:23 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
Am 04.09.2014 um 15:43 hat Richard W.M. Jones geschrieben:
> On Thu, Sep 04, 2014 at 03:17:51PM +0200, Kevin Wolf wrote:
> > Am 04.09.2014 um 15:07 hat Richard W.M. Jones geschrieben:
> > > On Thu, Sep 04, 2014 at 02:52:57PM +0200, Kevin Wolf wrote:
> > > > Am 04.09.2014 um 14:45 hat Richard W.M. Jones geschrieben:
> > > > > On Thu, Sep 04, 2014 at 02:35:22PM +0200, Kevin Wolf wrote:
> > > > > > Please change the code to always write zeros for FULL,
> > > > >
> > > > > How is this useful for anyone? You don't know if the underlying SAN
> > > > > is going to detect these zeroes or combine these blocks together.
> > > > > It's just slow for no reason.
> > > >
> > > > It's slow for the reason that the user has requested it. Do you doubt
> > > > that users can know what their backend is doing? Why are you insisting
> > > > on providing only the functionality that you personally need?
> > >
> > > I'm not! I'm trying to make sure we don't end up with a qemu
> > > interface which is useless for higher layers. You're proposing
> > > preallocation=full which will be slow but not actually give any
> > > guarantees, or preallocation=meta which is going to be fast but may
> > > not work, and I'm saying that's a dumb interface that's not useful.
> >
> > So what you propose is an interface that combines both and may
> > unpredictably be slow or fast, and doesn't give any guarantees either.
> > Why would this be any better?
> >
> > What is your specific use case of full preallocation that wants zero
> > writing, but only implicitly as a fallback? My expectation is that most
> > users want cheap preallocation if it's available, but don't bother to
> > write many gigabytes if it isn't.
>
> Stepping back, I think what we have are two general approaches:
>
> - do the exact thing I want
>
> - do the best effort you can
>
> [...]
>
> And it's not just here. qemu has other places where we'd like "do the
> best thing", not "do the exact thing" ... off the top of my head:
> selecting -cpu, discard support, O_DIRECT. Libguestfs has to go
> through hoops in these areas, often involving very hairy workarounds
> which reduce reliability. I'm not saying that more exact options
> aren't also welcome, but doing the best effort is very useful too.
The point is that different people have different ideas of what "do the
best thing" is. You may be surprised to learn that generally we don't
roll a dice to select our defaults, but do it based on what we think is
the best thing overall. And yes, because we have less information than
management tools, it often means that we have to take more conservative
defaults (works for everyone) than what you would want (all the features
that your shiny new guest can use).
> virt-manager, virt-install, virt-clone, libvirt, libguestfs, all
> currently do preallocation with fallback to writing data (via
> hand-written code). The reason is that customers using simple disks
> (not SANs etc) just want to be sure they're not going to run out of
> space during OS installation. For the vast majority of users,
> posix_fallocate makes this fast, but people using ext2 or *BSD get the
> slow-but-safe path.
Is this really a concern for many users? I think I never preallocated an
image to make sure it fits. If anything, I run df first, but generally I
assume that it works and if it doesn't, I check what happened.
Basically all users I saw asking for preallocation did it because they
were worried about performance and wanted to get rid of the overhead of
metadata updates during allocations.
> Now it may be that some qemu users are going to want a very exact
> preallocation mode, but that doesn't mean we can't have
> preallocation=besteffort too.
The definition of "besteffort" depends on what you want to achieve. It
is policy, and we generally try to keep policy out of qemu.
It isn't really clear that "make absolutely sure that you never get
-ENOSPC on the file system level, but in the layers below it's okay, and
the resulting performance doesn't matter" is the best thing for everyone.
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option
2014-09-04 15:23 ` Kevin Wolf
@ 2014-09-04 15:33 ` Richard W.M. Jones
0 siblings, 0 replies; 39+ messages in thread
From: Richard W.M. Jones @ 2014-09-04 15:33 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Hu Tao, qemu-devel, Max Reitz, Stefan Hajnoczi,
Yasunori Goto
On Thu, Sep 04, 2014 at 05:23:21PM +0200, Kevin Wolf wrote:
> The definition of "besteffort" depends on what you want to achieve. It
> is policy, and we generally try to keep policy out of qemu.
I think qemu *should* have a policy of make it work and don't fail - first -
and then offer a million knobs to optimize and customize things on top
of that.
But whatever ... what's one more thing that we have to provide fragile
workarounds for ...
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v13 6/6] qcow2: Add full preallocation option
2014-08-29 8:33 [Qemu-devel] [PATCH v13 0/6] qcow2, raw: add preallocation=full Hu Tao
` (4 preceding siblings ...)
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option Hu Tao
@ 2014-08-29 8:33 ` Hu Tao
2014-09-02 21:55 ` Max Reitz
2014-09-04 13:09 ` Kevin Wolf
5 siblings, 2 replies; 39+ messages in thread
From: Hu Tao @ 2014-08-29 8:33 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
preallocation=full allocates disk space by fallocating the space if
posix_fallocate() is available, otherwise by writing zeros to disk to
ensure disk space in any cases.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
block/qcow2.c | 61 +++++++++++++++++++++++++++++++++++++++-------
qemu-doc.texi | 7 +++---
qemu-img.texi | 7 +++---
tests/qemu-iotests/082.out | 54 ++++++++++++++++++++--------------------
4 files changed, 87 insertions(+), 42 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 95fb240..f222fff 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1772,6 +1772,56 @@ static int qcow2_create2(const char *filename, int64_t total_size,
Error *local_err = NULL;
int ret;
+ if (prealloc == PREALLOC_MODE_FULL) {
+ int64_t meta_size = 0;
+ uint64_t nreftablee, nrefblocke, nl1e, nl2e;
+ int64_t aligned_total_size = align_offset(total_size, cluster_size);
+
+ /* header: 1 cluster */
+ meta_size += cluster_size;
+
+ /* total size of L2 tables */
+ nl2e = aligned_total_size / cluster_size;
+ nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t));
+ meta_size += nl2e * sizeof(uint64_t);
+
+ /* total size of L1 tables */
+ nl1e = nl2e * sizeof(uint64_t) / cluster_size;
+ nl1e = align_offset(nl1e, cluster_size / sizeof(uint64_t));
+ meta_size += nl1e * sizeof(uint64_t);
+
+ /* total size of refcount blocks
+ *
+ * note: every host cluster is reference-counted, including metadata
+ * (even refcount blocks are recursively included).
+ * Let:
+ * a = total_size (this is the guest disk size)
+ * m = meta size not including refcount blocks and refcount tables
+ * c = cluster size
+ * y1 = number of refcount blocks entries
+ * y2 = meta size including everything
+ * then,
+ * y1 = (y2 + a)/c
+ * y2 = y1 * sizeof(u16) + y1 * sizeof(u16) * sizeof(u64) / c + m
+ * we can get y1:
+ * y1 = (a + m) / (c - sizeof(u16) - sizeof(u16) * sizeof(u64) / c)
+ */
+ nrefblocke = (aligned_total_size + meta_size + cluster_size) /
+ (cluster_size - sizeof(uint16_t) -
+ 1.0 * sizeof(uint16_t) * sizeof(uint64_t) / cluster_size);
+ nrefblocke = align_offset(nrefblocke, cluster_size / sizeof(uint16_t));
+ meta_size += nrefblocke * sizeof(uint16_t);
+
+ /* total size of refcount tables */
+ nreftablee = nrefblocke * sizeof(uint16_t) / cluster_size;
+ nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t));
+ meta_size += nreftablee * sizeof(uint64_t);
+
+ qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
+ aligned_total_size + meta_size);
+ qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]);
+ }
+
ret = bdrv_create_file(filename, opts, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1877,7 +1927,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
}
/* And if we're supposed to preallocate metadata, do that now */
- if (prealloc) {
+ if (prealloc != PREALLOC_MODE_OFF) {
BDRVQcowState *s = bs->opaque;
qemu_co_mutex_lock(&s->lock);
ret = preallocate(bs);
@@ -1958,13 +2008,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
}
- if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
- ret = -1;
- error_setg(errp, "Unsupported preallocate mode: %s",
- PreallocMode_lookup[prealloc]);
- goto finish;
- }
-
if (backing_file && prealloc) {
error_setg(errp, "Backing file and preallocation cannot be used at "
"the same time");
@@ -2525,7 +2568,7 @@ static QemuOptsList qcow2_create_opts = {
{
.name = BLOCK_OPT_PREALLOC,
.type = QEMU_OPT_STRING,
- .help = "Preallocation mode (allowed values: off, metadata)"
+ .help = "Preallocation mode (allowed values: off, metadata, full)"
},
{
.name = BLOCK_OPT_LAZY_REFCOUNTS,
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 2637765..b72ff7c 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -583,9 +583,10 @@ sizes can improve the image file size whereas larger cluster sizes generally
provide better performance.
@item preallocation
-Preallocation mode (allowed values: off, metadata). An image with preallocated
-metadata is initially larger but can improve performance when the image needs
-to grow.
+Preallocation mode (allowed values: @code{off}, @code{metadata}, @code{full}).
+An image with preallocated metadata is initially larger but can improve
+performance when the image needs to grow. @code{full} preallocation is like
+the same option of @code{raw} format, but sets up metadata also.
@item lazy_refcounts
If this option is set to @code{on}, reference count updates are postponed with
diff --git a/qemu-img.texi b/qemu-img.texi
index 063ec61..7307331 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -474,9 +474,10 @@ sizes can improve the image file size whereas larger cluster sizes generally
provide better performance.
@item preallocation
-Preallocation mode (allowed values: off, metadata). An image with preallocated
-metadata is initially larger but can improve performance when the image needs
-to grow.
+Preallocation mode (allowed values: @code{off}, @code{metadata}, @code{full}).
+An image with preallocated metadata is initially larger but can improve
+performance when the image needs to grow. @code{full} preallocation is like
+the same option of @code{raw} format, but sets up metadata also.
@item lazy_refcounts
If this option is set to @code{on}, reference count updates are postponed with
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 413e7ef..be6d3ca 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -64,7 +64,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -76,7 +76,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -88,7 +88,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -100,7 +100,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -112,7 +112,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -124,7 +124,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -136,7 +136,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -148,7 +148,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -175,7 +175,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
Testing: create -o help
@@ -253,7 +253,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -265,7 +265,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -277,7 +277,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -289,7 +289,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -301,7 +301,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -313,7 +313,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -325,7 +325,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -337,7 +337,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -364,7 +364,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
Testing: convert -o help
@@ -431,7 +431,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -443,7 +443,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -455,7 +455,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -467,7 +467,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -479,7 +479,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -491,7 +491,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -503,7 +503,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -515,7 +515,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
nocow Turn off copy-on-write (valid only on btrfs)
@@ -544,7 +544,7 @@ backing_file File name of a base image
backing_fmt Image format of the base image
encryption Encrypt the image
cluster_size qcow2 cluster size
-preallocation Preallocation mode (allowed values: off, metadata)
+preallocation Preallocation mode (allowed values: off, metadata, full)
lazy_refcounts Postpone refcount updates
Testing: convert -o help
--
1.9.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v13 6/6] qcow2: Add full preallocation option
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 6/6] qcow2: " Hu Tao
@ 2014-09-02 21:55 ` Max Reitz
2014-09-04 13:09 ` Kevin Wolf
1 sibling, 0 replies; 39+ messages in thread
From: Max Reitz @ 2014-09-02 21:55 UTC (permalink / raw)
To: Hu Tao, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Richard W.M. Jones, Stefan Hajnoczi,
Yasunori Goto
On 29.08.2014 10:33, Hu Tao wrote:
> preallocation=full allocates disk space by fallocating the space if
> posix_fallocate() is available, otherwise by writing zeros to disk to
> ensure disk space in any cases.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/qcow2.c | 61 +++++++++++++++++++++++++++++++++++++++-------
> qemu-doc.texi | 7 +++---
> qemu-img.texi | 7 +++---
> tests/qemu-iotests/082.out | 54 ++++++++++++++++++++--------------------
> 4 files changed, 87 insertions(+), 42 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 6/6] qcow2: Add full preallocation option
2014-08-29 8:33 ` [Qemu-devel] [PATCH v13 6/6] qcow2: " Hu Tao
2014-09-02 21:55 ` Max Reitz
@ 2014-09-04 13:09 ` Kevin Wolf
2014-09-09 3:23 ` Hu Tao
1 sibling, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2014-09-04 13:09 UTC (permalink / raw)
To: Hu Tao
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> preallocation=full allocates disk space by fallocating the space if
> posix_fallocate() is available, otherwise by writing zeros to disk to
> ensure disk space in any cases.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> block/qcow2.c | 61 +++++++++++++++++++++++++++++++++++++++-------
> qemu-doc.texi | 7 +++---
> qemu-img.texi | 7 +++---
> tests/qemu-iotests/082.out | 54 ++++++++++++++++++++--------------------
> 4 files changed, 87 insertions(+), 42 deletions(-)
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> + aligned_total_size + meta_size);
> + qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]);
> + }
This means that if used with a protocol that doesn't have a
preallocation option, it gets silently ignored. I'm not completely
decided yet whether that's a bug or a feature. :-)
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v13 6/6] qcow2: Add full preallocation option
2014-09-04 13:09 ` Kevin Wolf
@ 2014-09-09 3:23 ` Hu Tao
0 siblings, 0 replies; 39+ messages in thread
From: Hu Tao @ 2014-09-09 3:23 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Richard W.M. Jones, qemu-devel, Max Reitz,
Stefan Hajnoczi, Yasunori Goto
On Thu, Sep 04, 2014 at 03:09:08PM +0200, Kevin Wolf wrote:
> Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> > preallocation=full allocates disk space by fallocating the space if
> > posix_fallocate() is available, otherwise by writing zeros to disk to
> > ensure disk space in any cases.
> >
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> > block/qcow2.c | 61 +++++++++++++++++++++++++++++++++++++++-------
> > qemu-doc.texi | 7 +++---
> > qemu-img.texi | 7 +++---
> > tests/qemu-iotests/082.out | 54 ++++++++++++++++++++--------------------
> > 4 files changed, 87 insertions(+), 42 deletions(-)
>
> > + qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> > + aligned_total_size + meta_size);
> > + qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]);
> > + }
>
> This means that if used with a protocol that doesn't have a
> preallocation option, it gets silently ignored. I'm not completely
> decided yet whether that's a bug or a feature. :-)
We can add code to reject preallocation option if it is determined as a
bug later, but it seems not necessary currently.
Regards,
Hu
^ permalink raw reply [flat|nested] 39+ messages in thread