Re: [Qemu-block] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

On Sat 13 Apr 2019 05:48:11 PM CEST, Max Reitz  wrote:
>> Ok, you can leave out the second patch then. The first one should
>> still be correct, right?
>
> I just think it’s unnecessary because as of my series both
> backing_file and auto_backing_file serve the purpose.

Ok then!

Berto

Re: [Qemu-block] [PATCH v5 07/11] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy]

13.04.2019 19:08, Vladimir Sementsov-Ogievskiy wrote:
> 16.01.2019 19:02, Max Reitz wrote:
>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>> Backup-top filter does copy-before-write operation. It should be
>>> inserted above active disk and has a target node for CBW, like the
>>> following:
>>>
>>>  +---+
>>>  | Guest |
>>>  +---+---+
>>>  |r,w
>>>  v
>>>  +---+---+  target   +---+
>>>  | backup_top    |-->| target(qcow2) |
>>>  +---+---+   CBW +---+---+
>>>  |
>>> backing |r,w
>>>  v
>>>  +---+-+
>>>  | Active disk |
>>>  +-+
>>>
>>> The driver will be used in backup instead of write-notifiers.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/backup-top.h  |  43 +++
>>>   block/backup-top.c  | 306 
>>>   block/Makefile.objs |   2 +
>>>   3 files changed, 351 insertions(+)
>>>   create mode 100644 block/backup-top.h
>>>   create mode 100644 block/backup-top.c
>>
> 
> [..]
> 
>>> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>> + BlockDriverState *target,
>>> + HBitmap *copy_bitmap,
>>> + Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    BDRVBackupTopState *state;
>>> +    BlockDriverState *top = bdrv_new_open_driver(_backup_top_filter,
>>> + NULL, BDRV_O_RDWR, errp);
>>> +
>>> +    if (!top) {
>>> +    return NULL;
>>> +    }
>>> +
>>> +    top->implicit = true;
>>> +    top->total_sectors = source->total_sectors;
>>> +    top->opaque = state = g_new0(BDRVBackupTopState, 1);
>>> +    state->copy_bitmap = copy_bitmap;
>>> +
>>> +    bdrv_ref(target);
>>> +    state->target = bdrv_attach_child(top, target, "target", _file, 
>>> errp);
>>> +    if (!state->target) {
>>> +    bdrv_unref(target);
>>> +    bdrv_unref(top);
>>> +    return NULL;
>>> +    }
>>> +
>>> +    bdrv_set_aio_context(top, bdrv_get_aio_context(source));
>>> +    bdrv_set_aio_context(target, bdrv_get_aio_context(source));
>>> +
>>> +    bdrv_drained_begin(source);
>>> +
>>> +    bdrv_ref(top);
>>> +    bdrv_append(top, source, _err);
>>> +
>>> +    if (local_err) {
>>> +    bdrv_unref(top);
>>
>> This is done automatically by bdrv_append().
>>
>>> +    }
>>> +
>>> +    bdrv_drained_end(source);
>>> +
>>> +    if (local_err) {
>>> +    bdrv_unref_child(top, state->target);
>>> +    bdrv_unref(top);
>>> +    error_propagate(errp, local_err);
>>> +    return NULL;
>>> +    }
>>> +
>>> +    return top;
>>> +}
>>> +
>>> +void bdrv_backup_top_drop(BlockDriverState *bs)
>>> +{
>>> +    BDRVBackupTopState *s = bs->opaque;
>>> +
>>> +    AioContext *aio_context = bdrv_get_aio_context(bs);
>>> +
>>> +    aio_context_acquire(aio_context);
>>> +
>>> +    bdrv_drained_begin(bs);
>>> +
>>> +    bdrv_child_try_set_perm(bs->backing, 0, BLK_PERM_ALL, _abort);
>>> +    bdrv_replace_node(bs, backing_bs(bs), _abort);
>>> +    bdrv_set_backing_hd(bs, NULL, _abort);
>>
>> This is done automatically in bdrv_close(), and after bs has been
>> replaced by backing_bs(bs), I don't think new requests should come in,
>> so I don't think this needs to be done here.
> 
> Following movement of backup_top back to job->blk becomes impossible then,
> if we don't share WRITE on source in backup_top_child_perm.
> 
> And I think, this function should drop all relations created by
> bdrv_backup_top_append.
> 
>>
>>> +
>>> +    bdrv_drained_end(bs);
>>> +
>>> +    if (s->target) {
>>> +    bdrv_unref_child(bs, s->target);
>>> +    }
>>
>> And this should be done in a .bdrv_close() implementation, I think.
>>

and therefore this one too. We don't have .bdrv_open, so I'd prefer not
have bdrv_close. We create this child in _top_append, seems logical to
unref it in _top_drop.


-- 
Best regards,
Vladimir

[Qemu-block] [PATCH v3 7/7] iotests: Test qemu-img convert -C --salvage

[Max Reitz]

We do not support this combination (yet), so this should yield an error
message.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/082 | 1 +
 tests/qemu-iotests/082.out | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index d0afa46e9a..c58b37127f 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -163,6 +163,7 @@ echo === convert: -C and other options ===
 run_qemu_img convert -C -S 4k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
 run_qemu_img convert -C -S 8k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
 run_qemu_img convert -C -c -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+run_qemu_img convert -C --salvage -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
 
 echo
 echo === amend: Options specified more than once ===
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index d36938da9b..9d4f7574c9 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -555,6 +555,9 @@ qemu-img: Cannot enable copy offloading when -S is used
 Testing: convert -C -c -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
 qemu-img: Cannot enable copy offloading when -c is used
 
+Testing: convert -C --salvage -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot use copy offloading in salvaging mode
+
 === amend: Options specified more than once ===
 
 Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
-- 
2.20.1

[Qemu-block] [PATCH v3 5/7] blkdebug: Inject errors on .bdrv_co_block_status()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 qapi/block-core.json | 5 -
 block/blkdebug.c | 8 
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 717b13f7f5..2aa675a192 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3252,10 +3252,13 @@
 #
 # @flush: .bdrv_co_flush_to_disk()
 #
+# @block-status: .bdrv_co_block_status()
+#
 # Since: 4.1
 ##
 { 'enum': 'BlkdebugIOType',
-  'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush' ] }
+  'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush',
+'block-status' ] }
 
 ##
 # @BlkdebugInjectErrorOptions:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 69c608be6f..df9c8b1673 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -670,7 +670,15 @@ static int coroutine_fn 
