From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adEPo-0004q9-7e for qemu-devel@nongnu.org; Tue, 08 Mar 2016 04:58:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adEPn-0007Tz-76 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 04:58:56 -0500 Date: Tue, 8 Mar 2016 10:58:47 +0100 From: Kevin Wolf Message-ID: <20160308095847.GB5807@noname.str.redhat.com> References: <1455638000-18051-1-git-send-email-pbonzini@redhat.com> <20160217025722.GC30207@ad.usersys.redhat.com> <56C4595D.1020206@redhat.com> <20160223055704.GC19080@ad.usersys.redhat.com> <56CC37EB.7050901@redhat.com> <20160223124956.GA26812@ad.usersys.redhat.com> <56CC647A.6020803@redhat.com> <20160307165741.GB6464@noname.redhat.com> <20160307205654.GB31890@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0eh6TmSyL6TZE2Uz" Content-Disposition: inline In-Reply-To: <20160307205654.GB31890@stefanha-x1.localdomain> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] qed: fix bdrv_qed_drain List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Paolo Bonzini , Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org --0eh6TmSyL6TZE2Uz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 07.03.2016 um 21:56 hat Stefan Hajnoczi geschrieben: > On Mon, Mar 07, 2016 at 05:57:41PM +0100, Kevin Wolf wrote: > > Am 23.02.2016 um 14:54 hat Paolo Bonzini geschrieben: > > >=20 > > >=20 > > > On 23/02/2016 13:49, Fam Zheng wrote: > > > > On Tue, 02/23 11:43, Paolo Bonzini wrote: > > > >> > > > >> > > > >> On 23/02/2016 06:57, Fam Zheng wrote: > > > >>>>>> + qed_cancel_need_check_timer(s); > > > >>>>>> + qed_need_check_timer_cb(s); > > > >>>>>> + } > > > >>>>> > > > >>>>> What if an allocating write is queued (the else branch case)? I= ts completion > > > >>>>> will be in bdrv_drain and it could arm the need_check_timer whi= ch is wrong. > > > >>>>> > > > >>>>> We need to drain the allocating_write_reqs queue before checkin= g the timer. > > > >>>> > > > >>>> You're right, but how? That's what bdrv_drain(bs) does, it's a > > > >>>> chicken-and-egg problem. > > > >>> > > > >>> Maybe use an aio_poll loop before the if? > > > >> > > > >> That would not change the fact that you're reimplementing bdrv_dra= in > > > >> inside bdrv_qed_drain. > > > >=20 > > > > But it fulfills the contract of .bdrv_drain. This is the easy way, = the hard way > > > > would be iterating through the allocating_write_reqs list and proce= ss reqs one > > > > by one synchronously, which still involves aio_poll indirectly. > > >=20 > > > The easy way would be better then. > > >=20 > > > Stefan, any second opinion? > >=20 > > What's the status here? It would be good to have qed not segfaulting all > > the time. >=20 > I think the timer concept itself is troublesome. A radical approach but > limited to changing QED itself is to drop the timer and instead keep a > timestamp for the last allocating write request. Next time a write > request (allocating or rewriting) is about to complete, do the flush and > clear the need check flag as part of the write request (if 5 seconds > have passed since the timestamp). Isn't that kind of backwards, though, because it's very likely that after some new activity a second write request will come in soon and mark the image dirty again? Apart from that, doesn't it fail to provide the intended effect, that the image doesn't stay dirty for a long time and a repair isn't usually needed after a crash? I think rather than doing this, I'd just remove the mechanism altogether and instead mark the image clean when the guest flushes the disk cache. Kevin > Because the need check flag clearing is part of the write request > completion, it will integrate properly with drain. >=20 > Stefan --0eh6TmSyL6TZE2Uz Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJW3qJXAAoJEH8JsnLIjy/WvVAP/jWWlY521WjbZoT1SOIr1ii0 gicCwhXY5DyGKQrlQoI4NbPZupteIKB8PZV4LMSvksH2PSasuFWSx3D+zVTd/5Ic 5y9MtUnFKHe5Rpz0Motbtsnf2HEikdY0z9NzobOLU64+Y3jl/AmxUGaYqENNLZH+ Wti5l1F+qrcDpab7iL+t0Sc+I1sW77L04irBHRvmFzI8jmi5LIJt4+x0EDTjOyh6 RuNf/74RxrEpQe0/sgafbKl4o0DSNxt1xyoKGsQEpR+u1RYa2ZCL2gdJNfxW0mIc lTTBLK1LRifldf7S4BnKP0+tab0/F5kmL5tB1IjL5B5dKV26yAcS2AlCpr/eQ1gv rl+Xa4DlCzE/Xx4rk3OUSswZ3fYnkT1n5uWeTqvzh6B8xcTXfk54YUSJKU0Ge3VM nv3ggJc6NKejnZYfM7kFTVn83Bq5zcFdeHkNeeXan+Bwx4WQDrrjY9177vJIkPwU kbJhzWLtrWv18mAR2BfcvEJjXnRodV9mU/04wed2s211mefB32ITxAtUfpOnO760 eC8M74xO8Eel0rnYgGdLs4C0NAHCPTBGiBslHmQ3gIv10yZ86V89G2fhV8sCQYxq VAu1+pYRPOI3x3SDYnnY7/ZPalGaZnxd6IV8656hzRZ9NxixUOSGe/b6euUpiOWT B+BBX7MXKEKje9u97r5T =TcRS -----END PGP SIGNATURE----- --0eh6TmSyL6TZE2Uz--