From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akrPp-0002PL-Q1 for qemu-devel@nongnu.org; Tue, 29 Mar 2016 07:02:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akrPl-00089O-RO for qemu-devel@nongnu.org; Tue, 29 Mar 2016 07:02:29 -0400 Date: Tue, 29 Mar 2016 13:02:15 +0200 From: Kevin Wolf Message-ID: <20160329110215.GD4600@noname.redhat.com> References: <1458325289-17848-1-git-send-email-kwolf@redhat.com> <1458325289-17848-14-git-send-email-kwolf@redhat.com> <56F6F4B7.8010909@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KN5l+BnMqAQyZLvT" Content-Disposition: inline In-Reply-To: <56F6F4B7.8010909@redhat.com> Subject: Re: [Qemu-devel] [PATCH 13/20] iscsi: Support BDRV_REQ_FUA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org --KN5l+BnMqAQyZLvT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 26.03.2016 um 21:44 hat Max Reitz geschrieben: > On 18.03.2016 19:21, Kevin Wolf wrote: > > This replaces the existing hack in the iscsi driver that sent the FUA > > bit in writethrough mode and ignored the following flush in order to > > optimise the number of roundtrips (see commit 73b5394e). > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block/iscsi.c | 24 +++++++----------------- > > 1 file changed, 7 insertions(+), 17 deletions(-) > >=20 > > diff --git a/block/iscsi.c b/block/iscsi.c > > index 3b54536..4f75204 100644 > > --- a/block/iscsi.c > > +++ b/block/iscsi.c >=20 > [...] >=20 > > @@ -1851,7 +1840,8 @@ static BlockDriver bdrv_iscsi =3D { > > .bdrv_co_discard =3D iscsi_co_discard, > > .bdrv_co_write_zeroes =3D iscsi_co_write_zeroes, > > .bdrv_co_readv =3D iscsi_co_readv, > > - .bdrv_co_writev =3D iscsi_co_writev, > > + .bdrv_co_writev_flags =3D iscsi_co_writev_flags, > > + .supported_write_flags =3D BDRV_REQ_FUA, > > .bdrv_co_flush_to_disk =3D iscsi_co_flush, > > =20 > > #ifdef __linux__ > >=20 >=20 > Hm, wait, maybe not R-b. I can see three places in block/io.c which call > bdrv_co_writev(), and only one of them diverts to bdrv_co_writev_flags() > if that is available. Maybe we don't need to care about the > bounce-buffer case for write_zeroes, but I do think we need to care > about the COR case. >=20 > Of course bdrv_co_writev() can trivially be forwarded to > bdrv_co_writev_flags(), but I'm not sure who is supposed to do this > forwarding. I can imagine three ways: >=20 > (1) Keep a wrapper per block driver. Simple, but not so elegant. > (2) Make all bdrv_co_writev() callers call bdrv_co_writev_flags() if > the former is not available but the latter is. > (3) Introduce a generic function replacing every drv->bdrv_co_writev() > call which then decides which driver function to invoke. Good catch, thanks! Going for (1) for now, because I think (2) is even less elegant and while I have a slight preference for (3) from the code perspective, it could be argued that it impacts the hot write path of raw images and I don't want to deal with potential performance changes that late in the cycle. And now that I'm writing this, I realise that the hot path already calls =2Ebdrv_co_writev_flags, so that's not a real argument. But I've already implemented (1), so I'll leave it at that... The long term plan is anyway to convert everything to .bdrv_co_writev_flags. Kevin --KN5l+BnMqAQyZLvT Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJW+mC3AAoJEH8JsnLIjy/W19kP/0sv31fMGosyHgHg+8DpY2AC 3A2SsPVjHBB6n3Ign/BOCZRJQV8OwaPPkhTVIUvoGaoQdYWzaw0o4ZSr8u3dqESe 7MDA5Q+mW5i/S8lgr0lwQOY5atu1dOsCfIOjlTLrzkiOlUhRTBPJ3uf3E930v0sx 3EQ+Xbi54WgLwJhIQtaoZ6EONC/HMfBTh2BS44KHgGN6kMFb/bVLC4pLyouOEBQe /fkXac4USvGcdDPjVPNkQrYtI1MBXNfBj1Vz0mki07aV++vNhtkQ9yP5z01FqWtr f5RvXgbPzaxkVSm+DIGIPlUx+Flob+BwKHVe7bMtPA3mhC8eGyW1qoV9NSnJGZAP zrZryNF6PgYWxbN2jeqgX6IpM/ErrOtBTOqKUXrSgroIxAYXjNR0JM9dsnTa//BV EzjJHbcFxPFjTnlZ8oG93DnbkXe/hEyotCn4DfAeLqcQHXFB0eYDLwQc9rcjrdTi DcbqWptQmhwABs+NtrCH7vzawEecairP4pZKy+w3c7k5fpBRSUk8tKx9oWvuvh1v 86y1doGez6t6IhgPBmBKL1m4+z/ZXkluTE2QlKrUHhZDCLEbJcazItJHDUK9HTh7 wBDAau7aqj7RwPEVLmkZb6GbcaRHggcgW3QafMAVQpUzJA4HvTNi/5Wa/9qkAYMX KzmUuiwlSufj/3hxvFdg =xXIW -----END PGP SIGNATURE----- --KN5l+BnMqAQyZLvT--