* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-14 17:19 [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full Vladimir Sementsov-Ogievskiy
@ 2016-07-18 15:36 ` Denis V. Lunev
2016-07-20 17:36 ` Denis V. Lunev
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Denis V. Lunev @ 2016-07-18 15:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: stefanha, famz, mreitz, jcody, eblake, pbonzini, Kevin Wolf
On 07/14/2016 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
> Mirror can do up to 16 in-flight requests, but actually on full copy
> (the whole source disk is non-zero) in-flight is always 1. This happens
> as the request is not limited in size: the data occupies maximum available
> capacity of s->buf.
>
> The patch limits the size of the request to some artificial constant
> (1 Mb here), which is not that big or small. This effectively enables
> back parallelism in mirror code as it was designed.
>
> The result is important: the time to migrate 10 Gb disk is reduced from
> ~350 sec to 170 sec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>
> v2: in case of s->buf_size larger than default use it to limit io_sectors
>
> block/mirror.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index b1e633e..3ac3b4d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -23,7 +23,9 @@
>
> #define SLICE_TIME 100000000ULL /* ns */
> #define MAX_IN_FLIGHT 16
> -#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
> +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> +#define DEFAULT_MIRROR_BUF_SIZE \
> + (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
>
> /* The mirroring buffer is a list of granularity-sized chunks.
> * Free chunks are organized in a list.
> @@ -322,6 +324,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> int nb_chunks = 1;
> int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> + int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) / MAX_IN_FLIGHT,
> + MAX_IO_SECTORS);
>
> sector_num = hbitmap_iter_next(&s->hbi);
> if (sector_num < 0) {
> @@ -385,7 +389,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> nb_chunks * sectors_per_chunk,
> &io_sectors, &file);
> if (ret < 0) {
> - io_sectors = nb_chunks * sectors_per_chunk;
> + io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors);
> + } else if (ret & BDRV_BLOCK_DATA) {
> + io_sectors = MIN(io_sectors, max_io_sectors);
> }
>
> io_sectors -= io_sectors % sectors_per_chunk;
ping
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-14 17:19 [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full Vladimir Sementsov-Ogievskiy
2016-07-18 15:36 ` Denis V. Lunev
@ 2016-07-20 17:36 ` Denis V. Lunev
2016-07-20 19:08 ` Paolo Bonzini
2016-07-22 16:41 ` Max Reitz
` (3 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Denis V. Lunev @ 2016-07-20 17:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: stefanha, famz, mreitz, jcody, eblake, pbonzini, Kevin Wolf
On 07/14/2016 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
> Mirror can do up to 16 in-flight requests, but actually on full copy
> (the whole source disk is non-zero) in-flight is always 1. This happens
> as the request is not limited in size: the data occupies maximum available
> capacity of s->buf.
>
> The patch limits the size of the request to some artificial constant
> (1 Mb here), which is not that big or small. This effectively enables
> back parallelism in mirror code as it was designed.
>
> The result is important: the time to migrate 10 Gb disk is reduced from
> ~350 sec to 170 sec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>
> v2: in case of s->buf_size larger than default use it to limit io_sectors
>
> block/mirror.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index b1e633e..3ac3b4d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -23,7 +23,9 @@
>
> #define SLICE_TIME 100000000ULL /* ns */
> #define MAX_IN_FLIGHT 16
> -#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
> +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> +#define DEFAULT_MIRROR_BUF_SIZE \
> + (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
>
> /* The mirroring buffer is a list of granularity-sized chunks.
> * Free chunks are organized in a list.
> @@ -322,6 +324,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> int nb_chunks = 1;
> int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> + int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) / MAX_IN_FLIGHT,
> + MAX_IO_SECTORS);
>
> sector_num = hbitmap_iter_next(&s->hbi);
> if (sector_num < 0) {
> @@ -385,7 +389,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> nb_chunks * sectors_per_chunk,
> &io_sectors, &file);
> if (ret < 0) {
> - io_sectors = nb_chunks * sectors_per_chunk;
> + io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors);
> + } else if (ret & BDRV_BLOCK_DATA) {
> + io_sectors = MIN(io_sectors, max_io_sectors);
> }
>
> io_sectors -= io_sectors % sectors_per_chunk;
guys, what about this patch?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-20 17:36 ` Denis V. Lunev
@ 2016-07-20 19:08 ` Paolo Bonzini
2016-07-20 20:30 ` Denis V. Lunev
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2016-07-20 19:08 UTC (permalink / raw)
To: Denis V. Lunev
Cc: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel, stefanha,
famz, mreitz, jcody, eblake, Kevin Wolf
----- Original Message -----
> From: "Denis V. Lunev" <den@openvz.org>
> To: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-block@nongnu.org, qemu-devel@nongnu.org
> Cc: stefanha@redhat.com, famz@redhat.com, mreitz@redhat.com, jcody@redhat.com, eblake@redhat.com,
> pbonzini@redhat.com, "Kevin Wolf" <kwolf@redhat.com>
> Sent: Wednesday, July 20, 2016 7:36:00 PM
> Subject: Re: [PATCH v2] mirror: double performance of the bulk stage if the disc is full
>
> On 07/14/2016 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
> > Mirror can do up to 16 in-flight requests, but actually on full copy
> > (the whole source disk is non-zero) in-flight is always 1. This happens
> > as the request is not limited in size: the data occupies maximum available
> > capacity of s->buf.
> >
> > The patch limits the size of the request to some artificial constant
> > (1 Mb here), which is not that big or small. This effectively enables
> > back parallelism in mirror code as it was designed.
> >
> > The result is important: the time to migrate 10 Gb disk is reduced from
> > ~350 sec to 170 sec.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > CC: Fam Zheng <famz@redhat.com>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Max Reitz <mreitz@redhat.com>
> > CC: Jeff Cody <jcody@redhat.com>
> > CC: Eric Blake <eblake@redhat.com>
> > ---
> >
> > v2: in case of s->buf_size larger than default use it to limit io_sectors
> >
> > block/mirror.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index b1e633e..3ac3b4d 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -23,7 +23,9 @@
> >
> > #define SLICE_TIME 100000000ULL /* ns */
> > #define MAX_IN_FLIGHT 16
> > -#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
> > +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> > +#define DEFAULT_MIRROR_BUF_SIZE \
> > + (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
> >
> > /* The mirroring buffer is a list of granularity-sized chunks.
> > * Free chunks are organized in a list.
> > @@ -322,6 +324,8 @@ static uint64_t coroutine_fn
> > mirror_iteration(MirrorBlockJob *s)
> > int nb_chunks = 1;
> > int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> > int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> > + int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) /
> > MAX_IN_FLIGHT,
> > + MAX_IO_SECTORS);
> >
> > sector_num = hbitmap_iter_next(&s->hbi);
> > if (sector_num < 0) {
> > @@ -385,7 +389,9 @@ static uint64_t coroutine_fn
> > mirror_iteration(MirrorBlockJob *s)
> > nb_chunks * sectors_per_chunk,
> > &io_sectors, &file);
> > if (ret < 0) {
> > - io_sectors = nb_chunks * sectors_per_chunk;
> > + io_sectors = MIN(nb_chunks * sectors_per_chunk,
> > max_io_sectors);
> > + } else if (ret & BDRV_BLOCK_DATA) {
> > + io_sectors = MIN(io_sectors, max_io_sectors);
> > }
> >
> > io_sectors -= io_sectors % sectors_per_chunk;
> guys, what about this patch?
I think that at this point it has missed hard freeze. It's not up to me
whether to consider it a bugfix.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-20 19:08 ` Paolo Bonzini
@ 2016-07-20 20:30 ` Denis V. Lunev
0 siblings, 0 replies; 10+ messages in thread
From: Denis V. Lunev @ 2016-07-20 20:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel, stefanha,
famz, mreitz, jcody, eblake, Kevin Wolf
On 07/20/2016 10:08 PM, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Denis V. Lunev" <den@openvz.org>
>> To: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-block@nongnu.org, qemu-devel@nongnu.org
>> Cc: stefanha@redhat.com, famz@redhat.com, mreitz@redhat.com, jcody@redhat.com, eblake@redhat.com,
>> pbonzini@redhat.com, "Kevin Wolf" <kwolf@redhat.com>
>> Sent: Wednesday, July 20, 2016 7:36:00 PM
>> Subject: Re: [PATCH v2] mirror: double performance of the bulk stage if the disc is full
>>
>> On 07/14/2016 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Mirror can do up to 16 in-flight requests, but actually on full copy
>>> (the whole source disk is non-zero) in-flight is always 1. This happens
>>> as the request is not limited in size: the data occupies maximum available
>>> capacity of s->buf.
>>>
>>> The patch limits the size of the request to some artificial constant
>>> (1 Mb here), which is not that big or small. This effectively enables
>>> back parallelism in mirror code as it was designed.
>>>
>>> The result is important: the time to migrate 10 Gb disk is reduced from
>>> ~350 sec to 170 sec.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Fam Zheng <famz@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Max Reitz <mreitz@redhat.com>
>>> CC: Jeff Cody <jcody@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> ---
>>>
>>> v2: in case of s->buf_size larger than default use it to limit io_sectors
>>>
>>> block/mirror.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index b1e633e..3ac3b4d 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -23,7 +23,9 @@
>>>
>>> #define SLICE_TIME 100000000ULL /* ns */
>>> #define MAX_IN_FLIGHT 16
>>> -#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
>>> +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
>>> +#define DEFAULT_MIRROR_BUF_SIZE \
>>> + (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
>>>
>>> /* The mirroring buffer is a list of granularity-sized chunks.
>>> * Free chunks are organized in a list.
>>> @@ -322,6 +324,8 @@ static uint64_t coroutine_fn
>>> mirror_iteration(MirrorBlockJob *s)
>>> int nb_chunks = 1;
>>> int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
>>> int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
>>> + int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) /
>>> MAX_IN_FLIGHT,
>>> + MAX_IO_SECTORS);
>>>
>>> sector_num = hbitmap_iter_next(&s->hbi);
>>> if (sector_num < 0) {
>>> @@ -385,7 +389,9 @@ static uint64_t coroutine_fn
>>> mirror_iteration(MirrorBlockJob *s)
>>> nb_chunks * sectors_per_chunk,
>>> &io_sectors, &file);
>>> if (ret < 0) {
>>> - io_sectors = nb_chunks * sectors_per_chunk;
>>> + io_sectors = MIN(nb_chunks * sectors_per_chunk,
>>> max_io_sectors);
>>> + } else if (ret & BDRV_BLOCK_DATA) {
>>> + io_sectors = MIN(io_sectors, max_io_sectors);
>>> }
>>>
>>> io_sectors -= io_sectors % sectors_per_chunk;
>> guys, what about this patch?
> I think that at this point it has missed hard freeze. It's not up to me
> whether to consider it a bugfix.
>
> Paolo
I see. It's pity this patch has missed deadline...
The difference is really big.
OK ;) I think it will not be missed in the next
release.
Den
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-14 17:19 [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full Vladimir Sementsov-Ogievskiy
2016-07-18 15:36 ` Denis V. Lunev
2016-07-20 17:36 ` Denis V. Lunev
@ 2016-07-22 16:41 ` Max Reitz
2016-07-26 2:47 ` Jeff Cody
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Max Reitz @ 2016-07-22 16:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: stefanha, famz, jcody, eblake, pbonzini, Denis V. Lunev,
Kevin Wolf
[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]
On 14.07.2016 19:19, Vladimir Sementsov-Ogievskiy wrote:
> Mirror can do up to 16 in-flight requests, but actually on full copy
> (the whole source disk is non-zero) in-flight is always 1. This happens
> as the request is not limited in size: the data occupies maximum available
> capacity of s->buf.
>
> The patch limits the size of the request to some artificial constant
> (1 Mb here), which is not that big or small. This effectively enables
> back parallelism in mirror code as it was designed.
>
> The result is important: the time to migrate 10 Gb disk is reduced from
> ~350 sec to 170 sec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>
> v2: in case of s->buf_size larger than default use it to limit io_sectors
>
> block/mirror.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
I think that this can be considered a bugfix, but strictly speaking it's
a patch for Jeff's tree, so I'm hesitant to take it.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-14 17:19 [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2016-07-22 16:41 ` Max Reitz
@ 2016-07-26 2:47 ` Jeff Cody
2016-07-26 2:48 ` Jeff Cody
2016-08-03 10:52 ` Kevin Wolf
5 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2016-07-26 2:47 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, stefanha, famz, mreitz, eblake, pbonzini,
Denis V. Lunev, Kevin Wolf
On Thu, Jul 14, 2016 at 08:19:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Mirror can do up to 16 in-flight requests, but actually on full copy
> (the whole source disk is non-zero) in-flight is always 1. This happens
> as the request is not limited in size: the data occupies maximum available
> capacity of s->buf.
>
> The patch limits the size of the request to some artificial constant
> (1 Mb here), which is not that big or small. This effectively enables
> back parallelism in mirror code as it was designed.
>
> The result is important: the time to migrate 10 Gb disk is reduced from
> ~350 sec to 170 sec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>
> v2: in case of s->buf_size larger than default use it to limit io_sectors
>
> block/mirror.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index b1e633e..3ac3b4d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -23,7 +23,9 @@
>
> #define SLICE_TIME 100000000ULL /* ns */
> #define MAX_IN_FLIGHT 16
> -#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
> +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> +#define DEFAULT_MIRROR_BUF_SIZE \
> + (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
>
> /* The mirroring buffer is a list of granularity-sized chunks.
> * Free chunks are organized in a list.
> @@ -322,6 +324,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> int nb_chunks = 1;
> int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> + int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) / MAX_IN_FLIGHT,
> + MAX_IO_SECTORS);
>
> sector_num = hbitmap_iter_next(&s->hbi);
> if (sector_num < 0) {
> @@ -385,7 +389,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> nb_chunks * sectors_per_chunk,
> &io_sectors, &file);
> if (ret < 0) {
> - io_sectors = nb_chunks * sectors_per_chunk;
> + io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors);
> + } else if (ret & BDRV_BLOCK_DATA) {
> + io_sectors = MIN(io_sectors, max_io_sectors);
> }
>
> io_sectors -= io_sectors % sectors_per_chunk;
> --
> 1.8.3.1
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
I agree with Max, I think this qualifies as a bug fix, as the current
behavior is not what I'd expect as the intended behavior. I'll pull this in
through my tree, for my -rc1 pull req.
Jeff
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-14 17:19 [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2016-07-26 2:47 ` Jeff Cody
@ 2016-07-26 2:48 ` Jeff Cody
2016-08-03 10:52 ` Kevin Wolf
5 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2016-07-26 2:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, stefanha, famz, mreitz, eblake, pbonzini,
Denis V. Lunev, Kevin Wolf
On Thu, Jul 14, 2016 at 08:19:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Mirror can do up to 16 in-flight requests, but actually on full copy
> (the whole source disk is non-zero) in-flight is always 1. This happens
> as the request is not limited in size: the data occupies maximum available
> capacity of s->buf.
>
> The patch limits the size of the request to some artificial constant
> (1 Mb here), which is not that big or small. This effectively enables
> back parallelism in mirror code as it was designed.
>
> The result is important: the time to migrate 10 Gb disk is reduced from
> ~350 sec to 170 sec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>
> v2: in case of s->buf_size larger than default use it to limit io_sectors
>
> block/mirror.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index b1e633e..3ac3b4d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -23,7 +23,9 @@
>
> #define SLICE_TIME 100000000ULL /* ns */
> #define MAX_IN_FLIGHT 16
> -#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
> +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> +#define DEFAULT_MIRROR_BUF_SIZE \
> + (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
>
> /* The mirroring buffer is a list of granularity-sized chunks.
> * Free chunks are organized in a list.
> @@ -322,6 +324,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> int nb_chunks = 1;
> int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> + int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) / MAX_IN_FLIGHT,
> + MAX_IO_SECTORS);
>
> sector_num = hbitmap_iter_next(&s->hbi);
> if (sector_num < 0) {
> @@ -385,7 +389,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> nb_chunks * sectors_per_chunk,
> &io_sectors, &file);
> if (ret < 0) {
> - io_sectors = nb_chunks * sectors_per_chunk;
> + io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors);
> + } else if (ret & BDRV_BLOCK_DATA) {
> + io_sectors = MIN(io_sectors, max_io_sectors);
> }
>
> io_sectors -= io_sectors % sectors_per_chunk;
> --
> 1.8.3.1
>
Thanks,
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc.git block
-Jeff
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-07-14 17:19 [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2016-07-26 2:48 ` Jeff Cody
@ 2016-08-03 10:52 ` Kevin Wolf
2016-08-03 12:20 ` Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 10+ messages in thread
From: Kevin Wolf @ 2016-08-03 10:52 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, stefanha, famz, mreitz, jcody, eblake,
pbonzini, Denis V. Lunev
Am 14.07.2016 um 19:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Mirror can do up to 16 in-flight requests, but actually on full copy
> (the whole source disk is non-zero) in-flight is always 1. This happens
> as the request is not limited in size: the data occupies maximum available
> capacity of s->buf.
>
> The patch limits the size of the request to some artificial constant
> (1 Mb here), which is not that big or small. This effectively enables
> back parallelism in mirror code as it was designed.
>
> The result is important: the time to migrate 10 Gb disk is reduced from
> ~350 sec to 170 sec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
This broke qemu-iotests 109 for raw. Can you please check whether the
output changes are expected, and send a fix either for the code or for
the test case?
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full
2016-08-03 10:52 ` Kevin Wolf
@ 2016-08-03 12:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-08-03 12:20 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, qemu-devel, stefanha, famz, mreitz, jcody, eblake,
pbonzini, Denis V. Lunev
On 03.08.2016 13:52, Kevin Wolf wrote:
> Am 14.07.2016 um 19:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Mirror can do up to 16 in-flight requests, but actually on full copy
>> (the whole source disk is non-zero) in-flight is always 1. This happens
>> as the request is not limited in size: the data occupies maximum available
>> capacity of s->buf.
>>
>> The patch limits the size of the request to some artificial constant
>> (1 Mb here), which is not that big or small. This effectively enables
>> back parallelism in mirror code as it was designed.
>>
>> The result is important: the time to migrate 10 Gb disk is reduced from
>> ~350 sec to 170 sec.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
> This broke qemu-iotests 109 for raw. Can you please check whether the
> output changes are expected, and send a fix either for the code or for
> the test case?
>
> Kevin
As mirror becomes "more asynchronous" after this patch, in this test we
have the following behaviour change:
old behaviour:
we create one sync request, which occupies all memory for in-flight
requests, so it is only one in-flight request. It fails and we see
BLOCK_JOB_COMPLETED (with zero progress) immediately after BLOCK_JOB_ERROR
new behaviour
several requests are created in parallel, so some of them are finished
with success and we see BLOCK_JOB_COMPLETED with some progress..
I'm afaraid that this "some progress" may depend on disk speed or other
environment factors, so simply update 109.out to new value is not good
solution.
Anyway, I think
+ if (s->ret < 0) {
+ return 0;
+ }
should be added before io request creation in mirror_iteration - to not
create new async reqs after error. This reduce "some progress", but is
is still not zero, as some of async req's was started before an error
has occured. Moreover, some req's was finished with success before this
error for first chank has occured.
So, the progress of the job in case of error becomes unpredicatable and
should be filtered in io tests.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread