Re: [Qemu-block] [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset

[Denis V. Lunev]

On 06/30/2016 07:40 PM, John Snow wrote:


On 06/30/2016 05:12 AM, Denis V. Lunev wrote:

On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:

After loading bitmap from image and setting IN_USE flag in it's header,
corresponding data (bitmap table and data clusters) becomes inconsistent
and is no longer needed. It is better to free bitmap table and
corresponding clusters from the image immediately after loading the
bitmap than free them when the bitmap is saved, or deleted or set
non-persistent.

For now it is impossible to store only bitmap header without bitmap
table, as specification requires it. Storing zeroed bitmap table (one or
more clusters) is the only option to implement the behaviour similar to
specified above.

The same problem is for just storing empty bitmaps.

This patch allows storing only bitmap header for empty bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Additional note. Should we also allow here bitmap_table_offset = 1, like
in bitmap table, for the bitmap with all bits set? I am not sure that it
is needed at all, but just to keep the company..

   docs/specs/qcow2.txt | 5 -
   1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..9826222 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
   Offset into the image file at which the bitmap
table
   (described below) for the bitmap starts. Must be
aligned to
   a cluster boundary.
+Zero value means that bitmap table is not
allocated and the
+bitmap should be considered as empty (all bits
are zero).
  8 - 11:bitmap_table_size
-Number of entries in the bitmap table of the bitmap.
+Number of entries in the bitmap table of the
bitmap. It
+must be zero if bitmap_table_offset is zero.
 12 - 15:flags
   Bit

NACK

no guys, we can not make this change at the moment.
We do have QEMU available in the field which is working
with the currently specified format.

Den


But I think the new format is a /compatible/ change. Under the old spec,
I think this field is *NEVER* zero.

Am I wrong?

yes

but as far as I can understand this breaks backward compatibility,
i.e. software working with the current revision of the specification
will not be able to load new images.

Re: [Qemu-block] [Qemu-devel] [PATCH] spec/parallels: fix a mistake

[John Snow]

On 06/30/2016 01:12 PM, Denis V. Lunev wrote:
> On 06/30/2016 07:43 PM, John Snow wrote:
>>
>> On 06/30/2016 04:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> We have only one flag for now - Empty Image flag.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   docs/specs/parallels.txt | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/docs/specs/parallels.txt b/docs/specs/parallels.txt
>>> index b4fe229..e9271eb 100644
>>> --- a/docs/specs/parallels.txt
>>> +++ b/docs/specs/parallels.txt
>>> @@ -94,7 +94,7 @@ Bytes:
>>> Bit 0: Empty Image bit. If set, the image should be
>>>considered clear.
>>>   -  Bits 2-31: Unused.
>>> +  Bits 1-31: Unused.
>>>   56 - 63:ext_off
>>> Format Extension offset, an offset, in sectors, from
>>> the start of
>>>
>> Looks correct to me, but Denis should confirm.
>>
>> --js
> Funny ;)
> 
> I have already resent the version with a bit improved
> description, which was takes by Stefan.
> 
> Thank you for paying attention.
> 
> Den

I wouldn't normally pay attention to parallels spec changes, but this
one made it to my inbox. :)

Re: [Qemu-block] [Qemu-devel] [PATCH] spec/parallels: fix a mistake

[Denis V. Lunev]

On 06/30/2016 07:43 PM, John Snow wrote:


On 06/30/2016 04:15 AM, Vladimir Sementsov-Ogievskiy wrote:

We have only one flag for now - Empty Image flag.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/specs/parallels.txt | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/specs/parallels.txt b/docs/specs/parallels.txt
index b4fe229..e9271eb 100644
--- a/docs/specs/parallels.txt
+++ b/docs/specs/parallels.txt
@@ -94,7 +94,7 @@ Bytes:
Bit 0: Empty Image bit. If set, the image should be
   considered clear.
  
-  Bits 2-31: Unused.

+  Bits 1-31: Unused.
  
56 - 63:ext_off

Format Extension offset, an offset, in sectors, from the start 
of


Looks correct to me, but Denis should confirm.

--js

Funny ;)

I have already resent the version with a bit improved
description, which was takes by Stefan.

Thank you for paying attention.

Den

Re: [Qemu-block] [Qemu-devel] [PATCH] spec/parallels: fix a mistake

[John Snow]

On 06/30/2016 04:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> We have only one flag for now - Empty Image flag.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  docs/specs/parallels.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/specs/parallels.txt b/docs/specs/parallels.txt
> index b4fe229..e9271eb 100644
> --- a/docs/specs/parallels.txt
> +++ b/docs/specs/parallels.txt
> @@ -94,7 +94,7 @@ Bytes:
>Bit 0: Empty Image bit. If set, the image should be
>   considered clear.
>  
> -  Bits 2-31: Unused.
> +  Bits 1-31: Unused.
>  
>56 - 63:ext_off
>Format Extension offset, an offset, in sectors, from the start 
> of
> 

Looks correct to me, but Denis should confirm.

--js

