Re: [Qemu-devel] [PATCH for 2.7 1/1] qcow2: improve qcow2_co_write_zeroes()

[Eric Blake]

On 04/25/2016 04:20 AM, Denis V. Lunev wrote:
> On 04/25/2016 12:05 PM, Kevin Wolf wrote:
>> Am 23.04.2016 um 14:05 hat Denis V. Lunev geschrieben:
>>> Unfortunately Linux kernel could send non-aligned requests to qemu-nbd
>>> if the caller is using O_DIRECT and does not align in-memory data to
>>> page. Thus qemu-nbd will call block layer with non-aligned requests.

At first glance, I'd argue that any caller using O_DIRECT without
obeying memory alignment restrictions is broken; why should qemu have to
work around such broken callers?


> The program is 100% reproducible. The following sequence
> is performed:
> 
> #define _GNU_SOURCE
> 
> #include <stdio.h>
> #include <malloc.h>
> #include <string.h>
> #include <sys/fcntl.h>
> #include <unistd.h>
> 
> int main(int argc, char *argv[])
> {
>     char *buf;
>     int fd;
> 
>     if (argc != 2) {
>         return -1;
>     }
> 
>     fd = open(argv[1], O_WRONLY | O_DIRECT);
> 
>     do {
>         buf = memalign(512, 1024 * 1024);
>     } while (!(unsigned long)buf & (4096 - 1));

In other words, you are INTENTIONALLY grabbing an unaligned buffer for
use with an O_DIRECT fd, when O_DIRECT demands that you should be using
at least page alignment (4096 or greater).  Arguably, the bug is in your
program, not qemu.

That said, teaching qemu to split up a write_zeroes request into head,
tail, and aligned body, so at least the aligned part might benefit from
optimizations, seems like it might be worthwhile, particularly since my
pending NBD series changed from blk_write_zeroes (cluster-aligned) to
blk_pwrite_zeroes (byte-aligned), and it is conceivable that we  can
encounter a situation without O_DIRECT in the picture at all, where our
NBD server is connected to a client that specifically asks for the new
NBD_CMD_WRITE_ZEROES on any arbitrary byte alignment.

>     memset(buf, 0, 1024 * 1024);
>     write(fd, buf, 1024 * 1024);
>     return 0;
> }
> 
> This program is compiled as a.out.
> 
> Before the patch:
> hades ~/src/qemu $ qemu-img create -f qcow2 test.qcow2 64G
> Formatting 'test.qcow2', fmt=qcow2 size=68719476736 encryption=off
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> hades ~/src/qemu $ sudo ./qemu-nbd  --connect=/dev/nbd3 test.qcow2
> --detect-zeroes=on --aio=native --cache=none
> hades ~/src/qemu $ sudo ./a.out /dev/nbd3

Here, I'd argue that the kernel NBD module is buggy, for allowing a
user-space app to pass an unaligned buffer to an O_DIRECT fd.  But
that's outside the realm of qemu code.

But again, per my argument that you don't have to involve the kernel nor
O_DIRECT to be able to write a client that can attempt to cause an NBD
server to do unaligned operations, we can use this kernel bug as an
easier way to test any proposed fix to the qemu side of things, whether
or not the kernel module gets tightened in behavior down the road.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature