From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42429) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1DOE-0007fv-8B for qemu-devel@nongnu.org; Wed, 17 Dec 2014 07:07:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y1DO8-0005cG-25 for qemu-devel@nongnu.org; Wed, 17 Dec 2014 07:07:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39770) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1DO7-0005cA-Q4 for qemu-devel@nongnu.org; Wed, 17 Dec 2014 07:07:31 -0500 Message-ID: <549171FD.9040606@redhat.com> Date: Wed, 17 Dec 2014 07:07:25 -0500 From: John Snow MIME-Version: 1.0 References: <1417465816-19345-1-git-send-email-jsnow@redhat.com> <1417465816-19345-3-git-send-email-jsnow@redhat.com> <20141209160059.GD27053@stefanha-thinkpad.redhat.com> <87y4qfsmik.fsf@blackfin.pond.sub.org> <20141212105336.GA22222@stefanha-thinkpad.redhat.com> <87sighfgao.fsf@blackfin.pond.sub.org> In-Reply-To: <87sighfgao.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, Fam Zheng , Stefan Hajnoczi , qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, pbonzini@redhat.com On 12/15/2014 03:50 AM, Markus Armbruster wrote: > Stefan Hajnoczi writes: > >> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote: >>> Stefan Hajnoczi writes: >>> >>>> On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote: >>>>> diff --git a/block.c b/block.c >>>>> index e5c6ccf..3f27519 100644 >>>>> --- a/block.c >>>>> +++ b/block.c >>>>> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector >>>>> } >>>>> } >>>>> >>>>> +#define BDB_MIN_DEF_GRANULARITY 4096 >>>>> +#define BDB_MAX_DEF_GRANULARITY 65536 >>>>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY >>>>> + >>>>> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs) >>>> >>>> Long names are unwieldy but introducing multiple abbreviations is not a >>>> good solution, it makes the code more confusing (BDB vs dbm). >>>> >>>> I would call the function bdrv_get_default_bitmap_granularity(). >>>> >>>> The constants weren't necessary since the point of this function is to >>>> capture the default value. No one else should use the constants - >>>> otherwise there is a high probability that they are reimplementing this >>>> logic. I would just leave them as literals in the code. >>>> >>>>> diff --git a/blockdev.c b/blockdev.c >>>>> index 5651a8e..4d30b09 100644 >>>>> --- a/blockdev.c >>>>> +++ b/blockdev.c >>>>> @@ -1894,6 +1894,60 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, >>>>> aio_context_release(aio_context); >>>>> } >>>>> >>>>> +void qmp_block_dirty_bitmap_add(const char *device, const char *name, >>>>> + bool has_granularity, int64_t granularity, >>>>> + Error **errp) >>>>> +{ >>>>> + BlockDriverState *bs; >>>>> + >>>>> + bs = bdrv_lookup_bs(device, NULL, errp); >>>> >>>> Markus: I think we need to support node-name here so dirty bitmaps can >>>> be applied at any node in the graph? >>> >>> We need to consider node names for all new QMP commands. Whenever we >>> add a backend name parameter, like we do for the two new commands in >>> this patch, we need to decide whether nodes make sense, too. Do they? >>> >>> I'm afraid we haven't quite made up our mind what to do when nodes make >>> sense. >>> >>> Existing patterns of backend / node name parameters: >>> >>> 1. Backend name only >>> >>> Parameter is commonly called @device. Examples: eject, >>> block_set_io_throttle. >>> >>> Code uses blk_by_name() or bdrv_find(). The latter needs to be >>> converted to the former. >>> >>> 2. Backend or node name >>> >>> 2a. Two optional parameters, commonly called @device and @node-name, >>> of which exactly one must be given. Example: block_passwd. >>> >>> Code uses >>> >>> bs = bdrv_lookup_bs(has_device ? device : NULL, >>> has_node_name ? node_name : NULL, >>> &local_err); >>> >>> which is a roundabout way to say >>> >>> bs = bdrv_lookup_bs(device, node_name, &local_err); >>> >>> 2b. Single parameter. The single example is anonymous union >>> BlockdevRef. >>> >>> Code uses >>> >>> bs = bdrv_lookup_bs(reference, reference, errp); >>> >>> If we want to adopt "single parameter" going forward, we need to >>> decide on a naming convention. Existing commands are probably stuck >>> with @device for compatibility. Do we care for names enough to >>> improve on that? >>> >>> A convenience wrapper around bdrv_lookup_bs() to avoid stuttering >>> name argument could make sense. >> >> Initially only the backend needs dirty bitmap support (this satisfies >> the incremental backup use case). >> >> It is possible that future use cases will require node-name. >> >> I'm happy with just allowing the device parameter today and adding the >> node-name parameter if needed later. >> >> This conservative approach seems good because IMO we shouldn't add >> unused features to the API since they need to be tested and maintained >> forever. >> >> So maybe use a device argument with blk_by_name() for now. >> >> In the future switch to bdrv_lookup_bs() with has_device/has_node_name. >> >> If anyone strongly feels we should support node-name from day 1, I'm >> okay with that too but there needs to be a test case which actually >> exercises that code! > > Agree with not adding unused features. > > However, we should make up our minds how we want QMP to do backend and > node names in the future. I see two basic options: > > 1. Inertia > > Keep adding separate arguments for backend name (commonly called > @device) and node name (commonly called @node-name). If the command > can take only one, make it mandatory. If it can take either, make it > either/or. > > Cements the inconsistency between single parameter in BlockdevRef and > the two either/or parameters elsewhere. > > 2. Clean up the mess > > New commands take a single parameter. The command accepts backends > or nodes as they make sense for the command. Acceptable parameter > name needed. > > Either existing commands get changed to single parameter (with the > necessary compatibility and discoverability gunk), or they get > replaced by new commands. > > I'll analyze how the gunk could be done in a separate message, > hopefully soon. > OK, given the most recent email, it seems as if you would prefer to use "@device" for backends and "@node-name" for arbitrary node selection. This command only needs to support the backend for now, I think. Assuming we want to allow arbitrary nodes in the future, should I leave the parameters as @device but rename the monitor commands to allow for an arbitrary node version sometime later? I don't want to introduce a new command that creates a new mess for us to clean up as we unify these parameter semantics. I said I'd use your mail as a guide but I hadn't skimmed it yet to see how specific the prescriptions were ;)