Re: [Qemu-block] [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset

[John Snow]

On 06/30/2016 05:12 AM, Denis V. Lunev wrote:
> On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> After loading bitmap from image and setting IN_USE flag in it's header,
>> corresponding data (bitmap table and data clusters) becomes inconsistent
>> and is no longer needed. It is better to free bitmap table and
>> corresponding clusters from the image immediately after loading the
>> bitmap than free them when the bitmap is saved, or deleted or set
>> non-persistent.
>>
>> For now it is impossible to store only bitmap header without bitmap
>> table, as specification requires it. Storing zeroed bitmap table (one or
>> more clusters) is the only option to implement the behaviour similar to
>> specified above.
>>
>> The same problem is for just storing empty bitmaps.
>>
>> This patch allows storing only bitmap header for empty bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>
>> Additional note. Should we also allow here bitmap_table_offset = 1, like
>> in bitmap table, for the bitmap with all bits set? I am not sure that it
>> is needed at all, but just to keep the company..
>>
>>   docs/specs/qcow2.txt | 5 -
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 80cdfd0..9826222 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
>>   Offset into the image file at which the bitmap
>> table
>>   (described below) for the bitmap starts. Must be
>> aligned to
>>   a cluster boundary.
>> +Zero value means that bitmap table is not
>> allocated and the
>> +bitmap should be considered as empty (all bits
>> are zero).
>>  8 - 11:bitmap_table_size
>> -Number of entries in the bitmap table of the bitmap.
>> +Number of entries in the bitmap table of the
>> bitmap. It
>> +must be zero if bitmap_table_offset is zero.
>> 12 - 15:flags
>>   Bit
> NACK
> 
> no guys, we can not make this change at the moment.
> We do have QEMU available in the field which is working
> with the currently specified format.
> 
> Den
> 

But I think the new format is a /compatible/ change. Under the old spec,
I think this field is *NEVER* zero.

Am I wrong?

Re: [Qemu-block] [PATCH 1/1] mirror: fix request throttling in drive-mirror

[Jeff Cody]

On Wed, Jun 22, 2016 at 03:35:27PM +0300, Denis V. Lunev wrote:
> There are 2 deficiencies here:
> - mirror_iteration could start several requests inside. Thus we could
>   simply have more in_flight requests than MAX_IN_FLIGHT.
> - keeping this in mind throttling in mirror_run which is checking
>   s->in_flight == MAX_IN_FLIGHT is wrong.
> 
> The patch adds the check and throttling into mirror_iteration and fixes
> the check in mirror_run() to be sure.
> 
> Signed-off-by: Denis V. Lunev 
> CC: Jeff Cody 
> CC: Kevin Wolf 
> CC: Max Reitz 
> ---
>  block/mirror.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..e881ef6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  }
>  }
>  
> +while (s->in_flight >= MAX_IN_FLIGHT) {
> +trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
> +mirror_wait_for_io(s);
> +}
> +
>  mirror_clip_sectors(s, sector_num, _sectors);
>  switch (mirror_method) {
>  case MIRROR_METHOD_COPY:
> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
>   */
>  if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < 
> SLICE_TIME &&
>  s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> -if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> +if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>  (cnt == 0 && s->in_flight > 0)) {
>  trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
>  mirror_wait_for_io(s);
> -- 
> 2.1.4
>

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH 1/1] mirror: fix request throttling in drive-mirror

[Jeff Cody]

On Wed, Jun 22, 2016 at 03:35:27PM +0300, Denis V. Lunev wrote:
> There are 2 deficiencies here:
> - mirror_iteration could start several requests inside. Thus we could
>   simply have more in_flight requests than MAX_IN_FLIGHT.
> - keeping this in mind throttling in mirror_run which is checking
>   s->in_flight == MAX_IN_FLIGHT is wrong.
> 
> The patch adds the check and throttling into mirror_iteration and fixes
> the check in mirror_run() to be sure.
> 
> Signed-off-by: Denis V. Lunev 
> CC: Jeff Cody 
> CC: Kevin Wolf 
> CC: Max Reitz 
> ---
>  block/mirror.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..e881ef6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  }
>  }
>  
> +while (s->in_flight >= MAX_IN_FLIGHT) {
> +trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
> +mirror_wait_for_io(s);
> +}
> +
>  mirror_clip_sectors(s, sector_num, _sectors);
>  switch (mirror_method) {
>  case MIRROR_METHOD_COPY:
> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
>   */
>  if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < 
> SLICE_TIME &&
>  s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> -if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> +if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>  (cnt == 0 && s->in_flight > 0)) {
>  trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
>  mirror_wait_for_io(s);
> -- 
> 2.1.4
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

Re: [Qemu-block] [PATCH] block/iscsi: precache the allocation status of a target

[Paolo Bonzini]

On 30/06/2016 13:08, Peter Lieven wrote:
> this fills up the allocationmap at iscsi_open. This helps
> to reduce the number of get_block_status requests during runtime
> significantly.
> 
> Signed-off-by: Peter Lieven 
> ---
>  block/iscsi.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 0cdcedb..04fb0a3 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1774,6 +1774,22 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>   iscsilun->block_size) >> 
> BDRV_SECTOR_BITS;
>  if (iscsilun->lbprz) {
>  ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
> +if (ret == 0) {
> +unsigned int max_reqs = 64;
> +int64_t sector_num = 0;
> +while (max_reqs-- && sector_num < bs->total_sectors) {
> +int n;
> +BlockDriverState *file;
> +ret = bdrv_get_block_status(bs, sector_num,
> +BDRV_REQUEST_MAX_SECTORS,
> +, );
> +if (ret < 0) {
> +break;
> +}
> +sector_num += n;
> +ret = 0;
> +}
> +}
>  }
>  }
>  
> 

