From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y82Bh-0002Vw-Vb for qemu-devel@nongnu.org; Mon, 05 Jan 2015 02:34:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y82Be-0006ca-Mx for qemu-devel@nongnu.org; Mon, 05 Jan 2015 02:34:53 -0500 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:58662 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y82Be-0006c8-CR for qemu-devel@nongnu.org; Mon, 05 Jan 2015 02:34:50 -0500 Message-ID: <54AA3E84.8080003@kamp.de> Date: Mon, 05 Jan 2015 08:34:28 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1419931250-19259-1-git-send-email-den@openvz.org> <1419931250-19259-2-git-send-email-den@openvz.org> In-Reply-To: <1419931250-19259-2-git-send-email-den@openvz.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 1/8] block: prepare bdrv_co_do_write_zeroes to deal with large bl.max_write_zeroes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 30.12.2014 10:20, Denis V. Lunev wrote: > bdrv_co_do_write_zeroes split writes using bl.max_write_zeroes or > 16 MiB as a chunk size. This is implemented in this way to tolerate > buggy block backends which do not accept too big requests. > > Though if the bdrv_co_write_zeroes callback is not good enough, we > fallback to write data explicitely using bdrv_co_writev and we > create buffer to accomodate zeroes inside. The size of this buffer > is the size of the chunk. Thus if the underlying layer will have > bl.max_write_zeroes high enough, f.e. 4 GiB, the allocation can fail. > > Actually, there is no need to allocate such a big amount of memory. > We could simply allocate 1 MiB buffer and create iovec, which will > point to the same memory. > > Signed-off-by: Denis V. Lunev > CC: Kevin Wolf > CC: Stefan Hajnoczi > CC: Peter Lieven > --- > block.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/block.c b/block.c > index 4165d42..d69c121 100644 > --- a/block.c > +++ b/block.c > @@ -3173,14 +3173,18 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs, > * of 32768 512-byte sectors (16 MiB) per request. > */ > #define MAX_WRITE_ZEROES_DEFAULT 32768 > +/* allocate iovec with zeroes using 1 MiB chunks to avoid to big allocations */ > +#define MAX_ZEROES_CHUNK (1024 * 1024) > > static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, > int64_t sector_num, int nb_sectors, BdrvRequestFlags flags) > { > BlockDriver *drv = bs->drv; > QEMUIOVector qiov; > - struct iovec iov = {0}; > int ret = 0; > + void *chunk = NULL; > + > + qemu_iovec_init(&qiov, 0); > > int max_write_zeroes = bs->bl.max_write_zeroes ? > bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT; > @@ -3217,27 +3221,35 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, > } > > if (ret == -ENOTSUP) { > + int64_t num_bytes = (int64_t)num << BDRV_SECTOR_BITS; > + int chunk_size = MIN(MAX_ZEROES_CHUNK, num_bytes); > + > /* Fall back to bounce buffer if write zeroes is unsupported */ > - iov.iov_len = num * BDRV_SECTOR_SIZE; > - if (iov.iov_base == NULL) { > - iov.iov_base = qemu_try_blockalign(bs, num * BDRV_SECTOR_SIZE); > - if (iov.iov_base == NULL) { > + if (chunk == NULL) { > + chunk = qemu_try_blockalign(bs, chunk_size); > + if (chunk == NULL) { > ret = -ENOMEM; > goto fail; > } > - memset(iov.iov_base, 0, num * BDRV_SECTOR_SIZE); > + memset(chunk, 0, chunk_size); > + } > + > + while (num_bytes > 0) { > + int to_add = MIN(chunk_size, num_bytes); > + qemu_iovec_add(&qiov, chunk, to_add); This can and likely will fail for big num_bytes if you exceed IOV_MAX vectors. I would stick to the old method and limit the num to a reasonable value e.g. MAX_WRITE_ZEROES_DEFAULT. This becomes necessary as you set INT_MAX for max_write_zeroes. That hasn't been considered before in the original patch. Peter > + num_bytes -= to_add; > } > - qemu_iovec_init_external(&qiov, &iov, 1); > > ret = drv->bdrv_co_writev(bs, sector_num, num, &qiov); > > /* Keep bounce buffer around if it is big enough for all > * all future requests. > */ > - if (num < max_write_zeroes) { > - qemu_vfree(iov.iov_base); > - iov.iov_base = NULL; > + if (chunk_size != MAX_ZEROES_CHUNK) { > + qemu_vfree(chunk); > + chunk = NULL; > } > + qemu_iovec_reset(&qiov); > } > > sector_num += num; > @@ -3245,7 +3257,8 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, > } > > fail: > - qemu_vfree(iov.iov_base); > + qemu_iovec_destroy(&qiov); > + qemu_vfree(chunk); > return ret; > } > -- Mit freundlichen Grüßen Peter Lieven ........................................................... KAMP Netzwerkdienste GmbH Vestische Str. 89-91 | 46117 Oberhausen Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40 pl@kamp.de | http://www.kamp.de Geschäftsführer: Heiner Lante | Michael Lante Amtsgericht Duisburg | HRB Nr. 12154 USt-Id-Nr.: DE 120607556 ...........................................................