Re: [Qemu-devel] [PATCH] block: posix: Always allocate the first block

[Nir Soffer]

On Thu, Aug 22, 2019 at 5:28 PM Max Reitz <address@hidden> wrote:

> On 16.08.19 23:21, Nir Soffer wrote:
> > When creating an image with preallocation "off" or "falloc", the first
> > block of the image is typically not allocated. When using Gluster
> > storage backed by XFS filesystem, reading this block using direct I/O
> > succeeds regardless of request length, fooling alignment detection.
> >
> > In this case we fallback to a safe value (4096) instead of the optimal
> > value (512), which may lead to unneeded data copying when aligning
> > requests.  Allocating the first block avoids the fallback.
> >
> > When using preallocation=off, we always allocate at least one filesystem
> > block:
> >
> >     $ ./qemu-img create -f raw test.raw 1g
> >     Formatting 'test.raw', fmt=raw size=1073741824
> >
> >     $ ls -lhs test.raw
> >     4.0K -rw-r--r--. 1 nsoffer nsoffer 1.0G Aug 16 23:48 test.raw
> >
> > I did quick performance tests for these flows:
> > - Provisioning a VM with a new raw image.
> > - Copying disks with qemu-img convert to new raw target image
> >
> > I installed Fedora 29 server on raw sparse image, measuring the time
> > from clicking "Begin installation" until the "Reboot" button appears:
> >
> > Before(s)  After(s)     Diff(%)
> > -------------------------------
> >      356        389        +8.4
> >
> > I ran this only once, so we cannot tell much from these results.
>
> So you’d expect it to be fast but it was slower?  Well, you only ran it
> once and it isn’t really a precise benchmark...
>
> > The second test was cloning the installation image with qemu-img
> > convert, doing 10 runs:
> >
> >     for i in $(seq 10); do
> >         rm -f dst.raw
> >         sleep 10
> >         time ./qemu-img convert -f raw -O raw -t none -T none src.raw
> dst.raw
> >     done
> >
> > Here is a table comparing the total time spent:
> >
> > Type    Before(s)   After(s)    Diff(%)
> > ---------------------------------------
> > real      530.028    469.123      -11.4
> > user       17.204     10.768      -37.4
> > sys        17.881      7.011      -60.7
> >
> > Here we see very clear improvement in CPU usage.
> >
> > Signed-off-by: Nir Soffer <address@hidden>
> > ---
> >  block/file-posix.c         | 25 +++++++++++++++++++++++++
> >  tests/qemu-iotests/150.out |  1 +
> >  tests/qemu-iotests/160     |  4 ++++
> >  tests/qemu-iotests/175     | 19 +++++++++++++------
> >  tests/qemu-iotests/175.out |  8 ++++----
> >  tests/qemu-iotests/221.out | 12 ++++++++----
> >  tests/qemu-iotests/253.out | 12 ++++++++----
> >  7 files changed, 63 insertions(+), 18 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index b9c33c8f6c..3964dd2021 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1755,6 +1755,27 @@ static int handle_aiocb_discard(void *opaque)
> >      return ret;
> >  }
> >
> > +/*
> > + * Help alignment detection by allocating the first block.
> > + *
> > + * When reading with direct I/O from unallocated area on Gluster backed
> by XFS,
> > + * reading succeeds regardless of request length. In this case we
> fallback to
> > + * safe aligment which is not optimal. Allocating the first block
> avoids this
> > + * fallback.
> > + *
> > + * Returns: 0 on success, -errno on failure.
> > + */
> > +static int allocate_first_block(int fd)
> > +{
> > +    ssize_t n;
> > +
> > +    do {
> > +        n = pwrite(fd, "\0", 1, 0);
>
> This breaks when fd has been opened with O_DIRECT.
>

It seems that we always open images without O_DIRECT when creating an image
in qemu-img create, or when creating a target image in qemu-img convert.

Here is a trace of qemu-img create:

$ strace -f -tt -o /tmp/create.trace ./qemu-img create -f raw -o
preallocation=falloc /tmp/gv1/src.raw 1g
Formatting '/tmp/gv1/src.raw', fmt=raw size=1073741824 preallocation=falloc

1. open #1

17686 18:58:23.681921 openat(AT_FDCWD, "/tmp/gv1/src.raw",
O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 9
17686 18:58:23.683753 fstat(9, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
17686 18:58:23.683829 close(9)          = 0

2. open #2

17686 18:58:23.684146 openat(AT_FDCWD, "/tmp/gv1/src.raw",
O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 9
17686 18:58:23.684227 fstat(9, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
17686 18:58:23.684256 close(9)          = 0

3. open #3

17686 18:58:23.684339 openat(AT_FDCWD, "/tmp/gv1/src.raw",
O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 9
...
17688 18:58:23.690178 fstat(9, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
17688 18:58:23.690217 ftruncate(9, 0 <unfinished ...>
...
17688 18:58:23.700266 <... ftruncate resumed>) = 0
...
17688 18:58:23.700595 fstat(9,  <unfinished ...>
...
17688 18:58:23.700619 <... fstat resumed>{st_mode=S_IFREG|0600, st_size=0,
...}) = 0
...
17688 18:58:23.700651 fallocate(9, 0, 0, 1073741824 <unfinished ...>
...
17688 18:58:23.728141 <... fallocate resumed>) = 0
...
17688 18:58:23.728196 pwrite64(9, "\0", 1, 0) = 1
...
17686 18:58:23.738391 close(9)          = 0

Here is convert trace:

$ strace -f -tt -o /tmp/convert.trace ./qemu-img convert -f raw -O raw -t
none -T none /tmp/gv1/src.raw /tmp/gv1/dst.raw

1. open #1

18175 19:07:23.364417 openat(AT_FDCWD, "/tmp/gv1/dst.raw",
O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 10
18175 19:07:23.365282 fstat(10, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
18175 19:07:23.365323 close(10)         = 0

2. open #2

18175 19:07:23.365660 openat(AT_FDCWD, "/tmp/gv1/dst.raw",
O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 10
18175 19:07:23.365717 fstat(10, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
18175 19:07:23.365742 close(10)         = 0

3. open #3

18175 19:07:23.365839 openat(AT_FDCWD, "/tmp/gv1/dst.raw",
O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 10
...
18177 19:07:23.372112 fstat(10, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
18177 19:07:23.372138 ftruncate(10, 0)  = 0
...
18177 19:07:23.375760 fstat(10, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
18177 19:07:23.375788 ftruncate(10, 1073741824) = 0
18177 19:07:23.376383 pwrite64(10, "\0", 1, 0) = 1
...
18175 19:07:23.390989 close(10)         = 0

4. open #4

18175 19:07:23.391429 openat(AT_FDCWD, "/tmp/gv1/dst.raw",
O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 10
18175 19:07:23.392433 fstat(10, {st_mode=S_IFREG|0600, st_size=1073741824,
...}) = 0
18175 19:07:23.392483 close(10)         = 0

5. open #5

18175 19:07:23.392743 openat(AT_FDCWD, "/tmp/gv1/dst.raw",
O_RDWR|O_DIRECT|O_CLOEXEC) = 10
...
18175 19:07:23.393731 ioctl(10, BLKSSZGET, 0x7ffe75ead334) = -1 ENOSYS
(Function not implemented)
18175 19:07:23.393784 pread64(10, 0x558796451000, 1, 0) = -1 EINVAL
(Invalid argument)
18175 19:07:23.395362 pread64(10,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 512,
0) = 512
18175 19:07:23.395905 pread64(10,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
4096, 0) = 4096
...

(Which happens when you open some file with cache.direct=on, and then
> use e.g. QMP’s block_resize.)
>

What would be a command triggering this? I can add a test.

It isn’t that bad because eventually you simply ignore the error.  But
> it still makes me wonder whether we shouldn’t write like the biggest
> power of two that does not exceed the new file length or MAX_BLOCKSIZE.
>

It makes sense if there is a way to cause qemu-img to use O_DIRECT when
creating an image.

> +    } while (n == -1 && errno == EINTR);
> > +
> > +    return (n == -1) ? -errno : 0;
> > +}
> > +
> >  static int handle_aiocb_truncate(void *opaque)
> >  {
> >      RawPosixAIOData *aiocb = opaque;
> > @@ -1794,6 +1815,8 @@ static int handle_aiocb_truncate(void *opaque)
> >                  /* posix_fallocate() doesn't set errno. */
> >                  error_setg_errno(errp, -result,
> >                                   "Could not preallocate new data");
> > +            } else if (current_length == 0) {
> > +                allocate_first_block(fd);
>
> Should posix_fallocate() not take care of precisely this?
>

Only if the filesystem does not support fallocate() (e.g. NFS < 4.2).

In this case posix_fallocate() is doing:

  for (offset += (len - 1) % increment; len > 0; offset += increment)
    {
      len -= increment;
      if (offset < st.st_size)
        {
          unsigned char c;
          ssize_t rsize = __pread (fd, &c, 1, offset);
          if (rsize < 0)
            return errno;
          /* If there is a non-zero byte, the block must have been
             allocated already.  */
          else if (rsize == 1 && c != 0)
            continue;
        }
      if (__pwrite (fd, "", 1, offset) != 1)
        return errno;
    }

https://code.woboq.org/userspace/glibc/sysdeps/posix/posix_fallocate.c.html#96

So opening a file with O_DIRECT will break preallocation=falloc on such
filesystems,
and writing one byte in allocate_first_block() is safe.

> >              }
> >          } else {
> >              result = 0;
>
> [...]
>
> > diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160
> > index df89d3864b..ad2d054a47 100755
> > --- a/tests/qemu-iotests/160
> > +++ b/tests/qemu-iotests/160
> > @@ -57,6 +57,10 @@ for skip in $TEST_SKIP_BLOCKS; do
> >      $QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" skip="$skip" -O
> "$IMGFMT" \
> >          2> /dev/null
> >      TEST_IMG="$TEST_IMG.out" _check_test_img
> > +
> > +    # We always write the first byte of an image.
> > +    printf "\0" > "$TEST_IMG.out.dd"
> > +
> >      dd if="$TEST_IMG" of="$TEST_IMG.out.dd" skip="$skip" status=none
>
> Won’t this dd completely overwrite $TEST_IMG.out.dd (especially given
> the lack of conv=notrunc)?
>

There is an issue only if dd open the file with O_TRUNC. I will test this
again.

>
> >      echo
> > diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
> > index 51e62c8276..c6a3a7bb1e 100755
> > --- a/tests/qemu-iotests/175
> > +++ b/tests/qemu-iotests/175
> > @@ -37,14 +37,16 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> >  # the file size.  This function hides the resulting difference in the
> >  # stat -c '%b' output.
> >  # Parameter 1: Number of blocks an empty file occupies
> > -# Parameter 2: Image size in bytes
> > +# Parameter 2: Minimal number of blocks in an image
> > +# Parameter 3: Image size in bytes
> >  _filter_blocks()
> >  {
> >      extra_blocks=$1
> > -    img_size=$2
> > +    min_blocks=$2
> > +    img_size=$3
> >
> > -    sed -e "s/blocks=$extra_blocks\\(\$\\|[^0-9]\\)/nothing allocated/"
> \
> > -        -e "s/blocks=$((extra_blocks + img_size /
> 512))\\(\$\\|[^0-9]\\)/everything allocated/"
> > +    sed -e "s/blocks=$((extra_blocks +
> min_blocks))\\(\$\\|[^0-9]\\)/min allocation/" \
>
> I don’t think adding extra_blocks to min_blocks makes sense.  Just
> min_blocks alone should be what we want here.
>

We had failing tests (in vdsm) showing that filesystem may return more
blocs than
expected even for non-empty files, so this may be needed. I did not test
it yet with a filesystem that show this issue, but I found your instructions
how to create it.

Thanks for reviewing.

Nir