This can take a long time and the disks may not even be ever used.  I
don't think it's a good idea.

Paolo

[Qemu-block] [PATCH v2 4/6] block/qdev: Allow configuring rerror/werror with qdev properties

[Kevin Wolf]

The rerror/werror policies are implemented in the devices, so that's
where they should be configured. In comparison to the old options in
-drive, the qdev properties are only added to those devices that
actually support them.

If the option isn't given (or "auto" is specified), the setting of the
BlockBackend is used for compatibility with the old options. For block
jobs, "auto" is the same as "enospc".

Signed-off-by: Kevin Wolf 
---
 block/block-backend.c|  1 +
 blockjob.c   |  1 +
 hw/block/block.c | 12 
 hw/block/virtio-blk.c|  1 +
 hw/core/qdev-properties.c| 13 +
 hw/ide/qdev.c|  1 +
 hw/scsi/scsi-disk.c  |  1 +
 include/hw/block/block.h |  8 
 include/hw/qdev-properties.h |  4 
 qapi/block-core.json |  4 +++-
 10 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index a862f65..4fedaf2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1173,6 +1173,7 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, 
bool is_read,
 return BLOCK_ERROR_ACTION_REPORT;
 case BLOCKDEV_ON_ERROR_IGNORE:
 return BLOCK_ERROR_ACTION_IGNORE;
+case BLOCKDEV_ON_ERROR_AUTO:
 default:
 abort();
 }
diff --git a/blockjob.c b/blockjob.c
index 90c4e26..8c9ea35 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -523,6 +523,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
BlockdevOnError on_err,
 
 switch (on_err) {
 case BLOCKDEV_ON_ERROR_ENOSPC:
+case BLOCKDEV_ON_ERROR_AUTO:
 action = (error == ENOSPC) ?
  BLOCK_ERROR_ACTION_STOP : BLOCK_ERROR_ACTION_REPORT;
 break;
diff --git a/hw/block/block.c b/hw/block/block.c
index 396b0d5..8dc9d84 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,6 +54,7 @@ void blkconf_blocksizes(BlockConf *conf)
 void blkconf_apply_backend_options(BlockConf *conf)
 {
 BlockBackend *blk = conf->blk;
+BlockdevOnError rerror, werror;
 bool wce;
 
 switch (conf->wce) {
@@ -64,7 +65,18 @@ void blkconf_apply_backend_options(BlockConf *conf)
 abort();
 }
 
+rerror = conf->rerror;
+if (rerror == BLOCKDEV_ON_ERROR_AUTO) {
+rerror = blk_get_on_error(blk, true);
+}
+
+werror = conf->werror;
+if (werror == BLOCKDEV_ON_ERROR_AUTO) {
+werror = blk_get_on_error(blk, false);
+}
+
 blk_set_enable_write_cache(blk, wce);
+blk_set_on_error(blk, rerror, werror);
 }
 
 void blkconf_geometry(BlockConf *conf, int *ptrans,
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 5eb6ba1..ef736fd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -960,6 +960,7 @@ static void virtio_blk_instance_init(Object *obj)
 
 static Property virtio_blk_properties[] = {
 DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf),
+DEFINE_BLOCK_ERROR_PROPERTIES(VirtIOBlock, conf.conf),
 DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, conf.conf),
 DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial),
 DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true),
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index e3b2184..3deceee 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -539,6 +539,19 @@ PropertyInfo qdev_prop_losttickpolicy = {
 .set   = set_enum,
 };
 
+/* --- Block device error handling policy --- */
+
+QEMU_BUILD_BUG_ON(sizeof(BlockdevOnError) != sizeof(int));
+
+PropertyInfo qdev_prop_blockdev_on_error = {
+.name = "BlockdevOnError",
+.description = "Error handling policy, "
+   "report/ignore/enospc/stop/auto",
+.enum_table = BlockdevOnError_lookup,
+.get = get_enum,
+.set = set_enum,
+};
+
 /* --- BIOS CHS translation */
 
 QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) != sizeof(int));
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index e2badc3..5b11f22 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -264,6 +264,7 @@ static int ide_drive_initfn(IDEDevice *dev)
 
 #define DEFINE_IDE_DEV_PROPERTIES() \
 DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
 DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
 DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),\
 DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8c26a4e..8dbfc10 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2849,6 +2849,7 @@ static const TypeInfo scsi_disk_base_info = {
 
 #define DEFINE_SCSI_DISK_PROPERTIES()\
 DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),   \
+DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf), \
 DEFINE_PROP_STRING("ver", SCSIDiskState, version),   \
 DEFINE_PROP_STRING("serial", 

[Qemu-block] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/157 | 92 ++
 tests/qemu-iotests/157.out | 22 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 115 insertions(+)
 create mode 100755 tests/qemu-iotests/157
 create mode 100644 tests/qemu-iotests/157.out

diff --git a/tests/qemu-iotests/157 b/tests/qemu-iotests/157
new file mode 100755
index 000..956cbdb
--- /dev/null
+++ b/tests/qemu-iotests/157
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# Test command line configuration of block devices with qdev
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# 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 .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+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 file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nodefaults -nographic -monitor stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | 
_filter_generated_node_ids
+}
+
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Test uses IDE devices"
+fi
+
+size=128M
+drive="if=none,file="$TEST_IMG",driver=$IMGFMT"
+
+_make_test_img $size
+
+echo
+echo "=== Setting WCE with qdev and with manually created BB ==="
+echo
+
+# The qdev option takes precedence, but if it isn't given or 'auto', the BB
+# option is used instead.
+
+for cache in "writeback" "writethrough"; do
+for wce in "" ",write-cache=auto", ",write-cache=on", ",write-cache=off"; 
do
+echo "info block" \
+| run_qemu -drive "$drive,cache=$cache" \
+   -device "ide-hd,drive=none0$wce" \
+| grep -e "Testing" -e "Cache mode"
+done
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/157.out b/tests/qemu-iotests/157.out
new file mode 100644
index 000..2e41e83
--- /dev/null
+++ b/tests/qemu-iotests/157.out
@@ -0,0 +1,22 @@
+QA output created by 157
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+
+=== Setting WCE with qdev and with manually created BB ===
+
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback 
-device ide-hd,drive=none0
+Cache mode:   writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback 
-device ide-hd,drive=none0,write-cache=auto,
+Cache mode:   writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback 
-device ide-hd,drive=none0,write-cache=on,
+Cache mode:   writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback 
-device ide-hd,drive=none0,write-cache=off
+Cache mode:   writethrough
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough 
-device ide-hd,drive=none0
+Cache mode:   writethrough
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough 
-device ide-hd,drive=none0,write-cache=auto,
+Cache mode:   writethrough
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough 
-device ide-hd,drive=none0,write-cache=on,
+Cache mode:   writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough 
-device ide-hd,drive=none0,write-cache=off
+Cache mode:   writethrough
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1c6fcb6..3a3973e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -156,3 +156,4 @@
 154 rw auto backing quick
 155 rw auto
 156 rw auto quick
+157 auto
-- 
1.8.3.1

[Qemu-block] [PATCH v2 6/6] block: Remove BB options from blockdev-add

[Kevin Wolf]

werror/rerror are now available as qdev options. The stats-* options are
removed without an existing replacement; they should probably be
configurable with a separate QMP command like I/O throttling settings.

Removing id is left for another day because this involves updating
qemu-iotests cases to use node-name for everything. Before we can do
that, however, all QMP commands must support node-name.

Signed-off-by: Kevin Wolf 
---
 qapi/block-core.json | 18 +-
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6008986..88b78a7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2051,20 +2051,8 @@
 # @discard:   #optional discard-related options (default: ignore)
 # @cache: #optional cache-related options
 # @aio:   #optional AIO backend (default: threads)
-# @rerror:#optional how to handle read errors on the device
-# (default: report)
-# @werror:#optional how to handle write errors on the device
-# (default: enospc)
 # @read-only: #optional whether the block device should be read-only
 # (default: false)
-# @stats-account-invalid: #optional whether to include invalid
-# operations when computing last access statistics
-# (default: true) (Since 2.5)
-# @stats-account-failed: #optional whether to include failed
-# operations when computing latency and last
-# access statistics (default: true) (Since 2.5)
-# @stats-intervals: #optional list of intervals for collecting I/O
-#   statistics, in seconds (default: none) (Since 2.5)
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 # (default: off)
 #
