From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47799) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhryK-0001zl-O3 for qemu-devel@nongnu.org; Tue, 06 May 2014 22:52:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Whry9-00058L-JV for qemu-devel@nongnu.org; Tue, 06 May 2014 22:52:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21125) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Whry9-00057t-An for qemu-devel@nongnu.org; Tue, 06 May 2014 22:52:29 -0400 Message-ID: <53699FE4.5030004@redhat.com> Date: Tue, 06 May 2014 20:52:20 -0600 From: Eric Blake MIME-Version: 1.0 References: <1399422203-5861-1-git-send-email-pl@kamp.de> In-Reply-To: <1399422203-5861-1-git-send-email-pl@kamp.de> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0EDmfg2CH0riwOkn3aVQ0wmEBNPjvsJff" Subject: Re: [Qemu-devel] [PATCHv3] block: optimize zero writes with bdrv_write_zeroes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, famz@redhat.com, stefanha@redhat.com, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --0EDmfg2CH0riwOkn3aVQ0wmEBNPjvsJff Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 05/06/2014 06:23 PM, Peter Lieven wrote: > this patch tries to optimize zero write requests > by automatically using bdrv_write_zeroes if it is > supported by the format. >=20 > This significantly speeds up file system initialization and > should speed zero write test used to test backend storage > performance. >=20 >=20 > Signed-off-by: Peter Lieven > --- > v2->v3: - moved parameter parsing to blockdev_init > - added per device detect_zeroes status to=20 > hmp (info block -v) and qmp (query-block) [Eric] > - added support to enable detect-zeroes also > for hot added devices [Eric] > - added missing entry to qemu_common_drive_opts > - fixed description of qemu_iovec_is_zero [Fam] >=20 > =20 > +static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **e= rrp) > +{ > + if (!buf || !strcmp(buf, "off")) { > + return BDRV_DETECT_ZEROES_OFF; > + } else if (!strcmp(buf, "on")) { > + return BDRV_DETECT_ZEROES_ON; > + } else if (!strcmp(buf, "unmap")) { > + return BDRV_DETECT_ZEROES_UNMAP; > + } else { > + error_setg(errp, "invalid value for detect-zeroes: %s", > + buf); > + } > + return BDRV_DETECT_ZEROES_OFF; > +} Isn't there QAPI generated code that you can use instead of open-coding this conversion between string and enum values? > +++ b/qapi-schema.json > @@ -937,6 +937,8 @@ > # @encryption_key_missing: true if the backing device is encrypted but= an > # valid encryption key is missing > # > +# @detect_zeroes: detect and optimize zero writes (Since 2.1) > +# > # @bps: total throughput limit in bytes per second is specified > # > # @bps_rd: read throughput limit in bytes per second is specified > @@ -972,6 +974,7 @@ > 'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': '= str', > '*backing_file': 'str', 'backing_file_depth': 'int', > 'encrypted': 'bool', 'encryption_key_missing': 'bool', > + 'detect_zeroes': 'str', For new additions, we try to prefer dash over underscore. Eww - we've already been inconsistent in this struct, with backing_file vs. node-name= =2E Maybe s/detect_zeroes/detect-zeroes/. I obviously can't argue too strongly in light of the rest of the struct in isolation. However, you DID use detect-zeroes on the input side later in the patch, so being consistent between the two sites would be nice (given that node-name was named consistently). On the other hand, I _can_ argue strongly that open-coding this as 'str' is wrong. You already added an enum type, so use it: 'detect_zeroes': 'BlockdevDetectZeroesOptions', (hmm, in this context, it's not really an option, so maybe there is some other name we can bikeshed about; but beyond 'BlockdevDetectZeroes', I don't have any other good ideas) zero is one of those odd words with two different pluralized spellings both in common use. Code base currently has a 1:2 ratio between 'zeros' vs. 'zeroes', so I guess you're okay; on the other hand, 'man qemu-img' documents that 'convert -S' detects "zeros". > 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int', > 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', > 'image': 'ImageInfo', > @@ -4250,6 +4253,20 @@ > 'data': [ 'ignore', 'unmap' ] } > =20 > ## > +# @BlockdevDetectZeroesOptions > +# > +# Selects the operation mode for zero write detection. s/Selects/Describes/ since you are also going to use this enum on the output field. > +# > +# @off: Disabled > +# @on: Enabled Maybe more details? Seeing this doesn't tell me the tradeoffs involved in tweaking the parameter (one is faster, while the other uses less storage resources - so maybe mention those benefits). I see the documentation later on for the command line, so maybe repeating some of that here would help someone going by just the QMP interface. > +# @unmap: Enabled and even try to unmap blocks if possible > +# > +# Since: 2.1 > +## > +{ 'enum': 'BlockdevDetectZeroesOptions', > + 'data': [ 'off', 'on', 'unmap' ] } > + > +## > # @BlockdevAioOptions > # > # Selects the AIO backend to handle I/O requests > @@ -4327,7 +4346,8 @@ > '*aio': 'BlockdevAioOptions', > '*rerror': 'BlockdevOnError', > '*werror': 'BlockdevOnError', > - '*read-only': 'bool' } } > + '*read-only': 'bool', > + '*detect-zeroes': 'BlockdevDetectZeroesOptions' } } This part is fine. > @var{copy-on-read} is "on" or "off" and enables whether to copy read b= acking > file sectors into the image file. > +@item detect-zeroes=3D@var{detect-zeroes} > +@var{detect-zeroes} is "off", "on" or "unmap" and enables the automati= c > +conversion of plain zero writes by the OS to driver specific optimized= > +zero write commands. If "unmap" is chosen and @var{discard} is "on" > +a zero write may even be converted to an UNMAP operation. This looks good. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --0EDmfg2CH0riwOkn3aVQ0wmEBNPjvsJff Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTaZ/kAAoJEKeha0olJ0Nqv2wH/1P53bEFcxsVj6mj9PHnZ/ne /0klNqVigPFxvrU67aAK9e2Tpqndv2LGkJu5LzSP3V0g2dYDGMT535SZxFzNX9aq rQexzuoE4VequF5vFvOPZMXQeffL5ciaV+19i3yCEo8NHGu7Z1f7LIjJkYIUGf+7 1KXVfoeo04zWI/2eoAgcbftBEP2bT1rMYIa0bowXNqhZmiwj3ZlW5DDzQ5O89oU+ dcVR84l9w668uSHLD9a07i3bT72vK/+3ww5NzBwSdSJmjDmFi1pgOEkbcoDAKok9 GKTtLdf7ulZ8+4HzUlIQN85p0VSRFqeJo7XUQQ5KlsiwB09zwQIwNvJNmlGccKY= =uH6E -----END PGP SIGNATURE----- --0EDmfg2CH0riwOkn3aVQ0wmEBNPjvsJff--