[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH/RFC] block: Ensure that block size constraints a
From: |
Heinz Graalfs |
Subject: |
Re: [Qemu-devel] [PATCH/RFC] block: Ensure that block size constraints are considered |
Date: |
Fri, 23 Nov 2012 11:45:13 +0100 |
On Wed, 2012-11-21 at 10:15 +0100, 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.
>
OK
> > }
> >
> > 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
>