On Fri, Sep 01, 2023 at 08:02:31PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> Rather than opening the bindings file every time we must retrieve
> a binding, keep the contents in memory and write the file only
> if additions have been made. This simplifies the code, and should speed up
> alias lookups significantly. As a side effect, the aliases will be stored
> sorted by alias, which changes the way aliases are allocated if there are
> unused "holes" in the sequence of aliases. For example, if the bindings file
> contains mpathb, mpathy, and mpatha, in this order, the next new alias used to
> be mpathz and is now mpathc.
>
> Another side effect is that multipathd will not automatically pick up changes
> to the bindings file at runtime without a reconfigure operation. It is
> questionable whether these on-the-fly changes were a good idea in the first
> place, as inconsistent configurations may easily come to pass. It desired,
> it would be feasible to implement automatic update of the bindings using the
> existing inotify approach.
I'm not so much worried about someone manually changing the bindings
file outside of multipath. But it is possible for multipathd to miss
changes made by the multipath command. For instance, say that someone
has find_multipaths set to "yes" and adds a new device, but only a
single path comes up. They know there will be more paths later, so they
run
# multipath <new_path_dev>
to create a multipath device for this. Multipathd won't pick up this
new binding. If, for some reason the path goes away, and comes back
later, it will now be in the bindings file, but multipathd will still
have no record of the correct binding for it, which already exists in
the bindings file. I don't know if this needs something as complex as
inotify to handle. We could just record the mtime of the bindings file
when we read it. If it has changed when we call
get_user_friendly_alias() or get_user_friendly_wwid() we would call
check_alias_settings().
additional comments below.
> The new implementation of get_user_friendly_alias() is slightly different
> than before. The logic is summarized in a comment in the code. Unit tests
> will be provided that illustrate the changes.
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> libmultipath/alias.c | 369 ++++++++++++++++-----------------------
> libmultipath/alias.h | 2 +-
> libmultipath/configure.c | 3 +-
> 3 files changed, 157 insertions(+), 217 deletions(-)
>
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 2f9bc82..6003df0 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -50,8 +50,6 @@
> "# alias wwid\n" \
> "#\n"
>
> -static const char bindings_file_header[] = BINDINGS_FILE_HEADER;
> -
> struct binding {
> char *alias;
> char *wwid;
> @@ -80,6 +78,45 @@ static void _free_binding(struct binding *bdg)
> free(bdg);
> }
>
> +static const struct binding *get_binding_for_alias(const Bindings *bindings,
> + const char *alias)
> +{
> + const struct binding *bdg;
> + int i;
> +
> + if (!alias)
> + return NULL;
> + vector_foreach_slot(bindings, bdg, i) {
> + if (!strncmp(bdg->alias, alias, WWID_SIZE)) {
> + condlog(3, "Found matching alias [%s] in bindings file."
> + " Setting wwid to %s", alias, bdg->wwid);
> + return bdg;
> + }
> + }
> +
> + condlog(3, "No matching alias [%s] in bindings file.", alias);
> + return NULL;
> +}
> +
> +static const struct binding *get_binding_for_wwid(const Bindings *bindings,
> + const char *wwid)
> +{
> + const struct binding *bdg;
> + int i;
> +
> + if (!wwid)
> + return NULL;
> + vector_foreach_slot(bindings, bdg, i) {
> + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) {
> + condlog(3, "Found matching wwid [%s] in bindings file."
> + " Setting alias to %s", wwid, bdg->alias);
> + return bdg;
> + }
> + }
> + condlog(3, "No matching wwid [%s] in bindings file.", wwid);
> + return NULL;
> +}
> +
> static int add_binding(Bindings *bindings, const char *alias, const char *wwid)
> {
> struct binding *bdg;
> @@ -115,6 +152,24 @@ static int add_binding(Bindings *bindings, const char *alias, const char *wwid)
> return BINDING_ERROR;
> }
>
> +static int delete_binding(Bindings *bindings, const char *wwid)
> +{
> + struct binding *bdg;
> + int i;
> +
> + vector_foreach_slot(bindings, bdg, i) {
> + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) {
> + _free_binding(bdg);
> + break;
> + }
> + }
> + if (i >= VECTOR_SIZE(bindings))
> + return BINDING_NOTFOUND;
> +
> + vector_del_slot(bindings, i);
> + return BINDING_DELETED;
> +}
> +
> static int write_bindings_file(const Bindings *bindings, int fd)
> {
> struct binding *bnd;
> @@ -267,38 +322,15 @@ static bool id_already_taken(int id, const char *prefix, const char *map_wwid)
> return alias_already_taken(alias, map_wwid);
> }
>
> -/*
> - * Returns: 0 if matching entry in WWIDs file found
> - * -1 if an error occurs
> - * >0 a free ID that could be used for the WWID at hand
> - * *map_alias is set to a freshly allocated string with the matching alias if
> - * the function returns 0, or to NULL otherwise.
> - */
> -static int
> -lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
> - const char *prefix, int check_if_taken)
> +int get_free_id(const Bindings *bindings, const char *prefix, const char *map_wwid)
> {
> - char buf[LINE_MAX];
> - unsigned int line_nr = 0;
> - int id = 1;
> + const struct binding *bdg;
> + int i, id = 1;
> int biggest_id = 1;
> int smallest_bigger_id = INT_MAX;
>
> - *map_alias = NULL;
> -
> - rewind(f);
> - while (fgets(buf, LINE_MAX, f)) {
> - const char *alias, *wwid;
> - char *c, *saveptr;
> - int curr_id;
> -
> - line_nr++;
> - c = strpbrk(buf, "#\n\r");
> - if (c)
> - *c = '\0';
> - alias = strtok_r(buf, " \t", &saveptr);
> - if (!alias) /* blank line */
> - continue;
> + vector_foreach_slot(bindings, bdg, i) {
> + int curr_id = scan_devname(bdg->alias, prefix);
If we know that the bindings will be sorted by alias order, we don't
need to do all this. Something like this should work:
if (curr_id <= 0)
continue;
while (id < curr_id) {
if (!id_already_taken(id, prefix, map_wwid))
return id;
id++;
}
if (id == INT_MAX)
break;
id++;
}
if (id == INT_MAX)
condlog(0, "no more available user_friendly_names");
return id < INT_MAX ? id : -1;
}
>
> /*
> * Find an unused index - explanation of the algorithm
> @@ -333,8 +365,6 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
> * biggest_id is always > smallest_bigger_id, except in the
> * "perfectly ordered" case.
> */
> -
> - curr_id = scan_devname(alias, prefix);
> if (curr_id == id) {
> if (id < INT_MAX)
> id++;
> @@ -345,36 +375,15 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
> }
> if (curr_id > biggest_id)
> biggest_id = curr_id;
> +
> if (curr_id > id && curr_id < smallest_bigger_id)
> smallest_bigger_id = curr_id;
> - wwid = strtok_r(NULL, " \t", &saveptr);
> - if (!wwid){
> - condlog(3,
> - "Ignoring malformed line %u in bindings file",
> - line_nr);
> - continue;
> - }
> - if (strcmp(wwid, map_wwid) == 0){
> - condlog(3, "Found matching wwid [%s] in bindings file."
> - " Setting alias to %s", wwid, alias);
> - *map_alias = strdup(alias);
> - if (*map_alias == NULL) {
> - condlog(0, "Cannot copy alias from bindings "
> - "file: out of memory");
> - return -1;
> - }
> - return 0;
> - }
> }
> - if (!prefix && check_if_taken)
> - id = -1;
> - if (id >= smallest_bigger_id) {
> - if (biggest_id < INT_MAX)
> - id = biggest_id + 1;
> - else
> - id = -1;
> - }
> - if (id > 0 && check_if_taken) {
> +
> + if (id >= smallest_bigger_id)
> + id = biggest_id < INT_MAX ? biggest_id + 1 : -1;
> +
> + if (id > 0) {
> while(id_already_taken(id, prefix, map_wwid)) {
> if (id == INT_MAX) {
> id = -1;
> @@ -391,63 +400,16 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
> }
> }
> }
> - if (id < 0) {
> +
> + if (id < 0)
> condlog(0, "no more available user_friendly_names");
> - return -1;
> - } else
> - condlog(3, "No matching wwid [%s] in bindings file.", map_wwid);
> return id;
> }
>
> -static int
> -rlookup_binding(FILE *f, char *buff, const char *map_alias)
> -{
> - char line[LINE_MAX];
> - unsigned int line_nr = 0;
> -
> - buff[0] = '\0';
> -
> - while (fgets(line, LINE_MAX, f)) {
> - char *c, *saveptr;
> - const char *alias, *wwid;
> -
> - line_nr++;
> - c = strpbrk(line, "#\n\r");
> - if (c)
> - *c = '\0';
> - alias = strtok_r(line, " \t", &saveptr);
> - if (!alias) /* blank line */
> - continue;
> - wwid = strtok_r(NULL, " \t", &saveptr);
> - if (!wwid){
> - condlog(3,
> - "Ignoring malformed line %u in bindings file",
> - line_nr);
> - continue;
> - }
> - if (strlen(wwid) > WWID_SIZE - 1) {
> - condlog(3,
> - "Ignoring too large wwid at %u in bindings file", line_nr);
> - continue;
> - }
> - if (strcmp(alias, map_alias) == 0){
> - condlog(3, "Found matching alias [%s] in bindings file."
> - " Setting wwid to %s", alias, wwid);
> - strlcpy(buff, wwid, WWID_SIZE);
> - return 0;
> - }
> - }
> - condlog(3, "No matching alias [%s] in bindings file.", map_alias);
> -
> - return -1;
> -}
> -
> static char *
> -allocate_binding(int fd, const char *wwid, int id, const char *prefix)
> +allocate_binding(const char *filename, const char *wwid, int id, const char *prefix)
> {
> STRBUF_ON_STACK(buf);
> - off_t offset;
> - ssize_t len;
> char *alias, *c;
>
> if (id <= 0) {
> @@ -460,29 +422,22 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix)
> format_devname(&buf, id) == -1)
> return NULL;
>
> - if (print_strbuf(&buf, " %s\n", wwid) < 0)
> - return NULL;
> -
> - offset = lseek(fd, 0, SEEK_END);
> - if (offset < 0){
> - condlog(0, "Cannot seek to end of bindings file : %s",
> - strerror(errno));
> - return NULL;
> - }
> -
> - len = get_strbuf_len(&buf);
> alias = steal_strbuf_str(&buf);
>
> - if (write(fd, alias, len) != len) {
> - condlog(0, "Cannot write binding to bindings file : %s",
> - strerror(errno));
> - /* clear partial write */
> - if (ftruncate(fd, offset))
> - condlog(0, "Cannot truncate the header : %s",
> - strerror(errno));
> + if (add_binding(&global_bindings, alias, wwid) != BINDING_ADDED) {
> + condlog(0, "%s: cannot allocate new binding %s for %s",
> + __func__, alias, wwid);
> free(alias);
> return NULL;
> }
> +
> + if (update_bindings_file(&global_bindings, filename) == -1) {
> + condlog(1, "%s: deleting binding %s for %s", __func__, alias, wwid);
> + delete_binding(&global_bindings, wwid);
> + free(alias);
> + return NULL;
> + }
> +
We no longer need to end the alias at the space, since we're not
printing the wwid in the buffer.
> c = strchr(alias, ' ');
> if (c)
> *c = '\0';
> @@ -491,144 +446,130 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix)
> return alias;
> }
>
> +/*
> + * get_user_friendly_alias() action table
> + *
> + * The table shows the various cases, the actions taken, and the CI
> + * functions from tests/alias.c that represent them.
> + *
> + * - O: old alias given
> + * - A: old alias in table (y: yes, correct WWID; X: yes, wrong WWID)
> + * - W: wwid in table
> + * - U: old alias is used
> + *
> + * | No | O | A | W | U | action | function gufa_X |
> + * |----+---+---+---+---+--------------------------------------------+------------------------------|
> + * | 1 | n | - | n | - | get new alias | nomatch_Y |
> + * | 2 | n | - | y | - | use alias from bindings | match_a_Y |
> + * | 3 | y | n | n | n | add binding for old alias | old_nomatch_nowwidmatch |
> + * | 4 | y | n | n | y | error, no alias (can't add entry) | old_nomatch_nowwidmatch_used |
> + * | 5 | y | n | y | - | use alias from bindings (avoid duplicates) | old_nomatch_wwidmatch |
> + * | 6 | y | y | n | - | [ impossible ] | - |
> + * | 7 | y | y | y | n | use old alias == alias from bindings | old_match |
> + * | 8 | y | y | y | y | error, no alias (would be duplicate) | old_match_used |
> + * | 9 | y | X | n | - | get new alias | old_match_other |
> + * | 10 | y | X | y | - | use alias from bindings | old_match_other_wwidmatch |
> + *
> + * Notes:
> + * - "use alias from bindings" means that the alias from the bindings file will
> + * be tried; if it is in use, the alias selection will fail. No other
> + * bindings will be attempted.
> + * - "get new alias" fails if all aliases are used up, or if writing the
> + * bindings file fails.
> + */
> +
> char *get_user_friendly_alias(const char *wwid, const char *file, const char *alias_old,
> const char *prefix, bool bindings_read_only)
> {
> char *alias = NULL;
> int id = 0;
> - int fd, can_write;
> bool new_binding = false;
> - char buff[WWID_SIZE];
> - FILE *f;
> + bool old_alias_taken = false;
> + const struct binding *bdg;
>
> - fd = open_file(file, &can_write, bindings_file_header);
> - if (fd < 0)
> - return NULL;
> -
> - f = fdopen(fd, "r");
> - if (!f) {
> - condlog(0, "cannot fdopen on bindings file descriptor");
> - close(fd);
> - return NULL;
> - }
> -
> - if (!strlen(alias_old))
> + if (!*alias_old)
> goto new_alias;
>
> - /* lookup the binding. if it exists, the wwid will be in buff
> - * either way, id contains the id for the alias
> - */
> - rlookup_binding(f, buff, alias_old);
> -
> - if (strlen(buff) > 0) {
> - /* if buff is our wwid, it's already
> - * allocated correctly
> - */
> - if (strcmp(buff, wwid) == 0) {
> + /* See if there's a binding matching both alias_old and wwid */
> + bdg = get_binding_for_alias(&global_bindings, alias_old);
> + if (bdg) {
> + if (!strcmp(bdg->wwid, wwid)) {
I'm still not convinced that it is possible for mpp->alias_old to be set
when there isn't a multipath device with the alias_old and the desired
wwid. Unless I'm missing something we should be able to skip the
alias_already_taken() check.
> if (!alias_already_taken(alias_old, wwid))
> alias = strdup(alias_old);
> else
> condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map",
> alias_old, wwid);
> goto out;
> -
> } else {
> condlog(0, "alias %s already bound to wwid %s, cannot reuse",
> - alias_old, buff);
> - goto new_alias; ;
> + alias_old, bdg->wwid);
> + goto new_alias;
> }
> }
>
> - /*
> - * Look for an existing alias in the bindings file.
> - * Pass prefix = NULL, so lookup_binding() won't try to allocate a new id.
> - */
> - id = lookup_binding(f, wwid, &alias, NULL, 0);
> - if (alias) {
> - if (alias_already_taken(alias, wwid)) {
> - free(alias);
> - alias = NULL;
> - } else
> - condlog(3, "Use existing binding [%s] for WWID [%s]",
> - alias, wwid);
> - goto out;
> - }
> -
> /* allocate the existing alias in the bindings file */
> - id = scan_devname(alias_old, prefix);
> - if (id > 0 && id_already_taken(id, prefix, wwid)) {
again, I think it's safe to skip this check, since we're checking
alias_old.
> + if (alias_already_taken(alias_old, wwid)) {
> condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map",
> alias_old, wwid);
> + old_alias_taken = true;
> + } else
> + id = scan_devname(alias_old, prefix);
> +
> +new_alias:
> + /* Check for existing binding of WWID */
> + bdg = get_binding_for_wwid(&global_bindings, wwid);
> + if (bdg) {
> + if (!alias_already_taken(bdg->alias, wwid)) {
> + condlog(3, "Use existing binding [%s] for WWID [%s]",
> + bdg->alias, wwid);
> + alias = strdup(bdg->alias);
> + }
> goto out;
> }
>
> -new_alias:
> - if (id <= 0) {
> + /*
> + * old_alias_taken means that the WWID is not in the bindings file, but
> + * alias_old is currently taken by a different WWID. We shouldn't added
> + * a new binding in this case.
> + */
> + if (id <= 0 && !old_alias_taken) {
> /*
> * no existing alias was provided, or allocating it
> * failed. Try a new one.
> */
> - id = lookup_binding(f, wwid, &alias, prefix, 1);
> - if (id == 0 && alias_already_taken(alias, wwid)) {
> - free(alias);
> - alias = NULL;
> - }
> + id = get_free_id(&global_bindings, prefix, wwid);
> if (id <= 0)
> goto out;
> else
> new_binding = true;
> }
>
> - if (fflush(f) != 0) {
> - condlog(0, "cannot fflush bindings file stream : %s",
> - strerror(errno));
> - goto out;
> - }
> + if (!bindings_read_only && id > 0)
> + alias = allocate_binding(file, wwid, id, prefix);
>
> - if (can_write && !bindings_read_only) {
> - alias = allocate_binding(fd, wwid, id, prefix);
> - if (alias && !new_binding)
> - condlog(2, "Allocated existing binding [%s] for WWID [%s]",
> - alias, wwid);
> - }
> + if (alias && !new_binding)
> + condlog(2, "Allocated existing binding [%s] for WWID [%s]",
> + alias, wwid);
>
> out:
> - pthread_cleanup_push(free, alias);
> - fclose(f);
> - pthread_cleanup_pop(0);
> return alias;
> }
>
> -int
> -get_user_friendly_wwid(const char *alias, char *buff, const char *file)
> +int get_user_friendly_wwid(const char *alias, char *buff)
> {
> - int fd, unused;
> - FILE *f;
> + const struct binding *bdg;
>
> if (!alias || *alias == '\0') {
> condlog(3, "Cannot find binding for empty alias");
> return -1;
> }
>
> - fd = open_file(file, &unused, bindings_file_header);
> - if (fd < 0)
> - return -1;
> -
> - f = fdopen(fd, "r");
> - if (!f) {
> - condlog(0, "cannot fdopen on bindings file descriptor : %s",
> - strerror(errno));
> - close(fd);
> + bdg = get_binding_for_alias(&global_bindings, alias);
> + if (!bdg) {
> + *buff = '\0';
> return -1;
> }
> -
> - rlookup_binding(f, buff, alias);
> - if (!strlen(buff)) {
> - fclose(f);
> - return -1;
> - }
> -
> - fclose(f);
I think you mean bdg->wwid here.
-Ben
> + strlcpy(buff, bdg->alias, WWID_SIZE);
> return 0;
> }
>
> diff --git a/libmultipath/alias.h b/libmultipath/alias.h
> index 37b49d9..5ef6720 100644
> --- a/libmultipath/alias.h
> +++ b/libmultipath/alias.h
> @@ -2,7 +2,7 @@
> #define _ALIAS_H
>
> int valid_alias(const char *alias);
> -int get_user_friendly_wwid(const char *alias, char *buff, const char *file);
> +int get_user_friendly_wwid(const char *alias, char *buff);
> char *get_user_friendly_alias(const char *wwid, const char *file,
> const char *alias_old,
> const char *prefix, bool bindings_read_only);
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 029fbbd..d809490 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1378,8 +1378,7 @@ static int _get_refwwid(enum mpath_cmds cmd, const char *dev,
> refwwid = tmpwwid;
>
> /* or may be a binding */
> - else if (get_user_friendly_wwid(dev, tmpwwid,
> - conf->bindings_file) == 0)
> + else if (get_user_friendly_wwid(dev, tmpwwid) == 0)
> refwwid = tmpwwid;
>
> /* or may be an alias */
> --
> 2.41.0
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
