From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34418) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCjIj-0005sS-63 for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:02:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCjIe-00068v-6a for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:02:20 -0400 Received: from 5.mo177.mail-out.ovh.net ([46.105.39.154]:43848) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCjId-00068k-TL for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:02:16 -0400 Received: from player746.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo177.mail-out.ovh.net (Postfix) with ESMTP id DC6D7FF9732 for ; Tue, 14 Jun 2016 10:02:14 +0200 (CEST) References: <1464694565-16784-1-git-send-email-clg@kaod.org> <574D9F0F.7060904@redhat.com> <574D9FBB.60100@kaod.org> <574DA17D.5070505@redhat.com> <575EDE86.6080201@kaod.org> <575EE3B7.5080209@redhat.com> <575EF0AC.20305@kaod.org> <575F01EE.2050208@redhat.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <575FB9F9.4000003@kaod.org> Date: Tue, 14 Jun 2016 10:02:01 +0200 MIME-Version: 1.0 In-Reply-To: <575F01EE.2050208@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Peter Crosthwaite , Kevin Wolf , Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org >> #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=3D0x7fa81d4dd050, r= eq=3D, offset=3D30878208,=20 >> bytes=3D512, qiov=3D0x7fa7f47fee60, flags=3D0) >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1= 243 >> #5 0x00007fa81c669ecb in bdrv_co_pwritev (bs=3D0x7fa81d4dd050, offset= =3D8, bytes=3D512, qiov=3D0x7fa80d5191c0,=20 >> flags=3D(BDRV_REQ_COPY_ON_READ | BDRV_REQ_ZERO_WRITE | BDRV_REQ_MA= Y_UNMAP | BDRV_REQ_NO_SERIALISING | BDRV_REQ_FUA | unknown: 4278124256), = flags@entry=3D(unknown: 0)) >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1= 492 >=20 > That 'flags' value looks bogus... >=20 >> #6 0x00007fa81c65e367 in blk_co_pwritev (blk=3D0x7fa81d4c5b60, offset= =3D30878208, bytes=3D256, qiov=3D0x7fa80d5191c0,=20 >> flags=3D(unknown: 0)) at /home/legoater/work/qemu/qemu-ast2400-mai= nline.git/block/block-backend.c:788 >> #7 0x00007fa81c65e49b in blk_aio_write_entry (opaque=3D0x7fa7e849aca0= ) >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-= backend.c:977 >> #8 0x00007fa81c6c823a in coroutine_trampoline (i0=3D, = i1=3D) >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/corouti= ne-ucontext.c:78 >> #9 0x00007fa818ea8f00 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 >=20 > and we don't get anything further in the backtrace beyond coroutines, t= o > see who's sending the bad parameters. I recently debugged a bogus flag= s > in bdrv_aio_preadv, by hoisting an assert to occur before coroutines ar= e > used in blk_aio_prwv(): >=20 > https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02948.html >=20 > 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/o= r > 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. =20 >> #10 0x00007fa80d5189d0 in ?? () >> #11 0x0000000000000000 in ?? () >> (gdb) up 4 >> #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=3D0x7fa81d4dd050, r= eq=3D, offset=3D30878208,=20 >> bytes=3D512, qiov=3D0x7fa7f47fee60, flags=3D0) >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1= 243 >> 1243 assert(!qiov || bytes =3D=3D qiov->size); >> (gdb) p *qiov=20 >> $1 =3D {iov =3D 0x7fa81da671d0, niov =3D 1, nalloc =3D 1, size =3D 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 =3D MAX(BDRV_SECTOR_SIZE, bs->request_alignment); It calls bdrv_aligned_pwritev() which does the assert :=20 assert(!qiov || bytes =3D=3D qiov->size); This is because flash_sync_page(), in m25p80.c, now writes with a len of=20 0x100, which the page size in flash modules. commit 243e6f69c129=20 ("m25p80: Switch to byte-based block access") removed the alignment on=20 BDRV_SECTOR_SIZE.=20 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=20 one included. Thanks, C.=20 From: C=C3=A9dric Le Goater 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=3DUTF-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 =3D=3D qiov->size); bytes being rounded up to BDRV_SECTOR_SIZE and qiov->size =3D=3D 0x100 Signed-off-by: C=C3=A9dric Le Goater --- hw/block/m25p80.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) Index: qemu-ast2400-mainline.git/hw/block/m25p80.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =20 static void flash_sync_page(Flash *s, int page) { + int blk_sector, nb_sectors; QEMUIOVector iov; =20 if (!s->blk || blk_is_read_only(s->blk)) { return; } =20 + blk_sector =3D (page * s->pi->page_size) / BDRV_SECTOR_SIZE; + nb_sectors =3D 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); } =20 static inline void flash_sync_area(Flash *s, int64_t off, int64_t len) { + int64_t start, end, nb_sectors; QEMUIOVector iov; =20 if (!s->blk || blk_is_read_only(s->blk)) { @@ -381,9 +385,13 @@ static inline void flash_sync_area(Flash } =20 assert(!(len % BDRV_SECTOR_SIZE)); + start =3D off / BDRV_SECTOR_SIZE; + end =3D (off + len) / BDRV_SECTOR_SIZE; + nb_sectors =3D 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); } =20 static void flash_erase(Flash *s, int offset, FlashCMD cmd)