[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 1/1] mirror: restore offset after zeroing out th

From: Anton Nefedov
Subject: Re: [Qemu-devel] [PATCH 1/1] mirror: restore offset after zeroing out the image
Date: Mon, 30 Jan 2017 21:10:08 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

On 01/30/2017 08:33 PM, Denis V. Lunev wrote:
On 01/30/2017 08:16 PM, Eric Blake wrote:
On 01/30/2017 04:22 AM, Denis V. Lunev wrote:
If explicit zeroing out before mirroring is required for the target image,
it moves the block job offset counter to EOF, then offset and len counters
count the image size twice.

There is no harm but confusing stats (e.g. for 1G image the completion
counter starts from 1G and increases to 2G)

The patch fixed that problem by resetting the offset counter.
Counters are explicitly documented NOT tied to disk length; they are
merely estimates of proportional completion.  I'm not sure if this makes
the numbers jump backwards from the observer's viewpoint, but if you can
ever spot 1g/1g right before rewinding to 0g/1g (where pre-patch could
see 1g/2g), then that is a reason to not take this patch.

FWIW you'll actually see 1g/0g first (if you happen to catch that tiny phase :)

On the other
hand, your argument that the pre-patch behavior progressing towards 2g
has a very fast progression from 0-1g/2g, and then a much slower
1g-2g/2g, which makes the estimate of percent completion skewed, while a
newer progression of 0-1g/1g is more realistic, may have some merit.

I'm not sold on this patch yet, but stronger arguments in the commit
message may sway me.

+++ b/tests/qemu-iotests/094.out
@@ -3,9 +3,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 Formatting 'TEST_DIR/source.IMGFMT', fmt=IMGFMT size=67108864
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", 
"len": 67108864, "offset": 67108864, "speed": 0, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", 
"len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
 {"return": {}}
This part of the change is scary - a ready event showing 0/0 HAS been
known to confuse libvirt in the past.  Qemu should NEVER advertise a
ready event with 0/0, it should at least be 1/1 (because of the number
of clients that have workarounds to deal with older qemu behavior on 0/0
and which might misbehave if we ever issue that again).

I think this 094.out modification is not needed and actually breaks the test; apologies

-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", 
"len": 67108864, "offset": 67108864, "speed": 0, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", 
"len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
So NACK to the patch as currently written, but not necessarily to the
idea if you can give better progress numbers and never reach the state
of BLOCK_JOB_READY and BLOCK_JOB_COMPLETED with a 0/0 length/offset.

ok. fair enough. Thank you for the review.

Will it be better to (somehow) skip progressing below using
some condition during mirror_dirty_init() stage?

static void mirror_iteration_done(MirrorOp *op, int ret)

    if (ret >= 0) {
        if (s->cow_bitmap) {
            bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
        s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
<---- specifically this progressing

Isn't this going to look quite too invasive for such cause

best regards,

reply via email to

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