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

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 21/11/12 10:15, 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?
> 
> 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.

Christian