device-mapper-multipath/0027-multipathd-watch-bindings-file-with-inotify-timestam.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@suse.com>
Date: Thu, 7 Sep 2023 22:22:43 +0200
Subject: [PATCH] multipathd: watch bindings file with inotify + timestamp

Since "libmultipath: keep bindings in memory", we don't re-read the
bindings file after every modification. Add a notification mechanism
that makes multipathd aware of changes to the bindings file. Because
multipathd itself will change the bindings file, it must compare
timestamps in order to avoid reading the file repeatedly.

Because select_alias() can be called from multiple thread contexts (uxlsnr,
uevent handler), we need to add locking for the bindings file. The
timestamp must also be protected by a lock, because it can't be read
or written atomically.

Note: The notification mechanism expects the bindings file to be
atomically replaced by rename(2). Changes must be made in a temporary file and
applied using rename(2), as in update_bindings_file(). The inotify
mechanism deliberately does not listen to close-after-write events
that would be generated by editing the bindings file directly. This

Note also: new bindings will only be read from add_map_with_path(),
i.e. either during reconfigure(), or when a new map is created during
runtime. Existing maps will not be renamed if the binding file changes,
unless the user runs "multipathd reconfigure". This is not a change
wrt the previous code, but it should be mentioned anyway.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/alias.c              | 256 +++++++++++++++++++++++++-----
 libmultipath/alias.h              |   3 +-
 libmultipath/libmultipath.version |   5 +
 multipathd/uxlsnr.c               |  36 ++++-
 tests/alias.c                     |   3 +
 5 files changed, 256 insertions(+), 47 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 66e34e31..964b8a7b 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -10,6 +10,7 @@
 #include <stdio.h>
 #include <stdbool.h>
 #include <assert.h>
+#include <sys/inotify.h>
 
 #include "debug.h"
 #include "util.h"
@@ -22,6 +23,7 @@
 #include "config.h"
 #include "devmapper.h"
 #include "strbuf.h"
+#include "time-util.h"
 
 /*
  * significant parts of this file were taken from iscsi-bindings.c of the
@@ -50,6 +52,12 @@
 "# alias wwid\n" \
 "#\n"
 
+/* uatomic access only */
+static int bindings_file_changed = 1;
+
+static pthread_mutex_t timestamp_mutex = PTHREAD_MUTEX_INITIALIZER;
+static struct timespec bindings_last_updated;
+
 struct binding {
 	char *alias;
 	char *wwid;
@@ -60,6 +68,9 @@ struct binding {
  * an abstract type.
  */
 typedef struct _vector Bindings;
+
+/* Protect global_bindings */
+static pthread_mutex_t bindings_mutex = PTHREAD_MUTEX_INITIALIZER;
 static Bindings global_bindings = { .allocated = 0 };
 
 enum {
@@ -78,6 +89,27 @@ static void _free_binding(struct binding *bdg)
 	free(bdg);
 }
 
+static void free_bindings(Bindings *bindings)
+{
+	struct binding *bdg;
+	int i;
+
+	vector_foreach_slot(bindings, bdg, i)
+		_free_binding(bdg);
+	vector_reset(bindings);
+}
+
+static void set_global_bindings(Bindings *bindings)
+{
+	Bindings old_bindings;
+
+	pthread_mutex_lock(&bindings_mutex);
+	old_bindings = global_bindings;
+	global_bindings = *bindings;
+	pthread_mutex_unlock(&bindings_mutex);
+	free_bindings(&old_bindings);
+}
+
 static const struct binding *get_binding_for_alias(const Bindings *bindings,
 						   const char *alias)
 {
@@ -199,7 +231,8 @@ static int delete_binding(Bindings *bindings, const char *wwid)
 	return BINDING_DELETED;
 }
 
-static int write_bindings_file(const Bindings *bindings, int fd)
+static int write_bindings_file(const Bindings *bindings, int fd,
+			       struct timespec *ts)
 {
 	struct binding *bnd;
 	STRBUF_ON_STACK(content);
@@ -227,9 +260,56 @@ static int write_bindings_file(const Bindings *bindings, int fd)
 		}
 		len -= n;
 	}
+	fsync(fd);
+	if (ts) {
+		struct stat st;
+
+		if (fstat(fd, &st) == 0)
+			*ts = st.st_mtim;
+		else
+			clock_gettime(CLOCK_REALTIME_COARSE, ts);
+	}
 	return 0;
 }
 
