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

[Nir Soffer]

On Fri, Aug 23, 2019 at 4:58 PM Max Reitz <address@hidden> wrote:

> On 22.08.19 21:01, Nir Soffer wrote:
> > On Thu, Aug 22, 2019 at 9:11 PM Max Reitz <address@hidden
> > <mailto:address@hidden>> wrote:
> >
> >     On 22.08.19 18:39, Nir Soffer wrote:
> >     > On Thu, Aug 22, 2019 at 5:28 PM Max Reitz <address@hidden
> >     <mailto:address@hidden>
> >     > <mailto:address@hidden <mailto: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
> >     <mailto:address@hidden>
> >     >     <mailto:address@hidden <mailto: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.
> >
> >     Yes.  But you don’t call this function directly from image creation
> code
> >     but instead from the truncation function.  (The former also calls the
> >     latter, but truncating is also an operation on its own.)
> >
> >     [...]
> >
> >     >     (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.
> >
> >     block_resize, as I’ve said:
> >
> >     $ ./qemu-img create -f raw empty.img 0
> >
> >
> > This is extreme edge case - why would someone create such image?
>
> Because it works?
>
> This is generally the fist step of image creation with blockdev-create,
> because you don’t care about the size of the protocol layer.
>
> If you have a format layer that truncates the image to a fixed size and
> does not write anything into the first block itself (say because it uses
> a footer), then (with O_DIRECT) allocate_first_block() will fail
> (silently, because while it does return an error value, it is never
> checked and there is no comment that explains why we don’t check it)


The motivation is that this is an optimization for the special case of using
empty image, so it does not worth failing image creation.
I will add a comment about that.


> and
> the first block actually will not be allocated.
>
> I could show you that with VPC (which supports a fixed subformat where
> it uses a footer), but unfortunately that’s a bit broken right now
> (because of a bug in blockdev-create; I’ll send a patch).
>
> The test would go like this:
>
> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
> {"execute":"qmp_capabilities"}
>
> {"execute":"blockdev-create",
>  "arguments":{
>     "job-id":"create",
>     "options":{"driver":"file",
>                "filename":"test.img",
>                "size":0}}}
>
> [Wait until the job is pending]
>
> {"execute":"job-dismiss","arguments":{"id":"create"}}
>
> {"execute":"blockdev-add",
>  "arguments":{
>     "driver":"file",
>     "node-name":"protocol-node",
>     "filename":"test.img",
>     "cache":{"direct":true}}}
>
> {"execute":"blockdev-create",
>  "arguments":{
>     "job-id":"create",
>     "options":{"driver":"vpc",
>                "file":"protocol-node",
>                "subformat":"fixed",
>                "size":67108864,
>                "force-size":true}}}
>
> [Wait until the job is pending]
>
> {"execute":"job-dismiss","arguments":{"id":"create"}}
>
> {"execute":"quit"}
>
> And then:
>
> $ ./qemu-img map test.img
> Offset          Length          Mapped to       File
> 0x4000000       0x200           0x4000000       test.img
>
> The footer is mapped, but the first block is not allocated.
>

Thanks for the example.

I will need time to play with blockdev and understand the flows when image
are created. Do you think is would be useful to fix now only image creation
via qemu-img, and handle blockdev later?
...