| Message ID | 1467103170-5784-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 Thanks, C. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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) >
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 <clg@kaod.org> Thanks, C. >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> 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) >> >
On 06/28/2016 02: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. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/block/m25p80.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> and thanks for tracking this one down
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)
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. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/block/m25p80.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)