On Thu, Aug 18, 2022 at 11:06:29PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> add_binding() contains an optimization; it assumes that the list of
> bindings is alphabetically sorted by alias, and tries to maintain
> this order.
>
> But conf->mptable is sorted by wwid. Therefore check_alias_settings() may take
> a long time if the mptable is large.
>
> Create a copy of the mptable, sorted by alias, and use it for add_bindings().
> This speeds up check_alias_settings by roughly a factor of 10 (0.1s vs. 1s)
> for my test setup with 10000 entries in the mptable.
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> libmultipath/alias.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 548a118..60428fe 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -672,6 +672,26 @@ static void cleanup_fclose(void *p)
> fclose(p);
> }
>
> +static int alias_compar(const void *p1, const void *p2)
> +{
> + const char *alias1 = (*(struct mpentry * const *)p1)->alias;
> + const char *alias2 = (*(struct mpentry * const *)p2)->alias;
> +
> + if (!alias1 && !alias2)
> + return 0;
> + if (!alias1)
> + return 1;
> + if (!alias2)
> + return -1;
> + return strcmp(alias1, alias2);
> +}
> +
> +static void cleanup_vector_free(void *arg)
> +{
> + if (arg)
> + vector_free((vector)arg);
> +}
> +
> /*
> * check_alias_settings(): test for inconsistent alias configuration
> *
> @@ -693,10 +713,16 @@ int check_alias_settings(const struct config *conf)
> int can_write;
> int rc = 0, i, fd;
> Bindings bindings = {.allocated = 0, };
> + vector mptable = NULL;
> struct mpentry *mpe;
>
> pthread_cleanup_push_cast(free_bindings, &bindings);
> - vector_foreach_slot(conf->mptable, mpe, i) {
> + pthread_cleanup_push(cleanup_vector_free, mptable);
> + mptable = vector_convert(NULL, conf->mptable, struct mpentry *, identity);
> + if (!mptable)
> + return -1;
Are there other places in the code where we return without popping the
cleanup handler? According to the man page "POSIX.1 says that the effect
of using return, break, continue, or goto to prematurely leave a
block bracketed pthread_cleanup_push() and pthread_cleanup_pop() is
undefined. Portable applications should avoid doing this." It also says
that linux implements these as macros that expand to create code blocks.
So, I'm pretty sure this is safe in linux, but if we aren't already
doing it, we probably shouldn't start, especially since vector_convert()
doesn't have any pthread cancellation points, so we can just move the
push until after we are sure we successfully copied the vector.
> + vector_sort(mptable, alias_compar);
> + vector_foreach_slot(mptable, mpe, i) {
> if (!mpe->wwid || !mpe->alias)
Unless I'm missing something, merge_mptable() should have already
guaranteed that mpe->wwid must be non-NULL. Also, since mptable has all
of the entries with a NULL alias sorted to the end, as soon as we hit
one, we can stop checking.
-Ben
> continue;
> if (add_binding(&bindings, mpe->alias, mpe->wwid) ==
> @@ -710,6 +736,7 @@ int check_alias_settings(const struct config *conf)
> }
> /* This clears the bindings */
> pthread_cleanup_pop(1);
> + pthread_cleanup_pop(1);
>
> pthread_cleanup_push_cast(free_bindings, &bindings);
> fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
> --
> 2.37.1
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