+void handle_bindings_file_inotify(const struct inotify_event *event)
+{
+	struct config *conf;
+	const char *base;
+	bool changed = false;
+	struct stat st;
+	struct timespec ts = { 0, 0 };
+	int ret;
+
+	if (!(event->mask & IN_MOVED_TO))
+		return;
+
+	conf = get_multipath_config();
+	base = strrchr(conf->bindings_file, '/');
+	changed = base && base > conf->bindings_file &&
+		!strcmp(base + 1, event->name);
+	ret = stat(conf->bindings_file, &st);
+	put_multipath_config(conf);
+
+	if (!changed)
+		return;
+
+	pthread_mutex_lock(&timestamp_mutex);
+	if (ret == 0) {
+		ts = st.st_mtim;
+		changed = timespeccmp(&ts, &bindings_last_updated) > 0;
+	}
+	pthread_mutex_unlock(&timestamp_mutex);
+
+	if (changed) {
+		uatomic_xchg(&bindings_file_changed, 1);
+		condlog(3, "%s: bindings file must be re-read, new timestamp: %ld.%06ld",
+			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
+	} else
+		condlog(3, "%s: bindings file is up-to-date, timestamp: %ld.%06ld",
+			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
+}
+
 static int update_bindings_file(const Bindings *bindings,
 				const char *bindings_file)
 {
@@ -237,6 +317,7 @@ static int update_bindings_file(const Bindings *bindings,
 	int fd = -1;
 	char tempname[PATH_MAX];
 	mode_t old_umask;
+	struct timespec ts;
 
 	if (safe_sprintf(tempname, "%s.XXXXXX", bindings_file))
 		return -1;
@@ -248,7 +329,7 @@ static int update_bindings_file(const Bindings *bindings,
 	}
 	umask(old_umask);
 	pthread_cleanup_push(cleanup_fd_ptr, &fd);
-	rc = write_bindings_file(bindings, fd);
+	rc = write_bindings_file(bindings, fd, &ts);
 	pthread_cleanup_pop(1);
 	if (rc == -1) {
 		condlog(1, "failed to write new bindings file");
@@ -257,8 +338,12 @@ static int update_bindings_file(const Bindings *bindings,
 	}
 	if ((rc = rename(tempname, bindings_file)) == -1)
 		condlog(0, "%s: rename: %m", __func__);
-	else
+	else {
+		pthread_mutex_lock(&timestamp_mutex);
+		bindings_last_updated = ts;
+		pthread_mutex_unlock(&timestamp_mutex);
 		condlog(1, "updated bindings file %s", bindings_file);
+	}
 	return rc;
 }
 
@@ -387,6 +472,7 @@ int get_free_id(const Bindings *bindings, const char *prefix, const char *map_ww
 	return id;
 }
 
+/* Called with binding_mutex held */
 static char *
 allocate_binding(const char *filename, const char *wwid, int id, const char *prefix)
 {
@@ -423,6 +509,30 @@ allocate_binding(const char *filename, const char *wwid, int id, const char *pre
 	return alias;
 }
 
+enum {
+	BINDINGS_FILE_UP2DATE,
+	BINDINGS_FILE_READ,
+	BINDINGS_FILE_ERROR,
+	BINDINGS_FILE_BAD,
+};
+
+static int _read_bindings_file(const struct config *conf, Bindings *bindings,
+			       bool force);
+
+static void read_bindings_file(void)
+{
+	struct config *conf;
+	Bindings bindings = {.allocated = 0, };
+	int rc;
+
+	conf = get_multipath_config();
+	pthread_cleanup_push(put_multipath_config, conf);
+	rc = _read_bindings_file(conf, &bindings, false);
+	pthread_cleanup_pop(1);
+	if (rc == BINDINGS_FILE_READ)
+		set_global_bindings(&bindings);
+}
+
 /*
  * get_user_friendly_alias() action table
  *
@@ -463,6 +573,11 @@ char *get_user_friendly_alias(const char *wwid, const char *file, const char *al
 	bool new_binding = false;
 	const struct binding *bdg;
 
+	read_bindings_file();
+
+	pthread_mutex_lock(&bindings_mutex);
+	pthread_cleanup_push(cleanup_mutex, &bindings_mutex);
+
 	if (!*alias_old)
 		goto new_alias;
 
@@ -514,40 +629,40 @@ new_alias:
 			alias, wwid);
 
 out:
+	/* unlock bindings_mutex */
+	pthread_cleanup_pop(1);
 	return alias;
 }
 
 int get_user_friendly_wwid(const char *alias, char *buff)
 {
 	const struct binding *bdg;
+	int rc = -1;
 
 	if (!alias || *alias == '\0') {
 		condlog(3, "Cannot find binding for empty alias");
 		return -1;
 	}
 
+	read_bindings_file();
+
+	pthread_mutex_lock(&bindings_mutex);
+	pthread_cleanup_push(cleanup_mutex, &bindings_mutex);
 	bdg = get_binding_for_alias(&global_bindings, alias);
-	if (!bdg) {
+	if (bdg) {
+		strlcpy(buff, bdg->wwid, WWID_SIZE);
+		rc = 0;
+	} else
 		*buff = '\0';
-		return -1;
-	}
-	strlcpy(buff, bdg->wwid, WWID_SIZE);
-	return 0;
-}
-
-static void free_bindings(Bindings *bindings)
-{
-	struct binding *bdg;
-	int i;
-
-	vector_foreach_slot(bindings, bdg, i)
-		_free_binding(bdg);
-	vector_reset(bindings);
+	pthread_cleanup_pop(1);
+	return rc;
 }
 
 void cleanup_bindings(void)
 {
+	pthread_mutex_lock(&bindings_mutex);
 	free_bindings(&global_bindings);
+	pthread_mutex_unlock(&bindings_mutex);
 }
 
 enum {
@@ -595,7 +710,20 @@ static int _check_bindings_file(const struct config *conf, FILE *file,
 	char *line = NULL;
 	size_t line_len = 0;
 	ssize_t n;
-
+	char header[sizeof(BINDINGS_FILE_HEADER)];
+
+	header[sizeof(BINDINGS_FILE_HEADER) - 1] = '\0';
+	if (fread(header, sizeof(BINDINGS_FILE_HEADER) - 1, 1, file) < 1) {
+		condlog(2, "%s: failed to read header from %s", __func__,
+			conf->bindings_file);
+		fseek(file, 0, SEEK_SET);
+		rc = -1;
+	} else if (strcmp(header, BINDINGS_FILE_HEADER)) {
+		condlog(2, "%s: invalid header in %s", __func__,
+			conf->bindings_file);
+		fseek(file, 0, SEEK_SET);
+		rc = -1;
+	}
 	pthread_cleanup_push(cleanup_free_ptr, &line);
 	while ((n = getline(&line, &line_len, file)) >= 0) {
 		char *alias, *wwid;
@@ -643,6 +771,68 @@ static int mp_alias_compar(const void *p1, const void *p2)
 			    &((*(struct mpentry * const *)p2)->alias));
 }
 
+static int _read_bindings_file(const struct config *conf, Bindings *bindings,
+			       bool force)
+{
+	int can_write;
+	int rc = 0, ret, fd;
+	FILE *file;
+	struct stat st;
+	int has_changed = uatomic_xchg(&bindings_file_changed, 0);
+
+	if (!force) {
+		if (!has_changed) {
+			condlog(4, "%s: bindings are unchanged", __func__);
+			return BINDINGS_FILE_UP2DATE;
+		}
+	}
+
+	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
+	if (fd == -1)
+		return BINDINGS_FILE_ERROR;
+
+	file = fdopen(fd, "r");
+	if (file != NULL) {
+		condlog(3, "%s: reading %s", __func__, conf->bindings_file);
+
+		pthread_cleanup_push(cleanup_fclose, file);
+		ret = _check_bindings_file(conf, file, bindings);
+		if (ret == 0) {
+			struct timespec ts;
+
+			rc = BINDINGS_FILE_READ;
+			ret = fstat(fd, &st);
+			if (ret == 0)
+				ts = st.st_mtim;
+			else {
+				condlog(1, "%s: fstat failed (%m), using current time", __func__);
+				clock_gettime(CLOCK_REALTIME_COARSE, &ts);
+			}
+			pthread_mutex_lock(&timestamp_mutex);
+			bindings_last_updated = ts;
+			pthread_mutex_unlock(&timestamp_mutex);
+		} else if (ret == -1 && can_write && !conf->bindings_read_only) {
+			ret = update_bindings_file(bindings, conf->bindings_file);
+			if (ret == 0)
+				rc = BINDINGS_FILE_READ;
+			else
+				rc = BINDINGS_FILE_BAD;
+		} else {
+			condlog(0, "ERROR: bad settings in read-only bindings file %s",
+				conf->bindings_file);
+			rc = BINDINGS_FILE_BAD;
+		}
+		pthread_cleanup_pop(1);
+	} else {
+		condlog(1, "failed to fdopen %s: %m",
+			conf->bindings_file);
+		close(fd);
+		rc = BINDINGS_FILE_ERROR;
+	}
+
+	return rc;
+}
+
 /*
  * check_alias_settings(): test for inconsistent alias configuration
  *
@@ -661,8 +851,7 @@ static int mp_alias_compar(const void *p1, const void *p2)
  */
 int check_alias_settings(const struct config *conf)
 {
-	int can_write;
-	int rc = 0, i, fd;
+	int i, rc;
 	Bindings bindings = {.allocated = 0, };
 	vector mptable = NULL;
 	struct mpentry *mpe;
@@ -695,27 +884,12 @@ int check_alias_settings(const struct config *conf)
 	pthread_cleanup_pop(1);
 	pthread_cleanup_pop(1);
 
-	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
-	if (fd != -1) {
-		FILE *file = fdopen(fd, "r");
-
-		if (file != NULL) {
-			pthread_cleanup_push(cleanup_fclose, file);
-			rc = _check_bindings_file(conf, file, &bindings);
-			pthread_cleanup_pop(1);
-			if (rc == -1 && can_write && !conf->bindings_read_only)
-				rc = update_bindings_file(&bindings, conf->bindings_file);
-			else if (rc == -1)
-				condlog(0, "ERROR: bad settings in read-only bindings file %s",
-					conf->bindings_file);
-		} else {
-			condlog(1, "failed to fdopen %s: %m",
-				conf->bindings_file);
-			close(fd);
-		}
+	rc = _read_bindings_file(conf, &bindings, true);
+
+	if (rc == BINDINGS_FILE_READ) {
+		set_global_bindings(&bindings);
+		rc = 0;
 	}
 
-	cleanup_bindings();
-	global_bindings = bindings;
 	return rc;
 }
diff --git a/libmultipath/alias.h b/libmultipath/alias.h
index 5ef6720b..ca8911f4 100644
--- a/libmultipath/alias.h
+++ b/libmultipath/alias.h
@@ -10,5 +10,6 @@ char *get_user_friendly_alias(const char *wwid, const char *file,
 struct config;
 int check_alias_settings(const struct config *);
 void cleanup_bindings(void);
-
+struct inotify_event;
+void handle_bindings_file_inotify(const struct inotify_event *event);
 #endif /* _ALIAS_H */
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index ddd302f5..57e50c12 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -238,3 +238,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_20.1.0 {
+global:
+	handle_bindings_file_inotify;
+};
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 02e89fb4..d1f8f234 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -41,6 +41,7 @@
 #include "cli.h"
 #include "uxlsnr.h"
 #include "strbuf.h"
+#include "alias.h"
 
 /* state of client connection */
 enum {
@@ -190,6 +191,7 @@ void wakeup_cleanup(void *arg)
 struct watch_descriptors {
 	int conf_wd;
 	int dir_wd;
+	int mp_wd; /* /etc/multipath; for bindings file */
 };
 
 /* failing to set the watch descriptor is o.k. we just miss a warning
@@ -200,6 +202,8 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
 	struct config *conf;
 	int dir_reset = 0;
 	int conf_reset = 0;
+	int mp_reset = 0;
+	char *bindings_file __attribute__((cleanup(cleanup_charp))) = NULL;
 
 	if (notify_fd == -1)
 		return;
@@ -214,7 +218,10 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
 			conf_reset = 1;
 		if (wds->dir_wd == -1)
 			dir_reset = 1;
+		if (wds->mp_wd == -1)
+			mp_reset = 1;
 	}
+	bindings_file = strdup(conf->bindings_file);
 	put_multipath_config(conf);
 
 	if (dir_reset) {
@@ -235,7 +242,18 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
 		if (wds->conf_wd == -1)
 			condlog(3, "didn't set up notifications on /etc/multipath.conf: %m");
 	}
-	return;
+	if (mp_reset && bindings_file) {
+		char *slash = strrchr(bindings_file, '/');
+
+		if (slash && slash > bindings_file) {
+			*slash = '\0';
+			wds->mp_wd = inotify_add_watch(notify_fd, bindings_file,
+						       IN_MOVED_TO|IN_ONLYDIR);
+			if (wds->mp_wd == -1)
+				condlog(3, "didn't set up notifications on %s: %m",
+					bindings_file);
+		}
+	}
 }
 
 static void handle_inotify(int fd, struct watch_descriptors *wds)
@@ -256,12 +274,13 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 					inotify_rm_watch(fd, wds->conf_wd);
 				if (wds->dir_wd != -1)
 					inotify_rm_watch(fd, wds->dir_wd);
-				wds->conf_wd = wds->dir_wd = -1;
+				if (wds->mp_wd != -1)
+					inotify_rm_watch(fd, wds->mp_wd);
+				wds->conf_wd = wds->dir_wd = wds->mp_wd = -1;
 			}
 			break;
 		}
 
-		got_notify = 1;
 		for (ptr = buff; ptr < buff + len;
 		     ptr += sizeof(struct inotify_event) + event->len) {
 			event = (const struct inotify_event *) ptr;
@@ -273,7 +292,13 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 					wds->conf_wd = inotify_add_watch(notify_fd, DEFAULT_CONFIGFILE, IN_CLOSE_WRITE);
 				else if (wds->dir_wd == event->wd)
 					wds->dir_wd = -1;
+				else if (wds->mp_wd == event->wd)
+					wds->mp_wd = -1;
 			}
+			if (wds->mp_wd != -1 && wds->mp_wd == event->wd)
+				handle_bindings_file_inotify(event);
+			else
+				got_notify = 1;
 		}
 	}
 	if (got_notify)
@@ -599,7 +624,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	int max_pfds = MIN_POLLS + POLLFDS_BASE;
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
 	unsigned int sequence_nr = 0;
-	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
+	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1, .mp_wd = -1, };
 	struct vectors *vecs = trigger_data;
 
 	condlog(3, "uxsock: startup listener");
@@ -666,7 +691,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 
 		reset_watch(notify_fd, &wds, &sequence_nr);
 		polls[POLLFD_NOTIFY].fd = notify_fd;
-		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
+		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1
+					&& wds.mp_wd == -1))
 			polls[POLLFD_NOTIFY].events = 0;
 		else
 			polls[POLLFD_NOTIFY].events = POLLIN;
diff --git a/tests/alias.c b/tests/alias.c
index 7f3ff38a..9ae27567 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -1954,6 +1954,9 @@ int main(void)
 	int ret = 0;
 	init_test_verbosity(3);
 
+	/* avoid open_file() call in _read_bindings_file */
+	bindings_file_changed = 0;
+
 	ret += test_format_devname();
 	ret += test_scan_devname();
 	ret += test_lookup_binding();