* [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0
@ 2018-06-26 15:40 Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help Paolo Bonzini
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-26 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: mprivozn
The first three patches are more or less trivial bugfixes. The
last two allow libvirt to know the status of the qemu-pr-helper
process that is attached to a running QEMU instance.
Michal, can you check that the interface in the last two patches
is good for you?
Thanks,
Paolo
Paolo Bonzini (5):
pr-helper: fix --socket-path default in help
pr-helper: fix assertion failure on failed multipath PERSISTENT
RESERVE IN
pr-manager-helper: avoid SIGSEGV when writing to the socket fail
pr-manager: add query-pr-managers QMP command
pr-manager-helper: report event on connection/disconnection
include/scsi/pr-manager.h | 2 ++
qapi/block.json | 51 +++++++++++++++++++++++++++++++++++++++
scsi/pr-manager-helper.c | 39 +++++++++++++++++++++++++-----
scsi/pr-manager.c | 45 ++++++++++++++++++++++++++++++++++
scsi/qemu-pr-helper.c | 21 ++++++++++------
5 files changed, 144 insertions(+), 14 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help
2018-06-26 15:40 [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0 Paolo Bonzini
@ 2018-06-26 15:40 ` Paolo Bonzini
2018-06-26 16:13 ` Philippe Mathieu-Daudé
2018-06-26 16:24 ` Michal Privoznik
2018-06-26 15:40 ` [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN Paolo Bonzini
` (3 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-26 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: mprivozn
Currently --help shows "(default '(null)')" for the -k/--socket-path
option. Fix it by getting the default path in /var/run.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scsi/qemu-pr-helper.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index d0f83176e1..4057cf355c 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -74,8 +74,16 @@ static int uid = -1;
static int gid = -1;
#endif
+static void compute_default_paths(void)
+{
+ if (!socket_path) {
+ socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
+ }
+}
+
static void usage(const char *name)
{
+ compute_default_paths();
(printf) (
"Usage: %s [OPTIONS] FILE\n"
"Persistent Reservation helper program for QEMU\n"
@@ -845,13 +853,6 @@ static const char *socket_activation_validate_opts(void)
return NULL;
}
-static void compute_default_paths(void)
-{
- if (!socket_path) {
- socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
- }
-}
-
static void termsig_handler(int signum)
{
atomic_cmpxchg(&state, RUNNING, TERMINATE);
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN
2018-06-26 15:40 [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0 Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help Paolo Bonzini
@ 2018-06-26 15:40 ` Paolo Bonzini
2018-06-26 16:16 ` Philippe Mathieu-Daudé
2018-06-26 16:25 ` Michal Privoznik
2018-06-26 15:40 ` [Qemu-devel] [PATCH 3/5] pr-manager-helper: avoid SIGSEGV when writing to the socket fail Paolo Bonzini
` (2 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-26 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: mprivozn
The response size is expected to be zero if the SCSI status is not
"GOOD", but nothing was resetting it.
This can be reproduced simply by "sg_persist -s /dev/sdb" where /dev/sdb
in the guest is a scsi-block device corresponding to a multipath device
on the host.
Before:
PR in (Read full status): Aborted command
and on the host:
prh_write_response: Assertion `resp->sz == 0' failed.
After:
PR in (Read full status): bad field in cdb or parameter list
(perhaps unsupported service action)
Reported-by: Jiri Belka <jbelka@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scsi/qemu-pr-helper.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 4057cf355c..0218d65bbf 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -558,7 +558,11 @@ static int do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
#ifdef CONFIG_MPATH
if (is_mpath(fd)) {
/* multipath_pr_in fills the whole input buffer. */
- return multipath_pr_in(fd, cdb, sense, data, *resp_sz);
+ int r = multipath_pr_in(fd, cdb, sense, data, *resp_sz);
+ if (r != GOOD) {
+ *resp_sz = 0;
+ }
+ return r;
}
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/5] pr-manager-helper: avoid SIGSEGV when writing to the socket fail
2018-06-26 15:40 [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0 Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN Paolo Bonzini
@ 2018-06-26 15:40 ` Paolo Bonzini
2018-06-26 16:24 ` Michal Privoznik
2018-06-26 15:40 ` [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection Paolo Bonzini
4 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-26 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: mprivozn
When writing to the qemu-pr-helper socket failed, the persistent
reservation manager was correctly disconnecting the socket, but it
did not clear pr_mgr->ioc. So the rest of the code did not know
that the socket had been disconnected, accessed pr_mgr->ioc and
happily caused a crash.
To reproduce, it is enough to stop qemu-pr-helper between QEMU
startup and executing e.g. sg_persist -k /dev/sdb.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scsi/pr-manager-helper.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index 82ff6b6123..0c0fe389b7 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -71,6 +71,7 @@ static int pr_manager_helper_write(PRManagerHelper *pr_mgr,
if (n_written <= 0) {
assert(n_written != QIO_CHANNEL_ERR_BLOCK);
object_unref(OBJECT(pr_mgr->ioc));
+ pr_mgr->ioc = NULL;
return n_written < 0 ? -EINVAL : 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-26 15:40 [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0 Paolo Bonzini
` (2 preceding siblings ...)
2018-06-26 15:40 ` [Qemu-devel] [PATCH 3/5] pr-manager-helper: avoid SIGSEGV when writing to the socket fail Paolo Bonzini
@ 2018-06-26 15:40 ` Paolo Bonzini
2018-06-26 16:31 ` Michal Privoznik
2018-06-26 20:51 ` Eric Blake
2018-06-26 15:40 ` [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection Paolo Bonzini
4 siblings, 2 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-26 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: mprivozn
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection
2018-06-26 15:40 [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0 Paolo Bonzini
` (3 preceding siblings ...)
2018-06-26 15:40 ` [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command Paolo Bonzini
@ 2018-06-26 15:40 ` Paolo Bonzini
2018-06-26 16:23 ` Philippe Mathieu-Daudé
` (2 more replies)
4 siblings, 3 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-26 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: mprivozn
Let management know if there were any problems communicating with
qemu-pr-helper. The event is edge-triggered, and is sent every time
the connection status of the pr-manager-helper object changes.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qapi/block.json | 24 ++++++++++++++++++++++++
scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
2 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/qapi/block.json b/qapi/block.json
index dc3323c954..1882a8d107 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -334,6 +334,30 @@
{ 'event': 'DEVICE_TRAY_MOVED',
'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
+##
+# @PR_MANAGER_STATUS_CHANGED:
+#
+# Emitted whenever the connected status of a persistent reservation
+# manager changes.
+#
+# @id: The QOM path of the PR manager object
+#
+# @connected: true if the PR manager is connected to a backend
+#
+# Since: 2.12
+#
+# Example:
+#
+# <- { "event": "PR_MANAGER_STATUS_CHANGED",
+# "data": { "id": "/object/pr-helper0",
+# "connected": true
+# },
+# "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
+#
+##
+{ 'event': 'PR_MANAGER_STATUS_CHANGED',
+ 'data': { 'id': 'str', 'connected': 'bool' } }
+
##
# @QuorumOpType:
#
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index b11481be9e..6643caf4cf 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -17,6 +17,7 @@
#include "io/channel.h"
#include "io/channel-socket.h"
#include "pr-helper.h"
+#include "qapi/qapi-events-block.h"
#include <scsi/sg.h>
@@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
PRManager parent;
char *path;
+ char *qom_path;
QemuMutex lock;
QIOChannel *ioc;
@@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
goto out_close;
}
+ qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
+ &error_abort);
return 0;
out_close:
@@ -142,9 +146,11 @@ static int pr_manager_helper_run(PRManager *p,
uint32_t len;
PRHelperResponse resp;
+ int sense_len;
int ret;
int expected_dir;
int attempts;
+ bool was_connected = pr_mgr->ioc != NULL;
uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
@@ -222,15 +228,19 @@ static int pr_manager_helper_run(PRManager *p,
io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, PR_HELPER_SENSE_SIZE);
memcpy(io_hdr->sbp, resp.sense, io_hdr->sb_len_wr);
}
+ qemu_mutex_unlock(&pr_mgr->lock);
+ return ret;
out:
- if (ret < 0) {
- int sense_len = scsi_build_sense(io_hdr->sbp,
- SENSE_CODE(LUN_COMM_FAILURE));
- io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
- io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
- io_hdr->status = CHECK_CONDITION;
+ if (was_connected) {
+ qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
+ &error_abort);
}
+
+ sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
+ io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
+ io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
+ io_hdr->status = CHECK_CONDITION;
qemu_mutex_unlock(&pr_mgr->lock);
return ret;
}
@@ -251,6 +261,8 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
{
PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
+ pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
+
qemu_mutex_lock(&pr_mgr->lock);
pr_manager_helper_initialize(pr_mgr, errp);
qemu_mutex_unlock(&pr_mgr->lock);
@@ -276,6 +288,7 @@ static void pr_manager_helper_instance_finalize(Object *obj)
PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(obj);
object_unref(OBJECT(pr_mgr->ioc));
+ g_free(pr_mgr->qom_path);
qemu_mutex_destroy(&pr_mgr->lock);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help
2018-06-26 15:40 ` [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help Paolo Bonzini
@ 2018-06-26 16:13 ` Philippe Mathieu-Daudé
2018-06-26 16:24 ` Michal Privoznik
1 sibling, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-26 16:13 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: mprivozn
On 06/26/2018 12:40 PM, Paolo Bonzini wrote:
> Currently --help shows "(default '(null)')" for the -k/--socket-path
> option. Fix it by getting the default path in /var/run.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> scsi/qemu-pr-helper.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index d0f83176e1..4057cf355c 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -74,8 +74,16 @@ static int uid = -1;
> static int gid = -1;
> #endif
>
> +static void compute_default_paths(void)
> +{
> + if (!socket_path) {
> + socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> + }
> +}
> +
> static void usage(const char *name)
> {
> + compute_default_paths();
> (printf) (
> "Usage: %s [OPTIONS] FILE\n"
> "Persistent Reservation helper program for QEMU\n"
> @@ -845,13 +853,6 @@ static const char *socket_activation_validate_opts(void)
> return NULL;
> }
>
> -static void compute_default_paths(void)
> -{
> - if (!socket_path) {
> - socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> - }
> -}
> -
> static void termsig_handler(int signum)
> {
> atomic_cmpxchg(&state, RUNNING, TERMINATE);
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN
2018-06-26 15:40 ` [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN Paolo Bonzini
@ 2018-06-26 16:16 ` Philippe Mathieu-Daudé
2018-06-26 16:25 ` Michal Privoznik
1 sibling, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-26 16:16 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: mprivozn
On 06/26/2018 12:40 PM, Paolo Bonzini wrote:
> The response size is expected to be zero if the SCSI status is not
> "GOOD", but nothing was resetting it.
>
> This can be reproduced simply by "sg_persist -s /dev/sdb" where /dev/sdb
> in the guest is a scsi-block device corresponding to a multipath device
> on the host.
>
> Before:
>
> PR in (Read full status): Aborted command
>
> and on the host:
>
> prh_write_response: Assertion `resp->sz == 0' failed.
>
> After:
>
> PR in (Read full status): bad field in cdb or parameter list
> (perhaps unsupported service action)
>
> Reported-by: Jiri Belka <jbelka@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> scsi/qemu-pr-helper.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 4057cf355c..0218d65bbf 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -558,7 +558,11 @@ static int do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
> #ifdef CONFIG_MPATH
> if (is_mpath(fd)) {
> /* multipath_pr_in fills the whole input buffer. */
> - return multipath_pr_in(fd, cdb, sense, data, *resp_sz);
> + int r = multipath_pr_in(fd, cdb, sense, data, *resp_sz);
> + if (r != GOOD) {
> + *resp_sz = 0;
> + }
> + return r;
> }
> #endif
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection
2018-06-26 15:40 ` [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection Paolo Bonzini
@ 2018-06-26 16:23 ` Philippe Mathieu-Daudé
2018-06-27 8:04 ` Michal Privoznik
2018-06-27 10:16 ` Michal Privoznik
2 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-26 16:23 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: mprivozn
Hi Paolo,
On 06/26/2018 12:40 PM, Paolo Bonzini wrote:
> Let management know if there were any problems communicating with
> qemu-pr-helper. The event is edge-triggered, and is sent every time
> the connection status of the pr-manager-helper object changes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qapi/block.json | 24 ++++++++++++++++++++++++
> scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
> 2 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/block.json b/qapi/block.json
> index dc3323c954..1882a8d107 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -334,6 +334,30 @@
> { 'event': 'DEVICE_TRAY_MOVED',
> 'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
>
> +##
> +# @PR_MANAGER_STATUS_CHANGED:
> +#
> +# Emitted whenever the connected status of a persistent reservation
> +# manager changes.
> +#
> +# @id: The QOM path of the PR manager object
> +#
> +# @connected: true if the PR manager is connected to a backend
> +#
> +# Since: 2.12
3.0?
> +#
> +# Example:
> +#
> +# <- { "event": "PR_MANAGER_STATUS_CHANGED",
> +# "data": { "id": "/object/pr-helper0",
> +# "connected": true
> +# },
> +# "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'PR_MANAGER_STATUS_CHANGED',
> + 'data': { 'id': 'str', 'connected': 'bool' } }
> +
> ##
> # @QuorumOpType:
> #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index b11481be9e..6643caf4cf 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -17,6 +17,7 @@
> #include "io/channel.h"
> #include "io/channel-socket.h"
> #include "pr-helper.h"
> +#include "qapi/qapi-events-block.h"
>
> #include <scsi/sg.h>
>
> @@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
> PRManager parent;
>
> char *path;
> + char *qom_path;
>
> QemuMutex lock;
> QIOChannel *ioc;
> @@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
> goto out_close;
> }
>
> + qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
> + &error_abort);
> return 0;
>
> out_close:
> @@ -142,9 +146,11 @@ static int pr_manager_helper_run(PRManager *p,
>
> uint32_t len;
> PRHelperResponse resp;
> + int sense_len;
> int ret;
> int expected_dir;
> int attempts;
> + bool was_connected = pr_mgr->ioc != NULL;
> uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
>
> if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
> @@ -222,15 +228,19 @@ static int pr_manager_helper_run(PRManager *p,
> io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, PR_HELPER_SENSE_SIZE);
> memcpy(io_hdr->sbp, resp.sense, io_hdr->sb_len_wr);
> }
> + qemu_mutex_unlock(&pr_mgr->lock);
> + return ret;
Maybe this big function can be refactored in 2, the second being the
bottom code with the mutex locked.
>
> out:
> - if (ret < 0) {
> - int sense_len = scsi_build_sense(io_hdr->sbp,
> - SENSE_CODE(LUN_COMM_FAILURE));
> - io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> - io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> - io_hdr->status = CHECK_CONDITION;
> + if (was_connected) {
> + qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
> + &error_abort);
> }
> +
> + sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
> + io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> + io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> + io_hdr->status = CHECK_CONDITION;
> qemu_mutex_unlock(&pr_mgr->lock);
> return ret;
> }
> @@ -251,6 +261,8 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
> {
> PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
>
> + pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
> +
> qemu_mutex_lock(&pr_mgr->lock);
> pr_manager_helper_initialize(pr_mgr, errp);
> qemu_mutex_unlock(&pr_mgr->lock);
> @@ -276,6 +288,7 @@ static void pr_manager_helper_instance_finalize(Object *obj)
> PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(obj);
>
> object_unref(OBJECT(pr_mgr->ioc));
> + g_free(pr_mgr->qom_path);
> qemu_mutex_destroy(&pr_mgr->lock);
> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help
2018-06-26 15:40 ` [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help Paolo Bonzini
2018-06-26 16:13 ` Philippe Mathieu-Daudé
@ 2018-06-26 16:24 ` Michal Privoznik
1 sibling, 0 replies; 21+ messages in thread
From: Michal Privoznik @ 2018-06-26 16:24 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> Currently --help shows "(default '(null)')" for the -k/--socket-path
> option. Fix it by getting the default path in /var/run.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> scsi/qemu-pr-helper.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index d0f83176e1..4057cf355c 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -74,8 +74,16 @@ static int uid = -1;
> static int gid = -1;
> #endif
>
> +static void compute_default_paths(void)
> +{
> + if (!socket_path) {
> + socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> + }
> +}
> +
Pre-existing, but pidfile path is computed elsewhere. I'll post a patch
that cleans things up. Not a show stopper for this one though.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] pr-manager-helper: avoid SIGSEGV when writing to the socket fail
2018-06-26 15:40 ` [Qemu-devel] [PATCH 3/5] pr-manager-helper: avoid SIGSEGV when writing to the socket fail Paolo Bonzini
@ 2018-06-26 16:24 ` Michal Privoznik
0 siblings, 0 replies; 21+ messages in thread
From: Michal Privoznik @ 2018-06-26 16:24 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> When writing to the qemu-pr-helper socket failed, the persistent
> reservation manager was correctly disconnecting the socket, but it
> did not clear pr_mgr->ioc. So the rest of the code did not know
> that the socket had been disconnected, accessed pr_mgr->ioc and
> happily caused a crash.
>
> To reproduce, it is enough to stop qemu-pr-helper between QEMU
> startup and executing e.g. sg_persist -k /dev/sdb.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> scsi/pr-manager-helper.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN
2018-06-26 15:40 ` [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN Paolo Bonzini
2018-06-26 16:16 ` Philippe Mathieu-Daudé
@ 2018-06-26 16:25 ` Michal Privoznik
1 sibling, 0 replies; 21+ messages in thread
From: Michal Privoznik @ 2018-06-26 16:25 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> The response size is expected to be zero if the SCSI status is not
> "GOOD", but nothing was resetting it.
>
> This can be reproduced simply by "sg_persist -s /dev/sdb" where /dev/sdb
> in the guest is a scsi-block device corresponding to a multipath device
> on the host.
>
> Before:
>
> PR in (Read full status): Aborted command
>
> and on the host:
>
> prh_write_response: Assertion `resp->sz == 0' failed.
>
> After:
>
> PR in (Read full status): bad field in cdb or parameter list
> (perhaps unsupported service action)
>
> Reported-by: Jiri Belka <jbelka@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> scsi/qemu-pr-helper.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-26 15:40 ` [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command Paolo Bonzini
@ 2018-06-26 16:31 ` Michal Privoznik
2018-06-27 15:44 ` Paolo Bonzini
2018-06-26 20:51 ` Eric Blake
1 sibling, 1 reply; 21+ messages in thread
From: Michal Privoznik @ 2018-06-26 16:31 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-26 15:40 ` [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command Paolo Bonzini
2018-06-26 16:31 ` Michal Privoznik
@ 2018-06-26 20:51 ` Eric Blake
2018-06-27 15:34 ` Paolo Bonzini
1 sibling, 1 reply; 21+ messages in thread
From: Eric Blake @ 2018-06-26 20:51 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: mprivozn
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection
2018-06-26 15:40 ` [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection Paolo Bonzini
2018-06-26 16:23 ` Philippe Mathieu-Daudé
@ 2018-06-27 8:04 ` Michal Privoznik
2018-06-27 10:16 ` Michal Privoznik
2 siblings, 0 replies; 21+ messages in thread
From: Michal Privoznik @ 2018-06-27 8:04 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> Let management know if there were any problems communicating with
> qemu-pr-helper. The event is edge-triggered, and is sent every time
> the connection status of the pr-manager-helper object changes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qapi/block.json | 24 ++++++++++++++++++++++++
> scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
> 2 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/block.json b/qapi/block.json
> index dc3323c954..1882a8d107 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -334,6 +334,30 @@
> { 'event': 'DEVICE_TRAY_MOVED',
> 'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
>
> +##
> +# @PR_MANAGER_STATUS_CHANGED:
> +#
> +# Emitted whenever the connected status of a persistent reservation
> +# manager changes.
> +#
> +# @id: The QOM path of the PR manager object
> +#
> +# @connected: true if the PR manager is connected to a backend
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# <- { "event": "PR_MANAGER_STATUS_CHANGED",
> +# "data": { "id": "/object/pr-helper0",
> +# "connected": true
> +# },
> +# "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'PR_MANAGER_STATUS_CHANGED',
> + 'data': { 'id': 'str', 'connected': 'bool' } }
> +
> ##
> # @QuorumOpType:
> #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index b11481be9e..6643caf4cf 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -17,6 +17,7 @@
> #include "io/channel.h"
> #include "io/channel-socket.h"
> #include "pr-helper.h"
> +#include "qapi/qapi-events-block.h"
>
> #include <scsi/sg.h>
>
> @@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
> PRManager parent;
>
> char *path;
> + char *qom_path;
>
> QemuMutex lock;
> QIOChannel *ioc;
> @@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
> goto out_close;
> }
>
> + qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
> + &error_abort);
> return 0;
>
> out_close:
> @@ -142,9 +146,11 @@ static int pr_manager_helper_run(PRManager *p,
>
> uint32_t len;
> PRHelperResponse resp;
> + int sense_len;
> int ret;
> int expected_dir;
> int attempts;
> + bool was_connected = pr_mgr->ioc != NULL;
> uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
>
> if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
> @@ -222,15 +228,19 @@ static int pr_manager_helper_run(PRManager *p,
> io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, PR_HELPER_SENSE_SIZE);
> memcpy(io_hdr->sbp, resp.sense, io_hdr->sb_len_wr);
> }
> + qemu_mutex_unlock(&pr_mgr->lock);
> + return ret;
>
> out:
> - if (ret < 0) {
> - int sense_len = scsi_build_sense(io_hdr->sbp,
> - SENSE_CODE(LUN_COMM_FAILURE));
> - io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> - io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> - io_hdr->status = CHECK_CONDITION;
> + if (was_connected) {
> + qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
> + &error_abort);
> }
> +
> + sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
> + io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> + io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> + io_hdr->status = CHECK_CONDITION;
> qemu_mutex_unlock(&pr_mgr->lock);
> return ret;
> }
> @@ -251,6 +261,8 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
> {
> PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
>
> + pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
Might be worth returning just object_get_canonical_path_component() so
that the event looks like this:
{"event": "PR_MANAGER_STATUS_CHANGED", "data": {"connected": false,
"id": "pr-helper0"}}
(instead of full /object/pr-helper0 path). This corresponds to what
'query-pr-managers' returns too.
Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection
2018-06-26 15:40 ` [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection Paolo Bonzini
2018-06-26 16:23 ` Philippe Mathieu-Daudé
2018-06-27 8:04 ` Michal Privoznik
@ 2018-06-27 10:16 ` Michal Privoznik
2018-06-27 15:35 ` Paolo Bonzini
2 siblings, 1 reply; 21+ messages in thread
From: Michal Privoznik @ 2018-06-27 10:16 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> Let management know if there were any problems communicating with
> qemu-pr-helper. The event is edge-triggered, and is sent every time
> the connection status of the pr-manager-helper object changes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qapi/block.json | 24 ++++++++++++++++++++++++
> scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
> 2 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/block.json b/qapi/block.json
> index dc3323c954..1882a8d107 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -334,6 +334,30 @@
> { 'event': 'DEVICE_TRAY_MOVED',
> 'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
>
> +##
> +# @PR_MANAGER_STATUS_CHANGED:
> +#
> +# Emitted whenever the connected status of a persistent reservation
> +# manager changes.
> +#
> +# @id: The QOM path of the PR manager object
> +#
> +# @connected: true if the PR manager is connected to a backend
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# <- { "event": "PR_MANAGER_STATUS_CHANGED",
> +# "data": { "id": "/object/pr-helper0",
> +# "connected": true
> +# },
> +# "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'PR_MANAGER_STATUS_CHANGED',
> + 'data': { 'id': 'str', 'connected': 'bool' } }
> +
> ##
> # @QuorumOpType:
> #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index b11481be9e..6643caf4cf 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -17,6 +17,7 @@
> #include "io/channel.h"
> #include "io/channel-socket.h"
> #include "pr-helper.h"
> +#include "qapi/qapi-events-block.h"
>
> #include <scsi/sg.h>
>
> @@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
> PRManager parent;
>
> char *path;
> + char *qom_path;
>
> QemuMutex lock;
> QIOChannel *ioc;
> @@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
> goto out_close;
> }
>
> + qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
> + &error_abort);
> return 0;
Also, can we emit this earlier? For instance at the two places where we
unset pr_mgr->ioc:
diff --git i/scsi/pr-manager-helper.c w/scsi/pr-manager-helper.c
index 6643caf4cf..4ca15dff3f 100644
--- i/scsi/pr-manager-helper.c
+++ w/scsi/pr-manager-helper.c
@@ -48,6 +48,8 @@ static int pr_manager_helper_read(PRManagerHelper *pr_mgr,
if (r < 0) {
object_unref(OBJECT(pr_mgr->ioc));
+ qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
+ &error_abort);
pr_mgr->ioc = NULL;
return -EINVAL;
}
@@ -74,6 +76,8 @@ static int pr_manager_helper_write(PRManagerHelper *pr_mgr,
assert(n_written != QIO_CHANNEL_ERR_BLOCK);
object_unref(OBJECT(pr_mgr->ioc));
pr_mgr->ioc = NULL;
+ qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
+ &error_abort);
return n_written < 0 ? -EINVAL : 0;
}
@@ -150,7 +154,6 @@ static int pr_manager_helper_run(PRManager *p,
int ret;
int expected_dir;
int attempts;
- bool was_connected = pr_mgr->ioc != NULL;
uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
@@ -232,11 +235,6 @@ static int pr_manager_helper_run(PRManager *p,
return ret;
out:
- if (was_connected) {
- qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
- &error_abort);
- }
-
sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
@@ -261,7 +259,7 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
{
PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
- pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
+ pr_mgr->qom_path = object_get_canonical_path_component(OBJECT(pr_mgr));
qemu_mutex_lock(&pr_mgr->lock);
pr_manager_helper_initialize(pr_mgr, errp);
With your approach the event is emitted only after the 5 reconnect
attempts (which makes little sense IMO). With my approach the event is
emitted a bit sooner so that reconnect has at least chance of
succeeding.
I've written some patches for libvirt and with these changes it works
well.
Michal
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-26 20:51 ` Eric Blake
@ 2018-06-27 15:34 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-27 15:34 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: mprivozn
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection
2018-06-27 10:16 ` Michal Privoznik
@ 2018-06-27 15:35 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-27 15:35 UTC (permalink / raw)
To: Michal Privoznik, qemu-devel
On 27/06/2018 12:16, Michal Privoznik wrote:
> Also, can we emit this earlier? For instance at the two places where we
> unset pr_mgr->ioc:
>
> With your approach the event is emitted only after the 5 reconnect
> attempts (which makes little sense IMO). With my approach the event is
> emitted a bit sooner so that reconnect has at least chance of
> succeeding.
Much better, yes. I included your change.
Paolo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-26 16:31 ` Michal Privoznik
@ 2018-06-27 15:44 ` Paolo Bonzini
2018-06-28 7:05 ` Michal Prívozník
0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-27 15:44 UTC (permalink / raw)
To: Michal Privoznik, qemu-devel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-27 15:44 ` Paolo Bonzini
@ 2018-06-28 7:05 ` Michal Prívozník
2018-06-28 7:08 ` Paolo Bonzini
0 siblings, 1 reply; 21+ messages in thread
From: Michal Prívozník @ 2018-06-28 7:05 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command
2018-06-28 7:05 ` Michal Prívozník
@ 2018-06-28 7:08 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2018-06-28 7:08 UTC (permalink / raw)
To: Michal Prívozník, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-06-28 7:08 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 15:40 [Qemu-devel] [PATCH 0/5] pr-manager/qemu-pr-helper patches for QEMU 3.0 Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 1/5] pr-helper: fix --socket-path default in help Paolo Bonzini
2018-06-26 16:13 ` Philippe Mathieu-Daudé
2018-06-26 16:24 ` Michal Privoznik
2018-06-26 15:40 ` [Qemu-devel] [PATCH 2/5] pr-helper: fix assertion failure on failed multipath PERSISTENT RESERVE IN Paolo Bonzini
2018-06-26 16:16 ` Philippe Mathieu-Daudé
2018-06-26 16:25 ` Michal Privoznik
2018-06-26 15:40 ` [Qemu-devel] [PATCH 3/5] pr-manager-helper: avoid SIGSEGV when writing to the socket fail Paolo Bonzini
2018-06-26 16:24 ` Michal Privoznik
2018-06-26 15:40 ` [Qemu-devel] [PATCH 4/5] pr-manager: add query-pr-managers QMP command Paolo Bonzini
2018-06-26 16:31 ` Michal Privoznik
2018-06-27 15:44 ` Paolo Bonzini
2018-06-28 7:05 ` Michal Prívozník
2018-06-28 7:08 ` Paolo Bonzini
2018-06-26 20:51 ` Eric Blake
2018-06-27 15:34 ` Paolo Bonzini
2018-06-26 15:40 ` [Qemu-devel] [PATCH 5/5] pr-manager-helper: report event on connection/disconnection Paolo Bonzini
2018-06-26 16:23 ` Philippe Mathieu-Daudé
2018-06-27 8:04 ` Michal Privoznik
2018-06-27 10:16 ` Michal Privoznik
2018-06-27 15:35 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.