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

[Heinz Graalfs]

On Wed, 2012-11-21 at 11:00 +0100, Christian Borntraeger wrote:
> On 21/11/12 10:15, Kevin Wolf wrote:
> > 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.
> 
> 
> Right, should be ok to remove the check.
> 
> 
> > 
> >>  }
> >>  
> >>  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.
> 
> I think it seemed to work because the vectored I/O cases that we were testing 
> were properly
> aligned or were in the QEMU_AIO_MISALIGNED case which does bounce buffering 
> anyway.
> But I am not sure...
> 
> Heinz can you have a look at this and identify the exact place were it was 
> failing
> and why this patch helps?  (I just know it does). That might help to 
> understand
> if we also need to touch the iovs.

The pread() call in handle_aiocb_rw_linear() is failing with errno 22.
At least for this path the patch ensures that the length is correctly
set. I need to look into the vectored I/O part in more detail.

> Christian
> 
>