From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c8nDJ-00044a-N5 for qemu-devel@nongnu.org; Mon, 21 Nov 2016 06:56:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c8nDI-0006fR-KL for qemu-devel@nongnu.org; Mon, 21 Nov 2016 06:56:45 -0500 Date: Mon, 21 Nov 2016 12:56:36 +0100 From: Kevin Wolf Message-ID: <20161121115636.GC5876@noname.redhat.com> References: <1478798349-28983-1-git-send-email-kwolf@redhat.com> <1478798349-28983-7-git-send-email-kwolf@redhat.com> <20161118122112.GA4694@noname.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="rJwd6BRFiFCcLxzm" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC PATCH 6/8] quorum: Avoid bdrv_aio_writev() for rewrites List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Alberto Garcia , qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com --rJwd6BRFiFCcLxzm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 18.11.2016 um 22:11 hat Eric Blake geschrieben: > On 11/18/2016 06:21 AM, Kevin Wolf wrote: >=20 > >>> + ret =3D bdrv_co_pwritev(s->children[co->i], > >>> + acb->sector_num * BDRV_SECTOR_SIZE, > >>> + acb->nb_sectors * BDRV_SECTOR_SIZE, > >>> + acb->qiov, 0); > >>> + (void) ret; > >> > >> Why do you need 'ret' at all? If it's a placeholder to remind us to do > >> something with this value in the future, you can simply add a FIXME > >> comment. > >=20 > > I'm not sure whether we want to fix anything, it looks intentional to > > me. I just wanted to be explicit about the ignored return value, both > > for human readers and for tools like Coverity. >=20 > In bdrv_co_flush(), we have: >=20 > /* Return value is ignored - it's ok if wait queue is empty */ > qemu_co_queue_next(&bs->flush_queue); >=20 > I don't know if Coverity would squawk, but the cast to void looks a bit > stranger to me than a comment, where what we did in bdrv_co_flush() > seems reasonable. There's also the patch proposal to introduce > ignore_value() in place of a cast to void, which is a bit more > self-documenting about places that intentionally ignore a return value > while still shutting Coverity up: >=20 > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05165.html If you don't like the void cast, I can remove it. After all, the bdrv_aio_writev() call in the original code didn't have it either. I'm just surprised that it feels strange to both of you, I thought it was a well-known idiom for "this value is intentionally unused". > >=20 > >>> + /* one less rewrite to do */ > >>> + acb->rewrite_count--; > >>> + qemu_coroutine_enter_if_inactive(acb->co); > >> > >> I think you should only enter acb->co when acb->rewrite_count reaches > >> zero. > >> > >> In all other cases the main coroutine simply iterates inside the while= () > >> loop, verifies that the number is still positive and yields again. > >> > >> The same applies to all other cases of qemu_coroutine_enter_if_inactiv= e, > >> by the way (I failed to notice it in patch #5). > >=20 > > I think I like it better this way because it keeps the loop condition > > local to the caller instead of spreading it across the caller and the > > places that reenter. On the other hand, I can see that not doing the > > extra context switch might be a little more efficient. >=20 > Do we have a feel for how many context switches this would save? If > it's in the noise, cleaner code is probably a win; but if it is a > hotspot, then we should definitely try the optimization. Should normally be (num_children - 1) per request. With inconsistent results and enabled rewrites, add another one for each child that needs to be corrected. Kevin --rJwd6BRFiFCcLxzm Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJYMuD0AAoJEH8JsnLIjy/WgTcQAIGn68JjBcojvdZJHr5ttFqF rsGu1LWgnzNTknCRSSteRitUCgsMC4kue5QSxltaI6KvLlzTfDtezCUGaaZRYm1X XsjFDzAiwD/mt7jqHNy7rqzB25ZInOdxjs75TFf03AbjRlbm9WOqb+VZsq7n/ewK uKk4OioSOIIeM36UGrVsnZHHx6c9DCxhB3w6zBPIPYlndDLBYU24M09OSJCAqtHm XTT4IsMUff8TyrWq8lTQggiD20CKaaiWIH/LGXPkD2xcOzdrHQh07Z7SQlFSEuoY PuHgdROmlfdLpNsdvnvbmdcd8ENbGfvdIjQiG38BmnYRFItDt1a/r/tb5uBgNUUM C1iqGK11kuO0HHve3KJMmLE89edSdM46bFV0/CgEeVljaEb0snPB0GXwXoi5LUiR OJbLaeTHO3JKJJauzBqAL+R9O93xIP1INAyAxvXu+Yun3l3xXkKczxsRB+ftDA1e n7gVx25xi8Dmm99nlhjkFQLysyy+yyYkV0mVOOQ4OIWXmpUskfR5ZcX2JpdJKCI8 SslJIRRLmRgbHEZmRqoMYc4wyyF6AcqcKrrR51ei5zT2q2I1QWKuNuLKjwkhIpA0 P2DU5ViRcglMxFPuuGgOawx2uTJyil90BK01Q3Z9eoO5nSHi8JCkpVBE/mzwm2g4 cPgapik5y79c17fmi6qL =PAPH -----END PGP SIGNATURE----- --rJwd6BRFiFCcLxzm--