This command lets you query the connection status of each pr-manager-helper
object.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/scsi/pr-manager.h | 2 ++
qapi/block.json | 27 +++++++++++++++++++++++
scsi/pr-manager-helper.c | 13 +++++++++++
scsi/pr-manager.c | 45 +++++++++++++++++++++++++++++++++++++++
4 files changed, 87 insertions(+)
diff --git a/include/scsi/pr-manager.h b/include/scsi/pr-manager.h
index 5d2f13a5e4..adadca7954 100644
--- a/include/scsi/pr-manager.h
+++ b/include/scsi/pr-manager.h
@@ -33,8 +33,10 @@ typedef struct PRManagerClass {
/* <public> */
int (*run)(PRManager *pr_mgr, int fd, struct sg_io_hdr *hdr);
+ bool (*is_connected)(PRManager *pr_mgr);
} PRManagerClass;
+bool pr_manager_is_connected(PRManager *pr_mgr);
BlockAIOCB *pr_manager_execute(PRManager *pr_mgr,
AioContext *ctx, int fd,
struct sg_io_hdr *hdr,
diff --git a/qapi/block.json b/qapi/block.json
index c694524002..dc3323c954 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -77,6 +77,33 @@
{ 'struct': 'BlockdevSnapshotInternal',
'data': { 'device': 'str', 'name': 'str' } }
+##
+# @PRManagerInfo:
+#
+# Information about a persistent reservation manager
+#
+# @id: the identifier of the persistent reservation manager
+#
+# @is-connected: whether the persistent reservation manager is connected to
+# the underlying storage or helper
+#
+# Since: 3.0
+##
+{ 'struct': 'PRManagerInfo',
+ 'data': {'id': 'str', 'is-connected': 'bool'} }
+
+##
+# @query-pr-managers:
+#
+# Returns a list of information about each persistent reservation manager.
+#
+# Returns: a list of @PRManagerInfo for each persistent reservation manager
+#
+# Since: 3.0
+##
+{ 'command': 'query-pr-managers', 'returns': ['PRManagerInfo'] }
+
+
##
# @blockdev-snapshot-internal-sync:
#
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index 0c0fe389b7..b11481be9e 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -235,6 +235,18 @@ out:
return ret;
}
+static bool pr_manager_helper_is_connected(PRManager *p)
+{
+ PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
+ bool result;
+
+ qemu_mutex_lock(&pr_mgr->lock);
+ result = (pr_mgr->ioc != NULL);
+ qemu_mutex_unlock(&pr_mgr->lock);
+
+ return result;
+}
+
static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
{
PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
@@ -284,6 +296,7 @@ static void pr_manager_helper_class_init(ObjectClass *klass,
&error_abort);
uc_klass->complete = pr_manager_helper_complete;
prmgr_klass->run = pr_manager_helper_run;
+ prmgr_klass->is_connected = pr_manager_helper_is_connected;
}
static const TypeInfo pr_manager_helper_info = {
diff --git a/scsi/pr-manager.c b/scsi/pr-manager.c
index 87c45db5d4..b6c3056cc3 100644
--- a/scsi/pr-manager.c
+++ b/scsi/pr-manager.c
@@ -17,6 +17,10 @@
#include "block/thread-pool.h"
#include "scsi/pr-manager.h"
#include "trace.h"
+#include "qapi/qapi-types-block.h"
+#include "qapi/qapi-commands-block.h"
+
+#define PR_MANAGER_PATH "/objects"
typedef struct PRManagerData {
PRManager *pr_mgr;
@@ -64,6 +68,14 @@ BlockAIOCB *pr_manager_execute(PRManager *pr_mgr,
data, complete, opaque);
}
+bool pr_manager_is_connected(PRManager *pr_mgr)
+{
+ PRManagerClass *pr_mgr_class =
+ PR_MANAGER_GET_CLASS(pr_mgr);
+
+ return !pr_mgr_class->is_connected || pr_mgr_class->is_connected(pr_mgr);
+}
+
static const TypeInfo pr_manager_info = {
.parent = TYPE_OBJECT,
.name = TYPE_PR_MANAGER,
@@ -105,5 +117,38 @@ pr_manager_register_types(void)
type_register_static(&pr_manager_info);
}
+static int query_one_pr_manager(Object *object, void *opaque)
+{
+ PRManagerInfoList ***prev = opaque;
+ PRManagerInfoList *elem;
+ PRManagerInfo *info;
+ PRManager *pr_mgr;
+
+ pr_mgr = (PRManager *)object_dynamic_cast(object, TYPE_PR_MANAGER);
+ if (!pr_mgr) {
+ return 0;
+ }
+
+ elem = g_new0(PRManagerInfoList, 1);
+ info = g_new0(PRManagerInfo, 1);
+ info->id = object_get_canonical_path_component(object);
+ info->is_connected = pr_manager_is_connected(pr_mgr);
+ elem->value = info;
+ elem->next = NULL;
+
+ **prev = elem;
+ *prev = &elem->next;
+ return 0;
+}
+
+PRManagerInfoList *qmp_query_pr_managers(Error **errp)
+{
+ PRManagerInfoList *head = NULL;
+ PRManagerInfoList **prev = &head;
+ Object *container = container_get(object_get_root(), PR_MANAGER_PATH);
+
+ object_child_foreach(container, query_one_pr_manager, &prev);
+ return head;
+}
type_init(pr_manager_register_types);
--
2.17.1
On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> This command lets you query the connection status of each pr-manager-helper
> object.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/scsi/pr-manager.h | 2 ++
> qapi/block.json | 27 +++++++++++++++++++++++
> scsi/pr-manager-helper.c | 13 +++++++++++
> scsi/pr-manager.c | 45 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 87 insertions(+)
>
> diff --git a/include/scsi/pr-manager.h b/include/scsi/pr-manager.h
> index 5d2f13a5e4..adadca7954 100644
> --- a/include/scsi/pr-manager.h
> +++ b/include/scsi/pr-manager.h
> @@ -33,8 +33,10 @@ typedef struct PRManagerClass {
>
> /* <public> */
> int (*run)(PRManager *pr_mgr, int fd, struct sg_io_hdr *hdr);
> + bool (*is_connected)(PRManager *pr_mgr);
> } PRManagerClass;
>
> +bool pr_manager_is_connected(PRManager *pr_mgr);
> BlockAIOCB *pr_manager_execute(PRManager *pr_mgr,
> AioContext *ctx, int fd,
> struct sg_io_hdr *hdr,
> diff --git a/qapi/block.json b/qapi/block.json
> index c694524002..dc3323c954 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -77,6 +77,33 @@
> { 'struct': 'BlockdevSnapshotInternal',
> 'data': { 'device': 'str', 'name': 'str' } }
>
> +##
> +# @PRManagerInfo:
> +#
> +# Information about a persistent reservation manager
> +#
> +# @id: the identifier of the persistent reservation manager
> +#
> +# @is-connected: whether the persistent reservation manager is connected to
> +# the underlying storage or helper
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'PRManagerInfo',
> + 'data': {'id': 'str', 'is-connected': 'bool'} }
> +
> +##
> +# @query-pr-managers:
> +#
> +# Returns a list of information about each persistent reservation manager.
> +#
> +# Returns: a list of @PRManagerInfo for each persistent reservation manager
> +#
> +# Since: 3.0
> +##
> +{ 'command': 'query-pr-managers', 'returns': ['PRManagerInfo'] }
> +
> +
> ##
> # @blockdev-snapshot-internal-sync:
> #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index 0c0fe389b7..b11481be9e 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -235,6 +235,18 @@ out:
> return ret;
> }
>
> +static bool pr_manager_helper_is_connected(PRManager *p)
> +{
> + PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
> + bool result;
> +
> + qemu_mutex_lock(&pr_mgr->lock);
> + result = (pr_mgr->ioc != NULL);
I worry it is not that easy. pr_mgr->ioc is unset only when there's
PR_IN/PR_OUT command coming from the guest (in pr_manager_helper_run ->
pr_manager_helper_write). In fact, after 5/5 that is also the time when
the event is delivered. But that might be too late for mgmt app to
restart the helper process (although pr_manager_helper_run() tries to
reconnect for 5 seconds before giving up).
The patch is good code-wise though.
Michal
On 26/06/2018 18:31, Michal Privoznik wrote:
>>
>> +static bool pr_manager_helper_is_connected(PRManager *p)
>> +{
>> + PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
>> + bool result;
>> +
>> + qemu_mutex_lock(&pr_mgr->lock);
>> + result = (pr_mgr->ioc != NULL);
> I worry it is not that easy. pr_mgr->ioc is unset only when there's
> PR_IN/PR_OUT command coming from the guest (in pr_manager_helper_run ->
> pr_manager_helper_write). In fact, after 5/5 that is also the time when
> the event is delivered. But that might be too late for mgmt app to
> restart the helper process (although pr_manager_helper_run() tries to
> reconnect for 5 seconds before giving up).
That's true, however the important thing IMO is to have a QMP interface
that libvirt can use; everything else is just quality of implementation.
qemu-pr-helper anyway does something only when a guests sends it a PR
command - and with libvirt's per-guest model, that would (hopefully)
mean that the only case that remains is when someone manually kills the
qemu-pr-helper process. In that case there's a certain amount of PEBKAC
involved... :)
Paolo
> The patch is good code-wise though.
On 06/27/2018 05:44 PM, Paolo Bonzini wrote:
> On 26/06/2018 18:31, Michal Privoznik wrote:
>>>
>>> +static bool pr_manager_helper_is_connected(PRManager *p)
>>> +{
>>> + PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
>>> + bool result;
>>> +
>>> + qemu_mutex_lock(&pr_mgr->lock);
>>> + result = (pr_mgr->ioc != NULL);
>> I worry it is not that easy. pr_mgr->ioc is unset only when there's
>> PR_IN/PR_OUT command coming from the guest (in pr_manager_helper_run ->
>> pr_manager_helper_write). In fact, after 5/5 that is also the time when
>> the event is delivered. But that might be too late for mgmt app to
>> restart the helper process (although pr_manager_helper_run() tries to
>> reconnect for 5 seconds before giving up).
>
> That's true, however the important thing IMO is to have a QMP interface
> that libvirt can use; everything else is just quality of implementation.
>
> qemu-pr-helper anyway does something only when a guests sends it a PR
> command - and with libvirt's per-guest model, that would (hopefully)
> mean that the only case that remains is when someone manually kills the
> qemu-pr-helper process. In that case there's a certain amount of PEBKAC
> involved... :)
Unless an assert() is triggered ;-)
But since you merged my suggested changes in 5/5 libvirt can catch the
event pretty soon, so in my testing qemu was still left with 3-4
connection retries which is plenty.
Michal
On 28/06/2018 09:05, Michal Prívozník wrote: >> qemu-pr-helper anyway does something only when a guests sends it a PR >> command - and with libvirt's per-guest model, that would (hopefully) >> mean that the only case that remains is when someone manually kills the >> qemu-pr-helper process. In that case there's a certain amount of PEBKAC >> involved... :) > > Unless an assert() is triggered ;-) Indeed, but that should still happen "when a guest sends it a PR command". The problematic case would only arise if qemu-pr-helper crashes between the last time QEMU sent a command, and the time libvirt sends query-pr-managers. > But since you merged my suggested changes in 5/5 libvirt can catch the > event pretty soon, so in my testing qemu was still left with 3-4 > connection retries which is plenty. Good, thanks. Paolo
On 06/26/2018 10:40 AM, Paolo Bonzini wrote:
> This command lets you query the connection status of each pr-manager-helper
> object.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> +++ b/qapi/block.json
> @@ -77,6 +77,33 @@
> { 'struct': 'BlockdevSnapshotInternal',
> 'data': { 'device': 'str', 'name': 'str' } }
>
> +##
> +# @PRManagerInfo:
> +#
> +# Information about a persistent reservation manager
> +#
> +# @id: the identifier of the persistent reservation manager
> +#
> +# @is-connected: whether the persistent reservation manager is connected to
> +# the underlying storage or helper
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'PRManagerInfo',
> + 'data': {'id': 'str', 'is-connected': 'bool'} }
Bike-shedding: I think 'connected' is a reasonable (and shorter) name
for this member
> +
> +##
> +# @query-pr-managers:
> +#
> +# Returns a list of information about each persistent reservation manager.
> +#
> +# Returns: a list of @PRManagerInfo for each persistent reservation manager
> +#
> +# Since: 3.0
> +##
> +{ 'command': 'query-pr-managers', 'returns': ['PRManagerInfo'] }
> +
As a query command, does it make sense to consider whether this command
could be provided during preconfig?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 26/06/2018 22:51, Eric Blake wrote:
> On 06/26/2018 10:40 AM, Paolo Bonzini wrote:
>> This command lets you query the connection status of each
>> pr-manager-helper
>> object.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>
>> +++ b/qapi/block.json
>> @@ -77,6 +77,33 @@
>> { 'struct': 'BlockdevSnapshotInternal',
>> 'data': { 'device': 'str', 'name': 'str' } }
>> +##
>> +# @PRManagerInfo:
>> +#
>> +# Information about a persistent reservation manager
>> +#
>> +# @id: the identifier of the persistent reservation manager
>> +#
>> +# @is-connected: whether the persistent reservation manager is
>> connected to
>> +# the underlying storage or helper
>> +#
>> +# Since: 3.0
>> +##
>> +{ 'struct': 'PRManagerInfo',
>> + 'data': {'id': 'str', 'is-connected': 'bool'} }
>
> Bike-shedding: I think 'connected' is a reasonable (and shorter) name
> for this member
Sounds good.
>> +
>> +##
>> +# @query-pr-managers:
>> +#
>> +# Returns a list of information about each persistent reservation
>> manager.
>> +#
>> +# Returns: a list of @PRManagerInfo for each persistent reservation
>> manager
>> +#
>> +# Since: 3.0
>> +##
>> +{ 'command': 'query-pr-managers', 'returns': ['PRManagerInfo'] }
>> +
>
> As a query command, does it make sense to consider whether this command
> could be provided during preconfig?
Yes, definitely.
Paolo
© 2016 - 2025 Red Hat, Inc.