Re: [Qemu-devel] [PATCH 1/1] block: enforce minimal 4096 alignment in qemu_blockalign

[Kevin Wolf]

Am 29.01.2015 um 11:50 hat Denis V. Lunev geschrieben:
> The following sequence
>     int fd = open(argv[1], O_RDWR | O_CREAT | O_DIRECT, 0644);
>     for (i = 0; i < 100000; i++)
>             write(fd, buf, 4096);
> performs 5% better if buf is aligned to 4096 bytes rather then to
> 512 bytes on HDD with 512/4096 logical/physical sector size.
> 
> The difference is quite reliable.
> 
> On the other hand we do not want at the moment to enforce bounce
> buffering if guest request is aligned to 512 bytes. This patch
> forces page alignment when we really forced to perform memory
> allocation.
> 
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> ---
>  block.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index d45e4dd..38cf73f 100644
> --- a/block.c
> +++ b/block.c
> @@ -5293,7 +5293,11 @@ void bdrv_set_guest_block_size(BlockDriverState *bs, 
> int align)
>  
>  void *qemu_blockalign(BlockDriverState *bs, size_t size)
>  {
> -    return qemu_memalign(bdrv_opt_mem_align(bs), size);
> +    size_t align = bdrv_opt_mem_align(bs);
> +    if (align < 4096) {
> +        align = 4096;
> +    }
> +    return qemu_memalign(align, size);
>  }
>  
>  void *qemu_blockalign0(BlockDriverState *bs, size_t size)
> @@ -5307,6 +5311,9 @@ void *qemu_try_blockalign(BlockDriverState *bs, size_t 
> size)
>  
>      /* Ensure that NULL is never returned on success */
>      assert(align > 0);
> +    if (align < 4096) {
> +        align = 4096;
> +    }
>      if (size == 0) {
>          size = align;
>      }

This is the wrong place to make this change. First you're duplicating
logic in the callers of bdrv_opt_mem_align() instead of making it return
the right thing in the first place. Second, you're arguing with numbers
from a simple test case for O_DIRECT on Linux, but you're changing the
alignment for everyone instead of just the raw-posix driver which is
responsible for accessing Linux files.

Also, what's the real reason for the performance improvement? Having
page alignment? If so, actually querying the page size instead of
assuming 4k might be worth a thought.

Kevin