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. > > This significantly speeds up file system initialization and > should speed zero write test used to test backend storage > performance. >
> > Signed-off-by: Peter Lieven <[email protected]> > --- > v2->v3: - moved parameter parsing to blockdev_init > - added per device detect_zeroes status to > 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] > > > +static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp) > +{ > + 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. 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' ] } > > ## > +# @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 backing > file sectors into the image file. > +@item detect-zeroes=@var{detect-zeroes} > +@var{detect-zeroes} is "off", "on" or "unmap" and enables the automatic > +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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