@@ -2074,17 +2062,13 @@
 ##
 { 'union': 'BlockdevOptions',
   'base': { 'driver': 'BlockdevDriver',
+# TODO 'id' is a BB-level option, remove it
 '*id': 'str',
 '*node-name': 'str',
 '*discard': 'BlockdevDiscardOptions',
 '*cache': 'BlockdevCacheOptions',
 '*aio': 'BlockdevAioOptions',
-'*rerror': 'BlockdevOnError',
-'*werror': 'BlockdevOnError',
 '*read-only': 'bool',
-'*stats-account-invalid': 'bool',
-'*stats-account-failed': 'bool',
-'*stats-intervals': ['int'],
 '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
   'data': {
-- 
1.8.3.1

[Qemu-block] [PATCH v2 0/6] block: Move BB options from blockdev-add to qdev

[Kevin Wolf]

This series adds some device level options (write-cache, werror, rerror)
to qdev that used to be specified with -drive and made their way into
blockdev-add. They are at the same time removed from blockdev-add, which
is going to be changed in a later patch series so that it only creates
block nodes without a BlockBackend.

The -device drive=... option is also changed to allow specifying a
node-name rather than a BlockBackend name. In this case, an anonymous
BlockBackend is created internally.

Kevin Wolf (6):
  block/qdev: Allow node name for drive properties
  block/qdev: Allow configuring WCE with qdev properties
  commit: Fix use of error handling policy
  block/qdev: Allow configuring rerror/werror with qdev properties
  qemu-iotests: Test setting WCE with qdev
  block: Remove BB options from blockdev-add

 block/block-backend.c|  1 +
 block/commit.c   |  6 +--
 blockjob.c   |  1 +
 hw/block/block.c | 28 
 hw/block/nvme.c  |  1 +
 hw/block/virtio-blk.c|  2 +
 hw/core/qdev-properties-system.c | 22 --
 hw/core/qdev-properties.c| 13 ++
 hw/ide/qdev.c|  2 +
 hw/scsi/scsi-disk.c  |  2 +
 hw/usb/dev-storage.c |  1 +
 include/hw/block/block.h | 13 +-
 include/hw/qdev-properties.h |  4 ++
 qapi/block-core.json | 22 ++
 tests/qemu-iotests/157   | 92 
 tests/qemu-iotests/157.out   | 22 ++
 tests/qemu-iotests/group |  1 +
 17 files changed, 208 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/157
 create mode 100644 tests/qemu-iotests/157.out

-- 
1.8.3.1

[Qemu-block] [PATCH v2 2/6] block/qdev: Allow configuring WCE with qdev properties

[Kevin Wolf]

As cache.writeback is a BlockBackend property and as such more related
to the guest device than the BlockDriverState, we already removed it
from the blockdev-add interface. This patch adds the new way to set it,
as a qdev property of the corresponding guest device.

For example: -drive if=none,file=test.img,node-name=img
 -device ide-hd,drive=img,write-cache=off

Signed-off-by: Kevin Wolf 
---
 hw/block/block.c | 16 
 hw/block/nvme.c  |  1 +
 hw/block/virtio-blk.c|  1 +
 hw/ide/qdev.c|  1 +
 hw/scsi/scsi-disk.c  |  1 +
 hw/usb/dev-storage.c |  1 +
 include/hw/block/block.h |  5 -
 7 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 97a59d4..396b0d5 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -51,6 +51,22 @@ void blkconf_blocksizes(BlockConf *conf)
 }
 }
 
+void blkconf_apply_backend_options(BlockConf *conf)
+{
+BlockBackend *blk = conf->blk;
+bool wce;
+
+switch (conf->wce) {
+case ON_OFF_AUTO_ON:wce = true; break;
+case ON_OFF_AUTO_OFF:   wce = false; break;
+case ON_OFF_AUTO_AUTO:  wce = blk_enable_write_cache(blk); break;
+default:
+abort();
+}
+
+blk_set_enable_write_cache(blk, wce);
+}
+
 void blkconf_geometry(BlockConf *conf, int *ptrans,
   unsigned cyls_max, unsigned heads_max, unsigned secs_max,
   Error **errp)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9faad29..8234669 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -803,6 +803,7 @@ static int nvme_init(PCIDevice *pci_dev)
 return -1;
 }
 blkconf_blocksizes(>conf);
+blkconf_apply_backend_options(>conf);
 
 pci_conf = pci_dev->config;
 pci_conf[PCI_INTERRUPT_PIN] = 1;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index ae86e94..5eb6ba1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -904,6 +904,7 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 blkconf_blocksizes(>conf);
+blkconf_apply_backend_options(>conf);
 
 virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
 sizeof(struct virtio_blk_config));
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6842a55..e2badc3 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -180,6 +180,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
 return -1;
 }
 }
+blkconf_apply_backend_options(>conf);
 
 if (ide_init_drive(s, dev->conf.blk, kind,
dev->version, dev->serial, dev->model, dev->wwn,
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 36f8a85..8c26a4e 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2309,6 +2309,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
 return;
 }
 }
+blkconf_apply_backend_options(>conf);
 
 if (s->qdev.conf.discard_granularity == -1) {
 s->qdev.conf.discard_granularity =
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4d605b8..5938c59 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -603,6 +603,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error 
**errp)
 
 blkconf_serial(>conf, >serial);
 blkconf_blocksizes(>conf);
+blkconf_apply_backend_options(>conf);
 
 /*
  * Hack alert: this pretends to be a block device, but it's really
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 984660e..e511dc3 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -25,6 +25,7 @@ typedef struct BlockConf {
 uint32_t discard_granularity;
 /* geometry, not all devices use this */
 uint32_t cyls, heads, secs;
+OnOffAuto wce;
 } BlockConf;
 
 static inline unsigned int get_physical_block_exp(BlockConf *conf)
@@ -49,7 +50,8 @@ static inline unsigned int get_physical_block_exp(BlockConf 
*conf)
 DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
 DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),\
 DEFINE_PROP_UINT32("discard_granularity", _state, \
-   _conf.discard_granularity, -1)
+   _conf.discard_granularity, -1), \
+DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, ON_OFF_AUTO_AUTO)
 
 #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)  \
 DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
@@ -63,6 +65,7 @@ void blkconf_geometry(BlockConf *conf, int *trans,
   unsigned cyls_max, unsigned heads_max, unsigned secs_max,
   Error **errp);
 void blkconf_blocksizes(BlockConf *conf);
