* [Qemu-devel] [RFC] migration/block:limit the time used for block migration
@ 2017-03-28 9:23 Lidong Chen
2017-03-28 9:32 ` 858585 jemmy
2017-03-28 9:47 ` Juan Quintela
0 siblings, 2 replies; 7+ messages in thread
From: Lidong Chen @ 2017-03-28 9:23 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, famz, quintela, dgilbert, qemu-block, Lidong Chen
when migration with quick speed, mig_save_device_bulk invoke
bdrv_is_allocated too frequently, and cause vnc reponse slowly.
this patch limit the time used for bdrv_is_allocated.
Signed-off-by: Lidong Chen <lidongchen@tencent.com>
---
migration/block.c | 39 +++++++++++++++++++++++++++++++--------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index 7734ff7..d3e81ca 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -110,6 +110,7 @@ typedef struct BlkMigState {
int transferred;
int prev_progress;
int bulk_completed;
+ int time_ns_used;
/* Lock must be taken _inside_ the iothread lock and any AioContexts. */
QemuMutex lock;
@@ -263,6 +264,7 @@ static void blk_mig_read_cb(void *opaque, int ret)
blk_mig_unlock();
}
+#define BILLION 1000000000L
/* Called with no lock taken. */
static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
@@ -272,16 +274,33 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
BlockBackend *bb = bmds->blk;
BlkMigBlock *blk;
int nr_sectors;
+ struct timespec ts1, ts2;
+ int ret = 0;
+ int timeout_flag = 0;
if (bmds->shared_base) {
qemu_mutex_lock_iothread();
aio_context_acquire(blk_get_aio_context(bb));
/* Skip unallocated sectors; intentionally treats failure as
* an allocated sector */
- while (cur_sector < total_sectors &&
- !bdrv_is_allocated(blk_bs(bb), cur_sector,
- MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
- cur_sector += nr_sectors;
+ while (cur_sector < total_sectors) {
+ clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
+ ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
+ MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
+ clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
+
+ block_mig_state.time_ns_used += (ts2.tv_sec - ts1.tv_sec) * BILLION
+ + (ts2.tv_nsec - ts1.tv_nsec);
+
+ if (!ret) {
+ cur_sector += nr_sectors;
+ if (block_mig_state.time_ns_used > 100000) {
+ timeout_flag = 1;
+ break;
+ }
+ } else {
+ break;
+ }
}
aio_context_release(blk_get_aio_context(bb));
qemu_mutex_unlock_iothread();
@@ -292,6 +311,11 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
return 1;
}
+ if (timeout_flag == 1) {
+ bmds->cur_sector = bmds->completed_sectors = cur_sector;
+ return 0;
+ }
+
bmds->completed_sectors = cur_sector;
cur_sector &= ~((int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK - 1);
@@ -576,9 +600,6 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
}
bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
- sector += nr_sectors;
- bmds->cur_dirty = sector;
-
break;
}
sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
@@ -756,6 +777,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
}
blk_mig_reset_dirty_cursor();
+ block_mig_state.time_ns_used = 0;
/* control the rate of transfer */
blk_mig_lock();
@@ -764,7 +786,8 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
qemu_file_get_rate_limit(f) &&
(block_mig_state.submitted +
block_mig_state.read_done) <
- MAX_INFLIGHT_IO) {
+ MAX_INFLIGHT_IO &&
+ block_mig_state.time_ns_used <= 100000) {
blk_mig_unlock();
if (block_mig_state.bulk_completed == 0) {
/* first finish the bulk phase */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] migration/block:limit the time used for block migration
2017-03-28 9:23 [Qemu-devel] [RFC] migration/block:limit the time used for block migration Lidong Chen
@ 2017-03-28 9:32 ` 858585 jemmy
2017-03-28 9:47 ` Juan Quintela
1 sibling, 0 replies; 7+ messages in thread
From: 858585 jemmy @ 2017-03-28 9:32 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, Fam Zheng, quintela, dgilbert, qemu-block, Lidong Chen
when migrate the vm with quick speed, i find vnc response slowly.
the bug can be reproduce by this command:
virsh migrate-setspeed 165cf436-312f-47e7-90f2-f8aa63f34893 900
virsh migrate --live 165cf436-312f-47e7-90f2-f8aa63f34893
--copy-storage-inc qemu+ssh://10.59.163.38/system
and --copy-storage-all have no problem.
virsh migrate --live 165cf436-312f-47e7-90f2-f8aa63f34893
--copy-storage-all qemu+ssh://10.59.163.38/system
mig_save_device_bulk invoke bdrv_is_allocated, but bdrv_is_allocated maybe
wait for a long time.
so cause the main thread wait for a long time.
this patch limit the time wait for bdrv_is_allocated.
i do not find a better way to solve this bug, Any suggestion?
Thanks.
On Tue, Mar 28, 2017 at 5:23 PM, Lidong Chen <jemmy858585@gmail.com> wrote:
> when migration with quick speed, mig_save_device_bulk invoke
> bdrv_is_allocated too frequently, and cause vnc reponse slowly.
> this patch limit the time used for bdrv_is_allocated.
>
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
> migration/block.c | 39 +++++++++++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/migration/block.c b/migration/block.c
> index 7734ff7..d3e81ca 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -110,6 +110,7 @@ typedef struct BlkMigState {
> int transferred;
> int prev_progress;
> int bulk_completed;
> + int time_ns_used;
>
> /* Lock must be taken _inside_ the iothread lock and any
> AioContexts. */
> QemuMutex lock;
> @@ -263,6 +264,7 @@ static void blk_mig_read_cb(void *opaque, int ret)
> blk_mig_unlock();
> }
>
> +#define BILLION 1000000000L
> /* Called with no lock taken. */
>
> static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> @@ -272,16 +274,33 @@ static int mig_save_device_bulk(QEMUFile *f,
> BlkMigDevState *bmds)
> BlockBackend *bb = bmds->blk;
> BlkMigBlock *blk;
> int nr_sectors;
> + struct timespec ts1, ts2;
> + int ret = 0;
> + int timeout_flag = 0;
>
> if (bmds->shared_base) {
> qemu_mutex_lock_iothread();
> aio_context_acquire(blk_get_aio_context(bb));
> /* Skip unallocated sectors; intentionally treats failure as
> * an allocated sector */
> - while (cur_sector < total_sectors &&
> - !bdrv_is_allocated(blk_bs(bb), cur_sector,
> - MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
> - cur_sector += nr_sectors;
> + while (cur_sector < total_sectors) {
> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
> + ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
> + MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
> +
> + block_mig_state.time_ns_used += (ts2.tv_sec - ts1.tv_sec) *
> BILLION
> + + (ts2.tv_nsec - ts1.tv_nsec);
> +
> + if (!ret) {
> + cur_sector += nr_sectors;
> + if (block_mig_state.time_ns_used > 100000) {
> + timeout_flag = 1;
> + break;
> + }
> + } else {
> + break;
> + }
> }
> aio_context_release(blk_get_aio_context(bb));
> qemu_mutex_unlock_iothread();
> @@ -292,6 +311,11 @@ static int mig_save_device_bulk(QEMUFile *f,
> BlkMigDevState *bmds)
> return 1;
> }
>
> + if (timeout_flag == 1) {
> + bmds->cur_sector = bmds->completed_sectors = cur_sector;
> + return 0;
> + }
> +
> bmds->completed_sectors = cur_sector;
>
> cur_sector &= ~((int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK - 1);
> @@ -576,9 +600,6 @@ static int mig_save_device_dirty(QEMUFile *f,
> BlkMigDevState *bmds,
> }
>
> bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector,
> nr_sectors);
> - sector += nr_sectors;
> - bmds->cur_dirty = sector;
> -
> break;
> }
> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> @@ -756,6 +777,7 @@ static int block_save_iterate(QEMUFile *f, void
> *opaque)
> }
>
> blk_mig_reset_dirty_cursor();
> + block_mig_state.time_ns_used = 0;
>
> /* control the rate of transfer */
> blk_mig_lock();
> @@ -764,7 +786,8 @@ static int block_save_iterate(QEMUFile *f, void
> *opaque)
> qemu_file_get_rate_limit(f) &&
> (block_mig_state.submitted +
> block_mig_state.read_done) <
> - MAX_INFLIGHT_IO) {
> + MAX_INFLIGHT_IO &&
> + block_mig_state.time_ns_used <= 100000) {
> blk_mig_unlock();
> if (block_mig_state.bulk_completed == 0) {
> /* first finish the bulk phase */
> --
> 1.8.3.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] migration/block:limit the time used for block migration
2017-03-28 9:23 [Qemu-devel] [RFC] migration/block:limit the time used for block migration Lidong Chen
2017-03-28 9:32 ` 858585 jemmy
@ 2017-03-28 9:47 ` Juan Quintela
2017-03-29 13:21 ` 858585 jemmy
1 sibling, 1 reply; 7+ messages in thread
From: Juan Quintela @ 2017-03-28 9:47 UTC (permalink / raw)
To: Lidong Chen; +Cc: qemu-devel, stefanha, famz, dgilbert, qemu-block, Lidong Chen
Lidong Chen <jemmy858585@gmail.com> wrote:
> when migration with quick speed, mig_save_device_bulk invoke
> bdrv_is_allocated too frequently, and cause vnc reponse slowly.
> this patch limit the time used for bdrv_is_allocated.
>
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
> migration/block.c | 39 +++++++++++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/migration/block.c b/migration/block.c
> index 7734ff7..d3e81ca 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -110,6 +110,7 @@ typedef struct BlkMigState {
> int transferred;
> int prev_progress;
> int bulk_completed;
> + int time_ns_used;
An int that can only take values 0/1 is called a bool O:-)
> if (bmds->shared_base) {
> qemu_mutex_lock_iothread();
> aio_context_acquire(blk_get_aio_context(bb));
> /* Skip unallocated sectors; intentionally treats failure as
> * an allocated sector */
> - while (cur_sector < total_sectors &&
> - !bdrv_is_allocated(blk_bs(bb), cur_sector,
> - MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
> - cur_sector += nr_sectors;
> + while (cur_sector < total_sectors) {
> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
> + ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
> + MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
Do we really want to call clock_gettime each time that
bdrv_is_allocated() is called? My understanding is that clock_gettime
is expensive, but I don't know how expensive is brdrv_is_allocated()
And while we are at it, .... shouldn't we check since before the while?
> +
> + block_mig_state.time_ns_used += (ts2.tv_sec - ts1.tv_sec) * BILLION
> + + (ts2.tv_nsec - ts1.tv_nsec);
> +
> + if (!ret) {
> + cur_sector += nr_sectors;
> + if (block_mig_state.time_ns_used > 100000) {
> + timeout_flag = 1;
> + break;
> + }
> + } else {
> + break;
> + }
> }
> aio_context_release(blk_get_aio_context(bb));
> qemu_mutex_unlock_iothread();
> @@ -292,6 +311,11 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> return 1;
> }
>
> + if (timeout_flag == 1) {
> + bmds->cur_sector = bmds->completed_sectors = cur_sector;
> + return 0;
> + }
> +
> bmds->completed_sectors = cur_sector;
>
> cur_sector &= ~((int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK - 1);
> @@ -576,9 +600,6 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> }
>
> bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
> - sector += nr_sectors;
> - bmds->cur_dirty = sector;
> -
> break;
> }
> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> @@ -756,6 +777,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
> }
>
> blk_mig_reset_dirty_cursor();
> + block_mig_state.time_ns_used = 0;
>
> /* control the rate of transfer */
> blk_mig_lock();
> @@ -764,7 +786,8 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
> qemu_file_get_rate_limit(f) &&
> (block_mig_state.submitted +
> block_mig_state.read_done) <
> - MAX_INFLIGHT_IO) {
> + MAX_INFLIGHT_IO &&
> + block_mig_state.time_ns_used <= 100000) {
changed this 10.000 (and the one used previously) to one constant that
says BIG_DELAY, 100MS or whatever you fancy.
Thanks, Juan.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] migration/block:limit the time used for block migration
2017-03-28 9:47 ` Juan Quintela
@ 2017-03-29 13:21 ` 858585 jemmy
2017-03-29 15:57 ` Juan Quintela
2017-04-05 7:38 ` 858585 jemmy
0 siblings, 2 replies; 7+ messages in thread
From: 858585 jemmy @ 2017-03-29 13:21 UTC (permalink / raw)
To: quintela
Cc: qemu-devel, stefanha, Fam Zheng, dgilbert, qemu-block, Lidong Chen
On Tue, Mar 28, 2017 at 5:47 PM, Juan Quintela <quintela@redhat.com> wrote:
> Lidong Chen <jemmy858585@gmail.com> wrote:
>> when migration with quick speed, mig_save_device_bulk invoke
>> bdrv_is_allocated too frequently, and cause vnc reponse slowly.
>> this patch limit the time used for bdrv_is_allocated.
>>
>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>> ---
>> migration/block.c | 39 +++++++++++++++++++++++++++++++--------
>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration/block.c b/migration/block.c
>> index 7734ff7..d3e81ca 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -110,6 +110,7 @@ typedef struct BlkMigState {
>> int transferred;
>> int prev_progress;
>> int bulk_completed;
>> + int time_ns_used;
>
> An int that can only take values 0/1 is called a bool O:-)
time_ns_used is used to store how many ns used by bdrv_is_allocated.
>
>
>> if (bmds->shared_base) {
>> qemu_mutex_lock_iothread();
>> aio_context_acquire(blk_get_aio_context(bb));
>> /* Skip unallocated sectors; intentionally treats failure as
>> * an allocated sector */
>> - while (cur_sector < total_sectors &&
>> - !bdrv_is_allocated(blk_bs(bb), cur_sector,
>> - MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>> - cur_sector += nr_sectors;
>> + while (cur_sector < total_sectors) {
>> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
>> + ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
>> + MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
>> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
>
> Do we really want to call clock_gettime each time that
> bdrv_is_allocated() is called? My understanding is that clock_gettime
> is expensive, but I don't know how expensive is brdrv_is_allocated()
i write this code to measure the time used by brdrv_is_allocated()
279 static int max_time = 0;
280 int tmp;
288 clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
289 ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
290 MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
291 clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
292
293
294 tmp = (ts2.tv_sec - ts1.tv_sec)*1000000000L
295 + (ts2.tv_nsec - ts1.tv_nsec);
296 if (tmp > max_time) {
297 max_time=tmp;
298 fprintf(stderr, "max_time is %d\n", max_time);
299 }
the test result is below:
max_time is 37014
max_time is 1075534
max_time is 17180913
max_time is 28586762
max_time is 49563584
max_time is 103085447
max_time is 110836833
max_time is 120331438
so i think it's necessary to clock_gettime each time.
but clock_gettime only available on linux. maybe clock() is better.
>
> And while we are at it, .... shouldn't we check since before the while?
i also check it in block_save_iterate.
+ MAX_INFLIGHT_IO &&
+ block_mig_state.time_ns_used <= 100000) {
>
>
>> +
>> + block_mig_state.time_ns_used += (ts2.tv_sec - ts1.tv_sec) * BILLION
>> + + (ts2.tv_nsec - ts1.tv_nsec);
>> +
>> + if (!ret) {
>> + cur_sector += nr_sectors;
>> + if (block_mig_state.time_ns_used > 100000) {
>> + timeout_flag = 1;
>> + break;
>> + }
>> + } else {
>> + break;
>> + }
>> }
>> aio_context_release(blk_get_aio_context(bb));
>> qemu_mutex_unlock_iothread();
>> @@ -292,6 +311,11 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>> return 1;
>> }
>>
>> + if (timeout_flag == 1) {
>> + bmds->cur_sector = bmds->completed_sectors = cur_sector;
>> + return 0;
>> + }
>> +
>> bmds->completed_sectors = cur_sector;
>>
>> cur_sector &= ~((int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK - 1);
>> @@ -576,9 +600,6 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>> }
>>
>> bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
>> - sector += nr_sectors;
>> - bmds->cur_dirty = sector;
>> -
>> break;
>> }
>> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
>> @@ -756,6 +777,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>> }
>>
>> blk_mig_reset_dirty_cursor();
>> + block_mig_state.time_ns_used = 0;
>>
>> /* control the rate of transfer */
>> blk_mig_lock();
>> @@ -764,7 +786,8 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>> qemu_file_get_rate_limit(f) &&
>> (block_mig_state.submitted +
>> block_mig_state.read_done) <
>> - MAX_INFLIGHT_IO) {
>> + MAX_INFLIGHT_IO &&
>> + block_mig_state.time_ns_used <= 100000) {
>
> changed this 10.000 (and the one used previously) to one constant that
> says BIG_DELAY, 100MS or whatever you fancy.
ok,i will change to BIG_DELAY.
>
> Thanks, Juan.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] migration/block:limit the time used for block migration
2017-03-29 13:21 ` 858585 jemmy
@ 2017-03-29 15:57 ` Juan Quintela
2017-04-05 3:55 ` 858585 jemmy
2017-04-05 7:38 ` 858585 jemmy
1 sibling, 1 reply; 7+ messages in thread
From: Juan Quintela @ 2017-03-29 15:57 UTC (permalink / raw)
To: 858585 jemmy
Cc: qemu-devel, stefanha, Fam Zheng, dgilbert, qemu-block, Lidong Chen
858585 jemmy <jemmy858585@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 5:47 PM, Juan Quintela <quintela@redhat.com> wrote:
>> Lidong Chen <jemmy858585@gmail.com> wrote:
>>> when migration with quick speed, mig_save_device_bulk invoke
>>> bdrv_is_allocated too frequently, and cause vnc reponse slowly.
>>> this patch limit the time used for bdrv_is_allocated.
>>>
>>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>>> ---
>>> migration/block.c | 39 +++++++++++++++++++++++++++++++--------
>>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/migration/block.c b/migration/block.c
>>> index 7734ff7..d3e81ca 100644
>>> --- a/migration/block.c
>>> +++ b/migration/block.c
>>> @@ -110,6 +110,7 @@ typedef struct BlkMigState {
>>> int transferred;
>>> int prev_progress;
>>> int bulk_completed;
>>> + int time_ns_used;
>>
>> An int that can only take values 0/1 is called a bool O:-)
> time_ns_used is used to store how many ns used by bdrv_is_allocated.
Oops, I really mean timeout_flag, sorry :-(
>> Do we really want to call clock_gettime each time that
>> bdrv_is_allocated() is called? My understanding is that clock_gettime
>> is expensive, but I don't know how expensive is brdrv_is_allocated()
>
> i write this code to measure the time used by brdrv_is_allocated()
>
> 279 static int max_time = 0;
> 280 int tmp;
>
> 288 clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
> 289 ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
> 290 MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
> 291 clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
> 292
> 293
> 294 tmp = (ts2.tv_sec - ts1.tv_sec)*1000000000L
> 295 + (ts2.tv_nsec - ts1.tv_nsec);
> 296 if (tmp > max_time) {
> 297 max_time=tmp;
> 298 fprintf(stderr, "max_time is %d\n", max_time);
> 299 }
>
> the test result is below:
>
> max_time is 37014
> max_time is 1075534
> max_time is 17180913
> max_time is 28586762
> max_time is 49563584
> max_time is 103085447
> max_time is 110836833
> max_time is 120331438
this is around 120ms, no? It is quite a lot, really :-(
> so i think it's necessary to clock_gettime each time.
> but clock_gettime only available on linux. maybe clock() is better.
Thanks, Juan.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] migration/block:limit the time used for block migration
2017-03-29 15:57 ` Juan Quintela
@ 2017-04-05 3:55 ` 858585 jemmy
0 siblings, 0 replies; 7+ messages in thread
From: 858585 jemmy @ 2017-04-05 3:55 UTC (permalink / raw)
To: quintela
Cc: qemu-devel, stefanha, Fam Zheng, dgilbert, qemu-block, Lidong Chen
On Wed, Mar 29, 2017 at 11:57 PM, Juan Quintela <quintela@redhat.com> wrote:
>
> 858585 jemmy <jemmy858585@gmail.com> wrote:
> > On Tue, Mar 28, 2017 at 5:47 PM, Juan Quintela <quintela@redhat.com> wrote:
> >> Lidong Chen <jemmy858585@gmail.com> wrote:
> >>> when migration with quick speed, mig_save_device_bulk invoke
> >>> bdrv_is_allocated too frequently, and cause vnc reponse slowly.
> >>> this patch limit the time used for bdrv_is_allocated.
> >>>
> >>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> >>> ---
> >>> migration/block.c | 39 +++++++++++++++++++++++++++++++--------
> >>> 1 file changed, 31 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/migration/block.c b/migration/block.c
> >>> index 7734ff7..d3e81ca 100644
> >>> --- a/migration/block.c
> >>> +++ b/migration/block.c
> >>> @@ -110,6 +110,7 @@ typedef struct BlkMigState {
> >>> int transferred;
> >>> int prev_progress;
> >>> int bulk_completed;
> >>> + int time_ns_used;
> >>
> >> An int that can only take values 0/1 is called a bool O:-)
> > time_ns_used is used to store how many ns used by bdrv_is_allocated.
>
> Oops, I really mean timeout_flag, sorry :-(
>
> >> Do we really want to call clock_gettime each time that
> >> bdrv_is_allocated() is called? My understanding is that clock_gettime
> >> is expensive, but I don't know how expensive is brdrv_is_allocated()
> >
> > i write this code to measure the time used by brdrv_is_allocated()
> >
> > 279 static int max_time = 0;
> > 280 int tmp;
> >
> > 288 clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
> > 289 ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
> > 290 MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
> > 291 clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
> > 292
> > 293
> > 294 tmp = (ts2.tv_sec - ts1.tv_sec)*1000000000L
> > 295 + (ts2.tv_nsec - ts1.tv_nsec);
> > 296 if (tmp > max_time) {
> > 297 max_time=tmp;
> > 298 fprintf(stderr, "max_time is %d\n", max_time);
> > 299 }
> >
> > the test result is below:
> >
> > max_time is 37014
> > max_time is 1075534
> > max_time is 17180913
> > max_time is 28586762
> > max_time is 49563584
> > max_time is 103085447
> > max_time is 110836833
> > max_time is 120331438
>
> this is around 120ms, no? It is quite a lot, really :-(
i find the reason is mainly caused by qemu_co_mutex_lock invoked by
qcow2_co_get_block_status.
qemu_co_mutex_lock(&s->lock);
ret = qcow2_get_cluster_offset(bs, sector_num << 9, &bytes,
&cluster_offset);
qemu_co_mutex_unlock(&s->lock);
>
>
> > so i think it's necessary to clock_gettime each time.
> > but clock_gettime only available on linux. maybe clock() is better.
>
qemu_clock_get_ms(QEMU_CLOCK_REALTIME) is a better option.
> Thanks, Juan.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] migration/block:limit the time used for block migration
2017-03-29 13:21 ` 858585 jemmy
2017-03-29 15:57 ` Juan Quintela
@ 2017-04-05 7:38 ` 858585 jemmy
1 sibling, 0 replies; 7+ messages in thread
From: 858585 jemmy @ 2017-04-05 7:38 UTC (permalink / raw)
To: quintela
Cc: qemu-devel, stefanha, Fam Zheng, dgilbert, qemu-block, Lidong Chen
On Wed, Mar 29, 2017 at 9:21 PM, 858585 jemmy <jemmy858585@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 5:47 PM, Juan Quintela <quintela@redhat.com> wrote:
>> Lidong Chen <jemmy858585@gmail.com> wrote:
>>> when migration with quick speed, mig_save_device_bulk invoke
>>> bdrv_is_allocated too frequently, and cause vnc reponse slowly.
>>> this patch limit the time used for bdrv_is_allocated.
>>>
>>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>>> ---
>>> migration/block.c | 39 +++++++++++++++++++++++++++++++--------
>>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/migration/block.c b/migration/block.c
>>> index 7734ff7..d3e81ca 100644
>>> --- a/migration/block.c
>>> +++ b/migration/block.c
>>> @@ -110,6 +110,7 @@ typedef struct BlkMigState {
>>> int transferred;
>>> int prev_progress;
>>> int bulk_completed;
>>> + int time_ns_used;
>>
>> An int that can only take values 0/1 is called a bool O:-)
> time_ns_used is used to store how many ns used by bdrv_is_allocated.
>
>>
>>
>>> if (bmds->shared_base) {
>>> qemu_mutex_lock_iothread();
>>> aio_context_acquire(blk_get_aio_context(bb));
>>> /* Skip unallocated sectors; intentionally treats failure as
>>> * an allocated sector */
>>> - while (cur_sector < total_sectors &&
>>> - !bdrv_is_allocated(blk_bs(bb), cur_sector,
>>> - MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>>> - cur_sector += nr_sectors;
>>> + while (cur_sector < total_sectors) {
>>> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
>>> + ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
>>> + MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
>>> + clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
>>
>> Do we really want to call clock_gettime each time that
>> bdrv_is_allocated() is called? My understanding is that clock_gettime
>> is expensive, but I don't know how expensive is brdrv_is_allocated()
>
> i write this code to measure the time used by brdrv_is_allocated()
>
> 279 static int max_time = 0;
> 280 int tmp;
>
> 288 clock_gettime(CLOCK_MONOTONIC_RAW, &ts1);
> 289 ret = bdrv_is_allocated(blk_bs(bb), cur_sector,
> 290 MAX_IS_ALLOCATED_SEARCH, &nr_sectors);
> 291 clock_gettime(CLOCK_MONOTONIC_RAW, &ts2);
> 292
> 293
> 294 tmp = (ts2.tv_sec - ts1.tv_sec)*1000000000L
> 295 + (ts2.tv_nsec - ts1.tv_nsec);
> 296 if (tmp > max_time) {
> 297 max_time=tmp;
> 298 fprintf(stderr, "max_time is %d\n", max_time);
> 299 }
>
> the test result is below:
>
> max_time is 37014
> max_time is 1075534
> max_time is 17180913
> max_time is 28586762
> max_time is 49563584
> max_time is 103085447
> max_time is 110836833
> max_time is 120331438
>
> so i think it's necessary to clock_gettime each time.
> but clock_gettime only available on linux. maybe clock() is better.
>
>>
>> And while we are at it, .... shouldn't we check since before the while?
> i also check it in block_save_iterate.
> + MAX_INFLIGHT_IO &&
> + block_mig_state.time_ns_used <= 100000) {
>
>>
>>
>>> +
>>> + block_mig_state.time_ns_used += (ts2.tv_sec - ts1.tv_sec) * BILLION
>>> + + (ts2.tv_nsec - ts1.tv_nsec);
>>> +
>>> + if (!ret) {
>>> + cur_sector += nr_sectors;
>>> + if (block_mig_state.time_ns_used > 100000) {
>>> + timeout_flag = 1;
>>> + break;
>>> + }
>>> + } else {
>>> + break;
>>> + }
>>> }
>>> aio_context_release(blk_get_aio_context(bb));
>>> qemu_mutex_unlock_iothread();
>>> @@ -292,6 +311,11 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>>> return 1;
>>> }
>>>
>>> + if (timeout_flag == 1) {
>>> + bmds->cur_sector = bmds->completed_sectors = cur_sector;
>>> + return 0;
>>> + }
>>> +
>>> bmds->completed_sectors = cur_sector;
>>>
>>> cur_sector &= ~((int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK - 1);
>>> @@ -576,9 +600,6 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>>> }
>>>
>>> bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
>>> - sector += nr_sectors;
>>> - bmds->cur_dirty = sector;
>>> -
>>> break;
>>> }
>>> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
>>> @@ -756,6 +777,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>>> }
>>>
>>> blk_mig_reset_dirty_cursor();
>>> + block_mig_state.time_ns_used = 0;
>>>
>>> /* control the rate of transfer */
>>> blk_mig_lock();
>>> @@ -764,7 +786,8 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>>> qemu_file_get_rate_limit(f) &&
>>> (block_mig_state.submitted +
>>> block_mig_state.read_done) <
>>> - MAX_INFLIGHT_IO) {
>>> + MAX_INFLIGHT_IO &&
>>> + block_mig_state.time_ns_used <= 100000) {
>>
>> changed this 10.000 (and the one used previously) to one constant that
>> says BIG_DELAY, 100MS or whatever you fancy.
> ok,i will change to BIG_DELAY.
i find 10000 ns is too small, and will reduce the migration speed.
i will change BIG_DELAY to 500000. and the migration speed will not be effected.
i will resubmit this patch.
>
>>
>> Thanks, Juan.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-05 7:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28 9:23 [Qemu-devel] [RFC] migration/block:limit the time used for block migration Lidong Chen
2017-03-28 9:32 ` 858585 jemmy
2017-03-28 9:47 ` Juan Quintela
2017-03-29 13:21 ` 858585 jemmy
2017-03-29 15:57 ` Juan Quintela
2017-04-05 3:55 ` 858585 jemmy
2017-04-05 7:38 ` 858585 jemmy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.