| Message ID | 20181002113325.12911-2-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | improve block-latency-histogram-set | expand |
On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote: > Support modern way of device selecting. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 6 ++++-- > blockdev.c | 8 +++++--- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index ac3b48ee54..4efd60d8ab 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -489,7 +489,9 @@ > # If only @device parameter is specified, remove all present latency histograms > # for the device. Otherwise, add/reset some of (or all) latency histograms. > # > -# @device: device name to set latency histogram for. > +# @device: device name to set latency histogram for (better use @id). > +# > +# @id: The name or QOM path of the guest device. As long as we are renaming the command, there's no need to keep a legacy parameter around. Just get rid of device and replace it by id, rather than worrying about both. The introduction of the stable command does not have to carry any baggage left over from the x- preliminary version.
On 02.10.2018 17:22, Eric Blake wrote: > On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote: >> Support modern way of device selecting. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 6 ++++-- >> blockdev.c | 8 +++++--- >> 2 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index ac3b48ee54..4efd60d8ab 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -489,7 +489,9 @@ >> # If only @device parameter is specified, remove all present latency histograms >> # for the device. Otherwise, add/reset some of (or all) latency histograms. >> # >> -# @device: device name to set latency histogram for. >> +# @device: device name to set latency histogram for (better use @id). >> +# >> +# @id: The name or QOM path of the guest device. > > As long as we are renaming the command, there's no need to keep a legacy parameter around. Just get rid of device and replace it by id, rather than worrying about both. The introduction of the stable command does not have to carry any baggage left over from the x- preliminary version. > I'm afraid libvirt is not ready for this. It works either thru drive aliases or qom paths depending on capability that is not turned on yet. It takes some time before capability can be safely turned on (blockjob code is not ready AFAIK). Until that moment we can not use latency API without "device" parameter. Nikolay
02.10.2018 17:22, Eric Blake wrote: > On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote: >> Support modern way of device selecting. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 6 ++++-- >> blockdev.c | 8 +++++--- >> 2 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index ac3b48ee54..4efd60d8ab 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -489,7 +489,9 @@ >> # If only @device parameter is specified, remove all present >> latency histograms >> # for the device. Otherwise, add/reset some of (or all) latency >> histograms. >> # >> -# @device: device name to set latency histogram for. >> +# @device: device name to set latency histogram for (better use @id). >> +# >> +# @id: The name or QOM path of the guest device. > > As long as we are renaming the command, there's no need to keep a > legacy parameter around. Just get rid of device and replace it by id, > rather than worrying about both. The introduction of the stable > command does not have to carry any baggage left over from the x- > preliminary version. > Libvirt don't need both for now, for different scenarios?
On 10/2/18 9:30 AM, Vladimir Sementsov-Ogievskiy wrote: >>> -# @device: device name to set latency histogram for. >>> +# @device: device name to set latency histogram for (better use @id). >>> +# >>> +# @id: The name or QOM path of the guest device. >> >> As long as we are renaming the command, there's no need to keep a >> legacy parameter around. Just get rid of device and replace it by id, >> rather than worrying about both. The introduction of the stable >> command does not have to carry any baggage left over from the x- >> preliminary version. >> > > Libvirt don't need both for now, for different scenarios? If you want both to work because libvirt has reasons to use both, then the documentation needs to be more precise. "(better use @id)" does not tell me why I shouldn't use @device, or what difference in behavior I get by using one instead of the other. Furthermore, if @id is able to use the name of the guest device as an alternative to the QOM path, then how is that different from @device being the name of the guest device?
02.10.2018 17:35, Eric Blake wrote: > On 10/2/18 9:30 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> -# @device: device name to set latency histogram for. >>>> +# @device: device name to set latency histogram for (better use @id). >>>> +# >>>> +# @id: The name or QOM path of the guest device. >>> >>> As long as we are renaming the command, there's no need to keep a >>> legacy parameter around. Just get rid of device and replace it by >>> id, rather than worrying about both. The introduction of the stable >>> command does not have to carry any baggage left over from the x- >>> preliminary version. >>> >> >> Libvirt don't need both for now, for different scenarios? > > If you want both to work because libvirt has reasons to use both, then > the documentation needs to be more precise. "(better use @id)" does > not tell me why I shouldn't use @device, or what difference in > behavior I get by using one instead of the other. Furthermore, if @id > is able to use the name of the guest device as an alternative to the > QOM path, then how is that different from @device being the name of > the guest device? > Hm. It all looks a bit weird. I've just duplicated block_set_io_throttle interface: # @device: Block device name (deprecated, use @id instead) # # @id: The name or QOM path of the guest device (since: 2.8) blk = qmp_get_blk(arg->has_device ? arg->device : NULL, arg->has_id ? arg->id : NULL, errp); if (!blk) { return; } So, looks like "The name or" is wrong part. @id can be only path. However, I can call blk_by_name on @id, if blk_by_qdev_id failed.. So, variants: 1. both parameters, current code, but fix documentation (will be @id: QOM path of the guest device.) 2. only @id and only QOM path 3. only @id, but fullback to blk_by_name(@id) if failed to find qdev. Any option is ok for me, I don't care. What do you think?
On 10/2/18 9:58 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>> -# @device: device name to set latency histogram for. >>>>> +# @device: device name to set latency histogram for (better use @id). >>>>> +# >>>>> +# @id: The name or QOM path of the guest device. >>>> > Hm. It all looks a bit weird. I've just duplicated block_set_io_throttle > interface: But that interface was pre-existing under a stable name, so it had to worry about back-compat. Here, we are adding a new command, so we don't have to be stuck with back-compat ugliness. > > # @device: Block device name (deprecated, use @id instead) > # > # @id: The name or QOM path of the guest device (since: 2.8) > > blk = qmp_get_blk(arg->has_device ? arg->device : NULL, > arg->has_id ? arg->id : NULL, > errp); > if (!blk) { > return; > } > > > So, looks like "The name or" is wrong part. @id can be only path. > > However, I can call blk_by_name on @id, if blk_by_qdev_id failed.. > > So, variants: > > 1. both parameters, current code, but fix documentation (will be @id: > QOM path of the guest device.) > 2. only @id and only QOM path > 3. only @id, but fullback to blk_by_name(@id) if failed to find qdev. > > Any option is ok for me, I don't care. What do you think? I'm leaning towards 3.
diff --git a/qapi/block-core.json b/qapi/block-core.json index ac3b48ee54..4efd60d8ab 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -489,7 +489,9 @@ # If only @device parameter is specified, remove all present latency histograms # for the device. Otherwise, add/reset some of (or all) latency histograms. # -# @device: device name to set latency histogram for. +# @device: device name to set latency histogram for (better use @id). +# +# @id: The name or QOM path of the guest device. # # @boundaries: list of interval boundary values (see description in # BlockLatencyHistogramInfo definition). If specified, all @@ -547,7 +549,7 @@ # <- { "return": {} } ## { 'command': 'x-block-latency-histogram-set', - 'data': {'device': 'str', + 'data': {'*device': 'str', '*id': 'str', '*boundaries': ['uint64'], '*boundaries-read': ['uint64'], '*boundaries-write': ['uint64'], diff --git a/blockdev.c b/blockdev.c index a8755bd908..87f4ab3316 100644 --- a/blockdev.c +++ b/blockdev.c @@ -4368,20 +4368,22 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, } void qmp_x_block_latency_histogram_set( - const char *device, + bool has_device, const char *device, + bool has_id, const char *id, bool has_boundaries, uint64List *boundaries, bool has_boundaries_read, uint64List *boundaries_read, bool has_boundaries_write, uint64List *boundaries_write, bool has_boundaries_flush, uint64List *boundaries_flush, Error **errp) { - BlockBackend *blk = blk_by_name(device); + BlockBackend *blk; BlockAcctStats *stats; + blk = qmp_get_blk(has_device ? device : NULL, has_id ? id : NULL, errp); if (!blk) { - error_setg(errp, "Device '%s' not found", device); return; } + stats = blk_get_stats(blk); if (!has_boundaries && !has_boundaries_read && !has_boundaries_write &&
Support modern way of device selecting. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 6 ++++-- blockdev.c | 8 +++++--- 2 files changed, 9 insertions(+), 5 deletions(-)