| Message ID | 1272470181-15846-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/28/2010 10:56 AM, Kevin Wolf wrote: > This adds the wr_highest_sector blockstat which implements what is generally > known as the high watermark. It is the highest offset of a sector written to > the respective BlockDriverState since it has been opened. > > The query-blockstat QMP command is extended to add this value to the result, > and also to add the statistics of the underlying protocol in a new "parent" > field. Note that to get the "high watermark" of a qcow2 image, you need to look > into the wr_highest_sector field of the parent (which can be a file, a > host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself > is the highest offset on the _virtual_ disk that the guest has written to. > Is that the desirable behavior? When dealing with layered formats, would it be useful to print out stats for each of the layers? Regards, Anthony Liguori > Signed-off-by: Kevin Wolf<kwolf@redhat.com> > --- > block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++----------- > block_int.h | 1 + > 2 files changed, 62 insertions(+), 15 deletions(-) > > diff --git a/block.c b/block.c > index 91fecab..b75cef2 100644 > --- a/block.c > +++ b/block.c > @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num, > set_dirty_bitmap(bs, sector_num, nb_sectors, 1); > } > > + if (bs->wr_highest_sector< sector_num + nb_sectors - 1) { > + bs->wr_highest_sector = sector_num + nb_sectors - 1; > + } > + > return drv->bdrv_write(bs, sector_num, buf, nb_sectors); > } > > @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque) > qdict_get_int(qdict, "wr_bytes"), > qdict_get_int(qdict, "rd_operations"), > qdict_get_int(qdict, "wr_operations")); > + if (qdict_haskey(qdict, "parent")) { > + QObject *parent = qdict_get(qdict, "parent"); > + bdrv_stats_iter(parent, mon); > + } > } > > void bdrv_stats_print(Monitor *mon, const QObject *data) > @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon); > } > > +static QObject* bdrv_info_stats_bs(BlockDriverState *bs) > +{ > + QObject *res; > + QObject *parent = NULL; > + > + if (bs->file) { > + parent = bdrv_info_stats_bs(bs->file); > + } > + > + res = qobject_from_jsonf("{ 'device': %s, 'stats': {" > + "'rd_bytes': %" PRId64 "," > + "'wr_bytes': %" PRId64 "," > + "'rd_operations': %" PRId64 "," > + "'wr_operations': %" PRId64 "," > + "'wr_highest_offset': %" PRId64 > + "} }", > + bs->device_name, > + bs->rd_bytes, bs->wr_bytes, > + bs->rd_ops, bs->wr_ops, > + bs->wr_highest_sector * 512); > + if (parent) { > + QDict *dict = qobject_to_qdict(res); > + qdict_put_obj(dict, "parent", parent); > + } > + > + return res; > +} > + > /** > * bdrv_info_stats(): show block device statistics > * > @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > * - "wr_bytes": bytes written > * - "rd_operations": read operations > * - "wr_operations": write operations > - * > + * - "wr_highest_offset": Highest offset of a sector written since the > + * BlockDriverState has been opened > + * - "parent": Contains recursively the statistics of the underlying > + * protocol (e.g. the host file for a qcow2 image). If there is no > + * underlying protocol, this field is omitted. > + * > * Example: > * > * [ { "device": "ide0-hd0", > * "stats": { "rd_bytes": 512, > * "wr_bytes": 0, > * "rd_operations": 1, > - * "wr_operations": 0 } }, > + * "wr_operations": 0, > + * "wr_highest_offset": 0, > + * "parent": { > + * "stats": { "rd_bytes": 1024, > + * "wr_bytes": 0, > + * "rd_operations": 2, > + * "wr_operations": 0, > + * "wr_highest_offset": 0, > + * } > + * } } }, > * { "device": "ide1-cd0", > * "stats": { "rd_bytes": 0, > * "wr_bytes": 0, > * "rd_operations": 0, > - * "wr_operations": 0 } } ] > + * "wr_operations": 0, > + * "wr_highest_offset": 0 } }, > */ > void bdrv_info_stats(Monitor *mon, QObject **ret_data) > { > @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data) > devices = qlist_new(); > > QTAILQ_FOREACH(bs,&bdrv_states, list) { > - obj = qobject_from_jsonf("{ 'device': %s, 'stats': {" > - "'rd_bytes': %" PRId64 "," > - "'wr_bytes': %" PRId64 "," > - "'rd_operations': %" PRId64 "," > - "'wr_operations': %" PRId64 > - "} }", > - bs->device_name, > - bs->rd_bytes, bs->wr_bytes, > - bs->rd_ops, bs->wr_ops); > + obj = bdrv_info_stats_bs(bs); > qlist_append_obj(devices, obj); > } > > @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, > cb, opaque); > > if (ret) { > - /* Update stats even though technically transfer has not happened. */ > - bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > - bs->wr_ops ++; > + /* Update stats even though technically transfer has not happened. */ > + bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > + bs->wr_ops ++; > + if (bs->wr_highest_sector< sector_num + nb_sectors - 1) { > + bs->wr_highest_sector = sector_num + nb_sectors - 1; > + } > } > > return ret; > diff --git a/block_int.h b/block_int.h > index a3afe63..1a7240c 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -167,6 +167,7 @@ struct BlockDriverState { > uint64_t wr_bytes; > uint64_t rd_ops; > uint64_t wr_ops; > + uint64_t wr_highest_sector; > > /* Whether the disk can expand beyond total_sectors */ > int growable; >
On 04/28/2010 10:56 AM, Kevin Wolf wrote: > This adds the wr_highest_sector blockstat which implements what is generally > known as the high watermark. It is the highest offset of a sector written to > the respective BlockDriverState since it has been opened. > > The query-blockstat QMP command is extended to add this value to the result, > and also to add the statistics of the underlying protocol in a new "parent" > field. Note that to get the "high watermark" of a qcow2 image, you need to look > into the wr_highest_sector field of the parent (which can be a file, a > host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself > is the highest offset on the _virtual_ disk that the guest has written to. > > Signed-off-by: Kevin Wolf<kwolf@redhat.com> > I see, you did print out stats for each layer. I don't think we should take 2/2. I don't mind QMP having more features than the user monitor. Regards, Anthony Liguori > --- > block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++----------- > block_int.h | 1 + > 2 files changed, 62 insertions(+), 15 deletions(-) > > diff --git a/block.c b/block.c > index 91fecab..b75cef2 100644 > --- a/block.c > +++ b/block.c > @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num, > set_dirty_bitmap(bs, sector_num, nb_sectors, 1); > } > > + if (bs->wr_highest_sector< sector_num + nb_sectors - 1) { > + bs->wr_highest_sector = sector_num + nb_sectors - 1; > + } > + > return drv->bdrv_write(bs, sector_num, buf, nb_sectors); > } > > @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque) > qdict_get_int(qdict, "wr_bytes"), > qdict_get_int(qdict, "rd_operations"), > qdict_get_int(qdict, "wr_operations")); > + if (qdict_haskey(qdict, "parent")) { > + QObject *parent = qdict_get(qdict, "parent"); > + bdrv_stats_iter(parent, mon); > + } > } > > void bdrv_stats_print(Monitor *mon, const QObject *data) > @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon); > } > > +static QObject* bdrv_info_stats_bs(BlockDriverState *bs) > +{ > + QObject *res; > + QObject *parent = NULL; > + > + if (bs->file) { > + parent = bdrv_info_stats_bs(bs->file); > + } > + > + res = qobject_from_jsonf("{ 'device': %s, 'stats': {" > + "'rd_bytes': %" PRId64 "," > + "'wr_bytes': %" PRId64 "," > + "'rd_operations': %" PRId64 "," > + "'wr_operations': %" PRId64 "," > + "'wr_highest_offset': %" PRId64 > + "} }", > + bs->device_name, > + bs->rd_bytes, bs->wr_bytes, > + bs->rd_ops, bs->wr_ops, > + bs->wr_highest_sector * 512); > + if (parent) { > + QDict *dict = qobject_to_qdict(res); > + qdict_put_obj(dict, "parent", parent); > + } > + > + return res; > +} > + > /** > * bdrv_info_stats(): show block device statistics > * > @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > * - "wr_bytes": bytes written > * - "rd_operations": read operations > * - "wr_operations": write operations > - * > + * - "wr_highest_offset": Highest offset of a sector written since the > + * BlockDriverState has been opened > + * - "parent": Contains recursively the statistics of the underlying > + * protocol (e.g. the host file for a qcow2 image). If there is no > + * underlying protocol, this field is omitted. > + * > * Example: > * > * [ { "device": "ide0-hd0", > * "stats": { "rd_bytes": 512, > * "wr_bytes": 0, > * "rd_operations": 1, > - * "wr_operations": 0 } }, > + * "wr_operations": 0, > + * "wr_highest_offset": 0, > + * "parent": { > + * "stats": { "rd_bytes": 1024, > + * "wr_bytes": 0, > + * "rd_operations": 2, > + * "wr_operations": 0, > + * "wr_highest_offset": 0, > + * } > + * } } }, > * { "device": "ide1-cd0", > * "stats": { "rd_bytes": 0, > * "wr_bytes": 0, > * "rd_operations": 0, > - * "wr_operations": 0 } } ] > + * "wr_operations": 0, > + * "wr_highest_offset": 0 } }, > */ > void bdrv_info_stats(Monitor *mon, QObject **ret_data) > { > @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data) > devices = qlist_new(); > > QTAILQ_FOREACH(bs,&bdrv_states, list) { > - obj = qobject_from_jsonf("{ 'device': %s, 'stats': {" > - "'rd_bytes': %" PRId64 "," > - "'wr_bytes': %" PRId64 "," > - "'rd_operations': %" PRId64 "," > - "'wr_operations': %" PRId64 > - "} }", > - bs->device_name, > - bs->rd_bytes, bs->wr_bytes, > - bs->rd_ops, bs->wr_ops); > + obj = bdrv_info_stats_bs(bs); > qlist_append_obj(devices, obj); > } > > @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, > cb, opaque); > > if (ret) { > - /* Update stats even though technically transfer has not happened. */ > - bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > - bs->wr_ops ++; > + /* Update stats even though technically transfer has not happened. */ > + bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > + bs->wr_ops ++; > + if (bs->wr_highest_sector< sector_num + nb_sectors - 1) { > + bs->wr_highest_sector = sector_num + nb_sectors - 1; > + } > } > > return ret; > diff --git a/block_int.h b/block_int.h > index a3afe63..1a7240c 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -167,6 +167,7 @@ struct BlockDriverState { > uint64_t wr_bytes; > uint64_t rd_ops; > uint64_t wr_ops; > + uint64_t wr_highest_sector; > > /* Whether the disk can expand beyond total_sectors */ > int growable; >
On Wed, 28 Apr 2010 11:01:12 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 04/28/2010 10:56 AM, Kevin Wolf wrote: > > This adds the wr_highest_sector blockstat which implements what is generally > > known as the high watermark. It is the highest offset of a sector written to > > the respective BlockDriverState since it has been opened. > > > > The query-blockstat QMP command is extended to add this value to the result, > > and also to add the statistics of the underlying protocol in a new "parent" > > field. Note that to get the "high watermark" of a qcow2 image, you need to look > > into the wr_highest_sector field of the parent (which can be a file, a > > host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself > > is the highest offset on the _virtual_ disk that the guest has written to. > > > > Signed-off-by: Kevin Wolf<kwolf@redhat.com> > > > > I see, you did print out stats for each layer. > > I don't think we should take 2/2. I don't mind QMP having more features > than the user monitor. I don't either, but Kevin has said to me that this information is also good for the user Monitor. The real question here is whether or not we're going to stop supporting stability for the user Monitor and if so, when we'll break it. An arguable reasonable policy would be to try to maintain stability for existing commands. In this specific case, 'info blockstats' is used by libvirt afaik. So breaking it would mean that older libvirt versions won't be able to talk to newer qemu (taking libvirt just as real known example).
On Wed, 28 Apr 2010 17:56:20 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > This adds the wr_highest_sector blockstat which implements what is generally > known as the high watermark. It is the highest offset of a sector written to > the respective BlockDriverState since it has been opened. > > The query-blockstat QMP command is extended to add this value to the result, > and also to add the statistics of the underlying protocol in a new "parent" > field. Note that to get the "high watermark" of a qcow2 image, you need to look > into the wr_highest_sector field of the parent (which can be a file, a > host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself > is the highest offset on the _virtual_ disk that the guest has written to. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++----------- > block_int.h | 1 + > 2 files changed, 62 insertions(+), 15 deletions(-) > > diff --git a/block.c b/block.c > index 91fecab..b75cef2 100644 > --- a/block.c > +++ b/block.c > @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num, > set_dirty_bitmap(bs, sector_num, nb_sectors, 1); > } > > + if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { > + bs->wr_highest_sector = sector_num + nb_sectors - 1; > + } > + > return drv->bdrv_write(bs, sector_num, buf, nb_sectors); > } > > @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque) > qdict_get_int(qdict, "wr_bytes"), > qdict_get_int(qdict, "rd_operations"), > qdict_get_int(qdict, "wr_operations")); > + if (qdict_haskey(qdict, "parent")) { > + QObject *parent = qdict_get(qdict, "parent"); > + bdrv_stats_iter(parent, mon); > + } > } > > void bdrv_stats_print(Monitor *mon, const QObject *data) > @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon); > } > > +static QObject* bdrv_info_stats_bs(BlockDriverState *bs) > +{ > + QObject *res; > + QObject *parent = NULL; > + > + if (bs->file) { > + parent = bdrv_info_stats_bs(bs->file); > + } Doesn't build, I guess you meant 'backing_hd'. > + > + res = qobject_from_jsonf("{ 'device': %s, 'stats': {" > + "'rd_bytes': %" PRId64 "," > + "'wr_bytes': %" PRId64 "," > + "'rd_operations': %" PRId64 "," > + "'wr_operations': %" PRId64 "," > + "'wr_highest_offset': %" PRId64 > + "} }", > + bs->device_name, > + bs->rd_bytes, bs->wr_bytes, > + bs->rd_ops, bs->wr_ops, > + bs->wr_highest_sector * 512); > + if (parent) { > + QDict *dict = qobject_to_qdict(res); > + qdict_put_obj(dict, "parent", parent); > + } > + > + return res; > +} > + > /** > * bdrv_info_stats(): show block device statistics > * > @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > * - "wr_bytes": bytes written > * - "rd_operations": read operations > * - "wr_operations": write operations > - * > + * - "wr_highest_offset": Highest offset of a sector written since the > + * BlockDriverState has been opened > + * - "parent": Contains recursively the statistics of the underlying > + * protocol (e.g. the host file for a qcow2 image). If there is no > + * underlying protocol, this field is omitted. Looks like you are not pushing 'protocol', we already have 'format' though (not sure if they overlap). Also, 'device' is "" for 'parent'. You should consider not pushing the key in this case (and noting that it's optional in the doc above). > + * > * Example: > * > * [ { "device": "ide0-hd0", > * "stats": { "rd_bytes": 512, > * "wr_bytes": 0, > * "rd_operations": 1, > - * "wr_operations": 0 } }, > + * "wr_operations": 0, > + * "wr_highest_offset": 0, > + * "parent": { > + * "stats": { "rd_bytes": 1024, > + * "wr_bytes": 0, > + * "rd_operations": 2, > + * "wr_operations": 0, > + * "wr_highest_offset": 0, > + * } > + * } } }, > * { "device": "ide1-cd0", > * "stats": { "rd_bytes": 0, > * "wr_bytes": 0, > * "rd_operations": 0, > - * "wr_operations": 0 } } ] > + * "wr_operations": 0, > + * "wr_highest_offset": 0 } }, > */ > void bdrv_info_stats(Monitor *mon, QObject **ret_data) > { > @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data) > devices = qlist_new(); > > QTAILQ_FOREACH(bs, &bdrv_states, list) { > - obj = qobject_from_jsonf("{ 'device': %s, 'stats': {" > - "'rd_bytes': %" PRId64 "," > - "'wr_bytes': %" PRId64 "," > - "'rd_operations': %" PRId64 "," > - "'wr_operations': %" PRId64 > - "} }", > - bs->device_name, > - bs->rd_bytes, bs->wr_bytes, > - bs->rd_ops, bs->wr_ops); > + obj = bdrv_info_stats_bs(bs); > qlist_append_obj(devices, obj); > } > > @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, > cb, opaque); > > if (ret) { > - /* Update stats even though technically transfer has not happened. */ > - bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > - bs->wr_ops ++; > + /* Update stats even though technically transfer has not happened. */ > + bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > + bs->wr_ops ++; > + if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { > + bs->wr_highest_sector = sector_num + nb_sectors - 1; > + } > } > > return ret; > diff --git a/block_int.h b/block_int.h > index a3afe63..1a7240c 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -167,6 +167,7 @@ struct BlockDriverState { > uint64_t wr_bytes; > uint64_t rd_ops; > uint64_t wr_ops; > + uint64_t wr_highest_sector; > > /* Whether the disk can expand beyond total_sectors */ > int growable;
On 04/28/2010 12:04 PM, Luiz Capitulino wrote: > On Wed, 28 Apr 2010 11:01:12 -0500 > Anthony Liguori<anthony@codemonkey.ws> wrote: > > >> On 04/28/2010 10:56 AM, Kevin Wolf wrote: >> >>> This adds the wr_highest_sector blockstat which implements what is generally >>> known as the high watermark. It is the highest offset of a sector written to >>> the respective BlockDriverState since it has been opened. >>> >>> The query-blockstat QMP command is extended to add this value to the result, >>> and also to add the statistics of the underlying protocol in a new "parent" >>> field. Note that to get the "high watermark" of a qcow2 image, you need to look >>> into the wr_highest_sector field of the parent (which can be a file, a >>> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself >>> is the highest offset on the _virtual_ disk that the guest has written to. >>> >>> Signed-off-by: Kevin Wolf<kwolf@redhat.com> >>> >>> >> I see, you did print out stats for each layer. >> >> I don't think we should take 2/2. I don't mind QMP having more features >> than the user monitor. >> > I don't either, but Kevin has said to me that this information is also good > for the user Monitor. > > The real question here is whether or not we're going to stop supporting > stability for the user Monitor and if so, when we'll break it. > > An arguable reasonable policy would be to try to maintain stability for > existing commands. In this specific case, 'info blockstats' is used by > libvirt afaik. So breaking it would mean that older libvirt versions won't > be able to talk to newer qemu (taking libvirt just as real known example). > I think we should try our best to maintain compatibility. In this case, this change would break any non-QMP version of libvirt so it would be pretty painful for users. That's why I'm inclined to not take. It would be reasonable to add a new info command for the user monitor if the functionality is desirable. Regards, Anthony Liguori
On Wed, 28 Apr 2010 12:47:49 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 04/28/2010 12:04 PM, Luiz Capitulino wrote: > > On Wed, 28 Apr 2010 11:01:12 -0500 > > Anthony Liguori<anthony@codemonkey.ws> wrote: > > > > > >> On 04/28/2010 10:56 AM, Kevin Wolf wrote: > >> > >>> This adds the wr_highest_sector blockstat which implements what is generally > >>> known as the high watermark. It is the highest offset of a sector written to > >>> the respective BlockDriverState since it has been opened. > >>> > >>> The query-blockstat QMP command is extended to add this value to the result, > >>> and also to add the statistics of the underlying protocol in a new "parent" > >>> field. Note that to get the "high watermark" of a qcow2 image, you need to look > >>> into the wr_highest_sector field of the parent (which can be a file, a > >>> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself > >>> is the highest offset on the _virtual_ disk that the guest has written to. > >>> > >>> Signed-off-by: Kevin Wolf<kwolf@redhat.com> > >>> > >>> > >> I see, you did print out stats for each layer. > >> > >> I don't think we should take 2/2. I don't mind QMP having more features > >> than the user monitor. > >> > > I don't either, but Kevin has said to me that this information is also good > > for the user Monitor. > > > > The real question here is whether or not we're going to stop supporting > > stability for the user Monitor and if so, when we'll break it. > > > > An arguable reasonable policy would be to try to maintain stability for > > existing commands. In this specific case, 'info blockstats' is used by > > libvirt afaik. So breaking it would mean that older libvirt versions won't > > be able to talk to newer qemu (taking libvirt just as real known example). > > > > I think we should try our best to maintain compatibility. In this case, > this change would break any non-QMP version of libvirt so it would be > pretty painful for users. That's why I'm inclined to not take. > > It would be reasonable to add a new info command for the user monitor if > the functionality is desirable. Seems a good solution to me too.
Am 28.04.2010 18:01, schrieb Anthony Liguori: > On 04/28/2010 10:56 AM, Kevin Wolf wrote: >> This adds the wr_highest_sector blockstat which implements what is generally >> known as the high watermark. It is the highest offset of a sector written to >> the respective BlockDriverState since it has been opened. >> >> The query-blockstat QMP command is extended to add this value to the result, >> and also to add the statistics of the underlying protocol in a new "parent" >> field. Note that to get the "high watermark" of a qcow2 image, you need to look >> into the wr_highest_sector field of the parent (which can be a file, a >> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself >> is the highest offset on the _virtual_ disk that the guest has written to. >> >> Signed-off-by: Kevin Wolf<kwolf@redhat.com> >> > > I see, you did print out stats for each layer. > > I don't think we should take 2/2. I don't mind QMP having more features > than the user monitor. I agree, just posted it because it felt incomplete without it and I wanted to give you the choice. The only use case in the user monitor I saw for it would have been qemu development anyway. I can keep the patch in a local branch for this (or just rewrite it when/if I need it - it's small enough). Kevin
Am 28.04.2010 19:31, schrieb Luiz Capitulino: > On Wed, 28 Apr 2010 17:56:20 +0200 > Kevin Wolf <kwolf@redhat.com> wrote: > >> This adds the wr_highest_sector blockstat which implements what is generally >> known as the high watermark. It is the highest offset of a sector written to >> the respective BlockDriverState since it has been opened. >> >> The query-blockstat QMP command is extended to add this value to the result, >> and also to add the statistics of the underlying protocol in a new "parent" >> field. Note that to get the "high watermark" of a qcow2 image, you need to look >> into the wr_highest_sector field of the parent (which can be a file, a >> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself >> is the highest offset on the _virtual_ disk that the guest has written to. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++----------- >> block_int.h | 1 + >> 2 files changed, 62 insertions(+), 15 deletions(-) >> >> diff --git a/block.c b/block.c >> index 91fecab..b75cef2 100644 >> --- a/block.c >> +++ b/block.c >> @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num, >> set_dirty_bitmap(bs, sector_num, nb_sectors, 1); >> } >> >> + if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { >> + bs->wr_highest_sector = sector_num + nb_sectors - 1; >> + } >> + >> return drv->bdrv_write(bs, sector_num, buf, nb_sectors); >> } >> >> @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque) >> qdict_get_int(qdict, "wr_bytes"), >> qdict_get_int(qdict, "rd_operations"), >> qdict_get_int(qdict, "wr_operations")); >> + if (qdict_haskey(qdict, "parent")) { >> + QObject *parent = qdict_get(qdict, "parent"); >> + bdrv_stats_iter(parent, mon); >> + } >> } >> >> void bdrv_stats_print(Monitor *mon, const QObject *data) >> @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) >> qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon); >> } >> >> +static QObject* bdrv_info_stats_bs(BlockDriverState *bs) >> +{ >> + QObject *res; >> + QObject *parent = NULL; >> + >> + if (bs->file) { >> + parent = bdrv_info_stats_bs(bs->file); >> + } > > Doesn't build, I guess you meant 'backing_hd'. The patch is against git://repo.or.cz/qemu/kevin.git block, not master (like most block related patches lately). backing_hd is for qcow base images, so that's a different thing. >> + >> + res = qobject_from_jsonf("{ 'device': %s, 'stats': {" >> + "'rd_bytes': %" PRId64 "," >> + "'wr_bytes': %" PRId64 "," >> + "'rd_operations': %" PRId64 "," >> + "'wr_operations': %" PRId64 "," >> + "'wr_highest_offset': %" PRId64 >> + "} }", >> + bs->device_name, >> + bs->rd_bytes, bs->wr_bytes, >> + bs->rd_ops, bs->wr_ops, >> + bs->wr_highest_sector * 512); >> + if (parent) { >> + QDict *dict = qobject_to_qdict(res); >> + qdict_put_obj(dict, "parent", parent); >> + } >> + >> + return res; >> +} >> + >> /** >> * bdrv_info_stats(): show block device statistics >> * >> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) >> * - "wr_bytes": bytes written >> * - "rd_operations": read operations >> * - "wr_operations": write operations >> - * >> + * - "wr_highest_offset": Highest offset of a sector written since the >> + * BlockDriverState has been opened >> + * - "parent": Contains recursively the statistics of the underlying >> + * protocol (e.g. the host file for a qcow2 image). If there is no >> + * underlying protocol, this field is omitted. > > Looks like you are not pushing 'protocol', we already have 'format' > though (not sure if they overlap). You're talking about a new key 'protocol'? I'm not sure what it should describe, to be honest. > Also, 'device' is "" for 'parent'. You should consider not pushing the > key in this case (and noting that it's optional in the doc above). Ok, will change that for v2. Kevin
On Wed, Apr 28, 2010 at 12:47:49PM -0500, Anthony Liguori wrote: > On 04/28/2010 12:04 PM, Luiz Capitulino wrote: > >On Wed, 28 Apr 2010 11:01:12 -0500 > >Anthony Liguori<anthony@codemonkey.ws> wrote: > > > > > >>On 04/28/2010 10:56 AM, Kevin Wolf wrote: > >> > >>>This adds the wr_highest_sector blockstat which implements what is > >>>generally > >>>known as the high watermark. It is the highest offset of a sector > >>>written to > >>>the respective BlockDriverState since it has been opened. > >>> > >>>The query-blockstat QMP command is extended to add this value to the > >>>result, > >>>and also to add the statistics of the underlying protocol in a new > >>>"parent" > >>>field. Note that to get the "high watermark" of a qcow2 image, you need > >>>to look > >>>into the wr_highest_sector field of the parent (which can be a file, a > >>>host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState > >>>itself > >>>is the highest offset on the _virtual_ disk that the guest has written > >>>to. > >>> > >>>Signed-off-by: Kevin Wolf<kwolf@redhat.com> > >>> > >>> > >>I see, you did print out stats for each layer. > >> > >>I don't think we should take 2/2. I don't mind QMP having more features > >>than the user monitor. > >> > > I don't either, but Kevin has said to me that this information is also > > good > >for the user Monitor. > > > > The real question here is whether or not we're going to stop supporting > >stability for the user Monitor and if so, when we'll break it. > > > > An arguable reasonable policy would be to try to maintain stability for > >existing commands. In this specific case, 'info blockstats' is used by > >libvirt afaik. So breaking it would mean that older libvirt versions won't > >be able to talk to newer qemu (taking libvirt just as real known example). > > > > I think we should try our best to maintain compatibility. In this case, > this change would break any non-QMP version of libvirt so it would be > pretty painful for users. That's why I'm inclined to not take. I agree. There are alot of existing deployed libvirt's out there which all still use the user monitor. libvirt will enable JSON by default once QEMU brings out 0.13, but there'll still be people with older libvirt. We need to avoid breaking the user monitor for at least one or two releaes to allow time for apps to catch up with the switch to JSON. Mark the user monitor deprecated in 0.13 by all means, but keep compatability until either the 0.15 or 0.16 release if at all practical. Regards, Daniel
On Thu, 29 Apr 2010 12:00:45 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > >> /** > >> * bdrv_info_stats(): show block device statistics > >> * > >> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) > >> * - "wr_bytes": bytes written > >> * - "rd_operations": read operations > >> * - "wr_operations": write operations > >> - * > >> + * - "wr_highest_offset": Highest offset of a sector written since the > >> + * BlockDriverState has been opened > >> + * - "parent": Contains recursively the statistics of the underlying > >> + * protocol (e.g. the host file for a qcow2 image). If there is no > >> + * underlying protocol, this field is omitted. > > > > Looks like you are not pushing 'protocol', we already have 'format' > > though (not sure if they overlap). > > You're talking about a new key 'protocol'? I'm not sure what it should > describe, to be honest. I thought I read somewhere you were going to have a 'protocol' key as well (in addition to 'wr_highest_offset' and 'parent') but looks like I was just confused. Sorry for the noise.
diff --git a/block.c b/block.c index 91fecab..b75cef2 100644 --- a/block.c +++ b/block.c @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num, set_dirty_bitmap(bs, sector_num, nb_sectors, 1); } + if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { + bs->wr_highest_sector = sector_num + nb_sectors - 1; + } + return drv->bdrv_write(bs, sector_num, buf, nb_sectors); } @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque) qdict_get_int(qdict, "wr_bytes"), qdict_get_int(qdict, "rd_operations"), qdict_get_int(qdict, "wr_operations")); + if (qdict_haskey(qdict, "parent")) { + QObject *parent = qdict_get(qdict, "parent"); + bdrv_stats_iter(parent, mon); + } } void bdrv_stats_print(Monitor *mon, const QObject *data) @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon); } +static QObject* bdrv_info_stats_bs(BlockDriverState *bs) +{ + QObject *res; + QObject *parent = NULL; + + if (bs->file) { + parent = bdrv_info_stats_bs(bs->file); + } + + res = qobject_from_jsonf("{ 'device': %s, 'stats': {" + "'rd_bytes': %" PRId64 "," + "'wr_bytes': %" PRId64 "," + "'rd_operations': %" PRId64 "," + "'wr_operations': %" PRId64 "," + "'wr_highest_offset': %" PRId64 + "} }", + bs->device_name, + bs->rd_bytes, bs->wr_bytes, + bs->rd_ops, bs->wr_ops, + bs->wr_highest_sector * 512); + if (parent) { + QDict *dict = qobject_to_qdict(res); + qdict_put_obj(dict, "parent", parent); + } + + return res; +} + /** * bdrv_info_stats(): show block device statistics * @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data) * - "wr_bytes": bytes written * - "rd_operations": read operations * - "wr_operations": write operations - * + * - "wr_highest_offset": Highest offset of a sector written since the + * BlockDriverState has been opened + * - "parent": Contains recursively the statistics of the underlying + * protocol (e.g. the host file for a qcow2 image). If there is no + * underlying protocol, this field is omitted. + * * Example: * * [ { "device": "ide0-hd0", * "stats": { "rd_bytes": 512, * "wr_bytes": 0, * "rd_operations": 1, - * "wr_operations": 0 } }, + * "wr_operations": 0, + * "wr_highest_offset": 0, + * "parent": { + * "stats": { "rd_bytes": 1024, + * "wr_bytes": 0, + * "rd_operations": 2, + * "wr_operations": 0, + * "wr_highest_offset": 0, + * } + * } } }, * { "device": "ide1-cd0", * "stats": { "rd_bytes": 0, * "wr_bytes": 0, * "rd_operations": 0, - * "wr_operations": 0 } } ] + * "wr_operations": 0, + * "wr_highest_offset": 0 } }, */ void bdrv_info_stats(Monitor *mon, QObject **ret_data) { @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data) devices = qlist_new(); QTAILQ_FOREACH(bs, &bdrv_states, list) { - obj = qobject_from_jsonf("{ 'device': %s, 'stats': {" - "'rd_bytes': %" PRId64 "," - "'wr_bytes': %" PRId64 "," - "'rd_operations': %" PRId64 "," - "'wr_operations': %" PRId64 - "} }", - bs->device_name, - bs->rd_bytes, bs->wr_bytes, - bs->rd_ops, bs->wr_ops); + obj = bdrv_info_stats_bs(bs); qlist_append_obj(devices, obj); } @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, cb, opaque); if (ret) { - /* Update stats even though technically transfer has not happened. */ - bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; - bs->wr_ops ++; + /* Update stats even though technically transfer has not happened. */ + bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; + bs->wr_ops ++; + if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { + bs->wr_highest_sector = sector_num + nb_sectors - 1; + } } return ret; diff --git a/block_int.h b/block_int.h index a3afe63..1a7240c 100644 --- a/block_int.h +++ b/block_int.h @@ -167,6 +167,7 @@ struct BlockDriverState { uint64_t wr_bytes; uint64_t rd_ops; uint64_t wr_ops; + uint64_t wr_highest_sector; /* Whether the disk can expand beyond total_sectors */ int growable;
This adds the wr_highest_sector blockstat which implements what is generally known as the high watermark. It is the highest offset of a sector written to the respective BlockDriverState since it has been opened. The query-blockstat QMP command is extended to add this value to the result, and also to add the statistics of the underlying protocol in a new "parent" field. Note that to get the "high watermark" of a qcow2 image, you need to look into the wr_highest_sector field of the parent (which can be a file, a host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself is the highest offset on the _virtual_ disk that the guest has written to. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++----------- block_int.h | 1 + 2 files changed, 62 insertions(+), 15 deletions(-)