From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37542) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNSJw-0007wu-63 for qemu-devel@nongnu.org; Mon, 16 Feb 2015 15:31:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNSJr-0003N0-P3 for qemu-devel@nongnu.org; Mon, 16 Feb 2015 15:31:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34181) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNSJr-0003Ms-Hb for qemu-devel@nongnu.org; Mon, 16 Feb 2015 15:31:03 -0500 Message-ID: <54E25385.8020804@redhat.com> Date: Mon, 16 Feb 2015 15:31:01 -0500 From: John Snow MIME-Version: 1.0 References: <1423865338-8576-1-git-send-email-jsnow@redhat.com> <1423865338-8576-4-git-send-email-jsnow@redhat.com> <54E25194.4060502@redhat.com> In-Reply-To: <54E25194.4060502@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 02/16/2015 03:22 PM, Eric Blake wrote: > On 02/13/2015 03:08 PM, John Snow wrote: >> The new command pair is added to manage user created dirty bitmap. The > > s/manage/manage a/ > >> dirty bitmap's name is mandatory and must be unique for the same device, >> but different devices can have bitmaps with the same names. >> >> The granularity is an optional field. If it is not specified, we will >> choose a default granularity based on the cluster size if available, >> clamped to between 4K and 64K to mirror how the 'mirror' code was >> already choosing granularity. If we do not have cluster size info >> available, we choose 64K. This code has been factored out into a helper >> shared with block/mirror. > > The drive-mirror command in block-core.json documents that it supports a > granularity between 512 and 64M, not 64k. Is there a bug here? See > more below. qmp_drive_mirror DOES clamp the user-specified value to [512, 64M]. It does also allow '0', which gets passed through to mirror_start_job, which chooses a default based on the cluster_size, clamped to [512, 64K]. > >> >> This patch also introduces the 'block_dirty_bitmap_lookup' helper, >> which takes a device name and a dirty bitmap name and validates the >> lookup, returning NULL and setting errp if there is a problem with >> either field. This helper will be re-used in future patches in this >> series. >> >> The types added to block-core.json will be re-used in future patches >> in this series, see: >> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}' >> >> Signed-off-by: John Snow >> --- >> block.c | 20 ++++++++++ >> block/mirror.c | 10 +---- >> blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 1 + >> qapi/block-core.json | 55 +++++++++++++++++++++++++++ >> qmp-commands.hx | 51 +++++++++++++++++++++++++ >> 6 files changed, 228 insertions(+), 9 deletions(-) >> > >> + >> + if (has_granularity) { >> + if (granularity < 512 || !is_power_of_2(granularity)) { >> + error_setg(errp, "Granularity must be power of 2 " >> + "and at least 512"); >> + goto out; >> + } >> + } else { >> + /* Default to cluster size, if available: */ >> + granularity = bdrv_get_default_bitmap_granularity(bs); >> + } > > This enforces a minimum granularity, but should we also be enforcing a > maximum? That is, if the user does not provide granularity, we default > between 4k and 64k based on cluster size, but if the user DOES provide > granularity, we allow as small as 512, but as large as the user can > pass. Should we enforce a maximum of 64M? > > Not a bug as such, though not a purposeful decision on my part either. I don't have a concrete reason to prohibit what kinds of granularities the user can supply, but when it came to picking a default I decided to imitate what mirror was already doing, which I explain above. The existing code is basically: "Try to pick a sane default if the user didn't give us one, but allow the user to choose something bananas if they want to. Maybe they know better than we do."