From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52220) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WXFfu-0000M2-Fc for qemu-devel@nongnu.org; Mon, 07 Apr 2014 15:57:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WXFfo-0004o4-AO for qemu-devel@nongnu.org; Mon, 07 Apr 2014 15:57:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WXFfo-0004o0-1Y for qemu-devel@nongnu.org; Mon, 07 Apr 2014 15:57:40 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s37JBDIb004799 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 7 Apr 2014 15:11:14 -0400 Message-ID: <5342F839.6030309@redhat.com> Date: Mon, 07 Apr 2014 21:10:49 +0200 From: Max Reitz MIME-Version: 1.0 References: <1396891800-8627-1-git-send-email-mreitz@redhat.com> <1396891800-8627-2-git-send-email-mreitz@redhat.com> <5342F722.90503@redhat.com> In-Reply-To: <5342F722.90503@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] block-commit: Expose granularity List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 07.04.2014 21:06, Eric Blake wrote: > On 04/07/2014 11:29 AM, Max Reitz wrote: >> Allow QMP users to manipulate the granularity used in the block-commit >> command. >> >> Signed-off-by: Max Reitz >> --- >> @@ -214,6 +215,13 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, >> orig_base_flags = bdrv_get_flags(base); >> orig_overlay_flags = bdrv_get_flags(overlay_bs); >> >> + if (!granularity) { >> + granularity = COMMIT_BUFFER_SIZE; >> + } > Default granularity of 0 becomes buffer size... > >> @@ -1876,6 +1876,15 @@ void qmp_block_commit(const char *device, >> */ >> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT; >> >> + granularity = has_granularity ? granularity : 0; >> + >> + if (has_granularity && >> + (granularity < BDRV_SECTOR_SIZE || (granularity & (granularity - 1)))) >> + { >> + error_set(errp, QERR_INVALID_PARAMETER, "granularity"); > ...but this code rejects attempts for me to explicitly set granularity > to the default. Should it be 'if (granularity &&' instead of your > current wording? I intentionally rejected a granularity of 0, as I thought the user were always capable of dropping this parameter for obtaining the default. But since you are bringing this up, I guess there may be cases where a user is forced to give the parameter but wants to keep the default value nonetheless; I will change it to accept 0 in v2. > Also, is it worth using qemu-common.h's is_power_of_2 instead of > inlining it yourself? (I don't care, as I recognize the bit > manipulations, but other readers might prefer the named version for its > legibility) You are right, I will use the function from qemu-common.h. Max >> +++ b/qapi-schema.json >> @@ -2112,6 +2112,9 @@ >> # >> # @speed: #optional the maximum speed, in bytes per second >> # >> +# @granularity: #optional the granularity to be used for the operation, in >> +# bytes; has to be a power of two and at least 512 (since 2.1) >> +# > At least you documented here that an explicit '0' is rejected, even > though it might be nicer to allow it for the sake of requesting the > default even if the default later changes in size. > > Overall, though, I'm liking this series. >