blkdebug_co_block_status(BlockDriverState *bs,
  int64_t *map,
  BlockDriverState **file)
 {
+int err;
+
 assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
+
+err = rule_check(bs, offset, bytes, BLKDEBUGIO_TYPE_BLOCK_STATUS);
+if (err) {
+return err;
+}
+
 return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes,
   pnum, map, file);
 }
-- 
2.20.1

[Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option

[Max Reitz]

This new error option allows users of blkdebug to inject errors only on
certain kinds of I/O operations.  Users usually want to make a very
specific operation fail, not just any; but right now they simply hope
that the event that triggers the error injection is followed up with
that very operation.  That may not be true, however, because the block
layer is changing (including blkdebug, which may increase the number of
types of I/O operations on which to inject errors).

The new option's default has been chosen to keep backwards
compatibility.

Note that similar to the internal representation, we could choose to
expose this option as a list of I/O types.  But there is no practical
use for this, because as described above, users usually know exactly
which kind of operation they want to make fail, so there is no need to
specify multiple I/O types at once.  In addition, exposing this option
as a list would require non-trivial changes to qemu_opts_absorb_qdict().

Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 26 +++
 block/blkdebug.c | 50 
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..5141e91172 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3235,6 +3235,26 @@
 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
 'cor_write'] }
 
+##
+# @BlkdebugIOType:
+#
+# Kinds of I/O that blkdebug can inject errors in.
+#
+# @read: .bdrv_co_preadv()
+#
+# @write: .bdrv_co_pwritev()
+#
+# @write-zeroes: .bdrv_co_pwrite_zeroes()
+#
+# @discard: .bdrv_co_pdiscard()
+#
+# @flush: .bdrv_co_flush_to_disk()
+#
+# Since: 4.1
+##
+{ 'enum': 'BlkdebugIOType',
+  'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush' ] }
+
 ##
 # @BlkdebugInjectErrorOptions:
 #
@@ -3245,6 +3265,11 @@
 # @state:   the state identifier blkdebug needs to be in to
 #   actually trigger the event; defaults to "any"
 #
+# @iotype:  the type of I/O operations on which this error should
+#   be injected; defaults to "all read, write,
+#   write-zeroes, discard, and flush operations"
+#   (since: 4.1)
+#
 # @errno:   error identifier (errno) to be returned; defaults to
 #   EIO
 #
