From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57091) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avQ4X-0007Gf-NN for qemu-devel@nongnu.org; Wed, 27 Apr 2016 10:04:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avQ4T-0005hO-DH for qemu-devel@nongnu.org; Wed, 27 Apr 2016 10:04:09 -0400 References: <1461750767-23273-1-git-send-email-kwolf@redhat.com> <1461750767-23273-3-git-send-email-kwolf@redhat.com> From: Eric Blake Message-ID: <5720C6CA.1050708@redhat.com> Date: Wed, 27 Apr 2016 08:03:54 -0600 MIME-Version: 1.0 In-Reply-To: <1461750767-23273-3-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="iW9CVisl7gpWXDXg2wfcN4O0O3H2WPgiG" Subject: Re: [Qemu-devel] [PATCH 02/17] block: Introduce bdrv_driver_pwritev() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: famz@redhat.com, sw@weilnetz.de, qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --iW9CVisl7gpWXDXg2wfcN4O0O3H2WPgiG Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/27/2016 03:52 AM, Kevin Wolf wrote: > This is a function that simply calls into the block driver for doing a > write, providing the byte granularity interface we want to eventually > have everywhere, and using whatever interface that driver supports. >=20 > This one is a bit more interesting that the version for reads: It adds > support for .bdrv_co_writev_flags() everywhere, so that drivers > implementing this function can drop .bdrv_co_writev() now. >=20 > Signed-off-by: Kevin Wolf > --- > block/io.c | 51 ++++++++++++++++++++++++++++++++++++-------------= -- > block/iscsi.c | 8 -------- > block/nbd.c | 9 --------- > block/raw_bsd.c | 8 -------- > 4 files changed, 36 insertions(+), 40 deletions(-) >=20 > =20 > +static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs, > + uint64_t offset, uint64_t = bytes, > + QEMUIOVector *qiov, int fl= ags) > +{ > + BlockDriver *drv =3D bs->drv; > + int64_t sector_num =3D offset >> BDRV_SECTOR_BITS; > + unsigned int nb_sectors =3D bytes >> BDRV_SECTOR_BITS; > + int ret; > + > + assert((offset & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); > + assert((bytes & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); > + assert((bytes >> BDRV_SECTOR_BITS) <=3D BDRV_REQUEST_MAX_SECTORS);= > + > + if (drv->bdrv_co_writev_flags) { > + ret =3D drv->bdrv_co_writev_flags(bs, sector_num, nb_sectors, = qiov, > + flags); Not for this patch, but should we be doing something like assert((flags & ~drv->supported_write_flags) =3D=3D 0)? > + } else { > + assert(drv->supported_write_flags =3D=3D 0); > + ret =3D drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);= > + } > + > + if (ret =3D=3D 0 && (flags & BDRV_REQ_FUA) && > + !(drv->supported_write_flags & BDRV_REQ_FUA)) > + { > + ret =3D bdrv_co_flush(bs); Unrelated to your patch here, but in my NBD work, I ran into the situation where it would be nicer if drv->supported_write_flags were dynamic. That is, an NBD client can tell on a per-connection basis whether the server supports NBD_FLAG_FUA, but because supported_write_flags is a property of the driver, rather than a callback that is a function of the bs, NBD has to reflect it back to the block layer by advertising supported_write_flags =3D=3D BDRV_REQ_FUA alwa= ys, and when connecting to a less-capable server has to manually repeat the bdrv_co_flush(bs) fallback dance itself: http://git.qemu.org/?p=3Dqemu.git;a=3Dblob;f=3Dblock/nbd.c;h=3Df7ea3b3608= ;hb=3D3123bd8e#l358 Maybe we should do a patch series that converts supported_write_flags to be a function call that can have per-bs configuration, so that the NBD client can be simplified by letting the block layer take care of the FUA fallback. > @@ -1215,23 +1247,12 @@ static int coroutine_fn bdrv_aligned_pwritev(Bl= ockDriverState *bs, > } else if (flags & BDRV_REQ_ZERO_WRITE) { > bdrv_debug_event(bs, BLKDBG_PWRITEV_ZERO); > ret =3D bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors, fl= ags); > - } else if (drv->bdrv_co_writev_flags) { > - bdrv_debug_event(bs, BLKDBG_PWRITEV); > - ret =3D drv->bdrv_co_writev_flags(bs, sector_num, nb_sectors, = qiov, > - flags); Should bdrv_co_do_write_zeroes also be folded into bdrv_driver_pwritev()?= But what you have looks sane enough for: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --iW9CVisl7gpWXDXg2wfcN4O0O3H2WPgiG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXIMbKAAoJEKeha0olJ0NqY2IIAK99ucMtAhyibFlVQ0+opTNA P1iDYKfYP0aV67uplsuDiyWnEoH962YlYEz6rkJwavGe79FEEZ00lOMpfShaiAQ6 /AhI5F/NBvq8Oo+MUWbbKuAqbL4AqEerLUR7Y3botAU44Hx6AUJCr8qALAEkg6wX pR014v6C/gpgMtddCKQFpr4vXk6ZiNqe0Z9kBJsnzSaHSpmwkwicEG3LmNQre3RA 7RwzHrYA9sk4uAphc6+R1XDYu5ncrFVFhRsXy2g+LroQ9ZN5VLa2A4J1IMZeUPef PZk8lSr9gxNCn3LSfm+OcD28GZ4N2o35pzEssFkRUNUbKFw5xtEG0q25shVGW/0= =4lF6 -----END PGP SIGNATURE----- --iW9CVisl7gpWXDXg2wfcN4O0O3H2WPgiG--