From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wi2ob-0002U4-NM for qemu-devel@nongnu.org; Wed, 07 May 2014 10:27:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wi2oT-0002Q2-95 for qemu-devel@nongnu.org; Wed, 07 May 2014 10:27:21 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:51745 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wi2oS-0002PK-V8 for qemu-devel@nongnu.org; Wed, 07 May 2014 10:27:13 -0400 Message-ID: <536A42B0.1070307@kamp.de> Date: Wed, 07 May 2014 16:26:56 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1399422203-5861-1-git-send-email-pl@kamp.de> <53699FE4.5030004@redhat.com> In-Reply-To: <53699FE4.5030004@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, famz@redhat.com, stefanha@redhat.com, mreitz@redhat.com On 07.05.2014 04:52, Eric Blake wrote: > 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 >> --- >> 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? Actually I have no idea. As you pointed out in the qapi patch I sent it was quite hard for me to crawl through the whole stuff as one who is not familiar with it. Can somebody advise here? Anyhow, I wonder how this would work since qapi doesn't know the C Macros. > >> +++ 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). I tried to be consistent here. All "setters" use a dash while all "getters" have an underscore. Look e.g. at all the I/O thortteling parameters. One reason might be that a dash is not allowed as a member name in a struct. From this perspective the only inconsistent one is node-name. > > 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) I tried to, however it did not compile for me when I tried this. > > 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". The reason I choosed zeroEs is that the function in the background is named bdrv_write_zeroEs. > >> '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. Ok > >> +# >> +# @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. Will do. Peter > >> +# @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. >