| Message ID | 20230914145131.15165-3-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: user-friendly names rework | expand |
On Thu, Sep 14, 2023 at 04:51:30PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > 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. > Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> > Signed-off-by: Martin Wilck <mwilck@suse.com> > > libmultipath: protect global_bindings with a mutex > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > libmultipath: check timestamp of bindings file before reading it > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/alias.c | 252 +++++++++++++++++++++++++----- > libmultipath/alias.h | 3 +- > libmultipath/libmultipath.version | 5 + > multipathd/uxlsnr.c | 36 ++++- > tests/alias.c | 3 + > 5 files changed, 254 insertions(+), 45 deletions(-) > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c > index 66e34e3..964b8a7 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(×tamp_mutex); > + if (ret == 0) { > + ts = st.st_mtim; > + changed = timespeccmp(&ts, &bindings_last_updated) > 0; > + } > + pthread_mutex_unlock(×tamp_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(×tamp_mutex); > + bindings_last_updated = ts; > + pthread_mutex_unlock(×tamp_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(×tamp_mutex); > + bindings_last_updated = ts; > + pthread_mutex_unlock(×tamp_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"); > + rc = _read_bindings_file(conf, &bindings, true); > > - 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); > - } > + 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 5ef6720..ca8911f 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 ddd302f..57e50c1 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 02e89fb..d1f8f23 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 4d0adba..d41d396 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(); > -- > 2.42.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/alias.c b/libmultipath/alias.c index 66e34e3..964b8a7 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(×tamp_mutex); + if (ret == 0) { + ts = st.st_mtim; + changed = timespeccmp(&ts, &bindings_last_updated) > 0; + } + pthread_mutex_unlock(×tamp_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(×tamp_mutex); + bindings_last_updated = ts; + pthread_mutex_unlock(×tamp_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(×tamp_mutex); + bindings_last_updated = ts; + pthread_mutex_unlock(×tamp_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"); + rc = _read_bindings_file(conf, &bindings, true); - 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); - } + 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 5ef6720..ca8911f 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 ddd302f..57e50c1 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 02e89fb..d1f8f23 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 4d0adba..d41d396 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();