+void blkconf_apply_backend_options(BlockConf *conf);
 
 /* Hard disk geometry */
 
-- 
1.8.3.1

[Qemu-block] [PATCH v2 3/6] commit: Fix use of error handling policy

[Kevin Wolf]

Commit implemented the 'enospc' policy as 'ignore' if the error was not
ENOSPC. The QAPI documentation promises that it's treated as 'stop'.
Using the common block job error handling function fixes this.

Signed-off-by: Kevin Wolf 
---
 block/commit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 379efb7..cb6810d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -171,9 +171,9 @@ wait:
 bytes_written += n * BDRV_SECTOR_SIZE;
 }
 if (ret < 0) {
-if (s->on_error == BLOCKDEV_ON_ERROR_STOP ||
-s->on_error == BLOCKDEV_ON_ERROR_REPORT||
-(s->on_error == BLOCKDEV_ON_ERROR_ENOSPC && ret == -ENOSPC)) {
+BlockErrorAction action =
+block_job_error_action(>common, false, s->on_error, -ret);
+if (action == BLOCK_ERROR_ACTION_REPORT) {
 goto out;
 } else {
 n = 0;
-- 
1.8.3.1

[Qemu-block] [PATCH v2 1/6] block/qdev: Allow node name for drive properties

[Kevin Wolf]

If a node name instead of a BlockBackend name is specified as the driver
for a guest device, an anonymous BlockBackend is created now.

Signed-off-by: Kevin Wolf 
---
 hw/core/qdev-properties-system.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index df38b8a..69b451e 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -72,12 +72,21 @@ static void parse_drive(DeviceState *dev, const char *str, 
void **ptr,
 const char *propname, Error **errp)
 {
 BlockBackend *blk;
+bool blk_created = false;
 
 blk = blk_by_name(str);
 if (!blk) {
+BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
+if (bs) {
+blk = blk_new();
+blk_insert_bs(blk, bs);
+blk_created = true;
+}
+}
+if (!blk) {
 error_setg(errp, "Property '%s.%s' can't find value '%s'",
object_get_typename(OBJECT(dev)), propname, str);
-return;
+goto fail;
 }
 if (blk_attach_dev(blk, dev) < 0) {
 DriveInfo *dinfo = blk_legacy_dinfo(blk);
@@ -91,9 +100,16 @@ static void parse_drive(DeviceState *dev, const char *str, 
void **ptr,
 error_setg(errp, "Drive '%s' is already in use by another device",
str);
 }
-return;
+goto fail;
 }
+
 *ptr = blk;
+
+fail:
+if (blk_created) {
+/* If we need to keep a reference, blk_attach_dev() took it */
+blk_unref(blk);
+}
 }
 
 static void release_drive(Object *obj, const char *name, void *opaque)
@@ -127,7 +143,7 @@ static void set_drive(Object *obj, Visitor *v, const char 
*name, void *opaque,
 
 PropertyInfo qdev_prop_drive = {
 .name  = "str",
-.description = "ID of a drive to use as a backend",
+.description = "Node name or ID of a block device to use as a backend",
 .get   = get_drive,
 .set   = set_drive,
 .release = release_drive,
-- 
1.8.3.1

Re: [Qemu-block] [PATCH v2 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct

[Alberto Garcia]

On Wed 29 Jun 2016 07:20:55 PM CEST, Max Reitz wrote:
 I thought adding a new 'ID' field was simpler. The device name is
 still a device name (where it makes sense). The default ID is
 guaranteed to be valid and guaranteed not to clash with
 user-defined IDs. The API is (in my opinion) more clear.

 The only problems that I can think of:

 - BlockJobInfo and the events expose the 'device' field which is
   superfluous.
 - block-job-{pause,resume,...} can take an ID or a device name.
>>>
>>> Yes. There are two parts that I don't like about this.
>>>
>>> The first one is that we need additional code to keep track of the
>>> device name and to look it up.
>> 
>> I think this part is negligible, but ok.
>> 
>>> The other, more important one is that it couples block jobs more
>>> tightly with a BDS:
>>>
>>> * What do you with a background job that doesn't have a device name
>>>   because it doesn't work on a BDS? Will 'device' become optional
>>>   everywhere? But how is this less problematic for compatibility than
>>>   returning non-device-name IDs? (To be clear, I don't think either is
>>>   a real problem, but you can hardly dismiss one and accept the
>>>   other.)
>> 
>>> * And what do you do once we allow more than one job per device? Then
>>>   the device name isn't suitable for addressing the job any more. And
>>>   letting the client use the device name after it started the first
>>>   job, but not any more after it started the second one, feels wrong.
>> 
>> Fair enough. Unless Max, Eric or someone else has something else to add
>> I'll give it a try and see how it looks.
>
> Sorry for the late response, but then again I don't actually have an
> opinion either way.
>
> The thing I feel most strongly about is the issue of storing a general
> ID in a field named "device". However, as Kevin hinted at this
> becoming irrelevant with John's work on decoupling block jobs from the
> block layer.

I actually forgot to Cc him, I'm doing it now.

The idea is that I don't want to add anything now that is going to cause
headaches later. Adding a new 'id' field to block jobs and keeping
'device' feels more natural to me, but reusing the 'device' field and
allowing any ID set by the user requires less changes both to the code
and the API.

Berto

[Qemu-block] [PATCH] block/iscsi: precache the allocation status of a target

[Peter Lieven]

this fills up the allocationmap at iscsi_open. This helps
to reduce the number of get_block_status requests during runtime
significantly.

Signed-off-by: Peter Lieven 
---
 block/iscsi.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 0cdcedb..04fb0a3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1774,6 +1774,22 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
  iscsilun->block_size) >> BDRV_SECTOR_BITS;
 if (iscsilun->lbprz) {
 ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
+if (ret == 0) {
+unsigned int max_reqs = 64;
+int64_t sector_num = 0;
+while (max_reqs-- && sector_num < bs->total_sectors) {
+int n;
+BlockDriverState *file;
+ret = bdrv_get_block_status(bs, sector_num,
+BDRV_REQUEST_MAX_SECTORS,
+, );
+if (ret < 0) {
+break;
+}
+sector_num += n;
+ret = 0;
+}
+}
 }
 }
 
-- 
1.9.1

[Qemu-block] [PATCH V4] block/iscsi: allow caching of the allocation map

[Peter Lieven]

until now the allocation map was used only as a hint if a cluster
is allocated or not. If a block was not allocated (or Qemu had
no info about the allocation status) a get_block_status call was
issued to check the allocation status and possibly avoid
a subsequent read of unallocated sectors. If a block known to be
allocated the get_block_status call was omitted. In the other case
a get_block_status call was issued before every read to avoid
the necessity for a consistent allocation map. To avoid the
potential overhead of calling get_block_status for each and
every read request this took only place for the bigger requests.

This patch enhances this mechanism to cache the allocation
status and avoid calling get_block_status for blocks where
the allocation status has been queried before. This allows
for bypassing the read request even for smaller requests and
additionally omits calling get_block_status for known to be
unallocated blocks.

Signed-off-by: Peter Lieven 
---
v3->v4: - added comment to iscsi_allocmap_set_unallocated [Paolo]
- rebased to current master
v2->v3: - fix wording errors [Fam]
- reinit allocmap only if allocmap is present in
  bdrv_reopen_commit
v1->v2: - add more comments [Fam]
- free allocmap if allocation of allocmap_valid fails [Fam]
- fix indent and whitespace errors [Fam]
- account for cache mode changes on reopen

 block/iscsi.c | 240 +-
 1 file changed, 188 insertions(+), 52 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 9bb5ff6..0cdcedb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2,7 +2,7 @@
  * QEMU Block driver for iSCSI images
  *
  * Copyright (c) 2010-2011 Ronnie Sahlberg 
- * Copyright (c) 2012-2015 Peter Lieven 
+ * Copyright (c) 2012-2016 Peter Lieven 
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to 
deal
@@ -62,7 +62,23 @@ typedef struct IscsiLun {
 struct scsi_inquiry_logical_block_provisioning lbp;
 struct scsi_inquiry_block_limits bl;
 unsigned char *zeroblock;
-unsigned long *allocationmap;
+/* The allocmap tracks which clusters (pages) on the iSCSI target are
+ * allocated and which are not. In case a target returns zeros for
+ * unallocated pages (iscsilun->lprz) we can directly return zeros instead
+ * of reading zeros over the wire if a read request falls within an
+ * unallocated block. As there are 3 possible states we need 2 bitmaps to
+ * track. allocmap_valid keeps track if QEMU's information about a page is
+ * valid. allocmap tracks if a page is allocated or not. In case QEMU has 
no
+ * valid information about a page the corresponding allocmap entry should 
be
+ * switched to unallocated as well to force a new lookup of the allocation
+ * status as lookups are generally skipped if a page is suspect to be
+ * allocated. If a iSCSI target is opened with cache.direct = on the
+ * allocmap_valid does not exist turning all cached information invalid so
+ * that a fresh lookup is made for any page even if allocmap entry returns
+ * it's unallocated. */
+unsigned long *allocmap;
+unsigned long *allocmap_valid;
+long allocmap_size;
 int cluster_sectors;
 bool use_16_for_rw;
 bool write_protected;
@@ -423,37 +439,135 @@ static bool is_sector_request_lun_aligned(int64_t 
sector_num, int nb_sectors,
iscsilun);
 }
 
-static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
+static void iscsi_allocmap_free(IscsiLun *iscsilun)
 {
-return bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
-   iscsilun),
-   iscsilun->cluster_sectors));
+g_free(iscsilun->allocmap);
+g_free(iscsilun->allocmap_valid);
+iscsilun->allocmap = NULL;
+iscsilun->allocmap_valid = NULL;
 }
 
-static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t sector_num,
-int nb_sectors)
+
+static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
 {
-if (iscsilun->allocationmap == NULL) {
-return;
+iscsi_allocmap_free(iscsilun);
+
+iscsilun->allocmap_size =
+DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks, iscsilun),
+ iscsilun->cluster_sectors);
+
+iscsilun->allocmap = bitmap_try_new(iscsilun->allocmap_size);
+if (!iscsilun->allocmap) {
+return -ENOMEM;
 }
