Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack

[Cédric Le Goater]

On 06/28/2016 10:53 AM, Cédric Le Goater wrote:
> On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
>> When doing a read-modify-write cycle, QEMU uses the iovec after returning
>> from blk_aio_pwritev.  m25p80 puts the iovec on the stack of 
>> blk_aio_pwritev's
>> caller, which causes trouble in this case.  This has been a problem
>> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
>> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
>> In principle however it could have broken before when using -drive
>> if=mtd,cache=none on a disk with 4K native sectors.
> 
> Ah ! Thanks. That was a problem I was seeing.   
>
> 
> I was thinking we could just do synchronous writes :
> 
>       
> https://github.com/legoater/qemu/commit/aef3fe4db3be632077c581541fe30b4e36b5a6f7
> 
> and enable snapshotting like  : 
> 
>       
> https://github.com/legoater/qemu/commit/b90ccab7873fd3538b47396ec7c3ae35c8e13270
> 

Anyhow, I tested this patch on a P8/ppc64le system on which the 
assertion was very systematic :

        bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed.

It is gone.

Reviewed-and-tested-by: Cédric Le Goater <address@hidden>

Thanks, 

C. 
 
 
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  hw/block/m25p80.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 09b4767..dd6714d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
>>  
>>  static void blk_sync_complete(void *opaque, int ret)
>>  {
>> +    QEMUIOVector *iov = opaque;
>> +
>> +    qemu_iovec_destroy(iov);
>> +    g_free(iov);
>> +
>>      /* do nothing. Masters do not directly interact with the backing store,
>>       * only the working copy so no mutexing required.
>>       */
>> @@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
>>  
>>  static void flash_sync_page(Flash *s, int page)
>>  {
>> -    QEMUIOVector iov;
>> +    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>>  
>>      if (!s->blk || blk_is_read_only(s->blk)) {
>>          return;
>>      }
>>  
>> -    qemu_iovec_init(&iov, 1);
>> -    qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
>> +    qemu_iovec_init(iov, 1);
>> +    qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
>>                     s->pi->page_size);
>> -    blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
>> -                    blk_sync_complete, NULL);
>> +    blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
>> +                    blk_sync_complete, iov);
>>  }
>>  
>>  static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
>>  {
>> -    QEMUIOVector iov;
>> +    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>>  
>>      if (!s->blk || blk_is_read_only(s->blk)) {
>>          return;
>>      }
>>  
>>      assert(!(len % BDRV_SECTOR_SIZE));
>> -    qemu_iovec_init(&iov, 1);
>> -    qemu_iovec_add(&iov, s->storage + off, len);
>> -    blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
>> +    qemu_iovec_init(iov, 1);
>> +    qemu_iovec_add(iov, s->storage + off, len);
>> +    blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
>>  }
>>  
>>  static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>
>