Re: [Qemu-devel] [PATCH/RFC] block: Ensure that block size constraints are considered

[Kevin Wolf]

Am 21.11.2012 09:58, schrieb Christian Borntraeger:
> From: Heinz Graalfs <address@hidden>
> 
> While testing IPL code (booting) for s390x we faced some problems
> with cache=none on dasds (4k block size) on bdrv_preads with length
> values != block size.
> 
> This patch makes sure that bdrv_pread and friends work fine with
> unaligned access even with cache=none
>    - propagate alignment value also into bs->file struct
>    - modify the size in case of no cache to avoid EINVAL on
>      pread() etc. (file was opened with O_DIRECT).
> 
> This patch seems to cure the problems.
> 
> CC: Kevin Wolf <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Heinz Graalfs <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
>  block.c           |    3 +++
>  block/raw-posix.c |    6 ++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 854ebd6..f23c562 100644
> --- a/block.c
> +++ b/block.c
> @@ -4242,6 +4242,9 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
>  void bdrv_set_buffer_alignment(BlockDriverState *bs, int align)
>  {
>      bs->buffer_alignment = align;
> +    if ((bs->open_flags & BDRV_O_NOCACHE)) {
> +        bs->file->buffer_alignment = align;
> +    }

Any reason to restrict this to BDRV_O_NOCACHE?

There have been patches to change the BDRV_O_NOCACHE flag from the
monitor, in which case bdrv_set_buffer_alignment() wouldn't be called
anew and O_DIRECT requests start to fail again.

>  }
>  
>  void *qemu_blockalign(BlockDriverState *bs, size_t size)
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index f2f0404..baebf1d 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -700,6 +700,12 @@ static BlockDriverAIOCB *paio_submit(BlockDriverState 
> *bs, int fd,
>      acb->aio_nbytes = nb_sectors * 512;
>      acb->aio_offset = sector_num * 512;
>  
> +    /* O_DIRECT also requires an aligned length */
> +    if (bs->open_flags & BDRV_O_NOCACHE) {
> +        acb->aio_nbytes += acb->bs->buffer_alignment - 1;
> +        acb->aio_nbytes &= ~(acb->bs->buffer_alignment - 1);
> +    }

Modifying aio_nbytes, but not the iov looks wrong to me. This may work
in the handle_aiocb_rw_linear() code path, but not with actual vectored I/O.

Kevin