| Message ID | 1421080265-2228-4-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2015-01-12 at 11:30, 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. > > 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: Fam Zheng <famz@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > 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(-) > > diff --git a/block.c b/block.c > index bfeae6b..3eb77ee 100644 > --- a/block.c > +++ b/block.c > @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector > } > } > > +/** > + * Chooses a default granularity based on the existing cluster size, > + * but clamped between [4K, 64K]. Defaults to 64K in the case that there > + * is no cluster size information available. > + */ > +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) > +{ > + BlockDriverInfo bdi; > + uint64_t granularity; > + > + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { > + granularity = MAX(4096, bdi.cluster_size); > + granularity = MIN(65536, granularity); > + } else { > + granularity = 65536; > + } > + > + return granularity; > +} > + > void bdrv_dirty_iter_init(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) > { > diff --git a/block/mirror.c b/block/mirror.c > index d819952..fc545f1 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -667,15 +667,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_get_default_bitmap_granularity(target); > } > > assert ((granularity & (granularity - 1)) == 0); > diff --git a/blockdev.c b/blockdev.c > index 5651a8e..95251c7 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1173,6 +1173,48 @@ out_aio_context: > return NULL; > } > > +/** > + * Return a dirty bitmap (if present), after validating > + * the node reference and bitmap names. Returns NULL on error, > + * including when the BDS and/or bitmap is not found. > + */ > +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, > + const char *name, > + BlockDriverState **pbs, > + Error **errp) > +{ > + BlockDriverState *bs; > + BdrvDirtyBitmap *bitmap; > + > + if (!node_ref) { > + error_setg(errp, "Node reference cannot be NULL"); > + return NULL; > + } > + if (!name) { > + error_setg(errp, "Bitmap name cannot be NULL"); > + return NULL; > + } > + > + bs = bdrv_lookup_bs(node_ref, node_ref, errp); > + if (!bs) { > + error_setg(errp, "Node reference '%s' not found", node_ref); No need to throw the (hopefully) perfectly fine Error code returned by bdrv_lookup_bs() away. > + return NULL; > + } > + > + /* If caller provided a BDS*, provide the result of that lookup, too. */ > + if (pbs) { > + *pbs = bs; > + } > + > + bitmap = bdrv_find_dirty_bitmap(bs, name); > + if (!bitmap) { > + error_setg(errp, "Dirty bitmap not found: %s", name); I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error message looks like "foo because bar" to me. But it's up to you, dirty bitmap names most likely don't look like error reasons so nobody should be able to confuse it. So with the error_setg() in the fail path for bdrv_lookup_bs() removed: Reviewed-by: Max Reitz <mreitz@redhat.com>
On 01/16/2015 10:36 AM, Max Reitz wrote: > On 2015-01-12 at 11:30, 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. >> >> 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: Fam Zheng <famz@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> 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(-) >> >> diff --git a/block.c b/block.c >> index bfeae6b..3eb77ee 100644 >> --- a/block.c >> +++ b/block.c >> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, int64_t sector >> } >> } >> +/** >> + * Chooses a default granularity based on the existing cluster size, >> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >> + * is no cluster size information available. >> + */ >> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >> +{ >> + BlockDriverInfo bdi; >> + uint64_t granularity; >> + >> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >> + granularity = MAX(4096, bdi.cluster_size); >> + granularity = MIN(65536, granularity); >> + } else { >> + granularity = 65536; >> + } >> + >> + return granularity; >> +} >> + >> void bdrv_dirty_iter_init(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >> { >> diff --git a/block/mirror.c b/block/mirror.c >> index d819952..fc545f1 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >> } >> assert ((granularity & (granularity - 1)) == 0); >> diff --git a/blockdev.c b/blockdev.c >> index 5651a8e..95251c7 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1173,6 +1173,48 @@ out_aio_context: >> return NULL; >> } >> +/** >> + * Return a dirty bitmap (if present), after validating >> + * the node reference and bitmap names. Returns NULL on error, >> + * including when the BDS and/or bitmap is not found. >> + */ >> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, >> + const char *name, >> + BlockDriverState >> **pbs, >> + Error **errp) >> +{ >> + BlockDriverState *bs; >> + BdrvDirtyBitmap *bitmap; >> + >> + if (!node_ref) { >> + error_setg(errp, "Node reference cannot be NULL"); >> + return NULL; >> + } >> + if (!name) { >> + error_setg(errp, "Bitmap name cannot be NULL"); >> + return NULL; >> + } >> + >> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >> + if (!bs) { >> + error_setg(errp, "Node reference '%s' not found", node_ref); > > No need to throw the (hopefully) perfectly fine Error code returned by > bdrv_lookup_bs() away. > I just wanted an error message consistent with the parameter name, in this case. i.e., We couldn't find the "Node reference" instead of "device" or "node name." Just trying to distinguish the fact that this is an arbitrary reference in the error message. I can still remove it, but I am curious to see what Markus thinks of the names I have chosen before I monkey with the errors too much more. >> + return NULL; >> + } >> + >> + /* If caller provided a BDS*, provide the result of that lookup, >> too. */ >> + if (pbs) { >> + *pbs = bs; >> + } >> + >> + bitmap = bdrv_find_dirty_bitmap(bs, name); >> + if (!bitmap) { >> + error_setg(errp, "Dirty bitmap not found: %s", name); > > I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error > message looks like "foo because bar" to me. But it's up to you, dirty > bitmap names most likely don't look like error reasons so nobody should > be able to confuse it. > No, I agree. > So with the error_setg() in the fail path for bdrv_lookup_bs() removed: > > Reviewed-by: Max Reitz <mreitz@redhat.com> >
On 2015-01-16 at 11:48, John Snow wrote: > > > On 01/16/2015 10:36 AM, Max Reitz wrote: >> On 2015-01-12 at 11:30, 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. >>> >>> 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: Fam Zheng <famz@redhat.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> 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(-) >>> >>> diff --git a/block.c b/block.c >>> index bfeae6b..3eb77ee 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, int64_t sector >>> } >>> } >>> +/** >>> + * Chooses a default granularity based on the existing cluster size, >>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that >>> there >>> + * is no cluster size information available. >>> + */ >>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>> +{ >>> + BlockDriverInfo bdi; >>> + uint64_t granularity; >>> + >>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>> + granularity = MAX(4096, bdi.cluster_size); >>> + granularity = MIN(65536, granularity); >>> + } else { >>> + granularity = 65536; >>> + } >>> + >>> + return granularity; >>> +} >>> + >>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>> { >>> diff --git a/block/mirror.c b/block/mirror.c >>> index d819952..fc545f1 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>> } >>> assert ((granularity & (granularity - 1)) == 0); >>> diff --git a/blockdev.c b/blockdev.c >>> index 5651a8e..95251c7 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -1173,6 +1173,48 @@ out_aio_context: >>> return NULL; >>> } >>> +/** >>> + * Return a dirty bitmap (if present), after validating >>> + * the node reference and bitmap names. Returns NULL on error, >>> + * including when the BDS and/or bitmap is not found. >>> + */ >>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char >>> *node_ref, >>> + const char *name, >>> + BlockDriverState >>> **pbs, >>> + Error **errp) >>> +{ >>> + BlockDriverState *bs; >>> + BdrvDirtyBitmap *bitmap; >>> + >>> + if (!node_ref) { >>> + error_setg(errp, "Node reference cannot be NULL"); >>> + return NULL; >>> + } >>> + if (!name) { >>> + error_setg(errp, "Bitmap name cannot be NULL"); >>> + return NULL; >>> + } >>> + >>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>> + if (!bs) { >>> + error_setg(errp, "Node reference '%s' not found", node_ref); >> >> No need to throw the (hopefully) perfectly fine Error code returned by >> bdrv_lookup_bs() away. >> > > I just wanted an error message consistent with the parameter name, in > this case. i.e., We couldn't find the "Node reference" instead of > "device" or "node name." Just trying to distinguish the fact that this > is an arbitrary reference in the error message. > > I can still remove it, but I am curious to see what Markus thinks of > the names I have chosen before I monkey with the errors too much more. Very well then, but you should clean up the error returned by bdrv_lookup_bs() (call error_free()). Feel free to keep my R-b whichever decision you'll make (as long as the error returned by bdrv_lookup_bs() is not leaked). Max > >>> + return NULL; >>> + } >>> + >>> + /* If caller provided a BDS*, provide the result of that lookup, >>> too. */ >>> + if (pbs) { >>> + *pbs = bs; >>> + } >>> + >>> + bitmap = bdrv_find_dirty_bitmap(bs, name); >>> + if (!bitmap) { >>> + error_setg(errp, "Dirty bitmap not found: %s", name); >> >> I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error >> message looks like "foo because bar" to me. But it's up to you, dirty >> bitmap names most likely don't look like error reasons so nobody should >> be able to confuse it. >> > > No, I agree. > >> So with the error_setg() in the fail path for bdrv_lookup_bs() removed: >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >
On 01/16/2015 11:51 AM, Max Reitz wrote: > On 2015-01-16 at 11:48, John Snow wrote: >> >> >> On 01/16/2015 10:36 AM, Max Reitz wrote: >>> On 2015-01-12 at 11:30, 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. >>>> >>>> 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: Fam Zheng <famz@redhat.com> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> 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(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index bfeae6b..3eb77ee 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, int64_t sector >>>> } >>>> } >>>> +/** >>>> + * Chooses a default granularity based on the existing cluster size, >>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that >>>> there >>>> + * is no cluster size information available. >>>> + */ >>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>>> +{ >>>> + BlockDriverInfo bdi; >>>> + uint64_t granularity; >>>> + >>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>>> + granularity = MAX(4096, bdi.cluster_size); >>>> + granularity = MIN(65536, granularity); >>>> + } else { >>>> + granularity = 65536; >>>> + } >>>> + >>>> + return granularity; >>>> +} >>>> + >>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>>> { >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index d819952..fc545f1 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>>> } >>>> assert ((granularity & (granularity - 1)) == 0); >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 5651a8e..95251c7 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -1173,6 +1173,48 @@ out_aio_context: >>>> return NULL; >>>> } >>>> +/** >>>> + * Return a dirty bitmap (if present), after validating >>>> + * the node reference and bitmap names. Returns NULL on error, >>>> + * including when the BDS and/or bitmap is not found. >>>> + */ >>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char >>>> *node_ref, >>>> + const char *name, >>>> + BlockDriverState >>>> **pbs, >>>> + Error **errp) >>>> +{ >>>> + BlockDriverState *bs; >>>> + BdrvDirtyBitmap *bitmap; >>>> + >>>> + if (!node_ref) { >>>> + error_setg(errp, "Node reference cannot be NULL"); >>>> + return NULL; >>>> + } >>>> + if (!name) { >>>> + error_setg(errp, "Bitmap name cannot be NULL"); >>>> + return NULL; >>>> + } >>>> + >>>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>>> + if (!bs) { >>>> + error_setg(errp, "Node reference '%s' not found", node_ref); >>> >>> No need to throw the (hopefully) perfectly fine Error code returned by >>> bdrv_lookup_bs() away. >>> >> >> I just wanted an error message consistent with the parameter name, in >> this case. i.e., We couldn't find the "Node reference" instead of >> "device" or "node name." Just trying to distinguish the fact that this >> is an arbitrary reference in the error message. >> >> I can still remove it, but I am curious to see what Markus thinks of >> the names I have chosen before I monkey with the errors too much more. > > Very well then, but you should clean up the error returned by > bdrv_lookup_bs() (call error_free()). > > Feel free to keep my R-b whichever decision you'll make (as long as the > error returned by bdrv_lookup_bs() is not leaked). > > Max > Or a new helper designed specifically for single argument "BB -or- BDS" lookups, too, that uses the name of the parameter that we eventually decide upon. I'll clean up the leak for now. Thanks! >> >>>> + return NULL; >>>> + } >>>> + >>>> + /* If caller provided a BDS*, provide the result of that lookup, >>>> too. */ >>>> + if (pbs) { >>>> + *pbs = bs; >>>> + } >>>> + >>>> + bitmap = bdrv_find_dirty_bitmap(bs, name); >>>> + if (!bitmap) { >>>> + error_setg(errp, "Dirty bitmap not found: %s", name); >>> >>> I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error >>> message looks like "foo because bar" to me. But it's up to you, dirty >>> bitmap names most likely don't look like error reasons so nobody should >>> be able to confuse it. >>> >> >> No, I agree. >> >>> So with the error_setg() in the fail path for bdrv_lookup_bs() removed: >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> >> >
John Snow <jsnow@redhat.com> writes: > On 01/16/2015 10:36 AM, Max Reitz wrote: >> On 2015-01-12 at 11:30, 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. >>> >>> 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: Fam Zheng <famz@redhat.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> 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(-) >>> >>> diff --git a/block.c b/block.c >>> index bfeae6b..3eb77ee 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, int64_t sector >>> } >>> } >>> +/** >>> + * Chooses a default granularity based on the existing cluster size, >>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >>> + * is no cluster size information available. >>> + */ >>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>> +{ >>> + BlockDriverInfo bdi; >>> + uint64_t granularity; >>> + >>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>> + granularity = MAX(4096, bdi.cluster_size); >>> + granularity = MIN(65536, granularity); >>> + } else { >>> + granularity = 65536; >>> + } >>> + >>> + return granularity; >>> +} >>> + >>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>> { >>> diff --git a/block/mirror.c b/block/mirror.c >>> index d819952..fc545f1 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>> } >>> assert ((granularity & (granularity - 1)) == 0); >>> diff --git a/blockdev.c b/blockdev.c >>> index 5651a8e..95251c7 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -1173,6 +1173,48 @@ out_aio_context: >>> return NULL; >>> } >>> +/** >>> + * Return a dirty bitmap (if present), after validating >>> + * the node reference and bitmap names. Returns NULL on error, >>> + * including when the BDS and/or bitmap is not found. >>> + */ >>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, >>> + const char *name, >>> + BlockDriverState >>> **pbs, >>> + Error **errp) >>> +{ >>> + BlockDriverState *bs; >>> + BdrvDirtyBitmap *bitmap; >>> + >>> + if (!node_ref) { >>> + error_setg(errp, "Node reference cannot be NULL"); >>> + return NULL; >>> + } >>> + if (!name) { >>> + error_setg(errp, "Bitmap name cannot be NULL"); >>> + return NULL; >>> + } >>> + >>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>> + if (!bs) { >>> + error_setg(errp, "Node reference '%s' not found", node_ref); >> >> No need to throw the (hopefully) perfectly fine Error code returned by >> bdrv_lookup_bs() away. >> > > I just wanted an error message consistent with the parameter name, in > this case. i.e., We couldn't find the "Node reference" instead of > "device" or "node name." Just trying to distinguish the fact that this > is an arbitrary reference in the error message. > > I can still remove it, but I am curious to see what Markus thinks of > the names I have chosen before I monkey with the errors too much more. bdrv_lookup_bs() is an awkward interface. If @device is non-null, try to look up a backend (BB) named @device. If it exists, return the backend's root node (BDS). Else if @node_name is non-null, try to look up a node (BDS) named @node_name. If it exists, return it. Else, set this error: error_setg(errp, "Cannot find device=%s nor node_name=%s", device ? device : "", node_name ? node_name : ""); The error message is crap unless both device and node_name are non-null and different. Which is never the case: we always either pass two identical non-null arguments, or we pass a null and a non-null argument[*]. In other words, the error message is always crap. In case you wonder why @device takes precedence over node_name when both are given: makes no sense. But when both are given, they are always identical, and since backend and node names share a name space, only one can resolve. A couple of cleaner solutions come to mind: * Make bdrv_lookup_bs() suck less Assert its tacit preconditions: assert(device || node_name); assert(!device || !node_name || device == node_name); Then make it produce a decent error: if (device && node_name) { error_setg(errp, "Neither block backend nor node %s found", device); else if (device) { error_setg(errp, "Block backend %s not found", device); } else if (node_name) { error_setg(errp, "Block node %s not found", node_name); } Note how the three cases mirror the three usage patterns. Further note that the proposed error messages deviate from the existing practice of calling block backends "devices". Calling everything and its dog a "device" is traditional, but it's also lazy and confusing. End of digression. * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its callers Drop the Error ** parameter. Callers know whether a failed lookup was for a device name, a node name or both, and can set an appropriate error themselves. I'd still assert the preconditions. * Replace the function by one for each of its usage patterns I think that's what I'd do. [...] [*] See https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html
On 01/19/2015 05:08 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 01/16/2015 10:36 AM, Max Reitz wrote: >>> On 2015-01-12 at 11:30, 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. >>>> >>>> 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: Fam Zheng <famz@redhat.com> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> 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(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index bfeae6b..3eb77ee 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, int64_t sector >>>> } >>>> } >>>> +/** >>>> + * Chooses a default granularity based on the existing cluster size, >>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >>>> + * is no cluster size information available. >>>> + */ >>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>>> +{ >>>> + BlockDriverInfo bdi; >>>> + uint64_t granularity; >>>> + >>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>>> + granularity = MAX(4096, bdi.cluster_size); >>>> + granularity = MIN(65536, granularity); >>>> + } else { >>>> + granularity = 65536; >>>> + } >>>> + >>>> + return granularity; >>>> +} >>>> + >>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>>> { >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index d819952..fc545f1 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>>> } >>>> assert ((granularity & (granularity - 1)) == 0); >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 5651a8e..95251c7 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -1173,6 +1173,48 @@ out_aio_context: >>>> return NULL; >>>> } >>>> +/** >>>> + * Return a dirty bitmap (if present), after validating >>>> + * the node reference and bitmap names. Returns NULL on error, >>>> + * including when the BDS and/or bitmap is not found. >>>> + */ >>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, >>>> + const char *name, >>>> + BlockDriverState >>>> **pbs, >>>> + Error **errp) >>>> +{ >>>> + BlockDriverState *bs; >>>> + BdrvDirtyBitmap *bitmap; >>>> + >>>> + if (!node_ref) { >>>> + error_setg(errp, "Node reference cannot be NULL"); >>>> + return NULL; >>>> + } >>>> + if (!name) { >>>> + error_setg(errp, "Bitmap name cannot be NULL"); >>>> + return NULL; >>>> + } >>>> + >>>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>>> + if (!bs) { >>>> + error_setg(errp, "Node reference '%s' not found", node_ref); >>> >>> No need to throw the (hopefully) perfectly fine Error code returned by >>> bdrv_lookup_bs() away. >>> >> >> I just wanted an error message consistent with the parameter name, in >> this case. i.e., We couldn't find the "Node reference" instead of >> "device" or "node name." Just trying to distinguish the fact that this >> is an arbitrary reference in the error message. >> >> I can still remove it, but I am curious to see what Markus thinks of >> the names I have chosen before I monkey with the errors too much more. > > bdrv_lookup_bs() is an awkward interface. > > If @device is non-null, try to look up a backend (BB) named @device. If > it exists, return the backend's root node (BDS). > > Else if @node_name is non-null, try to look up a node (BDS) named > @node_name. If it exists, return it. > > Else, set this error: > > error_setg(errp, "Cannot find device=%s nor node_name=%s", > device ? device : "", > node_name ? node_name : ""); > > The error message is crap unless both device and node_name are non-null > and different. Which is never the case: we always either pass two > identical non-null arguments, or we pass a null and a non-null > argument[*]. In other words, the error message is always crap. > > In case you wonder why @device takes precedence over node_name when both > are given: makes no sense. But when both are given, they are always > identical, and since backend and node names share a name space, only one > can resolve. > > A couple of cleaner solutions come to mind: > > * Make bdrv_lookup_bs() suck less > > Assert its tacit preconditions: > > assert(device || node_name); > assert(!device || !node_name || device == node_name); > > Then make it produce a decent error: > > if (device && node_name) { > error_setg(errp, "Neither block backend nor node %s found", device); > else if (device) { > error_setg(errp, "Block backend %s not found", device); > } else if (node_name) { > error_setg(errp, "Block node %s not found", node_name); > } > > Note how the three cases mirror the three usage patterns. > > Further note that the proposed error messages deviate from the > existing practice of calling block backends "devices". Calling > everything and its dog a "device" is traditional, but it's also lazy > and confusing. End of digression. > > * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its > callers > > Drop the Error ** parameter. Callers know whether a failed lookup was > for a device name, a node name or both, and can set an appropriate > error themselves. > > I'd still assert the preconditions. > > * Replace the function by one for each of its usage patterns > > I think that's what I'd do. > > [...] > > > [*] See > https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html > I can submit a patch for making bdrv_lookup_bs "nicer," or at least its usage more clear, in a separate patch. If you really want me to fold it into this series, I'd invite you to review explicitly my usage of the parameter "node-ref" before I embark on cleaning up other interface areas. Does this naming scheme look sane to you, and fit with your general expectations? I can also add a "bdrv_lookup_noderef" function that takes only one argument, which will help enforce the "If both arguments are provided, they must be the same" paradigm. This patch (#3) covers my shot at a unified parameter, and you can see further consequences in #7, and #10 (transactions). CC'ing Eric Blake, as well, for comments on a "unified parameter" interface in general. Thanks, --js
John Snow <jsnow@redhat.com> writes: > On 01/19/2015 05:08 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 01/16/2015 10:36 AM, Max Reitz wrote: >>>> On 2015-01-12 at 11:30, 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. >>>>> >>>>> 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: Fam Zheng <famz@redhat.com> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>> --- >>>>> 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(-) >>>>> >>>>> diff --git a/block.c b/block.c >>>>> index bfeae6b..3eb77ee 100644 >>>>> --- a/block.c >>>>> +++ b/block.c >>>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>>>> BdrvDirtyBitmap *bitmap, int64_t sector >>>>> } >>>>> } >>>>> +/** >>>>> + * Chooses a default granularity based on the existing cluster size, >>>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >>>>> + * is no cluster size information available. >>>>> + */ >>>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>>>> +{ >>>>> + BlockDriverInfo bdi; >>>>> + uint64_t granularity; >>>>> + >>>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>>>> + granularity = MAX(4096, bdi.cluster_size); >>>>> + granularity = MIN(65536, granularity); >>>>> + } else { >>>>> + granularity = 65536; >>>>> + } >>>>> + >>>>> + return granularity; >>>>> +} >>>>> + >>>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>>>> { >>>>> diff --git a/block/mirror.c b/block/mirror.c >>>>> index d819952..fc545f1 100644 >>>>> --- a/block/mirror.c >>>>> +++ b/block/mirror.c >>>>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>>>> } >>>>> assert ((granularity & (granularity - 1)) == 0); >>>>> diff --git a/blockdev.c b/blockdev.c >>>>> index 5651a8e..95251c7 100644 >>>>> --- a/blockdev.c >>>>> +++ b/blockdev.c >>>>> @@ -1173,6 +1173,48 @@ out_aio_context: >>>>> return NULL; >>>>> } >>>>> +/** >>>>> + * Return a dirty bitmap (if present), after validating >>>>> + * the node reference and bitmap names. Returns NULL on error, >>>>> + * including when the BDS and/or bitmap is not found. >>>>> + */ >>>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, >>>>> + const char *name, >>>>> + BlockDriverState >>>>> **pbs, >>>>> + Error **errp) >>>>> +{ >>>>> + BlockDriverState *bs; >>>>> + BdrvDirtyBitmap *bitmap; >>>>> + >>>>> + if (!node_ref) { >>>>> + error_setg(errp, "Node reference cannot be NULL"); >>>>> + return NULL; >>>>> + } >>>>> + if (!name) { >>>>> + error_setg(errp, "Bitmap name cannot be NULL"); >>>>> + return NULL; >>>>> + } >>>>> + >>>>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>>>> + if (!bs) { >>>>> + error_setg(errp, "Node reference '%s' not found", node_ref); >>>> >>>> No need to throw the (hopefully) perfectly fine Error code returned by >>>> bdrv_lookup_bs() away. >>>> >>> >>> I just wanted an error message consistent with the parameter name, in >>> this case. i.e., We couldn't find the "Node reference" instead of >>> "device" or "node name." Just trying to distinguish the fact that this >>> is an arbitrary reference in the error message. >>> >>> I can still remove it, but I am curious to see what Markus thinks of >>> the names I have chosen before I monkey with the errors too much more. >> >> bdrv_lookup_bs() is an awkward interface. >> >> If @device is non-null, try to look up a backend (BB) named @device. If >> it exists, return the backend's root node (BDS). >> >> Else if @node_name is non-null, try to look up a node (BDS) named >> @node_name. If it exists, return it. >> >> Else, set this error: >> >> error_setg(errp, "Cannot find device=%s nor node_name=%s", >> device ? device : "", >> node_name ? node_name : ""); >> >> The error message is crap unless both device and node_name are non-null >> and different. Which is never the case: we always either pass two >> identical non-null arguments, or we pass a null and a non-null >> argument[*]. In other words, the error message is always crap. >> >> In case you wonder why @device takes precedence over node_name when both >> are given: makes no sense. But when both are given, they are always >> identical, and since backend and node names share a name space, only one >> can resolve. >> >> A couple of cleaner solutions come to mind: >> >> * Make bdrv_lookup_bs() suck less >> >> Assert its tacit preconditions: >> >> assert(device || node_name); >> assert(!device || !node_name || device == node_name); >> >> Then make it produce a decent error: >> >> if (device && node_name) { >> error_setg(errp, "Neither block backend nor node %s found", device); >> else if (device) { >> error_setg(errp, "Block backend %s not found", device); >> } else if (node_name) { >> error_setg(errp, "Block node %s not found", node_name); >> } >> >> Note how the three cases mirror the three usage patterns. >> >> Further note that the proposed error messages deviate from the >> existing practice of calling block backends "devices". Calling >> everything and its dog a "device" is traditional, but it's also lazy >> and confusing. End of digression. >> >> * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its >> callers >> >> Drop the Error ** parameter. Callers know whether a failed lookup was >> for a device name, a node name or both, and can set an appropriate >> error themselves. >> >> I'd still assert the preconditions. >> >> * Replace the function by one for each of its usage patterns >> >> I think that's what I'd do. >> >> [...] >> >> >> [*] See >> https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html >> > > I can submit a patch for making bdrv_lookup_bs "nicer," or at least > its usage more clear, in a separate patch. Yes, please. > If you really want me to fold it into this series, I'd invite you to > review explicitly my usage of the parameter "node-ref" before I embark > on cleaning up other interface areas. Follow-up patch is fine. Adding one more bad error message along the way before you fix them all doesn't bother me. > Does this naming scheme look sane to you, and fit with your general > expectations? > > I can also add a "bdrv_lookup_noderef" function that takes only one > argument, which will help enforce the "If both arguments are provided, > they must be the same" paradigm. > > This patch (#3) covers my shot at a unified parameter, and you can see > further consequences in #7, and #10 (transactions). Do we want to introduce a @node-ref naming convention? Currently, QMP calls parameters or members naming nodes @node-name or similar, and parameters/members naming backends @device or similar. The one place where we already accept either is called @reference in the schema, but it's a member of an anonymous union, so it's not really visible in QMP. Previously[*], we agreed (I think) to replace and deprecate the four commands that use the "pair of names" convention to identify a node. Their replacement would use the "single name" convention. The name can either be a node name or a backend name, and the latter automatically resolves to its root node. The "backend name resolves to its root node" convenience feature should be available consistently or not at all. I think the consensus it to want it consistently. Therefore, your new @node-ref is really the same as the existing @node-name, isn't it? Why a new naming convention @node-ref? Is it meant to be in addition to @node-name, or is it meant to replace it? > CC'ing Eric Blake, as well, for comments on a "unified parameter" > interface in general. Good move. [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html
On 01/20/2015 03:26 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 01/19/2015 05:08 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >>>> On 01/16/2015 10:36 AM, Max Reitz wrote: >>>>> On 2015-01-12 at 11:30, 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. >>>>>> >>>>>> 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: Fam Zheng <famz@redhat.com> >>>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>>> --- >>>>>> 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(-) >>>>>> >>>>>> diff --git a/block.c b/block.c >>>>>> index bfeae6b..3eb77ee 100644 >>>>>> --- a/block.c >>>>>> +++ b/block.c >>>>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>>>>> BdrvDirtyBitmap *bitmap, int64_t sector >>>>>> } >>>>>> } >>>>>> +/** >>>>>> + * Chooses a default granularity based on the existing cluster size, >>>>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >>>>>> + * is no cluster size information available. >>>>>> + */ >>>>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>>>>> +{ >>>>>> + BlockDriverInfo bdi; >>>>>> + uint64_t granularity; >>>>>> + >>>>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>>>>> + granularity = MAX(4096, bdi.cluster_size); >>>>>> + granularity = MIN(65536, granularity); >>>>>> + } else { >>>>>> + granularity = 65536; >>>>>> + } >>>>>> + >>>>>> + return granularity; >>>>>> +} >>>>>> + >>>>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>>>>> { >>>>>> diff --git a/block/mirror.c b/block/mirror.c >>>>>> index d819952..fc545f1 100644 >>>>>> --- a/block/mirror.c >>>>>> +++ b/block/mirror.c >>>>>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>>>>> } >>>>>> assert ((granularity & (granularity - 1)) == 0); >>>>>> diff --git a/blockdev.c b/blockdev.c >>>>>> index 5651a8e..95251c7 100644 >>>>>> --- a/blockdev.c >>>>>> +++ b/blockdev.c >>>>>> @@ -1173,6 +1173,48 @@ out_aio_context: >>>>>> return NULL; >>>>>> } >>>>>> +/** >>>>>> + * Return a dirty bitmap (if present), after validating >>>>>> + * the node reference and bitmap names. Returns NULL on error, >>>>>> + * including when the BDS and/or bitmap is not found. >>>>>> + */ >>>>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, >>>>>> + const char *name, >>>>>> + BlockDriverState >>>>>> **pbs, >>>>>> + Error **errp) >>>>>> +{ >>>>>> + BlockDriverState *bs; >>>>>> + BdrvDirtyBitmap *bitmap; >>>>>> + >>>>>> + if (!node_ref) { >>>>>> + error_setg(errp, "Node reference cannot be NULL"); >>>>>> + return NULL; >>>>>> + } >>>>>> + if (!name) { >>>>>> + error_setg(errp, "Bitmap name cannot be NULL"); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>>>>> + if (!bs) { >>>>>> + error_setg(errp, "Node reference '%s' not found", node_ref); >>>>> >>>>> No need to throw the (hopefully) perfectly fine Error code returned by >>>>> bdrv_lookup_bs() away. >>>>> >>>> >>>> I just wanted an error message consistent with the parameter name, in >>>> this case. i.e., We couldn't find the "Node reference" instead of >>>> "device" or "node name." Just trying to distinguish the fact that this >>>> is an arbitrary reference in the error message. >>>> >>>> I can still remove it, but I am curious to see what Markus thinks of >>>> the names I have chosen before I monkey with the errors too much more. >>> >>> bdrv_lookup_bs() is an awkward interface. >>> >>> If @device is non-null, try to look up a backend (BB) named @device. If >>> it exists, return the backend's root node (BDS). >>> >>> Else if @node_name is non-null, try to look up a node (BDS) named >>> @node_name. If it exists, return it. >>> >>> Else, set this error: >>> >>> error_setg(errp, "Cannot find device=%s nor node_name=%s", >>> device ? device : "", >>> node_name ? node_name : ""); >>> >>> The error message is crap unless both device and node_name are non-null >>> and different. Which is never the case: we always either pass two >>> identical non-null arguments, or we pass a null and a non-null >>> argument[*]. In other words, the error message is always crap. >>> >>> In case you wonder why @device takes precedence over node_name when both >>> are given: makes no sense. But when both are given, they are always >>> identical, and since backend and node names share a name space, only one >>> can resolve. >>> >>> A couple of cleaner solutions come to mind: >>> >>> * Make bdrv_lookup_bs() suck less >>> >>> Assert its tacit preconditions: >>> >>> assert(device || node_name); >>> assert(!device || !node_name || device == node_name); >>> >>> Then make it produce a decent error: >>> >>> if (device && node_name) { >>> error_setg(errp, "Neither block backend nor node %s found", device); >>> else if (device) { >>> error_setg(errp, "Block backend %s not found", device); >>> } else if (node_name) { >>> error_setg(errp, "Block node %s not found", node_name); >>> } >>> >>> Note how the three cases mirror the three usage patterns. >>> >>> Further note that the proposed error messages deviate from the >>> existing practice of calling block backends "devices". Calling >>> everything and its dog a "device" is traditional, but it's also lazy >>> and confusing. End of digression. >>> >>> * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its >>> callers >>> >>> Drop the Error ** parameter. Callers know whether a failed lookup was >>> for a device name, a node name or both, and can set an appropriate >>> error themselves. >>> >>> I'd still assert the preconditions. >>> >>> * Replace the function by one for each of its usage patterns >>> >>> I think that's what I'd do. >>> >>> [...] >>> >>> >>> [*] See >>> https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html >>> >> >> I can submit a patch for making bdrv_lookup_bs "nicer," or at least >> its usage more clear, in a separate patch. > > Yes, please. > >> If you really want me to fold it into this series, I'd invite you to >> review explicitly my usage of the parameter "node-ref" before I embark >> on cleaning up other interface areas. > > Follow-up patch is fine. Adding one more bad error message along the > way before you fix them all doesn't bother me. > >> Does this naming scheme look sane to you, and fit with your general >> expectations? >> >> I can also add a "bdrv_lookup_noderef" function that takes only one >> argument, which will help enforce the "If both arguments are provided, >> they must be the same" paradigm. >> >> This patch (#3) covers my shot at a unified parameter, and you can see >> further consequences in #7, and #10 (transactions). > > Do we want to introduce a @node-ref naming convention? > > Currently, QMP calls parameters or members naming nodes @node-name or > similar, and parameters/members naming backends @device or similar. The > one place where we already accept either is called @reference in the > schema, but it's a member of an anonymous union, so it's not really > visible in QMP. > > Previously[*], we agreed (I think) to replace and deprecate the four > commands that use the "pair of names" convention to identify a node. > Their replacement would use the "single name" convention. The name can > either be a node name or a backend name, and the latter automatically > resolves to its root node. > > The "backend name resolves to its root node" convenience feature should > be available consistently or not at all. I think the consensus it to > want it consistently. > > Therefore, your new @node-ref is really the same as the existing > @node-name, isn't it? bdrv_lookup_bs() as used in the patch, as that function exists today, will resolve backends to root nodes, so these QMP commands will operate with device/backend names or node-names. > Why a new naming convention @node-ref? Is it meant to be in addition to > @node-name, or is it meant to replace it? Is it the same? It was my understanding that we didn't have a QMP command currently that accepted /only/ nodes; from your previous mail characterizing the existing patterns: "3. Node name only No known example." So this patch is /intending/ to add a command wherein you can identify either a "node-name" or a "device," where the real goal is to obtain any arbitrary node -- so I used a new name. If we want a new unified parameter in the future, we should probably figure out what it is and start using it. I propose "node-ref." I *did* see that "reference" was already used for this purpose, but as you say, the semantics are somewhat different there, so I opted for a new name to not confuse the usages. Maybe this is what we want, maybe it isn't: A case could be made for either case. I'm making my case for node-ref: Short for Node Reference, it's different from "Node Name" in that it does not describe a single node's name, it's simply a reference to one. To me, this means that it could either be a node-name OR a backend-name, because a backend name could be considered a reference to the root node of that tree. So it seems generic-y enough to be a unified parameter. >> CC'ing Eric Blake, as well, for comments on a "unified parameter" >> interface in general. > > Good move. > > > [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html > Adding Eric back in, where'd he go? --js
John Snow <jsnow@redhat.com> writes: > On 01/20/2015 03:26 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 01/19/2015 05:08 AM, Markus Armbruster wrote: >>>> John Snow <jsnow@redhat.com> writes: >>>> >>>>> On 01/16/2015 10:36 AM, Max Reitz wrote: >>>>>> On 2015-01-12 at 11:30, 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. >>>>>>> >>>>>>> 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: Fam Zheng <famz@redhat.com> >>>>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>>>> --- >>>>>>> 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(-) >>>>>>> >>>>>>> diff --git a/block.c b/block.c >>>>>>> index bfeae6b..3eb77ee 100644 >>>>>>> --- a/block.c >>>>>>> +++ b/block.c >>>>>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>>>>>> BdrvDirtyBitmap *bitmap, int64_t sector >>>>>>> } >>>>>>> } >>>>>>> +/** >>>>>>> + * Chooses a default granularity based on the existing cluster size, >>>>>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >>>>>>> + * is no cluster size information available. >>>>>>> + */ >>>>>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>>>>>> +{ >>>>>>> + BlockDriverInfo bdi; >>>>>>> + uint64_t granularity; >>>>>>> + >>>>>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>>>>>> + granularity = MAX(4096, bdi.cluster_size); >>>>>>> + granularity = MIN(65536, granularity); >>>>>>> + } else { >>>>>>> + granularity = 65536; >>>>>>> + } >>>>>>> + >>>>>>> + return granularity; >>>>>>> +} >>>>>>> + >>>>>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>>>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>>>>>> { >>>>>>> diff --git a/block/mirror.c b/block/mirror.c >>>>>>> index d819952..fc545f1 100644 >>>>>>> --- a/block/mirror.c >>>>>>> +++ b/block/mirror.c >>>>>>> @@ -667,15 +667,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_get_default_bitmap_granularity(target); >>>>>>> } >>>>>>> assert ((granularity & (granularity - 1)) == 0); >>>>>>> diff --git a/blockdev.c b/blockdev.c >>>>>>> index 5651a8e..95251c7 100644 >>>>>>> --- a/blockdev.c >>>>>>> +++ b/blockdev.c >>>>>>> @@ -1173,6 +1173,48 @@ out_aio_context: >>>>>>> return NULL; >>>>>>> } >>>>>>> +/** >>>>>>> + * Return a dirty bitmap (if present), after validating >>>>>>> + * the node reference and bitmap names. Returns NULL on error, >>>>>>> + * including when the BDS and/or bitmap is not found. >>>>>>> + */ >>>>>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, >>>>>>> + const char *name, >>>>>>> + BlockDriverState >>>>>>> **pbs, >>>>>>> + Error **errp) >>>>>>> +{ >>>>>>> + BlockDriverState *bs; >>>>>>> + BdrvDirtyBitmap *bitmap; >>>>>>> + >>>>>>> + if (!node_ref) { >>>>>>> + error_setg(errp, "Node reference cannot be NULL"); >>>>>>> + return NULL; >>>>>>> + } >>>>>>> + if (!name) { >>>>>>> + error_setg(errp, "Bitmap name cannot be NULL"); >>>>>>> + return NULL; >>>>>>> + } >>>>>>> + >>>>>>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>>>>>> + if (!bs) { >>>>>>> + error_setg(errp, "Node reference '%s' not found", node_ref); >>>>>> >>>>>> No need to throw the (hopefully) perfectly fine Error code returned by >>>>>> bdrv_lookup_bs() away. >>>>>> >>>>> >>>>> I just wanted an error message consistent with the parameter name, in >>>>> this case. i.e., We couldn't find the "Node reference" instead of >>>>> "device" or "node name." Just trying to distinguish the fact that this >>>>> is an arbitrary reference in the error message. >>>>> >>>>> I can still remove it, but I am curious to see what Markus thinks of >>>>> the names I have chosen before I monkey with the errors too much more. >>>> >>>> bdrv_lookup_bs() is an awkward interface. >>>> >>>> If @device is non-null, try to look up a backend (BB) named @device. If >>>> it exists, return the backend's root node (BDS). >>>> >>>> Else if @node_name is non-null, try to look up a node (BDS) named >>>> @node_name. If it exists, return it. >>>> >>>> Else, set this error: >>>> >>>> error_setg(errp, "Cannot find device=%s nor node_name=%s", >>>> device ? device : "", >>>> node_name ? node_name : ""); >>>> >>>> The error message is crap unless both device and node_name are non-null >>>> and different. Which is never the case: we always either pass two >>>> identical non-null arguments, or we pass a null and a non-null >>>> argument[*]. In other words, the error message is always crap. >>>> >>>> In case you wonder why @device takes precedence over node_name when both >>>> are given: makes no sense. But when both are given, they are always >>>> identical, and since backend and node names share a name space, only one >>>> can resolve. >>>> >>>> A couple of cleaner solutions come to mind: >>>> >>>> * Make bdrv_lookup_bs() suck less >>>> >>>> Assert its tacit preconditions: >>>> >>>> assert(device || node_name); >>>> assert(!device || !node_name || device == node_name); >>>> >>>> Then make it produce a decent error: >>>> >>>> if (device && node_name) { >>>> error_setg(errp, "Neither block backend nor node %s found", device); >>>> else if (device) { >>>> error_setg(errp, "Block backend %s not found", device); >>>> } else if (node_name) { >>>> error_setg(errp, "Block node %s not found", node_name); >>>> } >>>> >>>> Note how the three cases mirror the three usage patterns. >>>> >>>> Further note that the proposed error messages deviate from the >>>> existing practice of calling block backends "devices". Calling >>>> everything and its dog a "device" is traditional, but it's also lazy >>>> and confusing. End of digression. >>>> >>>> * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its >>>> callers >>>> >>>> Drop the Error ** parameter. Callers know whether a failed lookup was >>>> for a device name, a node name or both, and can set an appropriate >>>> error themselves. >>>> >>>> I'd still assert the preconditions. >>>> >>>> * Replace the function by one for each of its usage patterns >>>> >>>> I think that's what I'd do. >>>> >>>> [...] >>>> >>>> >>>> [*] See >>>> https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html >>>> >>> >>> I can submit a patch for making bdrv_lookup_bs "nicer," or at least >>> its usage more clear, in a separate patch. >> >> Yes, please. >> >>> If you really want me to fold it into this series, I'd invite you to >>> review explicitly my usage of the parameter "node-ref" before I embark >>> on cleaning up other interface areas. >> >> Follow-up patch is fine. Adding one more bad error message along the >> way before you fix them all doesn't bother me. >> >>> Does this naming scheme look sane to you, and fit with your general >>> expectations? >>> >>> I can also add a "bdrv_lookup_noderef" function that takes only one >>> argument, which will help enforce the "If both arguments are provided, >>> they must be the same" paradigm. >>> >>> This patch (#3) covers my shot at a unified parameter, and you can see >>> further consequences in #7, and #10 (transactions). >> >> Do we want to introduce a @node-ref naming convention? >> >> Currently, QMP calls parameters or members naming nodes @node-name or >> similar, and parameters/members naming backends @device or similar. The >> one place where we already accept either is called @reference in the >> schema, but it's a member of an anonymous union, so it's not really >> visible in QMP. >> >> Previously[*], we agreed (I think) to replace and deprecate the four >> commands that use the "pair of names" convention to identify a node. >> Their replacement would use the "single name" convention. The name can >> either be a node name or a backend name, and the latter automatically >> resolves to its root node. >> >> The "backend name resolves to its root node" convenience feature should >> be available consistently or not at all. I think the consensus it to >> want it consistently. >> >> Therefore, your new @node-ref is really the same as the existing >> @node-name, isn't it? > > bdrv_lookup_bs() as used in the patch, as that function exists today, > will resolve backends to root nodes, so these QMP commands will > operate with device/backend names or node-names. > >> Why a new naming convention @node-ref? Is it meant to be in addition to >> @node-name, or is it meant to replace it? > > Is it the same? It was my understanding that we didn't have a QMP > command currently that accepted /only/ nodes; from your previous mail > characterizing the existing patterns: > > "3. Node name only > > No known example." > > So this patch is /intending/ to add a command wherein you can identify > either a "node-name" or a "device," where the real goal is to obtain > any arbitrary node -- so I used a new name. > > If we want a new unified parameter in the future, we should probably > figure out what it is and start using it. I propose "node-ref." > > I *did* see that "reference" was already used for this purpose, but as > you say, the semantics are somewhat different there, so I opted for a > new name to not confuse the usages. Maybe this is what we want, maybe > it isn't: A case could be made for either case. > > I'm making my case for node-ref: > > Short for Node Reference, it's different from "Node Name" in that it > does not describe a single node's name, it's simply a reference to > one. > To me, this means that it could either be a node-name OR a > backend-name, because a backend name could be considered a reference > to the root node of that tree. > > So it seems generic-y enough to be a unified parameter. I'm afraid I forgot much of the discussion we had before the break, and only now it's coming back, slowly. Quoting myself on naming parameters identifying nodes[*]: John Snow pointed out to me that we still haven't spelled out how this single parameter should be named. On obvious option is calling it node-name, or FOO-node-name when we have several. However, we'd then have two subtly different kinds of parameters called like that: the old ones accept *only* node names, the new ones also accept backend names, which automatically resolve to the backend's root node. Three ways to cope with that: * Find a better name. * Make the old ones accept backend names, too. Only a few, not that much work. However, there are exceptions: - blockdev-add's node-name *defines* the node name. - query-named-block-nodes's node-name *is* the node's name. * Stop worrying and embrace the inconsistency. The affected commands are headed for deprecation anyway. I think I'd go with "node" or "FOO-node" for parameters that reference nodes and accept both node names and backend names, and refrain from touching the existing node-name parameters. Let's go through existing uses of @node-name again: 1. Define a node name QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror 2. Report a node name QMP command query-named-block-nodes (type BlockDeviceInfo) 3. Node reference with backend names permitted for convenience New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and others 4. Node reference with backend names not permitted QMP commands drive-mirror @replaces, change-backing-file @image-node-name We may want to support the "backend name resolves to root node" convenience feature here, for consistency. Then this moves under 3. Note interface wart: change-backing-file additionally requires the backend owning the node. We need the backend to set op-blockers, we can't easily find it from the node, so we make the user point it out to us. 5. "Pair of names" node reference, specify exactly one QMP commands block_passwd, block_resize, blockdev-snapshot-sync We can ignore this one, because we intend to replace the commands and deprecate the old ones. If I understand you correctly, you're proposing to use @node-name or @FOO-node-name when the value must be a node name (items 1+2 and 4), and @node-ref or @FOO-node-ref where we additionally support the "backend name resolves to root node" convenience feature (item 3). Is that a fair description of your proposal? PRO: the name makes it clear when the convenience feature is supported. CON: if we eliminate 4 by supporting the convenience feature, we either create ugly exceptions to the naming convention, or rename the parameters. Opinions? >>> CC'ing Eric Blake, as well, for comments on a "unified parameter" >>> interface in general. >> >> Good move. >> >> >> [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html >> > > Adding Eric back in, where'd he go? Looks like you announced the cc:, but didn't actually do it, twice %-) Let me have a try. [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03151.html
On 01/21/2015 02:34 AM, Markus Armbruster wrote: > Opinions? > I'm still thinking about my reply (it's a big enough question that I want to make sure I consider ramifications), so this is just a meta-reply to let you know I'm tracking this conversation. >>>> CC'ing Eric Blake, as well, for comments on a "unified parameter" >>>> interface in general. >>> >>> Good move. >>> >>> >>> [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html >>> >> >> Adding Eric back in, where'd he go? > > Looks like you announced the cc:, but didn't actually do it, twice %-) I know that on some lists I'm subscribed to, mailman has an annoying configuration where if a list member is subscribed to the list and has requested that mailman not send dupe emails, then mailman rewrites CC: lines to drop that list member from the copy of the mail that other list readers receive. I hate the behavior, but it may be why you appear to not see me in cc, even though I've definitely been receiving the mails. At any rate, I have my mail filters set to flag mails that call out my name, even if I get left out from cc, so I can usually spot threads like this where you are trying to get my attention, regardless of whether you remembered to include me directly.
s/{'command'/{ 'command'/g
- to be consistent with other staff in qapi/block-core.json
and the same fix for block-dirty-bitmap-enable and
block-dirty-bitmap-disable
Best regards,
Vladimir
On 12.01.2015 19:30, 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.
>
> 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: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 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(-)
>
> diff --git a/block.c b/block.c
> index bfeae6b..3eb77ee 100644
> --- a/block.c
> +++ b/block.c
> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
> }
> }
>
> +/**
> + * Chooses a default granularity based on the existing cluster size,
> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there
> + * is no cluster size information available.
> + */
> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> +{
> + BlockDriverInfo bdi;
> + uint64_t granularity;
> +
> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
> + granularity = MAX(4096, bdi.cluster_size);
> + granularity = MIN(65536, granularity);
> + } else {
> + granularity = 65536;
> + }
> +
> + return granularity;
> +}
> +
> void bdrv_dirty_iter_init(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> {
> diff --git a/block/mirror.c b/block/mirror.c
> index d819952..fc545f1 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -667,15 +667,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_get_default_bitmap_granularity(target);
> }
>
> assert ((granularity & (granularity - 1)) == 0);
> diff --git a/blockdev.c b/blockdev.c
> index 5651a8e..95251c7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1173,6 +1173,48 @@ out_aio_context:
> return NULL;
> }
>
> +/**
> + * Return a dirty bitmap (if present), after validating
> + * the node reference and bitmap names. Returns NULL on error,
> + * including when the BDS and/or bitmap is not found.
> + */
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref,
> + const char *name,
> + BlockDriverState **pbs,
> + Error **errp)
> +{
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> +
> + if (!node_ref) {
> + error_setg(errp, "Node reference cannot be NULL");
> + return NULL;
> + }
> + if (!name) {
> + error_setg(errp, "Bitmap name cannot be NULL");
> + return NULL;
> + }
> +
> + bs = bdrv_lookup_bs(node_ref, node_ref, errp);
> + if (!bs) {
> + error_setg(errp, "Node reference '%s' not found", node_ref);
> + return NULL;
> + }
> +
> + /* If caller provided a BDS*, provide the result of that lookup, too. */
> + if (pbs) {
> + *pbs = bs;
> + }
> +
> + bitmap = bdrv_find_dirty_bitmap(bs, name);
> + if (!bitmap) {
> + error_setg(errp, "Dirty bitmap not found: %s", name);
> + return NULL;
> + }
> +
> + return bitmap;
> +}
> +
> /* New and old BlockDriverState structs for atomic group operations */
>
> typedef struct BlkTransactionState BlkTransactionState;
> @@ -1894,6 +1936,64 @@ 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 *node_ref, const char *name,
> + bool has_granularity, int64_t granularity,
> + Error **errp)
> +{
> + AioContext *aio_context;
> + BlockDriverState *bs;
> +
> + if (!name || name[0] == '\0') {
> + error_setg(errp, "Bitmap name cannot be empty");
> + return;
> + }
> +
> + bs = bdrv_lookup_bs(node_ref, node_ref, errp);
> + if (!bs) {
> + return;
> + }
> +
> + aio_context = bdrv_get_aio_context(bs);
> + aio_context_acquire(aio_context);
> +
> + 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);
> + }
> +
> + bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +
> + out:
> + aio_context_release(aio_context);
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name,
> + Error **errp)
> +{
> + AioContext *aio_context;
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> +
> + bitmap = block_dirty_bitmap_lookup(node_ref, name, &bs, errp);
> + if (!bitmap || !bs) {
> + return;
> + }
> +
> + aio_context = bdrv_get_aio_context(bs);
> + aio_context_acquire(aio_context);
> +
> + bdrv_dirty_bitmap_make_anon(bs, bitmap);
> + bdrv_release_dirty_bitmap(bs, bitmap);
> +
> + aio_context_release(aio_context);
> +}
> +
> 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 bf767af..44dd6ca 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -438,6 +438,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_get_default_bitmap_granularity(BlockDriverState *bs);
> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int nr_sectors);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d176324..f79d165 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -894,6 +894,61 @@
> '*on-target-error': 'BlockdevOnError' } }
>
> ##
> +# @BlockDirtyBitmap
> +#
> +# @node-ref: name of device/node-name which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> + 'data': { 'node-ref': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @node-ref: name of device/node-name 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': { 'node-ref': 'str', 'name': 'str', '*granularity': 'int' } }
> +
> +##
> +# @block-dirty-bitmap-add
> +#
> +# Create a dirty bitmap with a name on the node
> +#
> +# Returns: nothing on success
> +# If @node-ref 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 node
> +#
> +# Returns: nothing on success
> +# If @node-ref is not a valid block device, DeviceNotFound
> +# If @name is not found, GenericError with an explanation
> +#
> +# 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 6945d30..4ffca8a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1202,6 +1202,57 @@ Example:
> EQMP
>
> {
> + .name = "block-dirty-bitmap-add",
> + .args_type = "node-ref:B,name:s,granularity:i?",
> + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
> + },
> + {
> + .name = "block-dirty-bitmap-remove",
> + .args_type = "node-ref:B,name:s",
> + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
> + },
> +
> +SQMP
> +
> +block-dirty-bitmap-add
> +----------------------
> +Since 2.3
> +
> +Create a dirty bitmap with a name on the device, and start tracking the writes.
> +
> +Arguments:
> +
> +- "node-ref": device/node on which to create dirty bitmap (json-string)
> +- "name": name of the new dirty bitmap (json-string)
> +- "granularity": granularity to track writes with (int, optional)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "node-ref": "drive0",
> + "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +-------------------------
> +Since 2.3
> +
> +Stop write tracking and remove the dirty bitmap that was created with
> +block-dirty-bitmap-add.
> +
> +Arguments:
> +
> +- "node-ref": device/node on which to remove dirty bitmap (json-string)
> +- "name": name of the dirty bitmap to remove (json-string)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node-ref": "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,
Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben: > I'm afraid I forgot much of the discussion we had before the break, and > only now it's coming back, slowly. > > Quoting myself on naming parameters identifying nodes[*]: > > John Snow pointed out to me that we still haven't spelled out how this > single parameter should be named. > > On obvious option is calling it node-name, or FOO-node-name when we have > several. However, we'd then have two subtly different kinds of > parameters called like that: the old ones accept *only* node names, the > new ones also accept backend names, which automatically resolve to the > backend's root node. > > Three ways to cope with that: > > * Find a better name. > > * Make the old ones accept backend names, too. Only a few, not that > much work. However, there are exceptions: > > - blockdev-add's node-name *defines* the node name. > > - query-named-block-nodes's node-name *is* the node's name. > > * Stop worrying and embrace the inconsistency. The affected commands > are headed for deprecation anyway. > > I think I'd go with "node" or "FOO-node" for parameters that reference > nodes and accept both node names and backend names, and refrain from > touching the existing node-name parameters. Wasn't the conclusion last time that we would try to find a better name for new commands and leave old commands alone because they are going to become deprecated? That is, a combination of your first and last option? > Let's go through existing uses of @node-name again: > > 1. Define a node name > > QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror > > 2. Report a node name > > QMP command query-named-block-nodes (type BlockDeviceInfo) Whatever name we end up using, 1. and 2. should probably use the same. > 3. Node reference with backend names permitted for convenience > > New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and > others > > 4. Node reference with backend names not permitted > > QMP commands drive-mirror @replaces, change-backing-file > @image-node-name > > We may want to support the "backend name resolves to root node" > convenience feature here, for consistency. Then this moves under 3. > > Note interface wart: change-backing-file additionally requires the > backend owning the node. We need the backend to set op-blockers, we > can't easily find it from the node, so we make the user point it out > to us. These shouldn't be existing. As you say, we should move them to 3. > 5. "Pair of names" node reference, specify exactly one > > QMP commands block_passwd, block_resize, blockdev-snapshot-sync > > We can ignore this one, because we intend to replace the commands and > deprecate the old ones. Agreed, these shouldn't be existing either. > If I understand you correctly, you're proposing to use @node-name or > @FOO-node-name when the value must be a node name (items 1+2 and 4), and > @node-ref or @FOO-node-ref where we additionally support the "backend > name resolves to root node" convenience feature (item 3). > > Is that a fair description of your proposal? > > PRO: the name makes it clear when the convenience feature is supported. > > CON: if we eliminate 4 by supporting the convenience feature, we either > create ugly exceptions to the naming convention, or rename the > parameters. > > Opinions? If we don't have any cases where node names are allowed, but backend names are not, then there is no reason to have two different names. I've yet to see a reason for having commands that can accept node names, but not backend names. It's a bit different when the command can already accept both, but uses two separate arguments for it. But I think most of them will be deprecated, so we can ignore them here. As for the naming, I'm not that sure that it's even useful to add something to the field name. After all, this is really the _type_ of the object, not the name. We don't have fields like 'read-only-bool' either. If we're more specifically looking at things that actually refer to block devices, you already mentioned drive-mirrors @replaces, which is a great name in my opinion. @replaces-node-ref wouldn't improve anything. Likewise, blockdev-add already refers to 'file' and 'backing' instead of 'file-node' or 'backing-node-ref'. This probably means that FOO-node-{ref,name} shouldn't exist, because just FOO is as good or better. The question is a bit harder where there is only one node involved and we don't have a nice word to describe its role for the command. This is where we used to use 'device' in the past, when node-level addressing didn't exist yet. I think just 'node' would be fine there. Kevin
On 01/30/2015 09:32 AM, Kevin Wolf wrote: > Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben: >> I'm afraid I forgot much of the discussion we had before the break, and >> only now it's coming back, slowly. >> >> Quoting myself on naming parameters identifying nodes[*]: >> >> John Snow pointed out to me that we still haven't spelled out how this >> single parameter should be named. >> >> On obvious option is calling it node-name, or FOO-node-name when we have >> several. However, we'd then have two subtly different kinds of >> parameters called like that: the old ones accept *only* node names, the >> new ones also accept backend names, which automatically resolve to the >> backend's root node. >> >> Three ways to cope with that: >> >> * Find a better name. >> >> * Make the old ones accept backend names, too. Only a few, not that >> much work. However, there are exceptions: >> >> - blockdev-add's node-name *defines* the node name. >> >> - query-named-block-nodes's node-name *is* the node's name. >> >> * Stop worrying and embrace the inconsistency. The affected commands >> are headed for deprecation anyway. >> >> I think I'd go with "node" or "FOO-node" for parameters that reference >> nodes and accept both node names and backend names, and refrain from >> touching the existing node-name parameters. > > Wasn't the conclusion last time that we would try to find a better name > for new commands and leave old commands alone because they are going to > become deprecated? That is, a combination of your first and last option? > That was my impression, too: Use a new name for new commands and then slowly phase out the old things. This makes the new name clear as to what it supports (BOTH backends and nodes through a common namespace) to external management utilities like libvirt. That's why I just rolled 'node-ref.' >> Let's go through existing uses of @node-name again: >> >> 1. Define a node name >> >> QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror >> >> 2. Report a node name >> >> QMP command query-named-block-nodes (type BlockDeviceInfo) > > Whatever name we end up using, 1. and 2. should probably use the same. Should they? If these commands accept directly *node* names and have no chance of referencing a backend, maybe they should use different parameter names. > >> 3. Node reference with backend names permitted for convenience >> >> New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and >> others >> >> 4. Node reference with backend names not permitted >> >> QMP commands drive-mirror @replaces, change-backing-file >> @image-node-name >> >> We may want to support the "backend name resolves to root node" >> convenience feature here, for consistency. Then this moves under 3. >> >> Note interface wart: change-backing-file additionally requires the >> backend owning the node. We need the backend to set op-blockers, we >> can't easily find it from the node, so we make the user point it out >> to us. > > These shouldn't be existing. As you say, we should move them to 3. > Technically #3 here isn't a usage of "node-name," because... I didn't use node-name for these commands. Unless I am reading this list wrong, but it's definitely not an "existing use." I don't have any opinions about #4; presumably that's something we're aiming to phase out. >> 5. "Pair of names" node reference, specify exactly one >> >> QMP commands block_passwd, block_resize, blockdev-snapshot-sync >> >> We can ignore this one, because we intend to replace the commands and >> deprecate the old ones. > > Agreed, these shouldn't be existing either. > >> If I understand you correctly, you're proposing to use @node-name or >> @FOO-node-name when the value must be a node name (items 1+2 and 4), and >> @node-ref or @FOO-node-ref where we additionally support the "backend >> name resolves to root node" convenience feature (item 3). >> >> Is that a fair description of your proposal? >> >> PRO: the name makes it clear when the convenience feature is supported. >> >> CON: if we eliminate 4 by supporting the convenience feature, we either >> create ugly exceptions to the naming convention, or rename the >> parameters. >> >> Opinions? > > If we don't have any cases where node names are allowed, but backend > names are not, then there is no reason to have two different names. I've > yet to see a reason for having commands that can accept node names, but > not backend names. > > It's a bit different when the command can already accept both, but uses > two separate arguments for it. But I think most of them will be > deprecated, so we can ignore them here. > > As for the naming, I'm not that sure that it's even useful to add > something to the field name. After all, this is really the _type_ of the > object, not the name. We don't have fields like 'read-only-bool' either. > > If we're more specifically looking at things that actually refer to > block devices, you already mentioned drive-mirrors @replaces, which is a > great name in my opinion. @replaces-node-ref wouldn't improve anything. > Likewise, blockdev-add already refers to 'file' and 'backing' instead of > 'file-node' or 'backing-node-ref'. > > This probably means that FOO-node-{ref,name} shouldn't exist, because > just FOO is as good or better. The question is a bit harder where there > is only one node involved and we don't have a nice word to describe its > role for the command. This is where we used to use 'device' in the past, > when node-level addressing didn't exist yet. I think just 'node' would > be fine there. > > Kevin > I'd be happy with naming things "node" (or node-ref, either is fine) going forward; and leaving the old commands (node-name) alone. I seem to recall there was a reason we didn't want to just keep using node-name for the new unified parameters. It makes sense that if we don't keep a "this means node name ONLY" parameter for any command then there is no reason to make some distinction between that and "this parameter accepts both," but I think for purposes of libvirt, it is helpful to have a concrete distinction between versions that it can rely on. Could be mis-remembering, this whole discussion is spread out over months now.
Am 30.01.2015 um 18:04 hat John Snow geschrieben: > > > On 01/30/2015 09:32 AM, Kevin Wolf wrote: > >Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben: > >>I'm afraid I forgot much of the discussion we had before the break, and > >>only now it's coming back, slowly. > >> > >>Quoting myself on naming parameters identifying nodes[*]: > >> > >> John Snow pointed out to me that we still haven't spelled out how this > >> single parameter should be named. > >> > >> On obvious option is calling it node-name, or FOO-node-name when we have > >> several. However, we'd then have two subtly different kinds of > >> parameters called like that: the old ones accept *only* node names, the > >> new ones also accept backend names, which automatically resolve to the > >> backend's root node. > >> > >> Three ways to cope with that: > >> > >> * Find a better name. > >> > >> * Make the old ones accept backend names, too. Only a few, not that > >> much work. However, there are exceptions: > >> > >> - blockdev-add's node-name *defines* the node name. > >> > >> - query-named-block-nodes's node-name *is* the node's name. > >> > >> * Stop worrying and embrace the inconsistency. The affected commands > >> are headed for deprecation anyway. > >> > >> I think I'd go with "node" or "FOO-node" for parameters that reference > >> nodes and accept both node names and backend names, and refrain from > >> touching the existing node-name parameters. > > > >Wasn't the conclusion last time that we would try to find a better name > >for new commands and leave old commands alone because they are going to > >become deprecated? That is, a combination of your first and last option? > > > > That was my impression, too: Use a new name for new commands and > then slowly phase out the old things. This makes the new name clear > as to what it supports (BOTH backends and nodes through a common > namespace) to external management utilities like libvirt. > > That's why I just rolled 'node-ref.' Yes, I agree with that in principle. I called it 'node' below because that's shorter and doesn't include type information in the name, but if everyone preferred 'node-ref', I wouldn't have a problem with it either. > >>Let's go through existing uses of @node-name again: > >> > >>1. Define a node name > >> > >> QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror > >> > >>2. Report a node name > >> > >> QMP command query-named-block-nodes (type BlockDeviceInfo) > > > >Whatever name we end up using, 1. and 2. should probably use the same. > > Should they? If these commands accept directly *node* names and have > no chance of referencing a backend, maybe they should use different > parameter names. Note that these cases don't refer to a node, but they define/return the name of a node. No way that could ever be a backend name, unless we broke the code. > >>3. Node reference with backend names permitted for convenience > >> > >> New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and > >> others > >> > >>4. Node reference with backend names not permitted > >> > >> QMP commands drive-mirror @replaces, change-backing-file > >> @image-node-name > >> > >> We may want to support the "backend name resolves to root node" > >> convenience feature here, for consistency. Then this moves under 3. > >> > >> Note interface wart: change-backing-file additionally requires the > >> backend owning the node. We need the backend to set op-blockers, we > >> can't easily find it from the node, so we make the user point it out > >> to us. > > > >These shouldn't be existing. As you say, we should move them to 3. > > > > Technically #3 here isn't a usage of "node-name," because... I > didn't use node-name for these commands. Unless I am reading this > list wrong, but it's definitely not an "existing use." > > I don't have any opinions about #4; presumably that's something > we're aiming to phase out. Yes. Where "phasing out" simply means to extend it so that it accepts backend names. That should in theory be an easy change, except that @image-node-name has a name that isn't quite compatible with it... > >>5. "Pair of names" node reference, specify exactly one > >> > >> QMP commands block_passwd, block_resize, blockdev-snapshot-sync > >> > >> We can ignore this one, because we intend to replace the commands and > >> deprecate the old ones. > > > >Agreed, these shouldn't be existing either. > > > >>If I understand you correctly, you're proposing to use @node-name or > >>@FOO-node-name when the value must be a node name (items 1+2 and 4), and > >>@node-ref or @FOO-node-ref where we additionally support the "backend > >>name resolves to root node" convenience feature (item 3). > >> > >>Is that a fair description of your proposal? > >> > >>PRO: the name makes it clear when the convenience feature is supported. > >> > >>CON: if we eliminate 4 by supporting the convenience feature, we either > >>create ugly exceptions to the naming convention, or rename the > >>parameters. > >> > >>Opinions? > > > >If we don't have any cases where node names are allowed, but backend > >names are not, then there is no reason to have two different names. I've > >yet to see a reason for having commands that can accept node names, but > >not backend names. > > > >It's a bit different when the command can already accept both, but uses > >two separate arguments for it. But I think most of them will be > >deprecated, so we can ignore them here. > > > >As for the naming, I'm not that sure that it's even useful to add > >something to the field name. After all, this is really the _type_ of the > >object, not the name. We don't have fields like 'read-only-bool' either. > > > >If we're more specifically looking at things that actually refer to > >block devices, you already mentioned drive-mirrors @replaces, which is a > >great name in my opinion. @replaces-node-ref wouldn't improve anything. > >Likewise, blockdev-add already refers to 'file' and 'backing' instead of > >'file-node' or 'backing-node-ref'. > > > >This probably means that FOO-node-{ref,name} shouldn't exist, because > >just FOO is as good or better. The question is a bit harder where there > >is only one node involved and we don't have a nice word to describe its > >role for the command. This is where we used to use 'device' in the past, > >when node-level addressing didn't exist yet. I think just 'node' would > >be fine there. > > > >Kevin > > > > I'd be happy with naming things "node" (or node-ref, either is fine) > going forward; and leaving the old commands (node-name) alone. I > seem to recall there was a reason we didn't want to just keep using > node-name for the new unified parameters. It's probably a bad name when we accept backend names as well. And I'm not completely sure, but I think we considered removing the relative recent node-name again, which has to be probed by management tools anyway. We can't remove 'device', which has always been there, and keeping both as optional fields isn't really nice. > It makes sense that if we don't keep a "this means node name ONLY" > parameter for any command then there is no reason to make some > distinction between that and "this parameter accepts both," but I > think for purposes of libvirt, it is helpful to have a concrete > distinction between versions that it can rely on. Well, at least for new commands that doesn't matter. > Could be mis-remembering, this whole discussion is spread out over > months now. Yes, remembering all the details is hard... Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 30.01.2015 um 18:04 hat John Snow geschrieben: >> >> >> On 01/30/2015 09:32 AM, Kevin Wolf wrote: >> >Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben: >> >>I'm afraid I forgot much of the discussion we had before the break, and >> >>only now it's coming back, slowly. >> >> >> >>Quoting myself on naming parameters identifying nodes[*]: >> >> >> >> John Snow pointed out to me that we still haven't spelled out how this >> >> single parameter should be named. >> >> >> >> On obvious option is calling it node-name, or FOO-node-name when we have >> >> several. However, we'd then have two subtly different kinds of >> >> parameters called like that: the old ones accept *only* node names, the >> >> new ones also accept backend names, which automatically resolve to the >> >> backend's root node. >> >> >> >> Three ways to cope with that: >> >> >> >> * Find a better name. >> >> >> >> * Make the old ones accept backend names, too. Only a few, not that >> >> much work. However, there are exceptions: >> >> >> >> - blockdev-add's node-name *defines* the node name. >> >> >> >> - query-named-block-nodes's node-name *is* the node's name. >> >> >> >> * Stop worrying and embrace the inconsistency. The affected commands >> >> are headed for deprecation anyway. >> >> >> >> I think I'd go with "node" or "FOO-node" for parameters that reference >> >> nodes and accept both node names and backend names, and refrain from >> >> touching the existing node-name parameters. >> > >> >Wasn't the conclusion last time that we would try to find a better name >> >for new commands and leave old commands alone because they are going to >> >become deprecated? That is, a combination of your first and last option? >> > >> >> That was my impression, too: Use a new name for new commands and >> then slowly phase out the old things. This makes the new name clear >> as to what it supports (BOTH backends and nodes through a common >> namespace) to external management utilities like libvirt. >> >> That's why I just rolled 'node-ref.' > > Yes, I agree with that in principle. I called it 'node' below because > that's shorter and doesn't include type information in the name, but if > everyone preferred 'node-ref', I wouldn't have a problem with it either. >> >>Let's go through existing uses of @node-name again: >> >> >> >>1. Define a node name >> >> >> >> QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror >> >> >> >>2. Report a node name >> >> >> >> QMP command query-named-block-nodes (type BlockDeviceInfo) >> > >> >Whatever name we end up using, 1. and 2. should probably use the same. >> >> Should they? If these commands accept directly *node* names and have >> no chance of referencing a backend, maybe they should use different >> parameter names. Maybe. But even if we use different names for things that can only be node names, never backend names, 1. and 2. should use the same name, because they're both things that can only be node names. That's what Kevin said. > Note that these cases don't refer to a node, but they define/return the > name of a node. No way that could ever be a backend name, unless we > broke the code. > >> >>3. Node reference with backend names permitted for convenience >> >> >> >> New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and >> >> others >> >> >> >>4. Node reference with backend names not permitted >> >> >> >> QMP commands drive-mirror @replaces, change-backing-file >> >> @image-node-name >> >> >> >> We may want to support the "backend name resolves to root node" >> >> convenience feature here, for consistency. Then this moves under 3. >> >> >> >> Note interface wart: change-backing-file additionally requires the >> >> backend owning the node. We need the backend to set op-blockers, we >> >> can't easily find it from the node, so we make the user point it out >> >> to us. >> > >> >These shouldn't be existing. As you say, we should move them to 3. >> > >> >> Technically #3 here isn't a usage of "node-name," because... I >> didn't use node-name for these commands. Unless I am reading this >> list wrong, but it's definitely not an "existing use." >> >> I don't have any opinions about #4; presumably that's something >> we're aiming to phase out. > > Yes. Where "phasing out" simply means to extend it so that it accepts > backend names. That should in theory be an easy change, except that > @image-node-name has a name that isn't quite compatible with it... Our choice for 3. affects the phasing out of 4. Our choice for 3 is a naming convention for parameters referencing nodes that accept both node and backend names. If, after extending the code to accept backend names, the old names for 4. follow the naming convention for 3., we're done. Else, we still have an interface wart. We can live with it, or we can rename the parameter to follow the convention. >> >>5. "Pair of names" node reference, specify exactly one >> >> >> >> QMP commands block_passwd, block_resize, blockdev-snapshot-sync >> >> >> >> We can ignore this one, because we intend to replace the commands and >> >> deprecate the old ones. >> > >> >Agreed, these shouldn't be existing either. >> > >> >>If I understand you correctly, you're proposing to use @node-name or >> >>@FOO-node-name when the value must be a node name (items 1+2 and 4), and >> >>@node-ref or @FOO-node-ref where we additionally support the "backend >> >>name resolves to root node" convenience feature (item 3). >> >> >> >>Is that a fair description of your proposal? >> >> >> >>PRO: the name makes it clear when the convenience feature is supported. >> >> >> >>CON: if we eliminate 4 by supporting the convenience feature, we either >> >>create ugly exceptions to the naming convention, or rename the >> >>parameters. >> >> >> >>Opinions? >> > >> >If we don't have any cases where node names are allowed, but backend >> >names are not, then there is no reason to have two different names. I've >> >yet to see a reason for having commands that can accept node names, but >> >not backend names. Cases 1. and 2. But I'm not sure using different names there to emphasize "backend names not possible here" would be useful. >> >It's a bit different when the command can already accept both, but uses >> >two separate arguments for it. But I think most of them will be >> >deprecated, so we can ignore them here. Yes, case 5. >> >As for the naming, I'm not that sure that it's even useful to add >> >something to the field name. After all, this is really the _type_ of the >> >object, not the name. We don't have fields like 'read-only-bool' either. Yes, but there the type is actually bool, which makes the bool-ness perfectly obvious. Here, the type is string. That's why I feel a FOO-node convention could be useful. >> >If we're more specifically looking at things that actually refer to >> >block devices, you already mentioned drive-mirrors @replaces, which is a >> >great name in my opinion. @replaces-node-ref wouldn't improve anything. >> >Likewise, blockdev-add already refers to 'file' and 'backing' instead of >> >'file-node' or 'backing-node-ref'. >> > >> >This probably means that FOO-node-{ref,name} shouldn't exist, because >> >just FOO is as good or better. The question is a bit harder where there >> >is only one node involved and we don't have a nice word to describe its >> >role for the command. This is where we used to use 'device' in the past, >> >when node-level addressing didn't exist yet. I think just 'node' would >> >be fine there. I'm fine with just 'node'. I like it better than 'node-ref' or 'node-name'. What sets the 'node-name' convention apart is that the existing (FOO-)node-name already fit this convention. A quick grep finds nine occurences, in block-core.json and event.json. Advantage if we want to keep them, disadvantage if we want to get rid of them. >> > >> >Kevin >> > >> >> I'd be happy with naming things "node" (or node-ref, either is fine) >> going forward; and leaving the old commands (node-name) alone. I >> seem to recall there was a reason we didn't want to just keep using >> node-name for the new unified parameters. > > It's probably a bad name when we accept backend names as well. And I'm > not completely sure, but I think we considered removing the relative > recent node-name again, which has to be probed by management tools > anyway. We can't remove 'device', which has always been there, and > keeping both as optional fields isn't really nice. Sounds like we want to get rid of them. >> It makes sense that if we don't keep a "this means node name ONLY" >> parameter for any command Cases 1. and 2. >> then there is no reason to make some >> distinction between that and "this parameter accepts both," but I >> think for purposes of libvirt, it is helpful to have a concrete >> distinction between versions that it can rely on. > > Well, at least for new commands that doesn't matter. > >> Could be mis-remembering, this whole discussion is spread out over >> months now. > > Yes, remembering all the details is hard... Quite a hairball :)
On 02/02/2015 05:10 AM, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 30.01.2015 um 18:04 hat John Snow geschrieben: >>> >>> >>> On 01/30/2015 09:32 AM, Kevin Wolf wrote: >>>> Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben: >>>>> I'm afraid I forgot much of the discussion we had before the break, and >>>>> only now it's coming back, slowly. >>>>> >>>>> Quoting myself on naming parameters identifying nodes[*]: >>>>> >>>>> John Snow pointed out to me that we still haven't spelled out how this >>>>> single parameter should be named. >>>>> >>>>> On obvious option is calling it node-name, or FOO-node-name when we have >>>>> several. However, we'd then have two subtly different kinds of >>>>> parameters called like that: the old ones accept *only* node names, the >>>>> new ones also accept backend names, which automatically resolve to the >>>>> backend's root node. >>>>> >>>>> Three ways to cope with that: >>>>> >>>>> * Find a better name. >>>>> >>>>> * Make the old ones accept backend names, too. Only a few, not that >>>>> much work. However, there are exceptions: >>>>> >>>>> - blockdev-add's node-name *defines* the node name. >>>>> >>>>> - query-named-block-nodes's node-name *is* the node's name. >>>>> >>>>> * Stop worrying and embrace the inconsistency. The affected commands >>>>> are headed for deprecation anyway. >>>>> >>>>> I think I'd go with "node" or "FOO-node" for parameters that reference >>>>> nodes and accept both node names and backend names, and refrain from >>>>> touching the existing node-name parameters. >>>> >>>> Wasn't the conclusion last time that we would try to find a better name >>>> for new commands and leave old commands alone because they are going to >>>> become deprecated? That is, a combination of your first and last option? >>>> >>> >>> That was my impression, too: Use a new name for new commands and >>> then slowly phase out the old things. This makes the new name clear >>> as to what it supports (BOTH backends and nodes through a common >>> namespace) to external management utilities like libvirt. >>> >>> That's why I just rolled 'node-ref.' >> >> Yes, I agree with that in principle. I called it 'node' below because >> that's shorter and doesn't include type information in the name, but if >> everyone preferred 'node-ref', I wouldn't have a problem with it either. > > > >>>>> Let's go through existing uses of @node-name again: >>>>> >>>>> 1. Define a node name >>>>> >>>>> QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror >>>>> >>>>> 2. Report a node name >>>>> >>>>> QMP command query-named-block-nodes (type BlockDeviceInfo) >>>> >>>> Whatever name we end up using, 1. and 2. should probably use the same. >>> >>> Should they? If these commands accept directly *node* names and have >>> no chance of referencing a backend, maybe they should use different >>> parameter names. > > Maybe. But even if we use different names for things that can only be > node names, never backend names, 1. and 2. should use the same name, > because they're both things that can only be node names. That's what > Kevin said. > >> Note that these cases don't refer to a node, but they define/return the >> name of a node. No way that could ever be a backend name, unless we >> broke the code. >> >>>>> 3. Node reference with backend names permitted for convenience >>>>> >>>>> New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and >>>>> others >>>>> >>>>> 4. Node reference with backend names not permitted >>>>> >>>>> QMP commands drive-mirror @replaces, change-backing-file >>>>> @image-node-name >>>>> >>>>> We may want to support the "backend name resolves to root node" >>>>> convenience feature here, for consistency. Then this moves under 3. >>>>> >>>>> Note interface wart: change-backing-file additionally requires the >>>>> backend owning the node. We need the backend to set op-blockers, we >>>>> can't easily find it from the node, so we make the user point it out >>>>> to us. >>>> >>>> These shouldn't be existing. As you say, we should move them to 3. >>>> >>> >>> Technically #3 here isn't a usage of "node-name," because... I >>> didn't use node-name for these commands. Unless I am reading this >>> list wrong, but it's definitely not an "existing use." >>> >>> I don't have any opinions about #4; presumably that's something >>> we're aiming to phase out. >> >> Yes. Where "phasing out" simply means to extend it so that it accepts >> backend names. That should in theory be an easy change, except that >> @image-node-name has a name that isn't quite compatible with it... > > Our choice for 3. affects the phasing out of 4. > > Our choice for 3 is a naming convention for parameters referencing nodes > that accept both node and backend names. > > If, after extending the code to accept backend names, the old names for > 4. follow the naming convention for 3., we're done. > > Else, we still have an interface wart. We can live with it, or we can > rename the parameter to follow the convention. > >>>>> 5. "Pair of names" node reference, specify exactly one >>>>> >>>>> QMP commands block_passwd, block_resize, blockdev-snapshot-sync >>>>> >>>>> We can ignore this one, because we intend to replace the commands and >>>>> deprecate the old ones. >>>> >>>> Agreed, these shouldn't be existing either. >>>> >>>>> If I understand you correctly, you're proposing to use @node-name or >>>>> @FOO-node-name when the value must be a node name (items 1+2 and 4), and >>>>> @node-ref or @FOO-node-ref where we additionally support the "backend >>>>> name resolves to root node" convenience feature (item 3). >>>>> >>>>> Is that a fair description of your proposal? >>>>> >>>>> PRO: the name makes it clear when the convenience feature is supported. >>>>> >>>>> CON: if we eliminate 4 by supporting the convenience feature, we either >>>>> create ugly exceptions to the naming convention, or rename the >>>>> parameters. >>>>> >>>>> Opinions? >>>> >>>> If we don't have any cases where node names are allowed, but backend >>>> names are not, then there is no reason to have two different names. I've >>>> yet to see a reason for having commands that can accept node names, but >>>> not backend names. > > Cases 1. and 2. But I'm not sure using different names there to > emphasize "backend names not possible here" would be useful. > >>>> It's a bit different when the command can already accept both, but uses >>>> two separate arguments for it. But I think most of them will be >>>> deprecated, so we can ignore them here. > > Yes, case 5. > >>>> As for the naming, I'm not that sure that it's even useful to add >>>> something to the field name. After all, this is really the _type_ of the >>>> object, not the name. We don't have fields like 'read-only-bool' either. > > Yes, but there the type is actually bool, which makes the bool-ness > perfectly obvious. > > Here, the type is string. That's why I feel a FOO-node convention could > be useful. > >>>> If we're more specifically looking at things that actually refer to >>>> block devices, you already mentioned drive-mirrors @replaces, which is a >>>> great name in my opinion. @replaces-node-ref wouldn't improve anything. >>>> Likewise, blockdev-add already refers to 'file' and 'backing' instead of >>>> 'file-node' or 'backing-node-ref'. >>>> >>>> This probably means that FOO-node-{ref,name} shouldn't exist, because >>>> just FOO is as good or better. The question is a bit harder where there >>>> is only one node involved and we don't have a nice word to describe its >>>> role for the command. This is where we used to use 'device' in the past, >>>> when node-level addressing didn't exist yet. I think just 'node' would >>>> be fine there. > > I'm fine with just 'node'. I like it better than 'node-ref' or > 'node-name'. > > What sets the 'node-name' convention apart is that the existing > (FOO-)node-name already fit this convention. A quick grep finds nine > occurences, in block-core.json and event.json. Advantage if we want to > keep them, disadvantage if we want to get rid of them. > >>>> >>>> Kevin >>>> >>> >>> I'd be happy with naming things "node" (or node-ref, either is fine) >>> going forward; and leaving the old commands (node-name) alone. I >>> seem to recall there was a reason we didn't want to just keep using >>> node-name for the new unified parameters. >> >> It's probably a bad name when we accept backend names as well. And I'm >> not completely sure, but I think we considered removing the relative >> recent node-name again, which has to be probed by management tools >> anyway. We can't remove 'device', which has always been there, and >> keeping both as optional fields isn't really nice. > > Sounds like we want to get rid of them. > >>> It makes sense that if we don't keep a "this means node name ONLY" >>> parameter for any command > > Cases 1. and 2. > >>> then there is no reason to make some >>> distinction between that and "this parameter accepts both," but I >>> think for purposes of libvirt, it is helpful to have a concrete >>> distinction between versions that it can rely on. >> >> Well, at least for new commands that doesn't matter. >> >>> Could be mis-remembering, this whole discussion is spread out over >>> months now. >> >> Yes, remembering all the details is hard... > > Quite a hairball :) > I can roll v12 tomorrow using parameters named "node" if that sounds agreeable to everyone, fixing up the other issues noted by other reviewers in the process. Sound good? --js
diff --git a/block.c b/block.c index bfeae6b..3eb77ee 100644 --- a/block.c +++ b/block.c @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector } } +/** + * Chooses a default granularity based on the existing cluster size, + * but clamped between [4K, 64K]. Defaults to 64K in the case that there + * is no cluster size information available. + */ +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) +{ + BlockDriverInfo bdi; + uint64_t granularity; + + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { + granularity = MAX(4096, bdi.cluster_size); + granularity = MIN(65536, granularity); + } else { + granularity = 65536; + } + + return granularity; +} + void bdrv_dirty_iter_init(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) { diff --git a/block/mirror.c b/block/mirror.c index d819952..fc545f1 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -667,15 +667,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_get_default_bitmap_granularity(target); } assert ((granularity & (granularity - 1)) == 0); diff --git a/blockdev.c b/blockdev.c index 5651a8e..95251c7 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1173,6 +1173,48 @@ out_aio_context: return NULL; } +/** + * Return a dirty bitmap (if present), after validating + * the node reference and bitmap names. Returns NULL on error, + * including when the BDS and/or bitmap is not found. + */ +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref, + const char *name, + BlockDriverState **pbs, + Error **errp) +{ + BlockDriverState *bs; + BdrvDirtyBitmap *bitmap; + + if (!node_ref) { + error_setg(errp, "Node reference cannot be NULL"); + return NULL; + } + if (!name) { + error_setg(errp, "Bitmap name cannot be NULL"); + return NULL; + } + + bs = bdrv_lookup_bs(node_ref, node_ref, errp); + if (!bs) { + error_setg(errp, "Node reference '%s' not found", node_ref); + return NULL; + } + + /* If caller provided a BDS*, provide the result of that lookup, too. */ + if (pbs) { + *pbs = bs; + } + + bitmap = bdrv_find_dirty_bitmap(bs, name); + if (!bitmap) { + error_setg(errp, "Dirty bitmap not found: %s", name); + return NULL; + } + + return bitmap; +} + /* New and old BlockDriverState structs for atomic group operations */ typedef struct BlkTransactionState BlkTransactionState; @@ -1894,6 +1936,64 @@ 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 *node_ref, const char *name, + bool has_granularity, int64_t granularity, + Error **errp) +{ + AioContext *aio_context; + BlockDriverState *bs; + + if (!name || name[0] == '\0') { + error_setg(errp, "Bitmap name cannot be empty"); + return; + } + + bs = bdrv_lookup_bs(node_ref, node_ref, errp); + if (!bs) { + return; + } + + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + 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); + } + + bdrv_create_dirty_bitmap(bs, granularity, name, errp); + + out: + aio_context_release(aio_context); +} + +void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name, + Error **errp) +{ + AioContext *aio_context; + BlockDriverState *bs; + BdrvDirtyBitmap *bitmap; + + bitmap = block_dirty_bitmap_lookup(node_ref, name, &bs, errp); + if (!bitmap || !bs) { + return; + } + + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + bdrv_dirty_bitmap_make_anon(bs, bitmap); + bdrv_release_dirty_bitmap(bs, bitmap); + + aio_context_release(aio_context); +} + 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 bf767af..44dd6ca 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -438,6 +438,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_get_default_bitmap_granularity(BlockDriverState *bs); int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t cur_sector, int nr_sectors); diff --git a/qapi/block-core.json b/qapi/block-core.json index d176324..f79d165 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -894,6 +894,61 @@ '*on-target-error': 'BlockdevOnError' } } ## +# @BlockDirtyBitmap +# +# @node-ref: name of device/node-name which the bitmap is tracking +# +# @name: name of the dirty bitmap +# +# Since 2.3 +## +{ 'type': 'BlockDirtyBitmap', + 'data': { 'node-ref': 'str', 'name': 'str' } } + +## +# @BlockDirtyBitmapAdd +# +# @node-ref: name of device/node-name 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': { 'node-ref': 'str', 'name': 'str', '*granularity': 'int' } } + +## +# @block-dirty-bitmap-add +# +# Create a dirty bitmap with a name on the node +# +# Returns: nothing on success +# If @node-ref 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 node +# +# Returns: nothing on success +# If @node-ref is not a valid block device, DeviceNotFound +# If @name is not found, GenericError with an explanation +# +# 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 6945d30..4ffca8a 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1202,6 +1202,57 @@ Example: EQMP { + .name = "block-dirty-bitmap-add", + .args_type = "node-ref:B,name:s,granularity:i?", + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add, + }, + { + .name = "block-dirty-bitmap-remove", + .args_type = "node-ref:B,name:s", + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove, + }, + +SQMP + +block-dirty-bitmap-add +---------------------- +Since 2.3 + +Create a dirty bitmap with a name on the device, and start tracking the writes. + +Arguments: + +- "node-ref": device/node on which to create dirty bitmap (json-string) +- "name": name of the new dirty bitmap (json-string) +- "granularity": granularity to track writes with (int, optional) + +Example: + +-> { "execute": "block-dirty-bitmap-add", "arguments": { "node-ref": "drive0", + "name": "bitmap0" } } +<- { "return": {} } + +block-dirty-bitmap-remove +------------------------- +Since 2.3 + +Stop write tracking and remove the dirty bitmap that was created with +block-dirty-bitmap-add. + +Arguments: + +- "node-ref": device/node on which to remove dirty bitmap (json-string) +- "name": name of the dirty bitmap to remove (json-string) + +Example: + +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node-ref": "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,