From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:55841)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1XPWGG-0001vK-7T
	for qemu-devel@nongnu.org; Thu, 04 Sep 2014 08:35:40 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1XPWGA-0006VI-No
	for qemu-devel@nongnu.org; Thu, 04 Sep 2014 08:35:36 -0400
Received: from mx1.redhat.com ([209.132.183.28]:46859)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1XPWGA-0006VE-3k
	for qemu-devel@nongnu.org; Thu, 04 Sep 2014 08:35:30 -0400
Date: Thu, 4 Sep 2014 14:35:22 +0200
From: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20140904123522.GH3897@noname.str.redhat.com>
References: <cover.1409299732.git.hutao@cn.fujitsu.com>
	<67d97abc587e7c6985166dbe800686938ac8adb5.1409299732.git.hutao@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <67d97abc587e7c6985166dbe800686938ac8adb5.1409299732.git.hutao@cn.fujitsu.com>
Subject: Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation
	option
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Hu Tao <hutao@cn.fujitsu.com>
Cc: Fam Zheng <famz@redhat.com>, "Richard W.M. Jones" <rjones@redhat.com>, qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Yasunori Goto <y-goto@jp.fujitsu.com>

Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> This patch adds a new option preallocation for raw format, and implements
> full preallocation.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>

v12 was better, it wasn't doing only metadata preallocation when you
told it to do full preallocation.

> +    if (prealloc == PREALLOC_MODE_FULL) {
> +        /* posix_fallocate() doesn't set errno. */
> +        result = -posix_fallocate(fd, 0, total_size);
> +        if (result != 0) {
> +            buf = g_malloc0(65536);
> +            int64_t num = 0, left = total_size;
> +
> +            while (left > 0) {
> +                num = MIN(left, 65536);
> +                result = write(fd, buf, num);
> +                if (result < 0) {
> +                    result = -errno;
> +                    error_setg_errno(errp, -result,
> +                                     "Could not write to the new file");
> +                    g_free(buf);
> +                    goto out_close;
> +                }
> +                left -= num;
> +            }
> +            fsync(fd);
> +            g_free(buf);
>          }

This is totally pointless. If the file system doesn't support fallocate,
posix_fallocate() will already try writing zeros.

Having code to write zeros in qemu only makes sense if you implement a
real full preallocation mode, which doesn't use fallocate even when it's
supported.

Please change the code to always write zeros for FULL, and either
reintroduce FALLOC or use METADATA for the fallocate (without a fallback
to zero writing code). In fact, for metadata preallocation we should
probably directly use fallocate(), which has no slow zero-write fallback
in the libc.

Kevin