[Qemu-devel] [PATCH 0/3] qemu-img raw preallocation

[Qemu-devel] [PATCH 0/3] qemu-img raw preallocation

[Nir Soffer]

This series add missing tests for raw image preallocation, refine
preallocation=full and improve documentation.

Create on top of the commit	10ddfe7b6044 (qemu-img: Do not truncate
before preallocation) from Kevin block branch.

Nir Soffer (3):
  qemu-img: Add tests for raw image preallocation
  qemu-img: Truncate before full preallocation
  qemu-img: Improve documentation for PREALLOC_MODE_FALLOC

 block/file-posix.c         | 19 ++++++++++++++-
 tests/qemu-iotests/175     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/175.out | 18 ++++++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] qemu-img: Add tests for raw image preallocation

[Nir Soffer]

Add tests for creating raw image with and without the preallocation
option.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 tests/qemu-iotests/175     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/175.out | 18 ++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 80 insertions(+)
 create mode 100755 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
new file mode 100755
index 0000000..ca56e82
--- /dev/null
+++ b/tests/qemu-iotests/175
@@ -0,0 +1,61 @@
+#!/bin/bash
+#
+# Test creating raw image preallocation mode
+#
+# Copyright (C) 2017 Nir Soffer <nirsof@gmail.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=nirsof@gmail.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+size=1m
+
+echo
+echo "== creating image with default preallocation =="
+_make_test_img $size | _filter_imgfmt
+stat -c "size=%s, blocks=%b" $TEST_IMG
+
+for mode in off full falloc; do
+    echo
+    echo "== creating image with preallocation $mode =="
+    IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
+    stat -c "size=%s, blocks=%b" $TEST_IMG
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
new file mode 100644
index 0000000..76c02c6
--- /dev/null
+++ b/tests/qemu-iotests/175.out
@@ -0,0 +1,18 @@
+QA output created by 175
+
+== creating image with default preallocation ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+size=1048576, blocks=0
+
+== creating image with preallocation off ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=off
+size=1048576, blocks=0
+
+== creating image with preallocation full ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=full
+size=1048576, blocks=2048
+
+== creating image with preallocation falloc ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=falloc
+size=1048576, blocks=2048
+ *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 985b9a6..1f4bf03 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -167,3 +167,4 @@
 172 auto
 173 rw auto
 174 auto
+175 auto quick
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/3] qemu-img: Add tests for raw image preallocation

[Kevin Wolf]

Am 17.02.2017 um 01:51 hat Nir Soffer geschrieben:
> Add tests for creating raw image with and without the preallocation
> option.
> 
> Signed-off-by: Nir Soffer <nirsof@gmail.com>

Looks good, but 175 is already (multiply) taken. Not making this a
blocker, but I just want to remind everyone to check the mailing list
for pending patches which add new tests before using a new number in
order to avoid unnecessary rebases for everyone. In general, it's as
easy as searching for the string "175.out" in the mailbox.

The next free one seems to be 177 currently.

Kevin

Re: [Qemu-devel] [PATCH 1/3] qemu-img: Add tests for raw image preallocation

[Nir Soffer]

On Fri, Feb 17, 2017 at 11:14 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 17.02.2017 um 01:51 hat Nir Soffer geschrieben:
>> Add tests for creating raw image with and without the preallocation
>> option.
>>
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>
> Looks good, but 175 is already (multiply) taken. Not making this a
> blocker, but I just want to remind everyone to check the mailing list
> for pending patches which add new tests before using a new number in
> order to avoid unnecessary rebases for everyone. In general, it's as
> easy as searching for the string "175.out" in the mailbox.
>
> The next free one seems to be 177 currently.

Thanks, will change to 177 in the next version.

For next patches, what do you mean by "pending"? patches sent
to the block mailing list?

Nir

Re: [Qemu-devel] [PATCH 1/3] qemu-img: Add tests for raw image preallocation

[Kevin Wolf]

Am 17.02.2017 um 15:20 hat Nir Soffer geschrieben:
> On Fri, Feb 17, 2017 at 11:14 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 17.02.2017 um 01:51 hat Nir Soffer geschrieben:
> >> Add tests for creating raw image with and without the preallocation
> >> option.
> >>
> >> Signed-off-by: Nir Soffer <nirsof@gmail.com>
> >
> > Looks good, but 175 is already (multiply) taken. Not making this a
> > blocker, but I just want to remind everyone to check the mailing list
> > for pending patches which add new tests before using a new number in
> > order to avoid unnecessary rebases for everyone. In general, it's as
> > easy as searching for the string "175.out" in the mailbox.
> >
> > The next free one seems to be 177 currently.
> 
> Thanks, will change to 177 in the next version.

