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

[Heinz Graalfs <graalfs@linux.vnet.ibm.com>]

Hello Kevin,

I'm resending my answer as of Nov 23rd.

Is this still on your queue?

Heinz


On Wed, 2012-11-21 at 10:15 +0100, Kevin Wolf wrote:
> Am 21.11.2012 09:58, schrieb Christian Borntraeger:
> > From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > 
> > 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 <kwolf@redhat.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  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?


OK, can be removed

> 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.

Current coding ensures that read IO buffers always seem to be aligned
correctly. Whereas read length values are not always appropriate for an
O_DIRECT scenario.

For a 2048 formatted disk I verified that

1. non vectored IO - the length needs to be adapted several times,
   which is accomplished now by the patch.

2. vectored IO - the qiov's total length is always a multiple of the
   logical block size 
      (which is also verified in virtio_blk_handle_read())
   The particular iov length fields are already correctly setup as a
   multiple of the logical block size when processed in
   virtio_blk_handle_request().

> 
> Kevin
>