| Message ID | 1384338584-14065-3-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 13.11.2013 um 11:29 hat Fam Zheng geschrieben: > We have multiple dirty bitmaps in BDS now, switch QAPI to allow query > it (BlockInfo.dirty_bitmaps), and also drop old BlockInfo.dirty. > > Signed-off-by: Fam Zheng <famz@redhat.com> > diff --git a/qapi-schema.json b/qapi-schema.json > index 81a375b..931d710 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -948,8 +948,8 @@ > # @tray_open: #optional True if the device has a tray and it is open > # (only present if removable is true) > # > -# @dirty: #optional dirty bitmap information (only present if the dirty > -# bitmap is enabled) > +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the > +# driver has one or more dirty bitmaps) > # > # @io-status: #optional @BlockDeviceIoStatus. Only present if the device > # supports it and the VM is configured to stop on errors > @@ -963,7 +963,7 @@ > 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', > 'locked': 'bool', '*inserted': 'BlockDeviceInfo', > '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', > - '*dirty': 'BlockDirtyInfo' } } > + '*dirty-bitmaps': ['BlockDirtyInfo'] } } > > ## > # @query-block: I believe this is of limited use; if you ever have more than one dirty bitmap, we're lacking information to associate it with the job it belongs to. One option would be to extend BlockDirtyInfo to indicate this, but another might be to actually extend other commands like query-block-jobs to return information on the dirty bitmap associated with a specific job. I've applied it to block-next anyway, we still have some time to reconsider for 1.8. Kevin
Il 13/11/2013 15:19, Kevin Wolf ha scritto: > I believe this is of limited use; if you ever have more than one dirty > bitmap, we're lacking information to associate it with the job it > belongs to. One option would be to extend BlockDirtyInfo to indicate > this, but another might be to actually extend other commands like > query-block-jobs to return information on the dirty bitmap associated > with a specific job. I agree. Both query-block-jobs and query-migrate could be extended. Paolo
On Wed, Nov 13, 2013 at 10:19 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 13.11.2013 um 11:29 hat Fam Zheng geschrieben: >> We have multiple dirty bitmaps in BDS now, switch QAPI to allow query >> it (BlockInfo.dirty_bitmaps), and also drop old BlockInfo.dirty. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> > >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 81a375b..931d710 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -948,8 +948,8 @@ >> # @tray_open: #optional True if the device has a tray and it is open >> # (only present if removable is true) >> # >> -# @dirty: #optional dirty bitmap information (only present if the dirty >> -# bitmap is enabled) >> +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the >> +# driver has one or more dirty bitmaps) >> # >> # @io-status: #optional @BlockDeviceIoStatus. Only present if the device >> # supports it and the VM is configured to stop on errors >> @@ -963,7 +963,7 @@ >> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', >> 'locked': 'bool', '*inserted': 'BlockDeviceInfo', >> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', >> - '*dirty': 'BlockDirtyInfo' } } >> + '*dirty-bitmaps': ['BlockDirtyInfo'] } } >> >> ## >> # @query-block: > > I believe this is of limited use; if you ever have more than one dirty > bitmap, we're lacking information to associate it with the job it > belongs to. One option would be to extend BlockDirtyInfo to indicate > this, but another might be to actually extend other commands like > query-block-jobs to return information on the dirty bitmap associated > with a specific job. > > I've applied it to block-next anyway, we still have some time to > reconsider for 1.8. > Another case for this may be user enabled external dirty bitmap, which could be standalone from any block job. E.g. when we introduce a QMP command like: block-new-dirty-bitmap device=foo file=bar.bitmap This could be some code in block.c, could be a block job (really necessary?), or a block filter. I'm not sure... Fam
On 11/13/2013 07:19 AM, Kevin Wolf wrote: >> # >> -# @dirty: #optional dirty bitmap information (only present if the dirty >> -# bitmap is enabled) >> +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the >> +# driver has one or more dirty bitmaps) >> # >> # @io-status: #optional @BlockDeviceIoStatus. Only present if the device >> # supports it and the VM is configured to stop on errors >> @@ -963,7 +963,7 @@ >> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', >> 'locked': 'bool', '*inserted': 'BlockDeviceInfo', >> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', >> - '*dirty': 'BlockDirtyInfo' } } >> + '*dirty-bitmaps': ['BlockDirtyInfo'] } } >> >> ## >> # @query-block: > > I believe this is of limited use; if you ever have more than one dirty > bitmap, we're lacking information to associate it with the job it > belongs to. One option would be to extend BlockDirtyInfo to indicate > this, but another might be to actually extend other commands like > query-block-jobs to return information on the dirty bitmap associated > with a specific job. > > I've applied it to block-next anyway, we still have some time to > reconsider for 1.8. Indeed, expanding the per-job query output to list a single dirty bitmap tied to that job is probably more useful than listing all dirty bitmaps without context here. Since it's output-only, and marked optional, we can still withdraw this output even after 1.8 if we decide we don't like it and no one has reported wanting to use it. I was going to suggest _always_ outputting dirty-bitmaps, even if it is an empty array, if that makes it easier to detect that yes, this is a version of qemu new enough to have per-job dirty bitmaps but there are no jobs at the moment; on the other hand, doing that would mean the field is not marked optional, and then we would always have to output it for back-compat reasons. So keeping the field marked optional makes sense.
On 11/13/2013 03:29 AM, Fam Zheng wrote: > We have multiple dirty bitmaps in BDS now, switch QAPI to allow query > it (BlockInfo.dirty_bitmaps), and also drop old BlockInfo.dirty. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 20 ++++++++++++++++++++ > block/qapi.c | 5 +++++ > include/block/block.h | 1 + > qapi-schema.json | 6 +++--- > 4 files changed, 29 insertions(+), 3 deletions(-) > > +++ b/qapi-schema.json > @@ -948,8 +948,8 @@ > # @tray_open: #optional True if the device has a tray and it is open > # (only present if removable is true) > # > -# @dirty: #optional dirty bitmap information (only present if the dirty > -# bitmap is enabled) > +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the > +# driver has one or more dirty bitmaps) Worth adding a '(since 1.8)' designator (may have to be in a followup patch, since Kevin already put it in his staging tree) Also, we have an odd mix of tray_open and dirty-bitmaps (but that mix was already there with io-status); more reason that we should eventually add a patch for treating - and _ as synonyms in QMP keys.
> --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -948,8 +948,8 @@ > # @tray_open: #optional True if the device has a tray and it is open > # (only present if removable is true) > # > -# @dirty: #optional dirty bitmap information (only present if the dirty > -# bitmap is enabled) > +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the > +# driver has one or more dirty bitmaps) > # > # @io-status: #optional @BlockDeviceIoStatus. Only present if the device > # supports it and the VM is configured to stop on errors > @@ -963,7 +963,7 @@ > 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', > 'locked': 'bool', '*inserted': 'BlockDeviceInfo', > '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', > - '*dirty': 'BlockDirtyInfo' } } > + '*dirty-bitmaps': ['BlockDirtyInfo'] } } > > ## > # @query-block: > An old member is removed, is there a compatiable issue for user?
On 2013年11月14日 09:31, Wenchao Xia wrote: > >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -948,8 +948,8 @@ >> # @tray_open: #optional True if the device has a tray and it is open >> # (only present if removable is true) >> # >> -# @dirty: #optional dirty bitmap information (only present if the dirty >> -# bitmap is enabled) >> +# @dirty-bitmaps: #optional dirty bitmaps information (only present >> if the >> +# driver has one or more dirty bitmaps) >> # >> # @io-status: #optional @BlockDeviceIoStatus. Only present if the >> device >> # supports it and the VM is configured to stop on errors >> @@ -963,7 +963,7 @@ >> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', >> 'locked': 'bool', '*inserted': 'BlockDeviceInfo', >> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', >> - '*dirty': 'BlockDirtyInfo' } } >> + '*dirty-bitmaps': ['BlockDirtyInfo'] } } >> >> ## >> # @query-block: >> > An old member is removed, is there a compatiable issue for user? > We discussed this in v2 thread and the conclusion is no: http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg01446.html Fam
On 11/13/2013 06:31 PM, Wenchao Xia wrote: > >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -948,8 +948,8 @@ >> # @tray_open: #optional True if the device has a tray and it is open >> # (only present if removable is true) >> # >> -# @dirty: #optional dirty bitmap information (only present if the dirty >> -# bitmap is enabled) >> +# @dirty-bitmaps: #optional dirty bitmaps information (only present >> if the >> +# driver has one or more dirty bitmaps) >> # >> # @io-status: #optional @BlockDeviceIoStatus. Only present if the >> device >> # supports it and the VM is configured to stop on errors >> @@ -963,7 +963,7 @@ >> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', >> 'locked': 'bool', '*inserted': 'BlockDeviceInfo', >> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', >> - '*dirty': 'BlockDirtyInfo' } } >> + '*dirty-bitmaps': ['BlockDirtyInfo'] } } >> >> ## >> # @query-block: >> > An old member is removed, is there a compatiable issue for user? We already answered this; there is no back-compat issue with removing a variable that is already marked optional and which is used in an output-only type: https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg01446.html
于 2013/11/13 22:40, Fam Zheng 写道: > On Wed, Nov 13, 2013 at 10:19 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 13.11.2013 um 11:29 hat Fam Zheng geschrieben: >>> We have multiple dirty bitmaps in BDS now, switch QAPI to allow query >>> it (BlockInfo.dirty_bitmaps), and also drop old BlockInfo.dirty. >>> >>> Signed-off-by: Fam Zheng <famz@redhat.com> >> >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index 81a375b..931d710 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -948,8 +948,8 @@ >>> # @tray_open: #optional True if the device has a tray and it is open >>> # (only present if removable is true) >>> # >>> -# @dirty: #optional dirty bitmap information (only present if the dirty >>> -# bitmap is enabled) >>> +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the >>> +# driver has one or more dirty bitmaps) >>> # >>> # @io-status: #optional @BlockDeviceIoStatus. Only present if the device >>> # supports it and the VM is configured to stop on errors >>> @@ -963,7 +963,7 @@ >>> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', >>> 'locked': 'bool', '*inserted': 'BlockDeviceInfo', >>> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', >>> - '*dirty': 'BlockDirtyInfo' } } >>> + '*dirty-bitmaps': ['BlockDirtyInfo'] } } >>> >>> ## >>> # @query-block: >> >> I believe this is of limited use; if you ever have more than one dirty >> bitmap, we're lacking information to associate it with the job it >> belongs to. One option would be to extend BlockDirtyInfo to indicate >> this, but another might be to actually extend other commands like >> query-block-jobs to return information on the dirty bitmap associated >> with a specific job. >> >> I've applied it to block-next anyway, we still have some time to >> reconsider for 1.8. >> > > Another case for this may be user enabled external dirty bitmap, which > could be standalone from any block job. E.g. when we introduce a QMP > command like: > > block-new-dirty-bitmap device=foo file=bar.bitmap > > This could be some code in block.c, could be a block job (really > necessary?), or a block filter. I'm not sure... > > Fam > This command is for sure useful, but not quite a core block fuction, so hope it would not be in block.c. I think there is another problem need to solve: how to let user read "bar.bitmap", three options here: qemu-img dump, a new qmp command, a library. It seems a library is better(probably qmp interface is also needed).
On 2013年11月14日 10:13, Wenchao Xia wrote: > 于 2013/11/13 22:40, Fam Zheng 写道: >> On Wed, Nov 13, 2013 at 10:19 PM, Kevin Wolf <kwolf@redhat.com> wrote: >>> Am 13.11.2013 um 11:29 hat Fam Zheng geschrieben: >>>> We have multiple dirty bitmaps in BDS now, switch QAPI to allow query >>>> it (BlockInfo.dirty_bitmaps), and also drop old BlockInfo.dirty. >>>> >>>> Signed-off-by: Fam Zheng <famz@redhat.com> >>> >>>> diff --git a/qapi-schema.json b/qapi-schema.json >>>> index 81a375b..931d710 100644 >>>> --- a/qapi-schema.json >>>> +++ b/qapi-schema.json >>>> @@ -948,8 +948,8 @@ >>>> # @tray_open: #optional True if the device has a tray and it is open >>>> # (only present if removable is true) >>>> # >>>> -# @dirty: #optional dirty bitmap information (only present if the >>>> dirty >>>> -# bitmap is enabled) >>>> +# @dirty-bitmaps: #optional dirty bitmaps information (only present >>>> if the >>>> +# driver has one or more dirty bitmaps) >>>> # >>>> # @io-status: #optional @BlockDeviceIoStatus. Only present if the >>>> device >>>> # supports it and the VM is configured to stop on errors >>>> @@ -963,7 +963,7 @@ >>>> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', >>>> 'locked': 'bool', '*inserted': 'BlockDeviceInfo', >>>> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', >>>> - '*dirty': 'BlockDirtyInfo' } } >>>> + '*dirty-bitmaps': ['BlockDirtyInfo'] } } >>>> >>>> ## >>>> # @query-block: >>> >>> I believe this is of limited use; if you ever have more than one dirty >>> bitmap, we're lacking information to associate it with the job it >>> belongs to. One option would be to extend BlockDirtyInfo to indicate >>> this, but another might be to actually extend other commands like >>> query-block-jobs to return information on the dirty bitmap associated >>> with a specific job. >>> >>> I've applied it to block-next anyway, we still have some time to >>> reconsider for 1.8. >>> >> >> Another case for this may be user enabled external dirty bitmap, which >> could be standalone from any block job. E.g. when we introduce a QMP >> command like: >> >> block-new-dirty-bitmap device=foo file=bar.bitmap >> >> This could be some code in block.c, could be a block job (really >> necessary?), or a block filter. I'm not sure... >> >> Fam >> > This command is for sure useful, but not quite a core block fuction, > so hope it would not be in block.c. I think there is another problem > need to solve: how to let user read "bar.bitmap", three options here: > qemu-img dump, a new qmp command, a library. It seems a library is > better(probably qmp interface is also needed). > Yes, block.c is certainly too ad hoc and I don't like it either. I didn't go deep into this yet, I think we need to discuss these in a separate thread. Indeed there's much to decide: where the code lies, what format we would use and how to ensure consistency, etc. Fam
diff --git a/block.c b/block.c index ef7a55f..f57ae83 100644 --- a/block.c +++ b/block.c @@ -4379,6 +4379,26 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) } } +BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) +{ + BdrvDirtyBitmap *bm; + BlockDirtyInfoList *list = NULL; + BlockDirtyInfoList **plist = &list; + + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { + BlockDirtyInfo *info = g_malloc0(sizeof(BlockDirtyInfo)); + BlockDirtyInfoList *entry = g_malloc0(sizeof(BlockDirtyInfoList)); + info->count = bdrv_get_dirty_count(bs, bm); + info->granularity = + ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap)); + entry->value = info; + *plist = entry; + plist = &entry->next; + } + + return list; +} + int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector) { if (bitmap) { diff --git a/block/qapi.c b/block/qapi.c index 6b0cdcf..a32cb79 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -204,6 +204,11 @@ void bdrv_query_info(BlockDriverState *bs, info->io_status = bs->iostatus; } + if (!QLIST_EMPTY(&bs->dirty_bitmaps)) { + info->has_dirty_bitmaps = true; + info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs); + } + if (bs->drv) { info->has_inserted = true; info->inserted = g_malloc0(sizeof(*info->inserted)); diff --git a/include/block/block.h b/include/block/block.h index 06f424c..00f2711 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -391,6 +391,7 @@ struct HBitmapIter; typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); +BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); diff --git a/qapi-schema.json b/qapi-schema.json index 81a375b..931d710 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -948,8 +948,8 @@ # @tray_open: #optional True if the device has a tray and it is open # (only present if removable is true) # -# @dirty: #optional dirty bitmap information (only present if the dirty -# bitmap is enabled) +# @dirty-bitmaps: #optional dirty bitmaps information (only present if the +# driver has one or more dirty bitmaps) # # @io-status: #optional @BlockDeviceIoStatus. Only present if the device # supports it and the VM is configured to stop on errors @@ -963,7 +963,7 @@ 'data': {'device': 'str', 'type': 'str', 'removable': 'bool', 'locked': 'bool', '*inserted': 'BlockDeviceInfo', '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus', - '*dirty': 'BlockDirtyInfo' } } + '*dirty-bitmaps': ['BlockDirtyInfo'] } } ## # @query-block:
We have multiple dirty bitmaps in BDS now, switch QAPI to allow query it (BlockInfo.dirty_bitmaps), and also drop old BlockInfo.dirty. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 20 ++++++++++++++++++++ block/qapi.c | 5 +++++ include/block/block.h | 1 + qapi-schema.json | 6 +++--- 4 files changed, 29 insertions(+), 3 deletions(-)