If there needs to be a next version for other reasons. Otherwise it's
not important enough to respin, it just means that someone else will
have to rebase.

> For next patches, what do you mean by "pending"? patches sent
> to the block mailing list?

Yes, that's what I'm looking for when I add a new test case myself. It's
just an easy way to avoid stepping on each others toes.

Kevin

[Qemu-devel] [PATCH 2/3] qemu-img: Truncate before full preallocation

[Nir Soffer]

In commit 10ddfe7b6044 (qemu-img: Do not truncate before preallocation)
we moved truncate to the PREALLOC_MODE_OFF branch to avoid slowdown in
posix_fallocate().

However this change is not optimal when using PREALLOC_MODE_FULL, since
knowing the final size from the beginning could allow the file system
driver to do less allocations and possibly avoid fragmentation of the
file.

Now we truncate also before doing full preallocation.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 block/file-posix.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 442f080..d24e34b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1604,6 +1604,17 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 #endif
     case PREALLOC_MODE_FULL:
     {
+        /*
+         * Knowing the final size from the beginning could allow the file
+         * system driver to do less allocations and possibly avoid
+         * fragmentation of the file.
+         */
+        if (ftruncate(fd, total_size) != 0) {
+            result = -errno;
+            error_setg_errno(errp, -result, "Could not resize file");
+            goto out_close;
+        }
+
         int64_t num = 0, left = total_size;
         buf = g_malloc0(65536);
 
@@ -1642,6 +1653,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
         break;
     }
 
+out_close:
     if (qemu_close(fd) != 0 && result == 0) {
         result = -errno;
         error_setg_errno(errp, -result, "Could not close the new file");
-- 
2.9.3

[Qemu-devel] [PATCH 3/3] qemu-img: Improve documentation for PREALLOC_MODE_FALLOC

[Nir Soffer]

Now that we are truncating the file in both PREALLOC_MODE_FULL and
PREALLOC_MODE_OFF, not truncating in PREALLOC_MODE_FALLOC looks odd.
Add a comment explaining why we do not truncate in this case.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 block/file-posix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index d24e34b..20a261f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1594,9 +1594,14 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
     switch (prealloc) {
 #ifdef CONFIG_POSIX_FALLOCATE
     case PREALLOC_MODE_FALLOC:
-        /* posix_fallocate() doesn't set errno. */
+        /*
+         * Truncating before posix_fallocate() makes it about twice slower on
+         * file systems that do not support fallocate(), trying to check if a
+         * block is allocated before allocating it.
+         */
         result = -posix_fallocate(fd, 0, total_size);
         if (result != 0) {
+            /* posix_fallocate() doesn't set errno. */
             error_setg_errno(errp, -result,
                              "Could not preallocate data for the new file");
         }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 0/3] qemu-img raw preallocation

[Kevin Wolf]

Am 17.02.2017 um 01:51 hat Nir Soffer geschrieben:
> This series add missing tests for raw image preallocation, refine
> preallocation=full and improve documentation.
> 
> Create on top of the commit	10ddfe7b6044 (qemu-img: Do not truncate
> before preallocation) from Kevin block branch.

Thanks, applied to the block branch.

I changed the commit message of patch 2 so that it doesn't mention the
commit ID. This is because the commit ID is only stable once the commit
has made it into master. I also added a few words ("...so don't do that
here") to the comment in patch 3 because outside the context of the
patch, talking about a truncation seemed weird when there is no
truncation happening. I hope you're okay with these changes.

Kevin

Re: [Qemu-devel] [PATCH 0/3] qemu-img raw preallocation

[Nir Soffer]

On Wed, Feb 22, 2017 at 2:31 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 17.02.2017 um 01:51 hat Nir Soffer geschrieben:
>> This series add missing tests for raw image preallocation, refine
>> preallocation=full and improve documentation.
>>
>> Create on top of the commit   10ddfe7b6044 (qemu-img: Do not truncate
>> before preallocation) from Kevin block branch.
>
> Thanks, applied to the block branch.
>
> I changed the commit message of patch 2 so that it doesn't mention the
> commit ID. This is because the commit ID is only stable once the commit
> has made it into master. I also added a few words ("...so don't do that
> here") to the comment in patch 3 because outside the context of the
> patch, talking about a truncation seemed weird when there is no
> truncation happening. I hope you're okay with these changes.

Looks fine, thanks!