[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value |
Date: |
Tue, 14 Jun 2016 10:02:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.8.0 |
>> #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
>> req=<optimized out>, offset=30878208,
>> bytes=512, qiov=0x7fa7f47fee60, flags=0)
>> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243
>> #5 0x00007fa81c669ecb in bdrv_co_pwritev (bs=0x7fa81d4dd050, offset=8,
>> bytes=512, qiov=0x7fa80d5191c0,
>> flags=(BDRV_REQ_COPY_ON_READ | BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP
>> | BDRV_REQ_NO_SERIALISING | BDRV_REQ_FUA | unknown: 4278124256),
>> address@hidden(unknown: 0))
>> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1492
>
> That 'flags' value looks bogus...
>
>> #6 0x00007fa81c65e367 in blk_co_pwritev (blk=0x7fa81d4c5b60,
>> offset=30878208, bytes=256, qiov=0x7fa80d5191c0,
>> flags=(unknown: 0)) at
>> /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:788
>> #7 0x00007fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0)
>> at
>> /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977
>> #8 0x00007fa81c6c823a in coroutine_trampoline (i0=<optimized out>,
>> i1=<optimized out>)
>> at
>> /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78
>> #9 0x00007fa818ea8f00 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
>
> and we don't get anything further in the backtrace beyond coroutines, to
> see who's sending the bad parameters. I recently debugged a bogus flags
> in bdrv_aio_preadv, by hoisting an assert to occur before coroutines are
> used in blk_aio_prwv():
>
> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02948.html
>
> I've just posted v2 of that patch (now a 2/2 series), but in v2 no
> longer kept the assert at that point. But maybe the correct fix, and/or
> the hack for catching the bug prior to coroutines, will help you debug
> where the bad arguments are coming from.
That does not fix the assert.
>> #10 0x00007fa80d5189d0 in ?? ()
>> #11 0x0000000000000000 in ?? ()
>> (gdb) up 4
>> #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
>> req=<optimized out>, offset=30878208,
>> bytes=512, qiov=0x7fa7f47fee60, flags=0)
>> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243
>> 1243 assert(!qiov || bytes == qiov->size);
>> (gdb) p *qiov
>> $1 = {iov = 0x7fa81da671d0, niov = 1, nalloc = 1, size = 256}
So, it seems that the issue is coming from the fact that bdrv_co_pwritev()
does not handle alignments less than BDRV_SECTOR_SIZE :
/* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
It calls bdrv_aligned_pwritev() which does the assert :
assert(!qiov || bytes == qiov->size);
This is because flash_sync_page(), in m25p80.c, now writes with a len of
0x100, which the page size in flash modules. commit 243e6f69c129
("m25p80: Switch to byte-based block access") removed the alignment on
BDRV_SECTOR_SIZE.
So I think the safest is to restore the alignment on writes. see below.
If this is ok, I will send a little serie of fixes for m25p80 with this
one included.
Thanks,
C.
From: Cédric Le Goater <address@hidden>
Subject: [PATCH] m25p80: restore BDRV_SECTOR_SIZE alignment on writes
Date: Tue, 14 Jun 2016 09:32:22 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
commit 243e6f69c129 ("m25p80: Switch to byte-based block access")
removed the alignment of writes on BDRV_SECTOR_SIZE, so they are
now done on a page size (0x100) basis. This is not supported by the
bdrv routines which asserts in bdrv_aligned_pwritev() :
assert(!qiov || bytes == qiov->size);
bytes being rounded up to BDRV_SECTOR_SIZE and qiov->size == 0x100
Signed-off-by: Cédric Le Goater <address@hidden>
---
hw/block/m25p80.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
Index: qemu-ast2400-mainline.git/hw/block/m25p80.c
===================================================================
--- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c
+++ qemu-ast2400-mainline.git/hw/block/m25p80.c
@@ -359,21 +359,25 @@ static void blk_sync_complete(void *opaq
static void flash_sync_page(Flash *s, int page)
{
+ int blk_sector, nb_sectors;
QEMUIOVector iov;
if (!s->blk || blk_is_read_only(s->blk)) {
return;
}
+ blk_sector = (page * s->pi->page_size) / BDRV_SECTOR_SIZE;
+ nb_sectors = DIV_ROUND_UP(s->pi->page_size, BDRV_SECTOR_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,
+ qemu_iovec_add(&iov, s->storage + blk_sector * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE);
+ blk_aio_pwritev(s->blk, blk_sector * BDRV_SECTOR_SIZE, &iov, 0,
blk_sync_complete, NULL);
}
static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
{
+ int64_t start, end, nb_sectors;
QEMUIOVector iov;
if (!s->blk || blk_is_read_only(s->blk)) {
@@ -381,9 +385,13 @@ static inline void flash_sync_area(Flash
}
assert(!(len % BDRV_SECTOR_SIZE));
+ start = off / BDRV_SECTOR_SIZE;
+ end = (off + len) / BDRV_SECTOR_SIZE;
+ nb_sectors = end - start;
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_add(&iov, s->storage + (start * BDRV_SECTOR_SIZE),
+ nb_sectors * BDRV_SECTOR_SIZE);
+ blk_aio_pwritev(s->blk, start, &iov, 0, blk_sync_complete, NULL);
}
static void flash_erase(Flash *s, int offset, FlashCMD cmd)
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Cédric Le Goater, 2016/06/13
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Eric Blake, 2016/06/13
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Cédric Le Goater, 2016/06/13
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Eric Blake, 2016/06/13
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value,
Cédric Le Goater <=
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Kevin Wolf, 2016/06/14
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Cédric Le Goater, 2016/06/14
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Kevin Wolf, 2016/06/15
- Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Cédric Le Goater, 2016/06/15
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value, Cédric Le Goater, 2016/06/15