On Tue, 2022-08-23 at 16:27 -0500, Benjamin Marzinski wrote:
> 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?
Of course not. Stupid mistake.
> 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.
Right, thanks.
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
