Re: [Qemu-block] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Max Reitz]

On 10.06.2016 20:57, Max Reitz wrote:
> Issue #1: If the target image does not have a backing BDS before mirror
> completion, qemu tries really hard to give it a backing BDS. If the
> source has a backing BDS, it will actually always "succeed".
> In some cases, the target is not supposed to have a backing BDS, though
> (absolute-paths: because of sync=full; existing: because the target
> image does not have a backing file; blockdev-mirror: because of an
> explicit "backing": ""). Then, this is pretty bad behavior.
> 
> This should generally not change the target's visible data, but it still
> is ugly.
> 
> Issue #2: Currently the backing chain of the target is basically opened
> using bdrv_open_backing_file() (except for sometimes™). This results in
> multiple BDSs for a single physical file, which is bad. In most use
> cases, this is only temporary, but it still is bad.
> 
> If we can reuse the existing backing chain of the source (which is with
> drive-mirror in "absolute-paths" mode), we should just do so.

Following encouragement from Kevin on IRC, and apparently his acceptance
of my commit message proposal for patch 3, I have applied the series to
my block branch with said commit message added to patch 3 and the
superfluous imports removed from patch 4 (thanks, Fam!).

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Kevin Wolf]

Am 10.06.2016 um 20:57 hat Max Reitz geschrieben:
> Issue #1: If the target image does not have a backing BDS before mirror
> completion, qemu tries really hard to give it a backing BDS. If the
> source has a backing BDS, it will actually always "succeed".
> In some cases, the target is not supposed to have a backing BDS, though
> (absolute-paths: because of sync=full; existing: because the target
> image does not have a backing file; blockdev-mirror: because of an
> explicit "backing": ""). Then, this is pretty bad behavior.
> 
> This should generally not change the target's visible data, but it still
> is ugly.
> 
> Issue #2: Currently the backing chain of the target is basically opened
> using bdrv_open_backing_file() (except for sometimes™). This results in
> multiple BDSs for a single physical file, which is bad. In most use
> cases, this is only temporary, but it still is bad.
> 
> If we can reuse the existing backing chain of the source (which is with
> drive-mirror in "absolute-paths" mode), we should just do so.

Reviewed-by: Kevin Wolf 

I'll still wait to apply the series so that you have a chance to answer
Fam's question before it is in.

Kevin

Re: [Qemu-block] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Fam Zheng]

On Fri, 06/10 20:57, Max Reitz wrote:
> Issue #1: If the target image does not have a backing BDS before mirror
> completion, qemu tries really hard to give it a backing BDS. If the
> source has a backing BDS, it will actually always "succeed".
> In some cases, the target is not supposed to have a backing BDS, though
> (absolute-paths: because of sync=full; existing: because the target
> image does not have a backing file; blockdev-mirror: because of an
> explicit "backing": ""). Then, this is pretty bad behavior.
> 
> This should generally not change the target's visible data, but it still
> is ugly.
> 
> Issue #2: Currently the backing chain of the target is basically opened
> using bdrv_open_backing_file() (except for sometimes™). This results in
> multiple BDSs for a single physical file, which is bad. In most use
> cases, this is only temporary, but it still is bad.
> 
> If we can reuse the existing backing chain of the source (which is with
> drive-mirror in "absolute-paths" mode), we should just do so.

Looks good overall. I left a few trivial comments/questions in individual
patches.

Reviewed-by: Fam Zheng 

[Qemu-block] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Max Reitz]

Issue #1: If the target image does not have a backing BDS before mirror
completion, qemu tries really hard to give it a backing BDS. If the
source has a backing BDS, it will actually always "succeed".
In some cases, the target is not supposed to have a backing BDS, though
(absolute-paths: because of sync=full; existing: because the target
image does not have a backing file; blockdev-mirror: because of an
explicit "backing": ""). Then, this is pretty bad behavior.

This should generally not change the target's visible data, but it still
is ugly.

Issue #2: Currently the backing chain of the target is basically opened
using bdrv_open_backing_file() (except for sometimes™). This results in
multiple BDSs for a single physical file, which is bad. In most use
cases, this is only temporary, but it still is bad.

If we can reuse the existing backing chain of the source (which is with
drive-mirror in "absolute-paths" mode), we should just do so.


v3:
- Patch 1:
  - More verbose commit message [Kevin]
  - Changed comment to match code [Kevin]
- Patch 2:
  - Do not force use of the source backing chain for the target in
"existing" mode or with blockdev-mirror [Kevin]
- Instead keep doing what we've been doing for
  drive-mirror/existing, only that we should still drop the
  bdrv_set_backing_hd() in bdrv_replace_in_backing_chain()
- And for blockdev-mirror, just do not change the current backing
  chain at all; this is what we've been doing until now, unless the
  target BDS did not have a backing BDS yet
- Patch 3: Added, because it makes the next test a bit nicer
- Patch 4: Adjusted to v3 behavior, and added a new test for
  blockdev-mirror with a target whose backing file has been overridden
  using the "backing" option
- Patch 5: Added [Kevin]


git-backport-diff against v2:

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/5:[0005] [FC] 'block: Allow replacement of a BDS by its overlay'
002/5:[0057] [FC] 'block/mirror: Fix target backing BDS'
003/5:[down] 'block/null: Implement bdrv_refresh_filename()'
004/5:[0073] [FC] 'iotests: Add test for post-mirror backing chains'
005/5:[down] 'iotests: Add test for oVirt-like storage migration'


Max Reitz (5):
  block: Allow replacement of a BDS by its overlay
  block/mirror: Fix target backing BDS
  block/null: Implement bdrv_refresh_filename()
  iotests: Add test for post-mirror backing chains
  iotests: Add test for oVirt-like storage migration

 block.c|  24 +++--
 block/mirror.c |  39 +--
 block/null.c   |  20 
 blockdev.c |  15 ++-
 include/block/block_int.h  |  18 +++-
 tests/qemu-iotests/155 | 263 +
 tests/qemu-iotests/155.out |   5 +
 tests/qemu-iotests/156 | 174 ++
 tests/qemu-iotests/156.out |  48 +
 tests/qemu-iotests/group   |   2 +
 10 files changed, 584 insertions(+), 24 deletions(-)
 create mode 100755 tests/qemu-iotests/155
 create mode 100644 tests/qemu-iotests/155.out
 create mode 100755 tests/qemu-iotests/156
 create mode 100644 tests/qemu-iotests/156.out

-- 
2.8.3