From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44249) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WHvST-00019z-9M for qemu-devel@nongnu.org; Mon, 24 Feb 2014 08:20:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WHvSN-0001Yt-8S for qemu-devel@nongnu.org; Mon, 24 Feb 2014 08:20:32 -0500 Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:34871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WHvSM-0001YZ-UV for qemu-devel@nongnu.org; Mon, 24 Feb 2014 08:20:27 -0500 Received: by mail-wi0-f175.google.com with SMTP id hm4so3047108wib.8 for ; Mon, 24 Feb 2014 05:20:26 -0800 (PST) Date: Mon, 24 Feb 2014 14:20:23 +0100 From: Stefan Hajnoczi Message-ID: <20140224132023.GC15488@stefanha-thinkpad.redhat.com> References: <93A91DC620F1514FB39E878E5131C8CF0E3607FF@nkgeml511-mbx.china.huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <93A91DC620F1514FB39E878E5131C8CF0E3607FF@nkgeml511-mbx.china.huawei.com> Subject: Re: [Qemu-devel] [PATCH] drive-mirror: Change the amount of data base on granularity List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chentao (Boby)" Cc: Kevin Wolf , Fam Zheng , "Wangting (Kathy)" , qemu-devel , Qinling , Paolo Bonzini , "Zhangmin (Rudy)" , "Wubin (H)" On Sat, Jan 18, 2014 at 08:09:43AM +0000, Chentao (Boby) wrote: Please CC Kevin Wolf who co-maintains the QEMU block layer with me. Use scripts/get_maintainer.pl -f block/mirror.c to find the list of maintainers to CC. > Before, one iteration send the amount of data is continuous dirty block, maximum is mirror buffer size(default is 10M). > > This way has a low write/read performance. If image type is raw, first loop, all the data is dirty. > > One iteration, read 10M data and then write 10M data to target image, so read and write cannot be parallelized. > > > > Now, I change the amount of data in an iteration, it base on granularity. We can set the granularity to 1M,so it can send > > 10 times read request, and then send write request. Once a write request is done, it will have 1M free buffer to send next read request. > > So this way can allow read/write to be parallelized. > > > > This change can improve read and write performance. > > On my server: > > (write) MBps:55MB/S --> 90 MB/S utility:50%->85% > > > > Signed-off-by: Zhang Min > > This patch is not signed off by you. If you have permission to contribute this patch on behalf of Zhang Min, please add your own Signed-off-by: below. Please also remove the link. It should just be: First Last > --- > > block/mirror.c | 68 ++++++++++++++++++++++--------------------------------- > > 1 files changed, 27 insertions(+), 41 deletions(-) > > > > diff --git a/block/mirror.c b/block/mirror.c index 2932bab..1ba2862 100644 > > --- a/block/mirror.c Please do not send patches with Outlook or in HTML. Use git-send-email(1) to send patches that are properly formatted and can be applied with git-am(1) by the maintainers. For more info on patch submission guidelines, please see: http://qemu-project.org/Contribute/SubmitAPatch I have reformatted the remainder of this email. > +++ b/block/mirror.c > @@ -183,54 +183,40 @@ static void coroutine_fn mirror_iteration(MirrorBlockJob *s) > qemu_coroutine_yield(); > } > > - do { > - int added_sectors, added_chunks; > + int added_sectors, added_chunks; > > - if (!bdrv_get_dirty(source, s->dirty_bitmap, next_sector) || > - test_bit(next_chunk, s->in_flight_bitmap)) { > - assert(nb_sectors > 0); > - break; > - } > + added_sectors = sectors_per_chunk; > + if (s->cow_bitmap && !test_bit(next_chunk, s->cow_bitmap)) { > + bdrv_round_to_clusters(s->target, > + next_sector, added_sectors, > + &next_sector, &added_sectors); > > - added_sectors = sectors_per_chunk; > - if (s->cow_bitmap && !test_bit(next_chunk, s->cow_bitmap)) { > - bdrv_round_to_clusters(s->target, > - next_sector, added_sectors, > - &next_sector, &added_sectors); > - > - /* On the first iteration, the rounding may make us copy > - * sectors before the first dirty one. > - */ > - if (next_sector < sector_num) { > - assert(nb_sectors == 0); > - sector_num = next_sector; > - next_chunk = next_sector / sectors_per_chunk; > - } > + /* On the first iteration, the rounding may make us copy > + * sectors before the first dirty one. > + */ > + if (next_sector < sector_num) { > + assert(nb_sectors == 0); > + sector_num = next_sector; > + next_chunk = next_sector / sectors_per_chunk; > } > + } > > - added_sectors = MIN(added_sectors, end - (sector_num + nb_sectors)); > - added_chunks = (added_sectors + sectors_per_chunk - 1) / sectors_per_chunk; > + added_sectors = MIN(added_sectors, end - (sector_num + nb_sectors)); > + added_chunks = (added_sectors + sectors_per_chunk - 1) / > + sectors_per_chunk; > > - /* When doing COW, it may happen that there is not enough space for > - * a full cluster. Wait if that is the case. > - */ > - while (nb_chunks == 0 && s->buf_free_count < added_chunks) { > - trace_mirror_yield_buf_busy(s, nb_chunks, s->in_flight); > - qemu_coroutine_yield(); > - } > - if (s->buf_free_count < nb_chunks + added_chunks) { > - trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight); > - break; > - } > + /* When doing COW, it may happen that there is not enough space for > + * a full cluster. Wait if that is the case. > + */ > + while (nb_chunks == 0 && s->buf_free_count < added_chunks) { > + trace_mirror_yield_buf_busy(s, nb_chunks, s->in_flight); > + qemu_coroutine_yield(); > + } > > - /* We have enough free space to copy these sectors. */ > - bitmap_set(s->in_flight_bitmap, next_chunk, added_chunks); > + /* We have enough free space to copy these sectors. */ > + bitmap_set(s->in_flight_bitmap, next_chunk, added_chunks); > > - nb_sectors += added_sectors; > - nb_chunks += added_chunks; > - next_sector += added_sectors; > - next_chunk += added_chunks; > - } while (next_sector < end); > + nb_sectors += added_sectors; > + nb_chunks += added_chunks; I think further discussion will be required, this patch undoes something that the code is designed to do. You forgot to modify the big block comment above this code that explains that we are trying to extend the copy region to be at least one cluster and also to cover adjacent dirty blocks (to reduce the number of I/O requests). I have CCed Paolo Bonzini, who implemented most of block/mirror.c. Maybe he can discuss the purpose of this change with you.