From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42572) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCjrx-00055u-Fh for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:38:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCjru-0005vj-Oz for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:38:44 -0400 Date: Tue, 14 Jun 2016 10:38:32 +0200 From: Kevin Wolf Message-ID: <20160614083832.GC4916@noname.str.redhat.com> 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> <575FB9F9.4000003@kaod.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <575FB9F9.4000003@kaod.org> 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: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: Eric Blake , Peter Crosthwaite , Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org Am 14.06.2016 um 10:02 hat C=E9dric Le Goater geschrieben: > >> #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=3D0x7fa81d4dd050,= req=3D, offset=3D30878208,=20 > >> bytes=3D512, qiov=3D0x7fa7f47fee60, flags=3D0) > >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c= :1243 > >> #5 0x00007fa81c669ecb in bdrv_co_pwritev (bs=3D0x7fa81d4dd050, offs= et=3D8, bytes=3D512, qiov=3D0x7fa80d5191c0,=20 > >> flags=3D(BDRV_REQ_COPY_ON_READ | BDRV_REQ_ZERO_WRITE | BDRV_REQ_= MAY_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= :1492 > >=20 > > That 'flags' value looks bogus... > >=20 > >> #6 0x00007fa81c65e367 in blk_co_pwritev (blk=3D0x7fa81d4c5b60, offs= et=3D30878208, bytes=3D256, qiov=3D0x7fa80d5191c0,=20 > >> flags=3D(unknown: 0)) at /home/legoater/work/qemu/qemu-ast2400-m= ainline.git/block/block-backend.c:788 > >> #7 0x00007fa81c65e49b in blk_aio_write_entry (opaque=3D0x7fa7e849ac= a0) > >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/bloc= k-backend.c:977 > >> #8 0x00007fa81c6c823a in coroutine_trampoline (i0=3D= , i1=3D) > >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/corou= tine-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,= to > > see who's sending the bad parameters. I recently debugged a bogus fl= ags > > in bdrv_aio_preadv, by hoisting an assert to occur before coroutines = are > > 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= /or > > the hack for catching the bug prior to coroutines, will help you debu= g > > where the bad arguments are coming from. >=20 > That does not fix the assert. > =20 > >> #10 0x00007fa80d5189d0 in ?? () > >> #11 0x0000000000000000 in ?? () > >> (gdb) up 4 > >> #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=3D0x7fa81d4dd050,= req=3D, offset=3D30878208,=20 > >> bytes=3D512, qiov=3D0x7fa7f47fee60, flags=3D0) > >> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c= :1243 > >> 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 2= 56} >=20 > So, it seems that the issue is coming from the fact that bdrv_co_pwrite= v() > does not handle alignments less than BDRV_SECTOR_SIZE : >=20 > /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */ > uint64_t align =3D MAX(BDRV_SECTOR_SIZE, bs->request_alignment); >=20 > It calls bdrv_aligned_pwritev() which does the assert :=20 >=20 > assert(!qiov || bytes =3D=3D qiov->size); Yes, but between these two places, there is code that should actually enforce the right alignment: if ((offset + bytes) & (align - 1)) { ... } You can see in your backtrace that bdrv_aligned_pwritev() gets a different qiov than bdrv_co_pwritev() (which is local_qiov in the latter function). It's just unclear to me why this code extended bytes, but didn't add the tail_buf iovec to local_qiov. > This is because flash_sync_page(), in m25p80.c, now writes with a len o= f=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 >=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. No, bdrv_co_pwritev() is supposed to handle sub-sector writes correctly, so this is what we need to fix. Kevin