* [PATCH v5 00/21] libmultipath: checkers overhaul
@ 2018-11-02 12:21 Martin Wilck
2018-11-02 12:21 ` [PATCH v5 01/21] libmultipath: fix use of uninitialized memory in write() Martin Wilck
` (21 more replies)
0 siblings, 22 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Hi Christophe,
This is v5 of my "checkers overhaul" series. Changed wrt v4
are 03/22, 11/22, and 21/22. I re-post the whole series to
avoid confusion.
This series starts with a few minor fixes and then attempts
an overhaul of the checker code.
First, there's a block of patches to get rid of the "message"
char array in struct checker, replacing it with an integer.
This topic had been touched in recent discussion between Ben
and myself. You may want to collaps the patches in this block
(03/21-11/21) into one when commiting; compilation errors will
arise if only part of them is a applied.
The next larger block fixes problems with checkers that try
to check unsupported devices. It's an interesting experience
to configure wrong checkers for the existing devices and see
what happens. With these patches, paths won't be falsely
teared down any more in such situations.
The last patch cleans up the checker data structure by splitting
it into a "checker class" and the path "checker instance".
There's more work to do in this area, but this is a start.
----
changes v4->v5
- 03/21: made checker_message() const again, as 22/22 is gone.
Got rid of the static buffer in checker_message() by simply returning
the constant message strings, as suggested by Ben in his review of v4.
This implies printf format changes in callers; changed format in get_state().
- 11/21: Changed printf format in check_path() (see above).
- 21/22: rebased, no actual changes (kept Ben's Reviewed-by).
- 22/22: dropped, obsolete.
Changes v3->v4:
- 03/22: renamed CHECKER_LAST_GENERIC_MSGID -> CHECKER_GENERIC_MSGTABLE_SIZE
(Ben).
- 12/22: rebased on top of changed 03/22.
Changes v2->v3:
- 03/22: fixed one minor issue mentioned by Ben;
reverted the const-ification of checker_message(),
as it will be reverted in 22/22 anyway.
- 13/22: fix clariion checker semantics (Ben).
- 21/22: rebased on top of updated 03/22.
- 22/22: fix thread-safety issue from 03/22 (Ben).
Changes v1->v2:
- 11/22: rebased on top of "various multipath-tools patches" series
Martin Wilck (21):
libmultipath: fix use of uninitialized memory in write()
libmultipath: fix memory leaks from scandir() use
libmultipath/checkers: replace message by msgid
libmultipath/checkers: cciss_tur: use message id
libmultipath/checkers: directio: use message id
libmultipath/checkers: emc_clariion: use message id
libmultipath/checkers: hp_sw: use message id
libmultipath/checkers: rdac: use message id
libmultipath/checkers: readsector0: use message id
libmultipath/checkers: tur: use message id
multipathd: improve checker message logging
libmultipath/checkers: support unsupported paths
libmultipath: clariion checker: leave unsupported paths alone
libmultipath: hp_sw checker: leave unsupported paths alone
libmultipath: rdac checker: leave unsupported paths alone
libmultipath: tur checker: leave unsupported paths alone
libmultipath: pathinfo: don't blank wwid if checker fails
multipathd: check_path: improve logging for "unusable path" case
libmultipath: coalesce_paths: improve logging of orphaned paths
libmultipath: sync_map_state: log failing paths
libmultipath/checkers: cleanup class/instance model
libmultipath/checkers.c | 187 +++++++++++++++++----------
libmultipath/checkers.h | 69 ++++++----
libmultipath/checkers/cciss_tur.c | 13 +-
libmultipath/checkers/directio.c | 29 +++--
libmultipath/checkers/emc_clariion.c | 114 +++++++++++++---
libmultipath/checkers/hp_sw.c | 39 +++---
libmultipath/checkers/rdac.c | 92 +++++++++----
libmultipath/checkers/readsector0.c | 7 +-
libmultipath/checkers/tur.c | 60 +++++----
libmultipath/config.c | 10 +-
libmultipath/configure.c | 10 +-
libmultipath/discovery.c | 11 +-
libmultipath/foreign.c | 5 +-
libmultipath/foreign/nvme.c | 6 +-
libmultipath/print.c | 2 +-
libmultipath/propsel.c | 19 +--
libmultipath/structs_vec.c | 5 +-
libmultipath/sysfs.c | 5 +-
libmultipath/util.c | 9 ++
libmultipath/util.h | 9 ++
multipathd/main.c | 38 ++++--
21 files changed, 497 insertions(+), 242 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 01/21] libmultipath: fix use of uninitialized memory in write()
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 02/21] libmultipath: fix memory leaks from scandir() use Martin Wilck
` (20 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
valgrind complained about this.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/discovery.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 873035e5..3550c3a7 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -661,7 +661,7 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
} else {
snprintf(value, 11, "%u", mpp->fast_io_fail);
if (sysfs_attr_set_value(session_dev, "recovery_tmo",
- value, 11) <= 0) {
+ value, strlen(value)) <= 0) {
condlog(3, "%s: Failed to set recovery_tmo, "
" error %d", pp->dev, errno);
}
@@ -693,7 +693,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
if (mpp->dev_loss) {
snprintf(value, 11, "%u", mpp->dev_loss);
if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
- value, 11) <= 0)
+ value, strlen(value)) <= 0)
condlog(3, "%s: failed to update "
"I_T Nexus loss timeout, error %d",
pp->dev, errno);
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 02/21] libmultipath: fix memory leaks from scandir() use
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
2018-11-02 12:21 ` [PATCH v5 01/21] libmultipath: fix use of uninitialized memory in write() Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 03/21] libmultipath/checkers: replace message by msgid Martin Wilck
` (19 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
scandir() users must not only free the resulting dirent* array,
but also every member. Add a cleanup function, and fix the
existing users of scandir() in libmultipath.
Add a small helper macro for casting function pointers to the
type pthread_cleanup_push() expects.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/config.c | 10 ++++------
libmultipath/foreign.c | 5 ++++-
libmultipath/foreign/nvme.c | 6 +++++-
libmultipath/sysfs.c | 5 ++++-
libmultipath/util.c | 9 +++++++++
libmultipath/util.h | 9 +++++++++
6 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 0aef186a..5af7af58 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -639,17 +639,13 @@ free_config (struct config * conf)
FREE(conf);
}
-static void free_namelist(void *nl)
-{
- free(nl);
-}
-
/* if multipath fails to process the config directory, it should continue,
* with just a warning message */
static void
process_config_dir(struct config *conf, vector keywords, char *dir)
{
struct dirent **namelist;
+ struct scandir_result sr;
int i, n;
char path[LINE_MAX];
int old_hwtable_size;
@@ -669,7 +665,9 @@ process_config_dir(struct config *conf, vector keywords, char *dir)
return;
} else if (n == 0)
return;
- pthread_cleanup_push(free_namelist, namelist);
+ sr.di = namelist;
+ sr.n = n;
+ pthread_cleanup_push_cast(free_scandir_result, &sr);
for (i = 0; i < n; i++) {
if (!strstr(namelist[i]->d_name, ".conf"))
continue;
diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index 80b399ba..48e8d247 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -115,6 +115,7 @@ static int _init_foreign(const char *multipath_dir)
{
char pathbuf[PATH_MAX];
struct dirent **di;
+ struct scandir_result sr;
int r, i;
foreigns = vector_alloc();
@@ -135,7 +136,9 @@ static int _init_foreign(const char *multipath_dir)
return -r;
}
- pthread_cleanup_push(free, di);
+ sr.di = di;
+ sr.n = r;
+ pthread_cleanup_push_cast(free_scandir_result, &sr);
for (i = 0; i < r; i++) {
const char *msg, *fn, *c;
struct foreign *fgn;
diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index 8887a755..c753a747 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -27,6 +27,7 @@
#include <dirent.h>
#include <errno.h>
#include <ctype.h>
+#include "util.h"
#include "vector.h"
#include "generic.h"
#include "foreign.h"
@@ -534,6 +535,7 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
{
char pathbuf[PATH_MAX], realbuf[PATH_MAX];
struct dirent **di = NULL;
+ struct scandir_result sr;
struct udev_device *subsys;
struct nvme_path *path;
int r, i, n;
@@ -568,7 +570,9 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
return;
}
- pthread_cleanup_push(free, di);
+ sr.di = di;
+ sr.n = r;
+ pthread_cleanup_push_cast(free_scandir_result, &sr);
for (i = 0; i < r; i++) {
char *fn = di[i]->d_name;
struct udev_device *ctrl, *udev;
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index b7dacaad..558c8d6a 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -303,6 +303,7 @@ static void close_fd(void *arg)
bool sysfs_is_multipathed(const struct path *pp)
{
char pathbuf[PATH_MAX];
+ struct scandir_result sr;
struct dirent **di;
int n, r, i;
bool found = false;
@@ -323,7 +324,9 @@ bool sysfs_is_multipathed(const struct path *pp)
return false;
}
- pthread_cleanup_push(free, di);
+ sr.di = di;
+ sr.n = r;
+ pthread_cleanup_push_cast(free_scandir_result, &sr);
for (i = 0; i < r && !found; i++) {
long fd;
int nr;
diff --git a/libmultipath/util.c b/libmultipath/util.c
index d08112db..66c47611 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -496,3 +496,12 @@ void set_max_fds(int max_fds)
}
}
}
+
+void free_scandir_result(struct scandir_result *res)
+{
+ int i;
+
+ for (i = 0; i < res->n; i++)
+ FREE(res->di[i]);
+ FREE(res->di);
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index c2462950..a818e29a 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -30,4 +30,13 @@ void set_max_fds(int max_fds);
#define safe_snprintf(var, size, format, args...) \
snprintf(var, size, format, ##args) >= size
+#define pthread_cleanup_push_cast(f, arg) \
+ pthread_cleanup_push(((void (*)(void *))&f), (arg))
+
+struct scandir_result {
+ struct dirent **di;
+ int n;
+};
+void free_scandir_result(struct scandir_result *);
+
#endif /* _UTIL_H */
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 03/21] libmultipath/checkers: replace message by msgid
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
2018-11-02 12:21 ` [PATCH v5 01/21] libmultipath: fix use of uninitialized memory in write() Martin Wilck
2018-11-02 12:21 ` [PATCH v5 02/21] libmultipath: fix memory leaks from scandir() use Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 16:05 ` Benjamin Marzinski
2018-11-02 12:21 ` [PATCH v5 04/21] libmultipath/checkers: cciss_tur: use message id Martin Wilck
` (18 subsequent siblings)
21 siblings, 1 reply; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Replace the character array "message" in struct checker with
a "message ID" field.
The generic checker code defines a couple of standard message IDs
and corresponding messages. Checker-specific message IDs start
at CHECKER_FIRST_MSG. Checkers that implement specific message
IDs must provide a table for converting the IDs into actual log
messages.
This simplifies the checker data structure and the handling of
checker messages in the critical checker code path. It comes at
the cost that only constant message strings are supported. It
turns out that only a single checker log message (in the emc_clariion
checker) was dynamically generated, and the missing information can
be provided with a standard condlog message.
Follow-up patches implement this for the existing checkers.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers.c | 61 +++++++++++++++++++++++++++++++++-------
libmultipath/checkers.h | 43 ++++++++++++++++++++++++----
libmultipath/discovery.c | 4 +--
3 files changed, 90 insertions(+), 18 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 0bacc864..a1b5cb45 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char * name)
if (!c->free)
goto out;
+ c->msgtable_size = 0;
+ c->msgtable = dlsym(c->handle, "libcheck_msgtable");
+
+ if (c->msgtable != NULL) {
+ const char **p;
+
+ for (p = c->msgtable;
+ *p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
+ /* nothing */;
+
+ c->msgtable_size = p - c->msgtable;
+ } else
+ c->msgtable_size = 0;
+ condlog(3, "checker %s: message table size = %d",
+ c->name, c->msgtable_size);
+
done:
c->fd = -1;
c->sync = 1;
@@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
if (!c)
return PATH_WILD;
- c->message[0] = '\0';
+ c->msgid = CHECKER_MSGID_NONE;
if (c->disable) {
- MSG(c, "checker disabled");
+ c->msgid = CHECKER_MSGID_DISABLED;
return PATH_UNCHECKED;
}
if (!strncmp(c->name, NONE, 4))
return path_state;
if (c->fd < 0) {
- MSG(c, "no usable fd");
+ c->msgid = CHECKER_MSGID_NO_FD;
return PATH_WILD;
}
r = c->check(c);
@@ -248,25 +264,48 @@ int checker_selected (struct checker * c)
return (c->check) ? 1 : 0;
}
-char * checker_name (struct checker * c)
+const char *checker_name(const struct checker *c)
{
if (!c)
return NULL;
return c->name;
}
-char * checker_message (struct checker * c)
+static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
+ [CHECKER_MSGID_NONE] = "",
+ [CHECKER_MSGID_DISABLED] = " is disabled",
+ [CHECKER_MSGID_NO_FD] = " has no usable fd",
+ [CHECKER_MSGID_INVALID] = " provided invalid message id",
+ [CHECKER_MSGID_UP] = " reports path is up",
+ [CHECKER_MSGID_DOWN] = " reports path is down",
+ [CHECKER_MSGID_GHOST] = " reports path is ghost",
+};
+
+const char *checker_message(const struct checker *c)
{
- if (!c)
- return NULL;
- return c->message;
+ int id;
+
+ if (!c || c->msgid < 0 ||
+ (c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
+ c->msgid < CHECKER_FIRST_MSGID))
+ goto bad_id;
+
+ if (c->msgid < CHECKER_GENERIC_MSGTABLE_SIZE)
+ return generic_msg[c->msgid];
+
+ id = c->msgid - CHECKER_FIRST_MSGID;
+ if (id < c->cls->msgtable_size)
+ return c->cls->msgtable[id];
+
+bad_id:
+ return generic_msg[CHECKER_MSGID_NONE];
}
void checker_clear_message (struct checker *c)
{
if (!c)
return;
- c->message[0] = '\0';
+ c->msgid = CHECKER_MSGID_NONE;
}
void checker_get (char *multipath_dir, struct checker * dst, char * name)
@@ -288,10 +327,12 @@ void checker_get (char *multipath_dir, struct checker * dst, char * name)
dst->fd = src->fd;
dst->sync = src->sync;
strncpy(dst->name, src->name, CHECKER_NAME_LEN);
- strncpy(dst->message, src->message, CHECKER_MSG_LEN);
+ dst->msgid = CHECKER_MSGID_NONE;
dst->check = src->check;
dst->init = src->init;
dst->free = src->free;
+ dst->msgtable = src->msgtable;
+ dst->msgtable_size = src->msgtable_size;
dst->handle = NULL;
src->refcount++;
}
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 7b18a1ac..8e26f1df 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -97,6 +97,22 @@ enum path_check_state {
#define CHECKER_DEV_LEN 256
#define LIB_CHECKER_NAMELEN 256
+/*
+ * Generic message IDs for use in checkers.
+ */
+enum {
+ CHECKER_MSGID_NONE = 0,
+ CHECKER_MSGID_DISABLED,
+ CHECKER_MSGID_NO_FD,
+ CHECKER_MSGID_INVALID,
+ CHECKER_MSGID_UP,
+ CHECKER_MSGID_DOWN,
+ CHECKER_MSGID_GHOST,
+ CHECKER_GENERIC_MSGTABLE_SIZE,
+ CHECKER_FIRST_MSGID = 100, /* lowest msgid for checkers */
+ CHECKER_MSGTABLE_SIZE = 100, /* max msg table size for checkers */
+};
+
struct checker {
struct list_head node;
void *handle;
@@ -106,7 +122,7 @@ struct checker {
unsigned int timeout;
int disable;
char name[CHECKER_NAME_LEN];
- char message[CHECKER_MSG_LEN]; /* comm with callers */
+ short msgid; /* checker-internal extra status */
void * context; /* store for persistent data */
void ** mpcontext; /* store for persistent data shared
multipath-wide. Use MALLOC if
@@ -114,10 +130,10 @@ struct checker {
int (*check)(struct checker *);
int (*init)(struct checker *); /* to allocate the context */
void (*free)(struct checker *); /* to free the context */
+ const char**msgtable;
+ short msgtable_size;
};
-#define MSG(c, fmt, args...) snprintf((c)->message, CHECKER_MSG_LEN, fmt, ##args);
-
char * checker_state_name (int);
int init_checkers (char *);
void cleanup_checkers (void);
@@ -134,14 +150,29 @@ void checker_enable (struct checker *);
void checker_disable (struct checker *);
int checker_check (struct checker *, int);
int checker_selected (struct checker *);
-char * checker_name (struct checker *);
-char * checker_message (struct checker *);
+const char *checker_name (const struct checker *);
+/*
+ * This returns a string that's best prepended with "$NAME checker",
+ * where $NAME is the return value of checker_name().
+ */
+const char *checker_message(const struct checker *);
void checker_clear_message (struct checker *c);
void checker_get (char *, struct checker *, char *);
-/* Functions exported by path checker dynamic libraries (.so) */
+/* Prototypes for symbols exported by path checker dynamic libraries (.so) */
int libcheck_check(struct checker *);
int libcheck_init(struct checker *);
void libcheck_free(struct checker *);
+/*
+ * msgid => message map.
+ *
+ * It only needs to be provided if the checker defines specific
+ * message IDs.
+ * Message IDs available to checkers start at CHECKER_FIRST_MSG.
+ * The msgtable array is 0-based, i.e. msgtable[0] is the message
+ * for msgid == __CHECKER_FIRST_MSG.
+ * The table ends with a NULL element.
+ */
+extern const char *libcheck_msgtable[];
#endif /* _CHECKERS_H */
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3550c3a7..5e59e273 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1589,8 +1589,8 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
checker_name(c), checker_state_name(state));
if (state != PATH_UP && state != PATH_GHOST &&
strlen(checker_message(c)))
- condlog(3, "%s: checker msg is \"%s\"",
- pp->dev, checker_message(c));
+ condlog(3, "%s: %s checker%s",
+ pp->dev, checker_name(c), checker_message(c));
return state;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 04/21] libmultipath/checkers: cciss_tur: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (2 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 03/21] libmultipath/checkers: replace message by msgid Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 05/21] libmultipath/checkers: directio: " Martin Wilck
` (17 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/cciss_tur.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/libmultipath/checkers/cciss_tur.c b/libmultipath/checkers/cciss_tur.c
index 1cab2015..ea843742 100644
--- a/libmultipath/checkers/cciss_tur.c
+++ b/libmultipath/checkers/cciss_tur.c
@@ -42,9 +42,6 @@
#define TUR_CMD_LEN 6
#define HEAVY_CHECK_COUNT 10
-#define MSG_CCISS_TUR_UP "cciss_tur checker reports path is up"
-#define MSG_CCISS_TUR_DOWN "cciss_tur checker reports path is down"
-
struct cciss_tur_checker_context {
void * dummy;
};
@@ -69,7 +66,7 @@ int libcheck_check(struct checker * c)
IOCTL_Command_struct cic; // cciss ioctl command
if ((c->fd) < 0) {
- MSG(c,"no usable fd");
+ c->msgid = CHECKER_MSGID_NO_FD;
ret = -1;
goto out;
}
@@ -79,7 +76,7 @@ int libcheck_check(struct checker * c)
perror("Error: ");
fprintf(stderr, "cciss TUR failed in CCISS_GETLUNINFO: %s\n",
strerror(errno));
- MSG(c,MSG_CCISS_TUR_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
ret = PATH_DOWN;
goto out;
} else {
@@ -106,18 +103,18 @@ int libcheck_check(struct checker * c)
if (rc < 0) {
fprintf(stderr, "cciss TUR failed: %s\n",
strerror(errno));
- MSG(c,MSG_CCISS_TUR_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
ret = PATH_DOWN;
goto out;
}
if ((cic.error_info.CommandStatus | cic.error_info.ScsiStatus )) {
- MSG(c,MSG_CCISS_TUR_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
ret = PATH_DOWN;
goto out;
}
- MSG(c,MSG_CCISS_TUR_UP);
+ c->msgid = CHECKER_MSGID_UP;
ret = PATH_UP;
out:
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 05/21] libmultipath/checkers: directio: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (3 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 04/21] libmultipath/checkers: cciss_tur: use message id Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 06/21] libmultipath/checkers: emc_clariion: " Martin Wilck
` (16 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/directio.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index a80848d4..c4a0712e 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -18,10 +18,19 @@
#include "checkers.h"
#include "../libmultipath/debug.h"
-#define MSG_DIRECTIO_UNKNOWN "directio checker is not available"
-#define MSG_DIRECTIO_UP "directio checker reports path is up"
-#define MSG_DIRECTIO_DOWN "directio checker reports path is down"
-#define MSG_DIRECTIO_PENDING "directio checker is waiting on aio"
+enum {
+ MSG_DIRECTIO_UNKNOWN = CHECKER_FIRST_MSGID,
+ MSG_DIRECTIO_PENDING,
+ MSG_DIRECTIO_BLOCKSIZE,
+};
+
+#define _IDX(x) (MSG_DIRECTIO_##x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(UNKNOWN)] = " is not available",
+ [_IDX(PENDING)] = " is waiting on aio",
+ [_IDX(BLOCKSIZE)] = " cannot get blocksize, set default",
+ NULL,
+};
#define LOG(prio, fmt, args...) condlog(prio, "directio: " fmt, ##args)
@@ -54,7 +63,7 @@ int libcheck_init (struct checker * c)
}
if (ioctl(c->fd, BLKBSZGET, &ct->blksize) < 0) {
- MSG(c, "cannot get blocksize, set default");
+ c->msgid = MSG_DIRECTIO_BLOCKSIZE;
ct->blksize = 512;
}
if (ct->blksize > 4096) {
@@ -198,16 +207,16 @@ int libcheck_check (struct checker * c)
switch (ret)
{
case PATH_UNCHECKED:
- MSG(c, MSG_DIRECTIO_UNKNOWN);
+ c->msgid = MSG_DIRECTIO_UNKNOWN;
break;
case PATH_DOWN:
- MSG(c, MSG_DIRECTIO_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
break;
case PATH_UP:
- MSG(c, MSG_DIRECTIO_UP);
+ c->msgid = CHECKER_MSGID_UP;
break;
case PATH_PENDING:
- MSG(c, MSG_DIRECTIO_PENDING);
+ c->msgid = MSG_DIRECTIO_PENDING;
break;
default:
break;
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 06/21] libmultipath/checkers: emc_clariion: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (4 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 05/21] libmultipath/checkers: directio: " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 07/21] libmultipath/checkers: hp_sw: " Martin Wilck
` (15 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
emc_clariion is the only path checker that was using a non-constant
message ("read error" case). This isn't possible with the msgid
approach any more. Use condlog() for the dynamic log message and
simply report "read error" as checker message.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/emc_clariion.c | 60 +++++++++++++++++++---------
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index 9115b1b9..f8e55b93 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -41,6 +41,35 @@
((struct emc_clariion_checker_LU_context *)\
(*c->mpcontext))->inactive_snap = 0
+enum {
+ MSG_CLARIION_QUERY_FAILED = CHECKER_FIRST_MSGID,
+ MSG_CLARIION_QUERY_ERROR,
+ MSG_CLARIION_PATH_CONFIG,
+ MSG_CLARIION_UNIT_REPORT,
+ MSG_CLARIION_PATH_NOT_AVAIL,
+ MSG_CLARIION_LUN_UNBOUND,
+ MSG_CLARIION_WWN_CHANGED,
+ MSG_CLARIION_READ_ERROR,
+ MSG_CLARIION_PASSIVE_GOOD,
+};
+
+#define _IDX(x) (MSG_CLARIION_ ## x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(QUERY_FAILED)] = ": sending query command failed",
+ [_IDX(QUERY_ERROR)] = ": query command indicates error",
+ [_IDX(PATH_CONFIG)] =
+ ": Path not correctly configured for failover",
+ [_IDX(UNIT_REPORT)] =
+ ": Path unit report page in unknown format",
+ [_IDX(PATH_NOT_AVAIL)] =
+ ": Path not available for normal operations",
+ [_IDX(LUN_UNBOUND)] = ": Logical Unit is unbound or LUNZ",
+ [_IDX(WWN_CHANGED)] = ": Logical Unit WWN has changed",
+ [_IDX(READ_ERROR)] = ": Read error",
+ [_IDX(PASSIVE_GOOD)] = ": Active path is healthy",
+ NULL,
+};
+
struct emc_clariion_checker_path_context {
char wwn[16];
unsigned wwn_set;
@@ -116,17 +145,16 @@ int libcheck_check (struct checker * c)
io_hdr.timeout = c->timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
- MSG(c, "emc_clariion_checker: sending query command failed");
+ c->msgid = MSG_CLARIION_QUERY_FAILED;
return PATH_DOWN;
}
if (io_hdr.info & SG_INFO_OK_MASK) {
- MSG(c, "emc_clariion_checker: query command indicates error");
+ c->msgid = MSG_CLARIION_QUERY_ERROR;
return PATH_DOWN;
}
if (/* Verify the code page - right page & revision */
sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
- MSG(c, "emc_clariion_checker: Path unit report page in "
- "unknown format");
+ c->msgid = MSG_CLARIION_UNIT_REPORT;
return PATH_DOWN;
}
@@ -140,22 +168,19 @@ int libcheck_check (struct checker * c)
((sense_buffer[28] & 0x07) != 0x06))
/* Arraycommpath should be set to 1 */
|| (sense_buffer[30] & 0x04) != 0x04) {
- MSG(c, "emc_clariion_checker: Path not correctly configured "
- "for failover");
+ c->msgid = MSG_CLARIION_PATH_CONFIG;
return PATH_DOWN;
}
if ( /* LUN operations should indicate normal operations */
sense_buffer[48] != 0x00) {
- MSG(c, "emc_clariion_checker: Path not available for normal "
- "operations");
+ c->msgid = MSG_CLARIION_PATH_NOT_AVAIL;
return PATH_SHAKY;
}
if ( /* LUN should at least be bound somewhere and not be LUNZ */
sense_buffer[4] == 0x00) {
- MSG(c, "emc_clariion_checker: Logical Unit is unbound "
- "or LUNZ");
+ c->msgid = MSG_CLARIION_LUN_UNBOUND;
return PATH_DOWN;
}
@@ -166,8 +191,7 @@ int libcheck_check (struct checker * c)
*/
if (ct->wwn_set) {
if (memcmp(ct->wwn, &sense_buffer[10], 16) != 0) {
- MSG(c, "emc_clariion_checker: Logical Unit WWN "
- "has changed!");
+ c->msgid = MSG_CLARIION_WWN_CHANGED;
return PATH_DOWN;
}
} else {
@@ -202,14 +226,15 @@ int libcheck_check (struct checker * c)
condlog(3, "emc_clariion_checker: Active "
"path to inactive snapshot WWN %s.",
wwnstr);
- } else
- MSG(c, "emc_clariion_checker: Read "
+ } else {
+ condlog(3, "emc_clariion_checker: Read "
"error for WWN %s. Sense data are "
"0x%x/0x%x/0x%x.", wwnstr,
sbb[2]&0xf, sbb[12], sbb[13]);
+ c->msgid = MSG_CLARIION_READ_ERROR;
+ }
} else {
- MSG(c, "emc_clariion_checker: Active path is "
- "healthy.");
+ c->msgid = MSG_CLARIION_PASSIVE_GOOD;
/*
* Remove the path from the set of paths to inactive
* snapshot LUs if it was in this list since the
@@ -225,8 +250,7 @@ int libcheck_check (struct checker * c)
wwnstr);
ret = PATH_DOWN;
} else {
- MSG(c,
- "emc_clariion_checker: Passive path is healthy.");
+ c->msgid = MSG_CLARIION_PASSIVE_GOOD;
ret = PATH_UP; /* not ghost */
}
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 07/21] libmultipath/checkers: hp_sw: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (5 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 06/21] libmultipath/checkers: emc_clariion: " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 08/21] libmultipath/checkers: rdac: " Martin Wilck
` (14 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/hp_sw.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index 0ad34a6b..d7f1018c 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -27,10 +27,6 @@
#define MX_ALLOC_LEN 255
#define HEAVY_CHECK_COUNT 10
-#define MSG_HP_SW_UP "hp_sw checker reports path is up"
-#define MSG_HP_SW_DOWN "hp_sw checker reports path is down"
-#define MSG_HP_SW_GHOST "hp_sw checker reports path is ghost"
-
struct sw_checker_context {
void * dummy;
};
@@ -128,14 +124,14 @@ int libcheck_check(struct checker * c)
char buff[MX_ALLOC_LEN];
if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
- MSG(c, MSG_HP_SW_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
- }
+ };
if (do_tur(c->fd, c->timeout)) {
- MSG(c, MSG_HP_SW_GHOST);
+ c->msgid = CHECKER_MSGID_GHOST;
return PATH_GHOST;
}
- MSG(c, MSG_HP_SW_UP);
+ c->msgid = CHECKER_MSGID_UP;
return PATH_UP;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 08/21] libmultipath/checkers: rdac: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (6 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 07/21] libmultipath/checkers: hp_sw: " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 09/21] libmultipath/checkers: readsector0: " Martin Wilck
` (13 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/rdac.c | 64 +++++++++++++++++++++++++-----------
1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 5104e4e5..266f8e10 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -31,9 +31,7 @@
#define CURRENT_PAGE_CODE_VALUES 0
#define CHANGEABLE_PAGE_CODE_VALUES 1
-#define MSG_RDAC_UP "rdac checker reports path is up"
-#define MSG_RDAC_DOWN "rdac checker reports path is down"
-#define MSG_RDAC_GHOST "rdac checker reports path is ghost"
+#define MSG_RDAC_DOWN " reports path is down"
#define MSG_RDAC_DOWN_TYPE(STR) MSG_RDAC_DOWN": "STR
#define RTPG_UNAVAILABLE 0x3
@@ -219,41 +217,69 @@ struct volume_access_inq
char dontcare1[34];
};
-const char
-*checker_msg_string(struct volume_access_inq *inq)
+enum {
+ RDAC_MSGID_NOT_CONN = CHECKER_FIRST_MSGID,
+ RDAC_MSGID_IN_STARTUP,
+ RDAC_MSGID_NON_RESPONSIVE,
+ RDAC_MSGID_IN_RESET,
+ RDAC_MSGID_FW_DOWNLOADING,
+ RDAC_MSGID_QUIESCED,
+ RDAC_MSGID_SERVICE_MODE,
+ RDAC_MSGID_UNAVAILABLE,
+ RDAC_MSGID_INQUIRY_FAILED,
+};
+
+#define _IDX(x) (RDAC_MSGID_##x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(NOT_CONN)] = MSG_RDAC_DOWN_TYPE("lun not connected"),
+ [_IDX(IN_STARTUP)] = MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence"),
+ [_IDX(NON_RESPONSIVE)] =
+ MSG_RDAC_DOWN_TYPE("non-responsive to queries"),
+ [_IDX(IN_RESET)] = MSG_RDAC_DOWN_TYPE("ctlr held in reset"),
+ [_IDX(FW_DOWNLOADING)] =
+ MSG_RDAC_DOWN_TYPE("ctlr firmware downloading"),
+ [_IDX(QUIESCED)] = MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request"),
+ [_IDX(SERVICE_MODE)] = MSG_RDAC_DOWN_TYPE("ctlr is in service mode"),
+ [_IDX(UNAVAILABLE)] = MSG_RDAC_DOWN_TYPE("ctlr is unavailable"),
+ [_IDX(INQUIRY_FAILED)] = MSG_RDAC_DOWN_TYPE("inquiry failed"),
+ NULL,
+};
+
+static int
+checker_msg_string(const struct volume_access_inq *inq)
{
/* lun not connected */
if (((inq->PQ_PDT & 0xE0) == 0x20) || (inq->PQ_PDT & 0x7f))
- return MSG_RDAC_DOWN_TYPE("lun not connected");
+ return RDAC_MSGID_NOT_CONN;
/* if no tpg data is available, give the generic path down message */
if (!(inq->avtcvp & 0x10))
- return MSG_RDAC_DOWN;
+ return CHECKER_MSGID_DOWN;
/* controller is booting up */
if (((inq->aas_cur & 0x0F) == RTPG_TRANSITIONING) &&
(inq->aas_alt & 0x0F) != RTPG_TRANSITIONING)
- return MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence");
+ return RDAC_MSGID_IN_STARTUP;
/* if not unavailable, give generic message */
if ((inq->aas_cur & 0x0F) != RTPG_UNAVAILABLE)
- return MSG_RDAC_DOWN;
+ return CHECKER_MSGID_DOWN;
/* target port group unavailable */
switch (inq->vendor_specific_cur) {
case RTPG_UNAVAIL_NON_RESPONSIVE:
- return MSG_RDAC_DOWN_TYPE("non-responsive to queries");
+ return RDAC_MSGID_NON_RESPONSIVE;
case RTPG_UNAVAIL_IN_RESET:
- return MSG_RDAC_DOWN_TYPE("ctlr held in reset");
+ return RDAC_MSGID_IN_RESET;
case RTPG_UNAVAIL_CFW_DL1:
case RTPG_UNAVAIL_CFW_DL2:
- return MSG_RDAC_DOWN_TYPE("ctlr firmware downloading");
+ return RDAC_MSGID_FW_DOWNLOADING;
case RTPG_UNAVAIL_QUIESCED:
- return MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request");
+ return RDAC_MSGID_QUIESCED;
case RTPG_UNAVAIL_SERVICE_MODE:
- return MSG_RDAC_DOWN_TYPE("ctlr is in service mode");
+ return RDAC_MSGID_SERVICE_MODE;
default:
- return MSG_RDAC_DOWN_TYPE("ctlr is unavailable");
+ return RDAC_MSGID_UNAVAILABLE;
}
}
@@ -307,14 +333,14 @@ int libcheck_check(struct checker * c)
done:
switch (ret) {
case PATH_DOWN:
- MSG(c, "%s", (inqfail) ? MSG_RDAC_DOWN_TYPE("inquiry failed") :
- checker_msg_string(&inq));
+ c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
+ checker_msg_string(&inq));
break;
case PATH_UP:
- MSG(c, MSG_RDAC_UP);
+ c->msgid = CHECKER_MSGID_UP;
break;
case PATH_GHOST:
- MSG(c, MSG_RDAC_GHOST);
+ c->msgid = CHECKER_MSGID_GHOST;
break;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 09/21] libmultipath/checkers: readsector0: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (7 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 08/21] libmultipath/checkers: rdac: " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 10/21] libmultipath/checkers: tur: " Martin Wilck
` (12 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/readsector0.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/libmultipath/checkers/readsector0.c b/libmultipath/checkers/readsector0.c
index 1c2a868e..cf79e067 100644
--- a/libmultipath/checkers/readsector0.c
+++ b/libmultipath/checkers/readsector0.c
@@ -6,9 +6,6 @@
#include "checkers.h"
#include "libsg.h"
-#define MSG_READSECTOR0_UP "readsector0 checker reports path is up"
-#define MSG_READSECTOR0_DOWN "readsector0 checker reports path is down"
-
struct readsector0_checker_context {
void * dummy;
};
@@ -35,10 +32,10 @@ int libcheck_check (struct checker * c)
switch (ret)
{
case PATH_DOWN:
- MSG(c, MSG_READSECTOR0_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
break;
case PATH_UP:
- MSG(c, MSG_READSECTOR0_UP);
+ c->msgid = CHECKER_MSGID_UP;
break;
default:
break;
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 10/21] libmultipath/checkers: tur: use message id
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (8 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 09/21] libmultipath/checkers: readsector0: " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 11/21] multipathd: improve checker message logging Martin Wilck
` (11 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/tur.c | 54 ++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a6c88eb2..22734be2 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -29,12 +29,19 @@
#define TUR_CMD_LEN 6
#define HEAVY_CHECK_COUNT 10
-#define MSG_TUR_UP "tur checker reports path is up"
-#define MSG_TUR_DOWN "tur checker reports path is down"
-#define MSG_TUR_GHOST "tur checker reports path is in standby state"
-#define MSG_TUR_RUNNING "tur checker still running"
-#define MSG_TUR_TIMEOUT "tur checker timed out"
-#define MSG_TUR_FAILED "tur checker failed to initialize"
+enum {
+ MSG_TUR_RUNNING = CHECKER_FIRST_MSGID,
+ MSG_TUR_TIMEOUT,
+ MSG_TUR_FAILED,
+};
+
+#define _IDX(x) (MSG_ ## x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(TUR_RUNNING)] = " still running",
+ [_IDX(TUR_TIMEOUT)] = " timed out",
+ [_IDX(TUR_FAILED)] = " failed to initialize",
+ NULL,
+};
struct tur_checker_context {
dev_t devt;
@@ -47,7 +54,7 @@ struct tur_checker_context {
pthread_mutex_t lock;
pthread_cond_t active;
int holders; /* uatomic access only */
- char message[CHECKER_MSG_LEN];
+ int msgid;
};
int libcheck_init (struct checker * c)
@@ -99,7 +106,7 @@ void libcheck_free (struct checker * c)
}
static int
-tur_check(int fd, unsigned int timeout, char *msg)
+tur_check(int fd, unsigned int timeout, short *msgid)
{
struct sg_io_hdr io_hdr;
unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
@@ -118,7 +125,7 @@ retry:
io_hdr.timeout = timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
+ *msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
}
if ((io_hdr.status & 0x7e) == 0x18) {
@@ -126,7 +133,7 @@ retry:
* SCSI-3 arrays might return
* reservation conflict on TUR
*/
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
+ *msgid = CHECKER_MSGID_UP;
return PATH_UP;
}
if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -171,14 +178,14 @@ retry:
* LOGICAL UNIT NOT ACCESSIBLE,
* TARGET PORT IN STANDBY STATE
*/
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_GHOST);
+ *msgid = CHECKER_MSGID_GHOST;
return PATH_GHOST;
}
}
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
+ *msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
}
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
+ *msgid = CHECKER_MSGID_UP;
return PATH_UP;
}
@@ -244,7 +251,7 @@ static void *tur_thread(void *ctx)
{
struct tur_checker_context *ct = ctx;
int state, running;
- char msg[CHECKER_MSG_LEN];
+ short msgid;
/* This thread can be canceled, so setup clean up */
tur_thread_cleanup_push(ct);
@@ -254,13 +261,13 @@ static void *tur_thread(void *ctx)
minor(ct->devt));
tur_deep_sleep(ct);
- state = tur_check(ct->fd, ct->timeout, msg);
+ state = tur_check(ct->fd, ct->timeout, &msgid);
pthread_testcancel();
/* TUR checker done */
pthread_mutex_lock(&ct->lock);
ct->state = state;
- strlcpy(ct->message, msg, sizeof(ct->message));
+ ct->msgid = msgid;
pthread_cond_signal(&ct->active);
pthread_mutex_unlock(&ct->lock);
@@ -313,7 +320,7 @@ int libcheck_check(struct checker * c)
return PATH_UNCHECKED;
if (c->sync)
- return tur_check(c->fd, c->timeout, c->message);
+ return tur_check(c->fd, c->timeout, &c->msgid);
/*
* Async mode
@@ -325,13 +332,12 @@ int libcheck_check(struct checker * c)
pthread_cancel(ct->thread);
condlog(3, "%d:%d : tur checker timeout",
major(ct->devt), minor(ct->devt));
- MSG(c, MSG_TUR_TIMEOUT);
+ c->msgid = MSG_TUR_TIMEOUT;
tur_status = PATH_TIMEOUT;
} else {
pthread_mutex_lock(&ct->lock);
tur_status = ct->state;
- strlcpy(c->message, ct->message,
- sizeof(c->message));
+ c->msgid = ct->msgid;
pthread_mutex_unlock(&ct->lock);
}
ct->thread = 0;
@@ -344,7 +350,7 @@ int libcheck_check(struct checker * c)
ct->thread = 0;
pthread_mutex_lock(&ct->lock);
tur_status = ct->state;
- strlcpy(c->message, ct->message, sizeof(c->message));
+ c->msgid = ct->msgid;
pthread_mutex_unlock(&ct->lock);
}
} else {
@@ -376,7 +382,7 @@ int libcheck_check(struct checker * c)
/* Start new TUR checker */
pthread_mutex_lock(&ct->lock);
tur_status = ct->state = PATH_PENDING;
- ct->message[0] = '\0';
+ ct->msgid = CHECKER_MSGID_NONE;
pthread_mutex_unlock(&ct->lock);
ct->fd = c->fd;
ct->timeout = c->timeout;
@@ -392,7 +398,7 @@ int libcheck_check(struct checker * c)
ct->thread = 0;
condlog(3, "%d:%d : failed to start tur thread, using"
" sync mode", major(ct->devt), minor(ct->devt));
- return tur_check(c->fd, c->timeout, c->message);
+ return tur_check(c->fd, c->timeout, &c->msgid);
}
tur_timeout(&tsp);
pthread_mutex_lock(&ct->lock);
@@ -401,7 +407,7 @@ int libcheck_check(struct checker * c)
&tsp);
if (!r) {
tur_status = ct->state;
- strlcpy(c->message, ct->message, sizeof(c->message));
+ c->msgid = ct->msgid;
}
pthread_mutex_unlock(&ct->lock);
if (tur_status == PATH_PENDING) {
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 11/21] multipathd: improve checker message logging
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (9 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 10/21] libmultipath/checkers: tur: " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 16:54 ` Benjamin Marzinski
2018-11-02 12:21 ` [PATCH v5 12/21] libmultipath/checkers: support unsupported paths Martin Wilck
` (10 subsequent siblings)
21 siblings, 1 reply; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Don't rely on any variables being defined in LOG_MSG.
If message log level is low, don't bother to fetch the message.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index bf5f12a6..2f922db7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -89,12 +89,24 @@ static int use_watchdog;
#define FILE_NAME_SIZE 256
#define CMDSIZE 160
-#define LOG_MSG(a, b) \
-do { \
- if (pp->offline) \
- condlog(a, "%s: %s - path offline", pp->mpp->alias, pp->dev); \
- else if (strlen(b)) \
- condlog(a, "%s: %s - %s", pp->mpp->alias, pp->dev, b); \
+#define LOG_MSG(lvl, verb, pp) \
+do { \
+ if (lvl <= verb) { \
+ if (pp->offline) \
+ condlog(lvl, "%s: %s - path offline", \
+ pp->mpp->alias, pp->dev); \
+ else { \
+ const char *__m = \
+ checker_message(&pp->checker); \
+ \
+ if (strlen(__m)) \
+ condlog(lvl, "%s: %s - %s checker%s", \
+ pp->mpp->alias, \
+ pp->dev, \
+ checker_name(&pp->checker), \
+ __m); \
+ } \
+ } \
} while(0)
struct mpath_event_param
@@ -1811,7 +1823,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
int add_active;
int disable_reinstate = 0;
int oldchkrstate = pp->chkrstate;
- int retrigger_tries, checkint, max_checkint;
+ int retrigger_tries, checkint, max_checkint, verbosity;
struct config *conf;
int ret;
@@ -1828,6 +1840,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
retrigger_tries = conf->retrigger_tries;
checkint = conf->checkint;
max_checkint = conf->max_checkint;
+ verbosity = conf->verbosity;
put_multipath_config(conf);
if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
if (pp->retriggers < retrigger_tries) {
@@ -1970,7 +1983,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
int oldstate = pp->state;
pp->state = newstate;
- LOG_MSG(1, checker_message(&pp->checker));
+ LOG_MSG(1, verbosity, pp);
/*
* upon state change, reset the checkint
@@ -2058,7 +2071,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
return 0;
}
} else {
- LOG_MSG(4, checker_message(&pp->checker));
+ LOG_MSG(4, verbosity, pp);
if (pp->checkint != max_checkint) {
/*
* double the next check delay.
@@ -2088,9 +2101,9 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
log_checker_err = conf->log_checker_err;
put_multipath_config(conf);
if (log_checker_err == LOG_CHKR_ERR_ONCE)
- LOG_MSG(3, checker_message(&pp->checker));
+ LOG_MSG(3, verbosity, pp);
else
- LOG_MSG(2, checker_message(&pp->checker));
+ LOG_MSG(2, verbosity, pp);
}
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 12/21] libmultipath/checkers: support unsupported paths
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (10 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 11/21] multipathd: improve checker message logging Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 13/21] libmultipath: clariion checker: leave unsupported paths alone Martin Wilck
` (9 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
We should be able to distinguish the case where a checker
determines a path to be positively down from the case where
the checker fails to obtain necessary information, e.g.
because of a configuration problem (wrong checker).
Use PATH_WILD for the latter case, as it's hardly used now.
Provide a generic message for the situation that a path
checker can't handle a certain path.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers.c | 1 +
libmultipath/checkers.h | 7 +++++--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index a1b5cb45..90313c35 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -279,6 +279,7 @@ static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
[CHECKER_MSGID_UP] = " reports path is up",
[CHECKER_MSGID_DOWN] = " reports path is down",
[CHECKER_MSGID_GHOST] = " reports path is ghost",
+ [CHECKER_MSGID_UNSUPPORTED] = " doesn't support this device",
};
const char *checker_message(const struct checker *c)
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 8e26f1df..3cd46bdf 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -10,8 +10,10 @@
* Userspace (multipath/multipathd) path states
*
* PATH_WILD:
- * - Use: None of the checkers (returned if we don't have an fd)
- * - Description: Corner case where "fd < 0" for path fd (see checker_check())
+ * - Use: Any checker
+ * - Description: Corner case where "fd < 0" for path fd (see checker_check()),
+ * or where a checker detects an unsupported device
+ * (e.g. wrong checker configured for a given device).
*
* PATH_UNCHECKED:
* - Use: Only in directio checker
@@ -108,6 +110,7 @@ enum {
CHECKER_MSGID_UP,
CHECKER_MSGID_DOWN,
CHECKER_MSGID_GHOST,
+ CHECKER_MSGID_UNSUPPORTED,
CHECKER_GENERIC_MSGTABLE_SIZE,
CHECKER_FIRST_MSGID = 100, /* lowest msgid for checkers */
CHECKER_MSGTABLE_SIZE = 100, /* max msg table size for checkers */
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 13/21] libmultipath: clariion checker: leave unsupported paths alone
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (11 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 12/21] libmultipath/checkers: support unsupported paths Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 14/21] libmultipath: hp_sw " Martin Wilck
` (8 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/emc_clariion.c | 54 ++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index f8e55b93..6fc89113 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -20,6 +20,11 @@
#define INQUIRY_CMD 0x12
#define INQUIRY_CMDLEN 6
#define HEAVY_CHECK_COUNT 10
+#define SCSI_COMMAND_TERMINATED 0x22
+#define SCSI_CHECK_CONDITION 0x2
+#define RECOVERED_ERROR 0x01
+#define ILLEGAL_REQUEST 0x05
+#define SG_ERR_DRIVER_SENSE 0x08
/*
* Mechanism to track CLARiiON inactive snapshot LUs.
@@ -130,7 +135,9 @@ int libcheck_check (struct checker * c)
(struct emc_clariion_checker_path_context *)c->context;
char wwnstr[33];
int ret;
+ int retry_emc = 5;
+retry:
memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
memset(sense_buffer, 0, 128);
memset(sb, 0, SENSE_BUFF_LEN);
@@ -145,13 +152,60 @@ int libcheck_check (struct checker * c)
io_hdr.timeout = c->timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
+ if (errno == ENOTTY) {
+ c->msgid = CHECKER_MSGID_UNSUPPORTED;
+ return PATH_WILD;
+ }
c->msgid = MSG_CLARIION_QUERY_FAILED;
return PATH_DOWN;
}
+
if (io_hdr.info & SG_INFO_OK_MASK) {
+ switch (io_hdr.host_status) {
+ case DID_BUS_BUSY:
+ case DID_ERROR:
+ case DID_SOFT_ERROR:
+ case DID_TRANSPORT_DISRUPTED:
+ /* Transport error, retry */
+ if (--retry_emc)
+ goto retry;
+ break;
+ default:
+ break;
+ }
+ }
+
+ if (SCSI_CHECK_CONDITION == io_hdr.status ||
+ SCSI_COMMAND_TERMINATED == io_hdr.status ||
+ SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status)) {
+ if (io_hdr.sbp && (io_hdr.sb_len_wr > 2)) {
+ unsigned char *sbp = io_hdr.sbp;
+ int sense_key;
+
+ if (sbp[0] & 0x2)
+ sense_key = sbp[1] & 0xf;
+ else
+ sense_key = sbp[2] & 0xf;
+
+ if (sense_key == ILLEGAL_REQUEST) {
+ c->msgid = CHECKER_MSGID_UNSUPPORTED;
+ return PATH_WILD;
+ } else if (sense_key != RECOVERED_ERROR) {
+ condlog(1, "emc_clariion_checker: INQUIRY failed with sense key %02x",
+ sense_key);
+ c->msgid = MSG_CLARIION_QUERY_ERROR;
+ return PATH_DOWN;
+ }
+ }
+ }
+
+ if (io_hdr.info & SG_INFO_OK_MASK) {
+ condlog(1, "emc_clariion_checker: INQUIRY failed without sense, status %02x",
+ io_hdr.status);
c->msgid = MSG_CLARIION_QUERY_ERROR;
return PATH_DOWN;
}
+
if (/* Verify the code page - right page & revision */
sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
c->msgid = MSG_CLARIION_UNIT_REPORT;
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 14/21] libmultipath: hp_sw checker: leave unsupported paths alone
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (12 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 13/21] libmultipath: clariion checker: leave unsupported paths alone Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 15/21] libmultipath: rdac " Martin Wilck
` (7 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/hp_sw.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index d7f1018c..1a820223 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -24,6 +24,7 @@
#define SCSI_COMMAND_TERMINATED 0x22
#define SG_ERR_DRIVER_SENSE 0x08
#define RECOVERED_ERROR 0x01
+#define ILLEGAL_REQUEST 0x05
#define MX_ALLOC_LEN 255
#define HEAVY_CHECK_COUNT 10
@@ -68,14 +69,19 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
io_hdr.sbp = sense_b;
io_hdr.timeout = timeout * 1000;
- if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
- return 1;
+ if (ioctl(sg_fd, SG_IO, &io_hdr) < 0) {
+ if (errno == ENOTTY)
+ return PATH_WILD;
+ else
+ return PATH_DOWN;
+ }
/* treat SG_ERR here to get rid of sg_err.[ch] */
io_hdr.status &= 0x7e;
if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
(0 == io_hdr.driver_status))
- return 0;
+ return PATH_UP;
+
if ((SCSI_CHECK_CONDITION == io_hdr.status) ||
(SCSI_COMMAND_TERMINATED == io_hdr.status) ||
(SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status))) {
@@ -86,11 +92,13 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
sense_key = sense_buffer[1] & 0xf;
else
sense_key = sense_buffer[2] & 0xf;
- if(RECOVERED_ERROR == sense_key)
- return 0;
+ if (RECOVERED_ERROR == sense_key)
+ return PATH_UP;
+ else if (ILLEGAL_REQUEST == sense_key)
+ return PATH_WILD;
}
}
- return 1;
+ return PATH_DOWN;
}
static int
@@ -122,10 +130,15 @@ do_tur (int fd, unsigned int timeout)
int libcheck_check(struct checker * c)
{
char buff[MX_ALLOC_LEN];
+ int ret = do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout);
- if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
+ if (ret == PATH_WILD) {
+ c->msgid = CHECKER_MSGID_UNSUPPORTED;
+ return ret;
+ }
+ if (ret != PATH_UP) {
c->msgid = CHECKER_MSGID_DOWN;
- return PATH_DOWN;
+ return ret;
};
if (do_tur(c->fd, c->timeout)) {
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 15/21] libmultipath: rdac checker: leave unsupported paths alone
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (13 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 14/21] libmultipath: hp_sw " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 16/21] libmultipath: tur " Martin Wilck
` (6 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/rdac.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 266f8e10..8a3b73ec 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -26,6 +26,7 @@
#define SCSI_COMMAND_TERMINATED 0x22
#define SG_ERR_DRIVER_SENSE 0x08
#define RECOVERED_ERROR 0x01
+#define ILLEGAL_REQUEST 0x05
#define CURRENT_PAGE_CODE_VALUES 0
@@ -162,14 +163,14 @@ retry:
io_hdr.sbp = sense_b;
io_hdr.timeout = timeout * 1000;
- if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
- return 1;
+ if (ioctl(sg_fd, SG_IO, &io_hdr) < 0 && errno == ENOTTY)
+ return PATH_WILD;
/* treat SG_ERR here to get rid of sg_err.[ch] */
io_hdr.status &= 0x7e;
if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
(0 == io_hdr.driver_status))
- return 0;
+ return PATH_UP;
/* check if we need to retry this error */
if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -198,10 +199,14 @@ retry:
else
sense_key = sense_buffer[2] & 0xf;
if (RECOVERED_ERROR == sense_key)
- return 0;
+ return PATH_UP;
+ else if (ILLEGAL_REQUEST == sense_key)
+ return PATH_WILD;
+ condlog(1, "rdac checker: INQUIRY failed with sense key %02x",
+ sense_key);
}
}
- return 1;
+ return PATH_DOWN;
}
struct volume_access_inq
@@ -290,12 +295,14 @@ int libcheck_check(struct checker * c)
inqfail = 0;
memset(&inq, 0, sizeof(struct volume_access_inq));
- if (0 != do_inq(c->fd, 0xC9, &inq, sizeof(struct volume_access_inq),
- c->timeout)) {
- ret = PATH_DOWN;
+ ret = do_inq(c->fd, 0xC9, &inq, sizeof(struct volume_access_inq),
+ c->timeout);
+ if (ret != PATH_UP) {
inqfail = 1;
goto done;
- } else if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
+ }
+
+ if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
/* LUN not connected*/
ret = PATH_DOWN;
goto done;
@@ -332,6 +339,9 @@ int libcheck_check(struct checker * c)
done:
switch (ret) {
+ case PATH_WILD:
+ c->msgid = CHECKER_MSGID_UNSUPPORTED;
+ break;
case PATH_DOWN:
c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
checker_msg_string(&inq));
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 16/21] libmultipath: tur checker: leave unsupported paths alone
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (14 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 15/21] libmultipath: rdac " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 17/21] libmultipath: pathinfo: don't blank wwid if checker fails Martin Wilck
` (5 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers/tur.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 22734be2..a27474f9 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -125,6 +125,10 @@ retry:
io_hdr.timeout = timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
+ if (errno == ENOTTY) {
+ *msgid = CHECKER_MSGID_UNSUPPORTED;
+ return PATH_WILD;
+ }
*msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 17/21] libmultipath: pathinfo: don't blank wwid if checker fails
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (15 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 16/21] libmultipath: tur " Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 18/21] multipathd: check_path: improve logging for "unusable path" case Martin Wilck
` (4 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Blanking a WWID is a dangerous operation. E.g. configure() would
consider the path in question as invalid and orphan it if the
WWID is blank. Don't do this checker failures which may be transient
or indicate a badly configured or otherwise malfunctioning checker.
Moreover, we try to determine WWID even if path_offline returns
PATH_DOWN in the first place, so why should we not if the checker
has a problem?
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/discovery.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5e59e273..a6159e4a 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1944,9 +1944,6 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
if (path_state == PATH_UP) {
pp->chkrstate = pp->state = get_state(pp, conf, 0,
path_state);
- if (pp->state == PATH_UNCHECKED ||
- pp->state == PATH_WILD)
- goto blank;
if (pp->state == PATH_TIMEOUT)
pp->state = PATH_DOWN;
if (pp->state == PATH_UP && !pp->size) {
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 18/21] multipathd: check_path: improve logging for "unusable path" case
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (16 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 17/21] libmultipath: pathinfo: don't blank wwid if checker fails Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths Martin Wilck
` (3 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 2f922db7..c57aa392 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1897,7 +1897,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
}
if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
- condlog(2, "%s: unusable path", pp->dev);
+ condlog(2, "%s: unusable path - checker failed", pp->dev);
+ LOG_MSG(2, verbosity, pp);
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
pathinfo(pp, conf, 0);
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (17 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 18/21] multipathd: check_path: improve logging for "unusable path" case Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 20/21] libmultipath: sync_map_state: log failing paths Martin Wilck
` (2 subsequent siblings)
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/configure.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 09c3dcf2..ed3e30f5 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1020,14 +1020,18 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
}
}
vector_foreach_slot (pathvec, pp1, k) {
- int invalid = 0;
+ int invalid;
/* skip this path for some reason */
/* 1. if path has no unique id or wwid blacklisted */
+ if (strlen(pp1->wwid) == 0) {
+ orphan_path(pp1, "no WWID");
+ continue;
+ }
+
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
- if (strlen(pp1->wwid) == 0 || filter_path(conf, pp1) > 0)
- invalid = 1;
+ invalid = (filter_path(conf, pp1) > 0);
pthread_cleanup_pop(1);
if (invalid) {
orphan_path(pp1, "blacklisted");
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 20/21] libmultipath: sync_map_state: log failing paths
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (18 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-02 12:21 ` [PATCH v5 21/21] libmultipath/checkers: cleanup class/instance model Martin Wilck
2018-11-14 7:39 ` [PATCH v5 00/21] libmultipath: checkers overhaul Christophe Varoqui
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
Emit a log message when force-failing exisiting paths.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/structs_vec.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index f87d69d4..c85823a0 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -318,8 +318,11 @@ sync_map_state(struct multipath *mpp)
else if ((pp->dmstate == PSTATE_ACTIVE ||
pp->dmstate == PSTATE_UNDEF) &&
(pp->state == PATH_DOWN ||
- pp->state == PATH_SHAKY))
+ pp->state == PATH_SHAKY)) {
+ condlog(2, "sync_map_state: failing %s state %d dmstate %d",
+ pp->dev, pp->state, pp->dmstate);
dm_fail_path(mpp->alias, pp->dev_t);
+ }
}
}
}
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 21/21] libmultipath/checkers: cleanup class/instance model
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (19 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 20/21] libmultipath: sync_map_state: log failing paths Martin Wilck
@ 2018-11-02 12:21 ` Martin Wilck
2018-11-14 7:39 ` [PATCH v5 00/21] libmultipath: checkers overhaul Christophe Varoqui
21 siblings, 0 replies; 25+ messages in thread
From: Martin Wilck @ 2018-11-02 12:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
The checkers code implicitly uses a sort-of OOP class/instance model,
but very clumsily. Separate the checker "class" and "instance" cleanly,
and do a few further cleanups (constifications etc) on the way.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers.c | 131 ++++++++++++++++---------------
libmultipath/checkers.h | 23 ++----
libmultipath/checkers/directio.c | 2 +-
libmultipath/checkers/tur.c | 2 +-
libmultipath/print.c | 2 +-
libmultipath/propsel.c | 19 ++---
6 files changed, 89 insertions(+), 90 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 90313c35..848c4c34 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -8,6 +8,19 @@
#include "checkers.h"
#include "vector.h"
+struct checker_class {
+ struct list_head node;
+ void *handle;
+ int refcount;
+ int sync;
+ char name[CHECKER_NAME_LEN];
+ int (*check)(struct checker *);
+ int (*init)(struct checker *); /* to allocate the context */
+ void (*free)(struct checker *); /* to free the context */
+ const char **msgtable;
+ short msgtable_size;
+};
+
char *checker_state_names[] = {
"wild",
"unchecked",
@@ -23,38 +36,30 @@ char *checker_state_names[] = {
static LIST_HEAD(checkers);
-char * checker_state_name (int i)
+const char *checker_state_name(int i)
{
return checker_state_names[i];
}
-int init_checkers (char *multipath_dir)
-{
- if (!add_checker(multipath_dir, DEFAULT_CHECKER))
- return 1;
- return 0;
-}
-
-struct checker * alloc_checker (void)
+static struct checker_class *alloc_checker_class(void)
{
- struct checker *c;
+ struct checker_class *c;
- c = MALLOC(sizeof(struct checker));
+ c = MALLOC(sizeof(struct checker_class));
if (c) {
INIT_LIST_HEAD(&c->node);
c->refcount = 1;
- c->fd = -1;
}
return c;
}
-void free_checker (struct checker * c)
+void free_checker_class(struct checker_class *c)
{
if (!c)
return;
c->refcount--;
if (c->refcount) {
- condlog(3, "%s checker refcount %d",
+ condlog(4, "%s checker refcount %d",
c->name, c->refcount);
return;
}
@@ -71,17 +76,17 @@ void free_checker (struct checker * c)
void cleanup_checkers (void)
{
- struct checker * checker_loop;
- struct checker * checker_temp;
+ struct checker_class *checker_loop;
+ struct checker_class *checker_temp;
list_for_each_entry_safe(checker_loop, checker_temp, &checkers, node) {
- free_checker(checker_loop);
+ free_checker_class(checker_loop);
}
}
-struct checker * checker_lookup (char * name)
+static struct checker_class *checker_class_lookup(const char *name)
{
- struct checker * c;
+ struct checker_class *c;
if (!name || !strlen(name))
return NULL;
@@ -92,14 +97,15 @@ struct checker * checker_lookup (char * name)
return NULL;
}
-struct checker * add_checker (char *multipath_dir, char * name)
+static struct checker_class *add_checker_class(const char *multipath_dir,
+ const char *name)
{
char libname[LIB_CHECKER_NAMELEN];
struct stat stbuf;
- struct checker * c;
+ struct checker_class *c;
char *errstr;
- c = alloc_checker();
+ c = alloc_checker_class();
if (!c)
return NULL;
snprintf(c->name, CHECKER_NAME_LEN, "%s", name);
@@ -158,12 +164,11 @@ struct checker * add_checker (char *multipath_dir, char * name)
c->name, c->msgtable_size);
done:
- c->fd = -1;
c->sync = 1;
list_add(&c->node, &checkers);
return c;
out:
- free_checker(c);
+ free_checker_class(c);
return NULL;
}
@@ -176,16 +181,16 @@ void checker_set_fd (struct checker * c, int fd)
void checker_set_sync (struct checker * c)
{
- if (!c)
+ if (!c || !c->cls)
return;
- c->sync = 1;
+ c->cls->sync = 1;
}
void checker_set_async (struct checker * c)
{
- if (!c)
+ if (!c || !c->cls)
return;
- c->sync = 0;
+ c->cls->sync = 0;
}
void checker_enable (struct checker * c)
@@ -204,11 +209,11 @@ void checker_disable (struct checker * c)
int checker_init (struct checker * c, void ** mpctxt_addr)
{
- if (!c)
+ if (!c || !c->cls)
return 1;
c->mpcontext = mpctxt_addr;
- if (c->init)
- return c->init(c);
+ if (c->cls->init)
+ return c->cls->init(c);
return 0;
}
@@ -220,15 +225,16 @@ void checker_clear (struct checker *c)
void checker_put (struct checker * dst)
{
- struct checker * src;
+ struct checker_class *src;
- if (!dst || !strlen(dst->name))
+ if (!dst)
return;
- src = checker_lookup(dst->name);
- if (dst->free)
- dst->free(dst);
+ src = dst->cls;
+
+ if (src && src->free)
+ src->free(dst);
checker_clear(dst);
- free_checker(src);
+ free_checker_class(src);
}
int checker_check (struct checker * c, int path_state)
@@ -243,32 +249,35 @@ int checker_check (struct checker * c, int path_state)
c->msgid = CHECKER_MSGID_DISABLED;
return PATH_UNCHECKED;
}
- if (!strncmp(c->name, NONE, 4))
+ if (!strncmp(c->cls->name, NONE, 4))
return path_state;
if (c->fd < 0) {
c->msgid = CHECKER_MSGID_NO_FD;
return PATH_WILD;
}
- r = c->check(c);
+ r = c->cls->check(c);
return r;
}
-int checker_selected (struct checker * c)
+int checker_selected(const struct checker *c)
{
if (!c)
return 0;
- if (!strncmp(c->name, NONE, 4))
- return 1;
- return (c->check) ? 1 : 0;
+ return c->cls != NULL;
}
const char *checker_name(const struct checker *c)
{
- if (!c)
+ if (!c || !c->cls)
return NULL;
- return c->name;
+ return c->cls->name;
+}
+
+int checker_is_sync(const struct checker *c)
+{
+ return c && c->cls && c->cls->sync;
}
static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
@@ -309,31 +318,29 @@ void checker_clear_message (struct checker *c)
c->msgid = CHECKER_MSGID_NONE;
}
-void checker_get (char *multipath_dir, struct checker * dst, char * name)
+void checker_get(const char *multipath_dir, struct checker *dst,
+ const char *name)
{
- struct checker * src = NULL;
+ struct checker_class *src = NULL;
if (!dst)
return;
if (name && strlen(name)) {
- src = checker_lookup(name);
+ src = checker_class_lookup(name);
if (!src)
- src = add_checker(multipath_dir, name);
+ src = add_checker_class(multipath_dir, name);
}
- if (!src) {
- dst->check = NULL;
+ dst->cls = src;
+ if (!src)
return;
- }
- dst->fd = src->fd;
- dst->sync = src->sync;
- strncpy(dst->name, src->name, CHECKER_NAME_LEN);
- dst->msgid = CHECKER_MSGID_NONE;
- dst->check = src->check;
- dst->init = src->init;
- dst->free = src->free;
- dst->msgtable = src->msgtable;
- dst->msgtable_size = src->msgtable_size;
- dst->handle = NULL;
+
src->refcount++;
}
+
+int init_checkers(const char *multipath_dir)
+{
+ if (!add_checker_class(multipath_dir, DEFAULT_CHECKER))
+ return 1;
+ return 0;
+}
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 3cd46bdf..b2e8f9aa 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -116,32 +116,22 @@ enum {
CHECKER_MSGTABLE_SIZE = 100, /* max msg table size for checkers */
};
+struct checker_class;
struct checker {
- struct list_head node;
- void *handle;
- int refcount;
+ struct checker_class *cls;
int fd;
- int sync;
unsigned int timeout;
int disable;
- char name[CHECKER_NAME_LEN];
short msgid; /* checker-internal extra status */
void * context; /* store for persistent data */
void ** mpcontext; /* store for persistent data shared
multipath-wide. Use MALLOC if
you want to stuff data in. */
- int (*check)(struct checker *);
- int (*init)(struct checker *); /* to allocate the context */
- void (*free)(struct checker *); /* to free the context */
- const char**msgtable;
- short msgtable_size;
};
-char * checker_state_name (int);
-int init_checkers (char *);
+const char *checker_state_name(int);
+int init_checkers(const char *);
void cleanup_checkers (void);
-struct checker * add_checker (char *, char *);
-struct checker * checker_lookup (char *);
int checker_init (struct checker *, void **);
void checker_clear (struct checker *);
void checker_put (struct checker *);
@@ -152,7 +142,8 @@ void checker_set_fd (struct checker *, int);
void checker_enable (struct checker *);
void checker_disable (struct checker *);
int checker_check (struct checker *, int);
-int checker_selected (struct checker *);
+int checker_selected(const struct checker *);
+int checker_is_sync(const struct checker *);
const char *checker_name (const struct checker *);
/*
* This returns a string that's best prepended with "$NAME checker",
@@ -160,7 +151,7 @@ const char *checker_name (const struct checker *);
*/
const char *checker_message(const struct checker *);
void checker_clear_message (struct checker *c);
-void checker_get (char *, struct checker *, char *);
+void checker_get(const char *, struct checker *, const char *);
/* Prototypes for symbols exported by path checker dynamic libraries (.so) */
int libcheck_check(struct checker *);
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index c4a0712e..1b00b775 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -202,7 +202,7 @@ int libcheck_check (struct checker * c)
if (!ct)
return PATH_UNCHECKED;
- ret = check_state(c->fd, ct, c->sync, c->timeout);
+ ret = check_state(c->fd, ct, checker_is_sync(c), c->timeout);
switch (ret)
{
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a27474f9..63b19624 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -323,7 +323,7 @@ int libcheck_check(struct checker * c)
if (!ct)
return PATH_UNCHECKED;
- if (c->sync)
+ if (checker_is_sync(c))
return tur_check(c->fd, c->timeout, &c->msgid);
/*
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 7b610b94..fc40b0f0 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -615,7 +615,7 @@ static int
snprint_path_checker (char * buff, size_t len, const struct path * pp)
{
const struct checker * c = &pp->checker;
- return snprint_str(buff, len, c->name);
+ return snprint_str(buff, len, checker_name(c));
}
static int
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index fdb5953a..970a3b5c 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -479,26 +479,27 @@ check_rdac(struct path * pp)
int select_checker(struct config *conf, struct path *pp)
{
const char *origin;
- char *checker_name;
+ char *ckr_name;
struct checker * c = &pp->checker;
if (pp->detect_checker == DETECT_CHECKER_ON) {
origin = autodetect_origin;
if (check_rdac(pp)) {
- checker_name = RDAC;
+ ckr_name = RDAC;
goto out;
} else if (pp->tpgs > 0) {
- checker_name = TUR;
+ ckr_name = TUR;
goto out;
}
}
- do_set(checker_name, conf->overrides, checker_name, overrides_origin);
- do_set_from_hwe(checker_name, pp, checker_name, hwe_origin);
- do_set(checker_name, conf, checker_name, conf_origin);
- do_default(checker_name, DEFAULT_CHECKER);
+ do_set(checker_name, conf->overrides, ckr_name, overrides_origin);
+ do_set_from_hwe(checker_name, pp, ckr_name, hwe_origin);
+ do_set(checker_name, conf, ckr_name, conf_origin);
+ do_default(ckr_name, DEFAULT_CHECKER);
out:
- checker_get(conf->multipath_dir, c, checker_name);
- condlog(3, "%s: path_checker = %s %s", pp->dev, c->name, origin);
+ checker_get(conf->multipath_dir, c, ckr_name);
+ condlog(3, "%s: path_checker = %s %s", pp->dev,
+ checker_name(c), origin);
if (conf->checker_timeout) {
c->timeout = conf->checker_timeout;
condlog(3, "%s: checker timeout = %u s %s",
--
2.19.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 03/21] libmultipath/checkers: replace message by msgid
2018-11-02 12:21 ` [PATCH v5 03/21] libmultipath/checkers: replace message by msgid Martin Wilck
@ 2018-11-02 16:05 ` Benjamin Marzinski
0 siblings, 0 replies; 25+ messages in thread
From: Benjamin Marzinski @ 2018-11-02 16:05 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Fri, Nov 02, 2018 at 01:21:07PM +0100, Martin Wilck wrote:
> Replace the character array "message" in struct checker with
> a "message ID" field.
>
> The generic checker code defines a couple of standard message IDs
> and corresponding messages. Checker-specific message IDs start
> at CHECKER_FIRST_MSG. Checkers that implement specific message
> IDs must provide a table for converting the IDs into actual log
> messages.
>
> This simplifies the checker data structure and the handling of
> checker messages in the critical checker code path. It comes at
> the cost that only constant message strings are supported. It
> turns out that only a single checker log message (in the emc_clariion
> checker) was dynamically generated, and the missing information can
> be provided with a standard condlog message.
>
> Follow-up patches implement this for the existing checkers.
>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/checkers.c | 61 +++++++++++++++++++++++++++++++++-------
> libmultipath/checkers.h | 43 ++++++++++++++++++++++++----
> libmultipath/discovery.c | 4 +--
> 3 files changed, 90 insertions(+), 18 deletions(-)
>
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 0bacc864..a1b5cb45 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char * name)
> if (!c->free)
> goto out;
>
> + c->msgtable_size = 0;
> + c->msgtable = dlsym(c->handle, "libcheck_msgtable");
> +
> + if (c->msgtable != NULL) {
> + const char **p;
> +
> + for (p = c->msgtable;
> + *p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
> + /* nothing */;
> +
> + c->msgtable_size = p - c->msgtable;
> + } else
> + c->msgtable_size = 0;
> + condlog(3, "checker %s: message table size = %d",
> + c->name, c->msgtable_size);
> +
> done:
> c->fd = -1;
> c->sync = 1;
> @@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
> if (!c)
> return PATH_WILD;
>
> - c->message[0] = '\0';
> + c->msgid = CHECKER_MSGID_NONE;
> if (c->disable) {
> - MSG(c, "checker disabled");
> + c->msgid = CHECKER_MSGID_DISABLED;
> return PATH_UNCHECKED;
> }
> if (!strncmp(c->name, NONE, 4))
> return path_state;
>
> if (c->fd < 0) {
> - MSG(c, "no usable fd");
> + c->msgid = CHECKER_MSGID_NO_FD;
> return PATH_WILD;
> }
> r = c->check(c);
> @@ -248,25 +264,48 @@ int checker_selected (struct checker * c)
> return (c->check) ? 1 : 0;
> }
>
> -char * checker_name (struct checker * c)
> +const char *checker_name(const struct checker *c)
> {
> if (!c)
> return NULL;
> return c->name;
> }
>
> -char * checker_message (struct checker * c)
> +static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
> + [CHECKER_MSGID_NONE] = "",
> + [CHECKER_MSGID_DISABLED] = " is disabled",
> + [CHECKER_MSGID_NO_FD] = " has no usable fd",
> + [CHECKER_MSGID_INVALID] = " provided invalid message id",
> + [CHECKER_MSGID_UP] = " reports path is up",
> + [CHECKER_MSGID_DOWN] = " reports path is down",
> + [CHECKER_MSGID_GHOST] = " reports path is ghost",
> +};
> +
> +const char *checker_message(const struct checker *c)
> {
> - if (!c)
> - return NULL;
> - return c->message;
> + int id;
> +
> + if (!c || c->msgid < 0 ||
> + (c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
> + c->msgid < CHECKER_FIRST_MSGID))
> + goto bad_id;
> +
> + if (c->msgid < CHECKER_GENERIC_MSGTABLE_SIZE)
> + return generic_msg[c->msgid];
> +
> + id = c->msgid - CHECKER_FIRST_MSGID;
> + if (id < c->cls->msgtable_size)
> + return c->cls->msgtable[id];
> +
> +bad_id:
> + return generic_msg[CHECKER_MSGID_NONE];
> }
>
> void checker_clear_message (struct checker *c)
> {
> if (!c)
> return;
> - c->message[0] = '\0';
> + c->msgid = CHECKER_MSGID_NONE;
> }
>
> void checker_get (char *multipath_dir, struct checker * dst, char * name)
> @@ -288,10 +327,12 @@ void checker_get (char *multipath_dir, struct checker * dst, char * name)
> dst->fd = src->fd;
> dst->sync = src->sync;
> strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> - strncpy(dst->message, src->message, CHECKER_MSG_LEN);
> + dst->msgid = CHECKER_MSGID_NONE;
> dst->check = src->check;
> dst->init = src->init;
> dst->free = src->free;
> + dst->msgtable = src->msgtable;
> + dst->msgtable_size = src->msgtable_size;
> dst->handle = NULL;
> src->refcount++;
> }
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index 7b18a1ac..8e26f1df 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -97,6 +97,22 @@ enum path_check_state {
> #define CHECKER_DEV_LEN 256
> #define LIB_CHECKER_NAMELEN 256
>
> +/*
> + * Generic message IDs for use in checkers.
> + */
> +enum {
> + CHECKER_MSGID_NONE = 0,
> + CHECKER_MSGID_DISABLED,
> + CHECKER_MSGID_NO_FD,
> + CHECKER_MSGID_INVALID,
> + CHECKER_MSGID_UP,
> + CHECKER_MSGID_DOWN,
> + CHECKER_MSGID_GHOST,
> + CHECKER_GENERIC_MSGTABLE_SIZE,
> + CHECKER_FIRST_MSGID = 100, /* lowest msgid for checkers */
> + CHECKER_MSGTABLE_SIZE = 100, /* max msg table size for checkers */
> +};
> +
> struct checker {
> struct list_head node;
> void *handle;
> @@ -106,7 +122,7 @@ struct checker {
> unsigned int timeout;
> int disable;
> char name[CHECKER_NAME_LEN];
> - char message[CHECKER_MSG_LEN]; /* comm with callers */
> + short msgid; /* checker-internal extra status */
> void * context; /* store for persistent data */
> void ** mpcontext; /* store for persistent data shared
> multipath-wide. Use MALLOC if
> @@ -114,10 +130,10 @@ struct checker {
> int (*check)(struct checker *);
> int (*init)(struct checker *); /* to allocate the context */
> void (*free)(struct checker *); /* to free the context */
> + const char**msgtable;
> + short msgtable_size;
> };
>
> -#define MSG(c, fmt, args...) snprintf((c)->message, CHECKER_MSG_LEN, fmt, ##args);
> -
> char * checker_state_name (int);
> int init_checkers (char *);
> void cleanup_checkers (void);
> @@ -134,14 +150,29 @@ void checker_enable (struct checker *);
> void checker_disable (struct checker *);
> int checker_check (struct checker *, int);
> int checker_selected (struct checker *);
> -char * checker_name (struct checker *);
> -char * checker_message (struct checker *);
> +const char *checker_name (const struct checker *);
> +/*
> + * This returns a string that's best prepended with "$NAME checker",
> + * where $NAME is the return value of checker_name().
> + */
> +const char *checker_message(const struct checker *);
> void checker_clear_message (struct checker *c);
> void checker_get (char *, struct checker *, char *);
>
> -/* Functions exported by path checker dynamic libraries (.so) */
> +/* Prototypes for symbols exported by path checker dynamic libraries (.so) */
> int libcheck_check(struct checker *);
> int libcheck_init(struct checker *);
> void libcheck_free(struct checker *);
> +/*
> + * msgid => message map.
> + *
> + * It only needs to be provided if the checker defines specific
> + * message IDs.
> + * Message IDs available to checkers start at CHECKER_FIRST_MSG.
> + * The msgtable array is 0-based, i.e. msgtable[0] is the message
> + * for msgid == __CHECKER_FIRST_MSG.
> + * The table ends with a NULL element.
> + */
> +extern const char *libcheck_msgtable[];
>
> #endif /* _CHECKERS_H */
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 3550c3a7..5e59e273 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1589,8 +1589,8 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
> checker_name(c), checker_state_name(state));
> if (state != PATH_UP && state != PATH_GHOST &&
> strlen(checker_message(c)))
> - condlog(3, "%s: checker msg is \"%s\"",
> - pp->dev, checker_message(c));
> + condlog(3, "%s: %s checker%s",
> + pp->dev, checker_name(c), checker_message(c));
> return state;
> }
>
> --
> 2.19.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 11/21] multipathd: improve checker message logging
2018-11-02 12:21 ` [PATCH v5 11/21] multipathd: improve checker message logging Martin Wilck
@ 2018-11-02 16:54 ` Benjamin Marzinski
0 siblings, 0 replies; 25+ messages in thread
From: Benjamin Marzinski @ 2018-11-02 16:54 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Fri, Nov 02, 2018 at 01:21:15PM +0100, Martin Wilck wrote:
> Don't rely on any variables being defined in LOG_MSG.
> If message log level is low, don't bother to fetch the message.
>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> multipathd/main.c | 35 ++++++++++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index bf5f12a6..2f922db7 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -89,12 +89,24 @@ static int use_watchdog;
> #define FILE_NAME_SIZE 256
> #define CMDSIZE 160
>
> -#define LOG_MSG(a, b) \
> -do { \
> - if (pp->offline) \
> - condlog(a, "%s: %s - path offline", pp->mpp->alias, pp->dev); \
> - else if (strlen(b)) \
> - condlog(a, "%s: %s - %s", pp->mpp->alias, pp->dev, b); \
> +#define LOG_MSG(lvl, verb, pp) \
> +do { \
> + if (lvl <= verb) { \
> + if (pp->offline) \
> + condlog(lvl, "%s: %s - path offline", \
> + pp->mpp->alias, pp->dev); \
> + else { \
> + const char *__m = \
> + checker_message(&pp->checker); \
> + \
> + if (strlen(__m)) \
> + condlog(lvl, "%s: %s - %s checker%s", \
> + pp->mpp->alias, \
> + pp->dev, \
> + checker_name(&pp->checker), \
> + __m); \
> + } \
> + } \
> } while(0)
>
> struct mpath_event_param
> @@ -1811,7 +1823,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> int add_active;
> int disable_reinstate = 0;
> int oldchkrstate = pp->chkrstate;
> - int retrigger_tries, checkint, max_checkint;
> + int retrigger_tries, checkint, max_checkint, verbosity;
> struct config *conf;
> int ret;
>
> @@ -1828,6 +1840,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> retrigger_tries = conf->retrigger_tries;
> checkint = conf->checkint;
> max_checkint = conf->max_checkint;
> + verbosity = conf->verbosity;
> put_multipath_config(conf);
> if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
> if (pp->retriggers < retrigger_tries) {
> @@ -1970,7 +1983,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> int oldstate = pp->state;
> pp->state = newstate;
>
> - LOG_MSG(1, checker_message(&pp->checker));
> + LOG_MSG(1, verbosity, pp);
>
> /*
> * upon state change, reset the checkint
> @@ -2058,7 +2071,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> return 0;
> }
> } else {
> - LOG_MSG(4, checker_message(&pp->checker));
> + LOG_MSG(4, verbosity, pp);
> if (pp->checkint != max_checkint) {
> /*
> * double the next check delay.
> @@ -2088,9 +2101,9 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> log_checker_err = conf->log_checker_err;
> put_multipath_config(conf);
> if (log_checker_err == LOG_CHKR_ERR_ONCE)
> - LOG_MSG(3, checker_message(&pp->checker));
> + LOG_MSG(3, verbosity, pp);
> else
> - LOG_MSG(2, checker_message(&pp->checker));
> + LOG_MSG(2, verbosity, pp);
> }
> }
>
> --
> 2.19.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 00/21] libmultipath: checkers overhaul
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
` (20 preceding siblings ...)
2018-11-02 12:21 ` [PATCH v5 21/21] libmultipath/checkers: cleanup class/instance model Martin Wilck
@ 2018-11-14 7:39 ` Christophe Varoqui
21 siblings, 0 replies; 25+ messages in thread
From: Christophe Varoqui @ 2018-11-14 7:39 UTC (permalink / raw)
To: Martin Wilck; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 4670 bytes --]
Merged.
Thanks.
On Fri, Nov 2, 2018 at 1:23 PM Martin Wilck <mwilck@suse.com> wrote:
> Hi Christophe,
>
> This is v5 of my "checkers overhaul" series. Changed wrt v4
> are 03/22, 11/22, and 21/22. I re-post the whole series to
> avoid confusion.
>
> This series starts with a few minor fixes and then attempts
> an overhaul of the checker code.
>
> First, there's a block of patches to get rid of the "message"
> char array in struct checker, replacing it with an integer.
> This topic had been touched in recent discussion between Ben
> and myself. You may want to collaps the patches in this block
> (03/21-11/21) into one when commiting; compilation errors will
> arise if only part of them is a applied.
>
> The next larger block fixes problems with checkers that try
> to check unsupported devices. It's an interesting experience
> to configure wrong checkers for the existing devices and see
> what happens. With these patches, paths won't be falsely
> teared down any more in such situations.
>
> The last patch cleans up the checker data structure by splitting
> it into a "checker class" and the path "checker instance".
>
> There's more work to do in this area, but this is a start.
>
> ----
> changes v4->v5
> - 03/21: made checker_message() const again, as 22/22 is gone.
> Got rid of the static buffer in checker_message() by simply returning
> the constant message strings, as suggested by Ben in his review of v4.
> This implies printf format changes in callers; changed format in
> get_state().
> - 11/21: Changed printf format in check_path() (see above).
> - 21/22: rebased, no actual changes (kept Ben's Reviewed-by).
> - 22/22: dropped, obsolete.
>
> Changes v3->v4:
> - 03/22: renamed CHECKER_LAST_GENERIC_MSGID ->
> CHECKER_GENERIC_MSGTABLE_SIZE
> (Ben).
> - 12/22: rebased on top of changed 03/22.
>
> Changes v2->v3:
> - 03/22: fixed one minor issue mentioned by Ben;
> reverted the const-ification of checker_message(),
> as it will be reverted in 22/22 anyway.
> - 13/22: fix clariion checker semantics (Ben).
> - 21/22: rebased on top of updated 03/22.
> - 22/22: fix thread-safety issue from 03/22 (Ben).
>
> Changes v1->v2:
> - 11/22: rebased on top of "various multipath-tools patches" series
>
> Martin Wilck (21):
> libmultipath: fix use of uninitialized memory in write()
> libmultipath: fix memory leaks from scandir() use
> libmultipath/checkers: replace message by msgid
> libmultipath/checkers: cciss_tur: use message id
> libmultipath/checkers: directio: use message id
> libmultipath/checkers: emc_clariion: use message id
> libmultipath/checkers: hp_sw: use message id
> libmultipath/checkers: rdac: use message id
> libmultipath/checkers: readsector0: use message id
> libmultipath/checkers: tur: use message id
> multipathd: improve checker message logging
> libmultipath/checkers: support unsupported paths
> libmultipath: clariion checker: leave unsupported paths alone
> libmultipath: hp_sw checker: leave unsupported paths alone
> libmultipath: rdac checker: leave unsupported paths alone
> libmultipath: tur checker: leave unsupported paths alone
> libmultipath: pathinfo: don't blank wwid if checker fails
> multipathd: check_path: improve logging for "unusable path" case
> libmultipath: coalesce_paths: improve logging of orphaned paths
> libmultipath: sync_map_state: log failing paths
> libmultipath/checkers: cleanup class/instance model
>
> libmultipath/checkers.c | 187 +++++++++++++++++----------
> libmultipath/checkers.h | 69 ++++++----
> libmultipath/checkers/cciss_tur.c | 13 +-
> libmultipath/checkers/directio.c | 29 +++--
> libmultipath/checkers/emc_clariion.c | 114 +++++++++++++---
> libmultipath/checkers/hp_sw.c | 39 +++---
> libmultipath/checkers/rdac.c | 92 +++++++++----
> libmultipath/checkers/readsector0.c | 7 +-
> libmultipath/checkers/tur.c | 60 +++++----
> libmultipath/config.c | 10 +-
> libmultipath/configure.c | 10 +-
> libmultipath/discovery.c | 11 +-
> libmultipath/foreign.c | 5 +-
> libmultipath/foreign/nvme.c | 6 +-
> libmultipath/print.c | 2 +-
> libmultipath/propsel.c | 19 +--
> libmultipath/structs_vec.c | 5 +-
> libmultipath/sysfs.c | 5 +-
> libmultipath/util.c | 9 ++
> libmultipath/util.h | 9 ++
> multipathd/main.c | 38 ++++--
> 21 files changed, 497 insertions(+), 242 deletions(-)
>
> --
> 2.19.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 5562 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-11-14 7:39 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-02 12:21 [PATCH v5 00/21] libmultipath: checkers overhaul Martin Wilck
2018-11-02 12:21 ` [PATCH v5 01/21] libmultipath: fix use of uninitialized memory in write() Martin Wilck
2018-11-02 12:21 ` [PATCH v5 02/21] libmultipath: fix memory leaks from scandir() use Martin Wilck
2018-11-02 12:21 ` [PATCH v5 03/21] libmultipath/checkers: replace message by msgid Martin Wilck
2018-11-02 16:05 ` Benjamin Marzinski
2018-11-02 12:21 ` [PATCH v5 04/21] libmultipath/checkers: cciss_tur: use message id Martin Wilck
2018-11-02 12:21 ` [PATCH v5 05/21] libmultipath/checkers: directio: " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 06/21] libmultipath/checkers: emc_clariion: " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 07/21] libmultipath/checkers: hp_sw: " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 08/21] libmultipath/checkers: rdac: " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 09/21] libmultipath/checkers: readsector0: " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 10/21] libmultipath/checkers: tur: " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 11/21] multipathd: improve checker message logging Martin Wilck
2018-11-02 16:54 ` Benjamin Marzinski
2018-11-02 12:21 ` [PATCH v5 12/21] libmultipath/checkers: support unsupported paths Martin Wilck
2018-11-02 12:21 ` [PATCH v5 13/21] libmultipath: clariion checker: leave unsupported paths alone Martin Wilck
2018-11-02 12:21 ` [PATCH v5 14/21] libmultipath: hp_sw " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 15/21] libmultipath: rdac " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 16/21] libmultipath: tur " Martin Wilck
2018-11-02 12:21 ` [PATCH v5 17/21] libmultipath: pathinfo: don't blank wwid if checker fails Martin Wilck
2018-11-02 12:21 ` [PATCH v5 18/21] multipathd: check_path: improve logging for "unusable path" case Martin Wilck
2018-11-02 12:21 ` [PATCH v5 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths Martin Wilck
2018-11-02 12:21 ` [PATCH v5 20/21] libmultipath: sync_map_state: log failing paths Martin Wilck
2018-11-02 12:21 ` [PATCH v5 21/21] libmultipath/checkers: cleanup class/instance model Martin Wilck
2018-11-14 7:39 ` [PATCH v5 00/21] libmultipath: checkers overhaul Christophe Varoqui
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.