| Message ID | 1417465816-19345-3-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/01/2014 01:30 PM, John Snow wrote: > From: Fam Zheng <famz@redhat.com> > > The new command pair is added to manage user created dirty bitmap. The > 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 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: Fam Zheng <famz@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > +block-dirty-bitmap-add > +---------------------- > + > +Create a dirty bitmap with a name on the device, and start tracking the writes. > + > +Arguments: > + > +- "device": device name to create dirty bitmap (json-string) > +- "name": name of the new dirty bitmap (json-string) > +- "granularity": granularity to track writes with (int) Worth mentioning that this is optional? Reviewed-by: Eric Blake <eblake@redhat.com>
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? > + if (!bs) { > + return; > + } These new monitor commands need to acquire/release the BlockDriverState's AioContext like the other monitor commands. This ensures that BDS accesses in other threads are synchronized (stopped while we run). AioContext *aio_context; aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); ... out: aio_context_release(aio_context);
Stefan Hajnoczi <stefanha@gmail.com> 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. 3. Node name only No known example. 4. Backend and node name The backend parameter is commonly called @device. The node parameter name varies. Example: blockdev-snapshot-sync's @snapshot-node-name, change-backing-file's @image-node-name. Code uses either bdrv_find_node(node_name) or bdrv_lookup_bs(NULL, node_name, &local_err). This patch invents yet another code pattern: bdrv_lookup_bs(device, NULL, errp); I take this as a sign that something's amiss, either with the existing patterns or with the new one :) [...]
On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@gmail.com> 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! Stefan
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote: >> Stefan Hajnoczi <stefanha@gmail.com> 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.
On 12/15/2014 03:50 AM, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@gmail.com> writes: > >> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote: >>> Stefan Hajnoczi <stefanha@gmail.com> 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 ;)
John Snow <jsnow@redhat.com> writes: > On 12/15/2014 03:50 AM, Markus Armbruster wrote: >> Stefan Hajnoczi <stefanha@gmail.com> writes: >> >>> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote: >>>> Stefan Hajnoczi <stefanha@gmail.com> 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. Whenever you think "only backend for now", you should pause and think hard whether the command makes sense only for backends, or whether we merely choose to implement only backends for now. If it's the former, go ahead and call it @device. It's a stupid name, but it's traditional. If it's the latter: > 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. As you wrote, calling it @device now will make future generalization to nodes awkward. I'd lean towards calling it @node-name right away, documenting "backend root nodes only" as a restriction. But others could have different opinions. Let's hash it out. > I said I'd use your mail as a guide but I hadn't skimmed it yet to see > how specific the prescriptions were ;) Okay :)
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) +{ + BlockDriverInfo bdi; + uint64_t granularity; + + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { + granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size); + granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity); + } else { + granularity = BDB_DEFAULT_GRANULARITY; + } + + return granularity; +} + void bdrv_dirty_iter_init(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) { diff --git a/block/mirror.c b/block/mirror.c index 858e4ff..3633632 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -664,15 +664,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, MirrorBlockJob *s; if (granularity == 0) { - /* Choose the default granularity based on the target file's cluster - * size, clamped between 4k and 64k. */ - BlockDriverInfo bdi; - if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { - granularity = MAX(4096, bdi.cluster_size); - granularity = MIN(65536, granularity); - } else { - granularity = 65536; - } + granularity = bdrv_dbm_calc_def_granularity(target); } assert ((granularity & (granularity - 1)) == 0); 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); + if (!bs) { + return; + } + + if (!name || name[0] == '\0') { + error_setg(errp, "Bitmap name cannot be empty"); + return; + } + if (has_granularity) { + if (granularity < 512 || !is_power_of_2(granularity)) { + error_setg(errp, "Granularity must be power of 2 " + "and at least 512"); + return; + } + } else { + /* Default to cluster size, if available: */ + granularity = bdrv_dbm_calc_def_granularity(bs); + } + + bdrv_create_dirty_bitmap(bs, granularity, name, errp); +} + +void qmp_block_dirty_bitmap_remove(const char *device, const char *name, + Error **errp) +{ + BlockDriverState *bs; + BdrvDirtyBitmap *bitmap; + + bs = bdrv_lookup_bs(device, NULL, errp); + if (!bs) { + return; + } + + if (!name || name[0] == '\0') { + error_setg(errp, "Bitmap name cannot be empty"); + return; + } + bitmap = bdrv_find_dirty_bitmap(bs, name); + if (!bitmap) { + error_setg(errp, "Dirty bitmap not found: %s", name); + return; + } + + bdrv_dirty_bitmap_make_anon(bs, bitmap); + bdrv_release_dirty_bitmap(bs, bitmap); +} + int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) { const char *id = qdict_get_str(qdict, "id"); diff --git a/include/block/block.h b/include/block/block.h index 52fb6b2..066ded6 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -439,6 +439,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs); int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); diff --git a/qapi/block-core.json b/qapi/block-core.json index d176324..da86e91 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -894,6 +894,61 @@ '*on-target-error': 'BlockdevOnError' } } ## +# @BlockDirtyBitmap +# +# @device: name of device which the bitmap is tracking +# +# @name: name of the dirty bitmap +# +# Since 2.3 +## +{ 'type': 'BlockDirtyBitmap', + 'data': { 'device': 'str', 'name': 'str' } } + +## +# @BlockDirtyBitmapAdd +# +# @device: name of device which the bitmap is tracking +# +# @name: name of the dirty bitmap +# +# @granularity: #optional the bitmap granularity, default is 64k for +# block-dirty-bitmap-add +# +# Since 2.3 +## +{ 'type': 'BlockDirtyBitmapAdd', + 'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } } + +## +# @block-dirty-bitmap-add +# +# Create a dirty bitmap with a name on the device +# +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# If @name is already taken, GenericError with an explanation +# +# Since 2.3 +## +{'command': 'block-dirty-bitmap-add', + 'data': 'BlockDirtyBitmapAdd' } + +## +# @block-dirty-bitmap-remove +# +# Remove a dirty bitmap on the device +# +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# If @name is not found, GenericError with an explaining message +# +# Since 2.3 +## +{'command': 'block-dirty-bitmap-remove', + 'data': 'BlockDirtyBitmap' } + +## # @block_set_io_throttle: # # Change I/O throttle limits for a block drive. diff --git a/qmp-commands.hx b/qmp-commands.hx index d4b0010..03465a2 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1202,6 +1202,55 @@ Example: EQMP { + .name = "block-dirty-bitmap-add", + .args_type = "device:B,name:s,granularity:i?", + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add, + }, + { + .name = "block-dirty-bitmap-remove", + .args_type = "device:B,name:s", + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove, + }, + +SQMP + +block-dirty-bitmap-add +---------------------- + +Create a dirty bitmap with a name on the device, and start tracking the writes. + +Arguments: + +- "device": device name to create dirty bitmap (json-string) +- "name": name of the new dirty bitmap (json-string) +- "granularity": granularity to track writes with (int) + +Example: + +-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0", + "name": "bitmap0" } } +<- { "return": {} } + +block-dirty-bitmap-remove +------------------------- + +Stop write tracking and remove the dirty bitmap that was created with +block-dirty-bitmap-add. + +Arguments: + +- "device": device name to remove dirty bitmap (json-string) +- "name": name of the dirty bitmap to remove (json-string) + +Example: + +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "device": "drive0", + "name": "bitmap0" } } +<- { "return": {} } + +EQMP + + { .name = "blockdev-snapshot-sync", .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?", .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,