@@ -3262,6 +3287,7 @@
 { 'struct': 'BlkdebugInjectErrorOptions',
   'data': { 'event': 'BlkdebugEvent',
 '*state': 'int',
+'*iotype': 'BlkdebugIOType',
 '*errno': 'int',
 '*sector': 'int',
 '*once': 'bool',
diff --git a/block/blkdebug.c b/block/blkdebug.c
index efd9441625..ca84b12e32 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -75,6 +75,7 @@ typedef struct BlkdebugRule {
 int state;
 union {
 struct {
+uint64_t iotype_mask;
 int error;
 int immediately;
 int once;
@@ -91,6 +92,9 @@ typedef struct BlkdebugRule {
 QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
 } BlkdebugRule;
 
+QEMU_BUILD_BUG_MSG(BLKDEBUGIO_TYPE__MAX > 64,
+   "BlkdebugIOType mask does not fit into an uint64_t");
+
 static QemuOptsList inject_error_opts = {
 .name = "inject-error",
 .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head),
@@ -103,6 +107,10 @@ static QemuOptsList inject_error_opts = {
 .name = "state",
 .type = QEMU_OPT_NUMBER,
 },
+{
+.name = "iotype",
+.type = QEMU_OPT_STRING,
+},
 {
 .name = "errno",
 .type = QEMU_OPT_NUMBER,
@@ -162,6 +170,8 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 int event;
 struct BlkdebugRule *rule;
 int64_t sector;
+BlkdebugIOType iotype;
+Error *local_error = NULL;
 
 /* Find the right event for the rule */
 event_name = qemu_opt_get(opts, "event");
@@ -192,6 +202,26 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 sector = qemu_opt_get_number(opts, "sector", -1);
 rule->options.inject.offset =
 sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
+
+iotype = qapi_enum_parse(_lookup,
+ qemu_opt_get(opts, "iotype"),
+ BLKDEBUGIO_TYPE__MAX, _error);
+if (local_error) {
+error_propagate(errp, local_error);
+return -1;
+}
+if (iotype != BLKDEBUGIO_TYPE__MAX) {
+rule->options.inject.iotype_mask = (1ull << iotype);
+} else {
+/* Apply the default */
+rule->options.inject.iotype_mask =
+(1ull << BLKDEBUGIO_TYPE_READ)
+| (1ull << BLKDEBUGIO_TYPE_WRITE)
+| (1ull << BLKDEBUGIO_TYPE_WRITE_ZEROES)
+| (1ull << BLKDEBUGIO_TYPE_DISCARD)
+| (1ull << BLKDEBUGIO_TYPE_FLUSH);
+}
+
 break;
 
 case 

[Qemu-block] [PATCH v3 4/7] blkdebug: Add "none" event

[Max Reitz]

Together with @iotypes and @sector, this can be used to trap e.g. the
first read or write access to a certain sector without having to know
what happens internally in the block layer, i.e. which "real" events
happen right before such an access.

Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 4 +++-
 block/blkdebug.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5141e91172..717b13f7f5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3215,6 +3215,8 @@
 #
 # @cor_write: a write due to copy-on-read (since 2.11)
 #
+# @none: triggers once at creation of the blkdebug node (since 4.1)
+#
 # Since: 2.9
 ##
 { 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
@@ -3233,7 +3235,7 @@
 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
-'cor_write'] }
+'cor_write', 'none' ] }
 
 ##
 # @BlkdebugIOType:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index ca84b12e32..69c608be6f 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -491,6 +491,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto out;
 }
 
+bdrv_debug_event(bs, BLKDBG_NONE);
+
 ret = 0;
 out:
 if (ret < 0) {
-- 
2.20.1

[Qemu-block] [PATCH v3 6/7] iotests: Test qemu-img convert --salvage

[Max Reitz]

This test converts a simple image to another, but blkdebug injects
block_status and read faults at some offsets.  The resulting image
should be the same as the input image, except that sectors that could
not be read have to be 0.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/249 | 163 +
 tests/qemu-iotests/249.out |  43 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 207 insertions(+)
 create mode 100755 tests/qemu-iotests/249
 create mode 100644 tests/qemu-iotests/249.out

diff --git a/tests/qemu-iotests/249 b/tests/qemu-iotests/249
new file mode 100755
index 00..d3883d75f3
--- /dev/null
+++ b/tests/qemu-iotests/249
@@ -0,0 +1,163 @@
+#!/bin/bash
+#
+# Test qemu-img convert --salvage
+#
+# Copyright (C) 2018 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=mre...@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
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+
+TEST_IMG="$TEST_IMG.orig" _make_test_img 64M
+
+$QEMU_IO -c 'write -P 42 0 64M' "$TEST_IMG.orig" | _filter_qemu_io
+
+
+sector_size=512
+
+# Offsets on which to fail block-status.  Keep in ascending order so
+# the indexing done by _filter_offsets will appear in ascending order
+# in the output as well.
+status_fail_offsets="$((16 * 1024 * 1024 + 8192))
+ $((33 * 1024 * 1024 + 512))"
+
+# Offsets on which to fail reads.  Keep in ascending order for the
+# same reason.
+# Element 1 is shared with $status_fail_offsets on purpose.
+# Elements 2 and later test what happens when a continuous range of
+# sectors is inaccessible.
+read_fail_offsets="$((32 * 1024 * 1024 - 65536))
+   $((33 * 1024 * 1024 + 512))
+   $(seq $((34 * 1024 * 1024)) $sector_size \
+ $((34 * 1024 * 1024 + 4096 - $sector_size)))"
+
+
+# blkdebug must be above the format layer so it can intercept all
+# block-status events
+source_img="json:{'driver': 'blkdebug',
+  'image': {
+  'driver': '$IMGFMT',
+  'file': {
+  'driver': 'file',
+  'filename': '$TEST_IMG.orig'
+  }
+  },
+  'inject-error': ["
+
+for ofs in $status_fail_offsets
+do
+source_img+="{ 'event': 'none',
+   'iotype': 'block-status',
+   'errno': 5,
+   'sector': $((ofs / sector_size)) },"
+done
+
+for ofs in $read_fail_offsets
+do
+source_img+="{ 'event': 'none',
+   'iotype': 'read',
+   'errno': 5,
+   'sector': $((ofs / sector_size)) },"
+done
+
+# Remove the trailing comma and terminate @inject-error and json:{}
+source_img="${source_img%,} ] }"
+
+
+echo
+
+
+_filter_offsets() {
+filters=
+
+index=0
+for ofs in $2
+do
+filters+=" -e s/$(printf "$1" $ofs)/status_fail_offset_$index/"
+index=$((index + 1))
+done
+
+index=0
+for ofs in $3
+do
+filters+=" -e s/$(printf "$1" $ofs)/read_fail_offset_$index/"
+index=$((index + 1))
+done
+
+sed $filters
+}
+
+# While determining the number of allocated sectors in the input
+# image, we should see one block status warning per element of
+# $status_fail_offsets.
+#
+# Then, the image is read.  Since the block status is queried in
+# basically the same way, the same warnings as in the previous step
+# should reappear.  Interleaved with those we should see a read
+# warning per element of $read_fail_offsets.
+# Note that $read_fail_offsets and $status_fail_offsets share an
+# element (read_fail_offset_1 == status_fail_offset_1), so
+# "status_fail_offset_1" in the output is the same as
+# "read_fail_offset_1".
+$QEMU_IMG convert --salvage "$source_img" "$TEST_IMG" 2>&1 \
+| _filter_offsets '%i' "$status_fail_offsets" "$read_fail_offsets"
+
+echo
+
+# The offsets where the block status could not be determined should
+# have been treated as containing data and thus should be correct in
+# the output 

[Qemu-block] [PATCH v3 0/7] qemu-img: Add salvaging mode to convert

[Max Reitz]

Hi,

This series adds a --salvage option to qemu-img convert.  With this,
qemu-img will not abort when it encounters an I/O error.  Instead, it
tries to narrow it down and will treat the affected sectors as being
completely 0 (and print a warning).

Testing this is not so easy, because while real I/O errors during read
operations should be treated as described above, errors encountered
during bdrv_block_status() should just be ignored and the affected
sectors should be considered allocated.  But blkdebug does not yet have
a way to intercept this, and:

(1) Just adding a new block-status event would be silly, because I don't
want an event, I want it to fail on a certain kind of operation, on
a certain sector range, independently of any events, so why can't we
just do that?  See patch 4.

(2) If we just make blkdebug intercept .bdrv_co_block_status() like all
other kinds of operations, at least iotest 041 fails, which does
exactly that silly thing: It uses the read_aio event to wait for any
read.  But it turns out that there may be a bdrv_*block_status()
call in between, so suddenly the wrong operation yields an error.
As I said, the real fault here is that it does not really make sense
to pray that the operation you want to fail is the one that is
immediately executed after some event that you hope will trigger
that operation.
See patch 3.

So patch 3 allows blkdebug users to select which kind of I/O operation
they actually want to make fail, and patch 4 allows them to not use any
event, but to have a rule active all the time.

Together, we can then enable error injection for block-status in patch 5
and make use of event=none iotype=block-status in patch 6.


v3:
- Patch 2: Forbid combination of -C and --salvage instead of noting that
   it does nothing [Vladimir]
  (Also, a rebase conflict)
- Patch 7 (new): Test that


git-backport-diff against v3:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[] [--] 'qemu-img: Move quiet into ImgConvertState'
002/7:[0012] [FC] 'qemu-img: Add salvaging mode to convert'
003/7:[] [--] 'blkdebug: Add @iotype error option'
004/7:[] [--] 'blkdebug: Add "none" event'
005/7:[] [--] 'blkdebug: Inject errors on .bdrv_co_block_status()'
006/7:[] [--] 'iotests: Test qemu-img convert --salvage'
007/7:[down] 'iotests: Test qemu-img convert -C --salvage'


Max Reitz (7):
  qemu-img: Move quiet into ImgConvertState
  qemu-img: Add salvaging mode to convert
  blkdebug: Add @iotype error option
  blkdebug: Add "none" event
  blkdebug: Inject errors on .bdrv_co_block_status()
  iotests: Test qemu-img convert --salvage
  iotests: Test qemu-img convert -C --salvage

 qapi/block-core.json   |  33 +++-
 block/blkdebug.c   |  60 --
 qemu-img.c | 103 +--
 qemu-img-cmds.hx   |   4 +-
 qemu-img.texi  |   4 +
 tests/qemu-iotests/082 |   1 +
 tests/qemu-iotests/082.out |   3 +
 tests/qemu-iotests/249 | 163 +
 tests/qemu-iotests/249.out |  43 ++
 tests/qemu-iotests/group   |   1 +
 10 files changed, 376 insertions(+), 39 deletions(-)
 create mode 100755 tests/qemu-iotests/249
 create mode 100644 tests/qemu-iotests/249.out

-- 
2.20.1

[Qemu-block] [PATCH v3 1/7] qemu-img: Move quiet into ImgConvertState

[Max Reitz]

Move img_convert()'s quiet flag into the ImgConvertState so it is
accessible by nested functions.  -q dictates that it suppresses anything
but errors, so if those functions want to emit warnings, they need to
query this flag first.  (There currently are no such warnings, but there
will be as of the next patch.)

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-img.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index aa6f81f1ea..c53666aa41 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1569,6 +1569,7 @@ typedef struct ImgConvertState {
 int64_t target_backing_sectors; /* negative if unknown */
 bool wr_in_order;
 bool copy_range;
+bool quiet;
 int min_sparse;
 int alignment;
 size_t cluster_sectors;
@@ -2005,7 +2006,7 @@ static int img_convert(int argc, char **argv)
 QDict *open_opts = NULL;
 char *options = NULL;
 Error *local_err = NULL;
-bool writethrough, src_writethrough, quiet = false, image_opts = false,
+bool writethrough, src_writethrough, image_opts = false,
  skip_create = false, progress = false, tgt_image_opts = false;
 int64_t ret = -EINVAL;
 bool force_share = false;
@@ -2113,7 +2114,7 @@ static int img_convert(int argc, char **argv)
 src_cache = optarg;
 break;
 case 'q':
-quiet = true;
+s.quiet = true;
 break;
 case 'n':
 skip_create = true;
@@ -2202,7 +2203,7 @@ static int img_convert(int argc, char **argv)
 }
 
 /* Initialize before goto out */
-if (quiet) {
+if (s.quiet) {
 progress = false;
 }
 qemu_progress_init(progress, 1.0);
@@ -2213,7 +2214,7 @@ static int img_convert(int argc, char **argv)
 
 for (bs_i = 0; bs_i < s.src_num; bs_i++) {
 s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
-   fmt, src_flags, src_writethrough, quiet,
+   fmt, src_flags, src_writethrough, s.quiet,
force_share);
 if (!s.src[bs_i]) {
 ret = -1;
@@ -2376,7 +2377,7 @@ static int img_convert(int argc, char **argv)
 
 if (skip_create) {
 s.target = img_open(tgt_image_opts, out_filename, out_fmt,
-flags, writethrough, quiet, false);
+flags, writethrough, s.quiet, false);
 } else {
 /* TODO ultimately we should allow --target-image-opts
  * to be used even when -n is not given.
@@ -2384,7 +2385,7 @@ static int img_convert(int argc, char **argv)
  * to allow filenames in option syntax
  */
 s.target = img_open_file(out_filename, open_opts, out_fmt,
- flags, writethrough, quiet, false);
+ flags, writethrough, s.quiet, false);
 open_opts = NULL; /* blk_new_open will have freed it */
 }
 if (!s.target) {
-- 
2.20.1

[Qemu-block] [PATCH v3 2/7] qemu-img: Add salvaging mode to convert

[Max Reitz]

This adds a salvaging mode (--salvage) to qemu-img convert which ignores
read errors and treats the respective areas as containing only zeroes.
This can be used for instance to at least partially recover the data
from terminally corrupted qcow2 images.

Signed-off-by: Max Reitz 
---
 qemu-img.c   | 90 +---
 qemu-img-cmds.hx |  4 +--
 qemu-img.texi|  4 +++
 3 files changed, 75 insertions(+), 23 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c53666aa41..1637ab9ac7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -66,6 +66,7 @@ enum {
 OPTION_SIZE = 264,
 OPTION_PREALLOCATION = 265,
 OPTION_SHRINK = 266,
+OPTION_SALVAGE = 267,
 };
 
 typedef enum OutputFormat {
@@ -1569,6 +1570,7 @@ typedef struct ImgConvertState {
 int64_t target_backing_sectors; /* negative if unknown */
 bool wr_in_order;
 bool copy_range;
+bool salvage;
 bool quiet;
 int min_sparse;
 int alignment;
@@ -1616,25 +1618,44 @@ static int convert_iteration_sectors(ImgConvertState 
*s, int64_t sector_num)
 }
 
 if (s->sector_next_status <= sector_num) {
-int64_t count = n * BDRV_SECTOR_SIZE;
+uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE;
+int64_t count;
 
-if (s->target_has_backing) {
+do {
+count = n * BDRV_SECTOR_SIZE;
+
+if (s->target_has_backing) {
+ret = bdrv_block_status(blk_bs(s->src[src_cur]), offset,
+count, , NULL, NULL);
+} else {
+ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
+  offset, count, , NULL,
+  NULL);
+}
+
+if (ret < 0) {
+if (s->salvage) {
+if (n == 1) {
+if (!s->quiet) {
+warn_report("error while reading block status at "
+"offset %" PRIu64 ": %s", offset,
+strerror(-ret));
+}
+/* Just try to read the data, then */
+ret = BDRV_BLOCK_DATA;
+count = BDRV_SECTOR_SIZE;
+} else {
+/* Retry on a shorter range */
+n = DIV_ROUND_UP(n, 4);
+}
+} else {
+error_report("error while reading block status at offset "
+ "%" PRIu64 ": %s", offset, strerror(-ret));
+return ret;
+}
+}
+} while (ret < 0);
 
-ret = bdrv_block_status(blk_bs(s->src[src_cur]),
-(sector_num - src_cur_offset) *
-BDRV_SECTOR_SIZE,
-count, , NULL, NULL);
-} else {
-ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
-  (sector_num - src_cur_offset) *
-  BDRV_SECTOR_SIZE,
-  count, , NULL, NULL);
-}
-if (ret < 0) {
-error_report("error while reading block status of sector %" PRId64
- ": %s", sector_num, strerror(-ret));
-return ret;
-}
 n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
 
 if (ret & BDRV_BLOCK_ZERO) {
@@ -1671,6 +1692,7 @@ static int convert_iteration_sectors(ImgConvertState *s, 
int64_t sector_num)
 static int coroutine_fn convert_co_read(ImgConvertState *s, int64_t sector_num,
 int nb_sectors, uint8_t *buf)
 {
+uint64_t single_read_until = 0;
 int n, ret;
 QEMUIOVector qiov;
 
@@ -1679,6 +1701,7 @@ static int coroutine_fn convert_co_read(ImgConvertState 
*s, int64_t sector_num,
 BlockBackend *blk;
 int src_cur;
 int64_t bs_sectors, src_cur_offset;
+uint64_t offset;
 
 /* In the case of compression with multiple source files, we can get a
  * nb_sectors that spreads into the next part. So we must be able to
@@ -1687,14 +1710,30 @@ static int coroutine_fn convert_co_read(ImgConvertState 
*s, int64_t sector_num,
 blk = s->src[src_cur];
 bs_sectors = s->src_sectors[src_cur];
 
+offset = (sector_num - src_cur_offset) << BDRV_SECTOR_BITS;
+
 n = MIN(nb_sectors, bs_sectors - (sector_num - src_cur_offset));
+if (single_read_until > offset) {
+n = 1;
+}
 qemu_iovec_init_buf(, buf, n << BDRV_SECTOR_BITS);
 
-ret = blk_co_preadv(
-blk, (sector_num - src_cur_offset) << BDRV_SECTOR_BITS,
-n << BDRV_SECTOR_BITS, , 0);
+ret = 

Re: [Qemu-block] [PATCH v3 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Max Reitz]

On 25.02.19 20:08, Max Reitz wrote:
> This series implements .bdrv_refresh_filename() for the ssh block
> driver, along with an appropriate .bdrv_dirname() so we don't chop off
> query strings for backing files with relative filenames.
> 
> 
> v3:
> - Keep iotests 104 and 207 working
> 
> 
> Max Reitz (2):
>   block/ssh: Implement .bdrv_refresh_filename()
>   block/ssh: Implement .bdrv_dirname()
> 
>  block/ssh.c   | 73 ---
>  tests/qemu-iotests/207| 10 ++---
>  tests/qemu-iotests/207.out| 10 ++---
>  tests/qemu-iotests/common.rc  |  2 +-
>  tests/qemu-iotests/iotests.py |  2 +-
>  5 files changed, 80 insertions(+), 17 deletions(-)
Applied to my block-next branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 0/3] qemu-img: Allow rebase with no input base

[Max Reitz]

Ping again

(I feel like I just need to start merging unreviewed patches until I
break something (can't take that long) so you get so scared of my
patches that you at least refuse them outright)

On 13.07.18 13:14, Max Reitz wrote:
> This series allows using qemu-img rebase on images that do not have a
> backing file.  Right now, this fails with the rather cryptic error
> message:
> 
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver 
> requires a file name
> 
> Yeah, well, OK.
> 
> With how rebase currently works, this would lead to the overlay being
> filled with zeroes, however.  This is where patch 2 comes in and instead
> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
> the input’s backing file’s EOF.
> 
> (Note that additionally we could try to punch holes in the overlay
> whenever it matches the new backing file, but that’s something I’ll put
> off for later.  (We don’t even have a reliable method for punching holes
> into an overlay yet, although I would like to have such because it could
> make active commit more efficient.))
> 
> 
> And patch 3 adds the usual test.
> 
> 
> Max Reitz (3):
>   qemu-img: Allow rebase with no input base
>   qemu-img: Use zero writes after source backing EOF
>   iotests: Add test for rebase without input base
> 
>  qemu-img.c | 72 +++---
>  tests/qemu-iotests/024 | 70 
>  tests/qemu-iotests/024.out | 37 
>  3 files changed, 150 insertions(+), 29 deletions(-)
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 07/11] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy]

16.01.2019 19:02, Max Reitz wrote:
> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>> Backup-top filter does copy-before-write operation. It should be
>> inserted above active disk and has a target node for CBW, like the
>> following:
>>
>>  +---+
>>  | Guest |
>>  +---+---+
>>  |r,w
>>  v
>>  +---+---+  target   +---+
>>  | backup_top|-->| target(qcow2) |
>>  +---+---+   CBW +---+---+
>>  |
>> backing |r,w
>>  v
>>  +---+-+
>>  | Active disk |
>>  +-+
>>
>> The driver will be used in backup instead of write-notifiers.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/backup-top.h  |  43 +++
>>   block/backup-top.c  | 306 
>>   block/Makefile.objs |   2 +
>>   3 files changed, 351 insertions(+)
>>   create mode 100644 block/backup-top.h
>>   create mode 100644 block/backup-top.c
> 

[..]

>> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>> + BlockDriverState *target,
>> + HBitmap *copy_bitmap,
>> + Error **errp)
>> +{
>> +Error *local_err = NULL;
>> +BDRVBackupTopState *state;
>> +BlockDriverState *top = bdrv_new_open_driver(_backup_top_filter,
>> + NULL, BDRV_O_RDWR, errp);
>> +
>> +if (!top) {
>> +return NULL;
>> +}
>> +
>> +top->implicit = true;
>> +top->total_sectors = source->total_sectors;
>> +top->opaque = state = g_new0(BDRVBackupTopState, 1);
>> +state->copy_bitmap = copy_bitmap;
>> +
>> +bdrv_ref(target);
>> +state->target = bdrv_attach_child(top, target, "target", _file, 
>> errp);
>> +if (!state->target) {
>> +bdrv_unref(target);
>> +bdrv_unref(top);
>> +return NULL;
>> +}
>> +
>> +bdrv_set_aio_context(top, bdrv_get_aio_context(source));
>> +bdrv_set_aio_context(target, bdrv_get_aio_context(source));
>> +
>> +bdrv_drained_begin(source);
>> +
>> +bdrv_ref(top);
>> +bdrv_append(top, source, _err);
>> +
>> +if (local_err) {
>> +bdrv_unref(top);
> 
> This is done automatically by bdrv_append().
> 
>> +}
>> +
>> +bdrv_drained_end(source);
>> +
>> +if (local_err) {
>> +bdrv_unref_child(top, state->target);
>> +bdrv_unref(top);
>> +error_propagate(errp, local_err);
>> +return NULL;
>> +}
>> +
>> +return top;
>> +}
>> +
>> +void bdrv_backup_top_drop(BlockDriverState *bs)
>> +{
>> +BDRVBackupTopState *s = bs->opaque;
>> +
>> +AioContext *aio_context = bdrv_get_aio_context(bs);
>> +
>> +aio_context_acquire(aio_context);
>> +
>> +bdrv_drained_begin(bs);
>> +
>> +bdrv_child_try_set_perm(bs->backing, 0, BLK_PERM_ALL, _abort);
>> +bdrv_replace_node(bs, backing_bs(bs), _abort);
>> +bdrv_set_backing_hd(bs, NULL, _abort);
> 
> This is done automatically in bdrv_close(), and after bs has been
> replaced by backing_bs(bs), I don't think new requests should come in,
> so I don't think this needs to be done here.

Following movement of backup_top back to job->blk becomes impossible then,
if we don't share WRITE on source in backup_top_child_perm.

And I think, this function should drop all relations created by
bdrv_backup_top_append.

> 
>> +
>> +bdrv_drained_end(bs);
>> +
>> +if (s->target) {
>> +bdrv_unref_child(bs, s->target);
>> +}
> 
> And this should be done in a .bdrv_close() implementation, I think.
> 
> Max
> 
>> +bdrv_unref(bs);
>> +
>> +aio_context_release(aio_context);
>> +}
>> +


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Max Reitz]

On 13.04.19 10:46, Alberto Garcia wrote:
> On Sat 13 Apr 2019 02:56:57 AM CEST, Max Reitz wrote:
>>> Patch 2 fixes a different (but slightly related) issue that I found
>>> while preparing the first patch.
>>
>> I think the real problem is that bs->backing_file is not a cache for
>> bs->backing->bs->filename.
>>
>> In fact, every user of bs->backing_file expects it to be something
>> different.  Some expect it to be a cache for
>> bs->backing->bs->filename, some expect it to be what the image header
>> says (i.e., it if’s a relative path, it’s relative to the overlay),
>> some expect it to be what the image header says, but relative paths to
>> be translated so they are relative to qemu’s CWD.
>>
>> All of this should be cleaned up and this is what patch 7 in my "block:
>> Deal with filters" series does:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html
> 
> Ok, you can leave out the second patch then. The first one should still
> be correct, right?

I just think it’s unnecessary because as of my series both backing_file
and auto_backing_file serve the purpose.

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH for-next 2/2] qemu-img: Make create hint at protocol options

[Max Reitz]

qemu-img create allows giving just a format and "-o help" to get a list
of the options supported by that format.  Users may not realize that the
protocol level may offer even more options, which they only get to see
by specifying a filename.

This patch adds a note to hint at that fact.

Signed-off-by: Max Reitz 
---
 qemu-img.c | 13 -
 tests/qemu-iotests/082.out | 20 
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index aa6f81f1ea..0016e24efe 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -269,9 +269,20 @@ static int print_block_option_help(const char *filename, 
const char *fmt)
 create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 }
 
-printf("Supported options:\n");
+if (filename) {
+printf("Supported options:\n");
+} else {
+printf("Supported %s options:\n", fmt);
+}
 qemu_opts_print_help(create_opts, false);
 qemu_opts_free(create_opts);
+
+if (!filename) {
+printf("\n"
+   "The protocol level may support further options.\n"
+   "Specify the target filename to include those options.\n");
+}
+
 return 0;
 }
 
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 9a23b68511..7e25706813 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -242,7 +242,7 @@ Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2 
-o ,, -o help TEST_DIR
 qemu-img: Invalid option list: ,,
 
 Testing: create -f qcow2 -o help
-Supported options:
+Supported qcow2 options:
   backing_file= - File name of a base image
   backing_fmt=  - Image format of the base image
   cluster_size=- qcow2 cluster size
@@ -263,10 +263,16 @@ Supported options:
   refcount_bits=- Width of a reference count entry in bits
   size=- Virtual disk size
 
+The protocol level may support further options.
+Specify the target filename to include those options.
+
 Testing: create -o help
-Supported options:
+Supported raw options:
   size=- Virtual disk size
 
+The protocol level may support further options.
+Specify the target filename to include those options.
+
 Testing: create -f bochs -o help
 qemu-img: Format driver 'bochs' does not support image creation
 
@@ -516,7 +522,7 @@ Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2 
-o ,, -o help TEST_DI
 qemu-img: Invalid option list: ,,
 
 Testing: convert -O qcow2 -o help
-Supported options:
+Supported qcow2 options:
   backing_file= - File name of a base image
   backing_fmt=  - Image format of the base image
   cluster_size=- qcow2 cluster size
@@ -537,10 +543,16 @@ Supported options:
   refcount_bits=- Width of a reference count entry in bits
   size=- Virtual disk size
 
+The protocol level may support further options.
+Specify the target filename to include those options.
+
 Testing: convert -o help
-Supported options:
+Supported raw options:
   size=- Virtual disk size
 
+The protocol level may support further options.
+Specify the target filename to include those options.
+
 Testing: convert -O bochs -o help
 qemu-img: Format driver 'bochs' does not support image creation
 
-- 
2.20.1

[Qemu-block] [PATCH for-next 0/2] qemu-img: Make create hint at protocol options

[Max Reitz]

https://bugzilla.redhat.com/show_bug.cgi?id=1698863 reports that while
"qemu-img create -f raw" supports the "preallocation" option, it is not
listed under "-o help".

It turns out it is, but only if you specify a target filename.  Users
may not realize this, but a note should help.


Max Reitz (2):
  iotests: Perform the correct test in 082
  qemu-img: Make create hint at protocol options

 qemu-img.c | 13 -
 tests/qemu-iotests/082 |  5 -
 tests/qemu-iotests/082.out | 25 ++---
 3 files changed, 34 insertions(+), 9 deletions(-)

-- 
2.20.1

[Qemu-block] [PATCH for-next 1/2] iotests: Perform the correct test in 082

[Max Reitz]

In the "amend" section of 082, we perform a single "convert" test
(namely "convert -o help").  That does not make sense, especially
because we have done exactly that "convert" test earlier in 082 already.

Replacing "convert" by "amend" yields an error, which is correct because
there is no point in "amend" having a default format.  The user has to
either specify the format, or give a file for qemu-img to probe.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/082 | 5 -
 tests/qemu-iotests/082.out | 5 ++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index d0afa46e9a..278511dba4 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -212,7 +212,10 @@ run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" 
-o ,, -o help "$TEST_I
 
 # Leave out everything that isn't needed
 run_qemu_img amend -f $IMGFMT -o help
-run_qemu_img convert -o help
+
+# amend requires specifying either a format explicitly, or a file
+# which it can probe
+run_qemu_img amend -o help
 
 # Try help option for a format that does not support amendment
 run_qemu_img amend -f bochs -o help
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index d36938da9b..9a23b68511 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -831,9 +831,8 @@ Creation options for 'qcow2':
 
 Note that not all of these options may be amendable.
 
-Testing: convert -o help
-Supported options:
-  size=- Virtual disk size
+Testing: amend -o help
+qemu-img: Expecting one image file name
 
 Testing: amend -f bochs -o help
 qemu-img: Format driver 'bochs' does not support option amendment
-- 
2.20.1

Re: [Qemu-block] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

On Sat 13 Apr 2019 02:56:57 AM CEST, Max Reitz wrote:
>> Patch 2 fixes a different (but slightly related) issue that I found
>> while preparing the first patch.
>
> I think the real problem is that bs->backing_file is not a cache for
> bs->backing->bs->filename.
>
> In fact, every user of bs->backing_file expects it to be something
> different.  Some expect it to be a cache for
> bs->backing->bs->filename, some expect it to be what the image header
> says (i.e., it if’s a relative path, it’s relative to the overlay),
> some expect it to be what the image header says, but relative paths to
> be translated so they are relative to qemu’s CWD.
>
> All of this should be cleaned up and this is what patch 7 in my "block:
> Deal with filters" series does:
>
> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

Ok, you can leave out the second patch then. The first one should still
be correct, right?

Berto