-bitmap_set(iscsilun->allocationmap,
-   sector_num / iscsilun->cluster_sectors,
-   DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
+
+if (open_flags & BDRV_O_NOCACHE) {
+/* in case that cache.direct = on all allocmap 

Re: [Qemu-block] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset

[Denis V. Lunev]

On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:

After loading bitmap from image and setting IN_USE flag in it's header,
corresponding data (bitmap table and data clusters) becomes inconsistent
and is no longer needed. It is better to free bitmap table and
corresponding clusters from the image immediately after loading the
bitmap than free them when the bitmap is saved, or deleted or set
non-persistent.

For now it is impossible to store only bitmap header without bitmap
table, as specification requires it. Storing zeroed bitmap table (one or
more clusters) is the only option to implement the behaviour similar to
specified above.

The same problem is for just storing empty bitmaps.

This patch allows storing only bitmap header for empty bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Additional note. Should we also allow here bitmap_table_offset = 1, like
in bitmap table, for the bitmap with all bits set? I am not sure that it
is needed at all, but just to keep the company..

  docs/specs/qcow2.txt | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..9826222 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
  Offset into the image file at which the bitmap table
  (described below) for the bitmap starts. Must be aligned 
to
  a cluster boundary.
+Zero value means that bitmap table is not allocated and the
+bitmap should be considered as empty (all bits are zero).
  
   8 - 11:bitmap_table_size

-Number of entries in the bitmap table of the bitmap.
+Number of entries in the bitmap table of the bitmap. It
+must be zero if bitmap_table_offset is zero.
  
  12 - 15:flags

  Bit

NACK

no guys, we can not make this change at the moment.
We do have QEMU available in the field which is working
with the currently specified format.

Den

Re: [Qemu-block] [PATCH 1/1] spec/parallels: fix a mistake

[Stefan Hajnoczi]

On Thu, Jun 30, 2016 at 11:19:30AM +0300, Denis V. Lunev wrote:
> From: Vladimir Sementsov-Ogievskiy 
> 
> We have only one flag for now - Empty Image flag. The patch fixes unused
> bits specification and marks bit 1 as usused.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Denis V. Lunev 
> CC: Stefan Hajnoczi 
> ---
>  docs/specs/parallels.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

[Qemu-block] [PATCH 1/1] spec/parallels: fix a mistake

[Denis V. Lunev]

From: Vladimir Sementsov-Ogievskiy 

We have only one flag for now - Empty Image flag. The patch fixes unused
bits specification and marks bit 1 as usused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Stefan Hajnoczi 
---
 docs/specs/parallels.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/specs/parallels.txt b/docs/specs/parallels.txt
index b4fe229..e9271eb 100644
--- a/docs/specs/parallels.txt
+++ b/docs/specs/parallels.txt
@@ -94,7 +94,7 @@ Bytes:
   Bit 0: Empty Image bit. If set, the image should be
  considered clear.
 
-  Bits 2-31: Unused.
+  Bits 1-31: Unused.
 
   56 - 63:ext_off
   Format Extension offset, an offset, in sectors, from the start of
-- 
2.1.4

[Qemu-block] [PATCH] spec/parallels: fix a mistake

[Vladimir Sementsov-Ogievskiy]

We have only one flag for now - Empty Image flag.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/specs/parallels.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/specs/parallels.txt b/docs/specs/parallels.txt
index b4fe229..e9271eb 100644
--- a/docs/specs/parallels.txt
+++ b/docs/specs/parallels.txt
@@ -94,7 +94,7 @@ Bytes:
   Bit 0: Empty Image bit. If set, the image should be
  considered clear.
 
-  Bits 2-31: Unused.
+  Bits 1-31: Unused.
 
   56 - 63:ext_off
   Format Extension offset, an offset, in sectors, from the start of
-- 
1.8.3.1

[Qemu-block] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset

[Vladimir Sementsov-Ogievskiy]

After loading bitmap from image and setting IN_USE flag in it's header,
corresponding data (bitmap table and data clusters) becomes inconsistent
and is no longer needed. It is better to free bitmap table and
corresponding clusters from the image immediately after loading the
bitmap than free them when the bitmap is saved, or deleted or set
non-persistent.

For now it is impossible to store only bitmap header without bitmap
table, as specification requires it. Storing zeroed bitmap table (one or
more clusters) is the only option to implement the behaviour similar to
specified above.

The same problem is for just storing empty bitmaps.

This patch allows storing only bitmap header for empty bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Additional note. Should we also allow here bitmap_table_offset = 1, like
in bitmap table, for the bitmap with all bits set? I am not sure that it
is needed at all, but just to keep the company..

 docs/specs/qcow2.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..9826222 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
 Offset into the image file at which the bitmap table
 (described below) for the bitmap starts. Must be aligned to
 a cluster boundary.
+Zero value means that bitmap table is not allocated and the
+bitmap should be considered as empty (all bits are zero).
 
  8 - 11:bitmap_table_size
-Number of entries in the bitmap table of the bitmap.
+Number of entries in the bitmap table of the bitmap. It
+must be zero if bitmap_table_offset is zero.
 
 12 - 15:flags
 Bit
-- 
1.8.3.1