On Tue, 2022-08-23 at 15:48 -0500, Benjamin Marzinski wrote:
> On Thu, Aug 18, 2022 at 11:06:28PM +0200, mwilck@xxxxxxxx wrote:
> > From: Martin Wilck <mwilck@xxxxxxxx>
> >
> > If the mptable is very large (for example, in a configuration with
> > lots of maps assigned individual aliases), merge_mptable may get
> > very slow because it needs to make O(n^2) string comparisons (with
> > n being the number of mptable entries). WWID strings often differ
> > only in the last few bytes, causing a lot of CPU time spent in
> > strcmp().
> >
> > merge_mptable is executed whenever multipath or multipathd is
> > started, that
> > is, also for "multipath -u" and "multipath -U" invocations from
> > udev rules.
> > Optimize it by sorting the mptable before merging. This way we
> > don't need
> > to iterate towards the end of the vector searching for duplicates.
> >
> > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > ---
> > libmultipath/config.c | 15 +++++++++++++--
> > libmultipath/vector.c | 8 ++++++++
> > libmultipath/vector.h | 1 +
> > 3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/libmultipath/config.c b/libmultipath/config.c
> > index ab8b26e..a2c79a4 100644
> > --- a/libmultipath/config.c
> > +++ b/libmultipath/config.c
> > @@ -520,6 +520,14 @@ merge_mpe(struct mpentry *dst, struct mpentry
> > *src)
> > merge_num(mode);
> > }
> >
> > +static int wwid_compar(const void *p1, const void *p2)
> > +{
> > + const char *wwid1 = (*(struct mpentry * const *)p1)->wwid;
> > + const char *wwid2 = (*(struct mpentry * const *)p2)->wwid;
> > +
> > + return strcmp(wwid1, wwid2);
> > +}
> > +
> > void merge_mptable(vector mptable)
> > {
> > struct mpentry *mp1, *mp2;
> > @@ -533,10 +541,13 @@ void merge_mptable(vector mptable)
> > free_mpe(mp1);
> > continue;
> > }
> > + }
> > + vector_sort(mptable, wwid_compar);
> > + vector_foreach_slot(mptable, mp1, i) {
> > j = i + 1;
> > vector_foreach_slot_after(mptable, mp2, j) {
> > - if (!mp2->wwid || strcmp(mp1->wwid, mp2-
> > >wwid))
> > - continue;
> > + if (strcmp(mp1->wwid, mp2->wwid))
> > + break;
> > condlog(1, "%s: duplicate multipath config
> > section for %s",
> > __func__, mp1->wwid);
> > merge_mpe(mp2, mp1);
>
> The way merge_mpe() works, values are only copied from mp1's
> variables
> if the corresponding variable is unset in mp2. This requires the
> original order of mp1 and mp2 to be unchanged by the sorting
> algorithm,
> but according to the qsort() man page "If two members compare as
> equal,
> their order in the sorted array is undefined."
>
> One quick and dirty way we could fix this is to add a variable to
> struct
> mptable called config_idx, which is its index in the config file. If
> the wwids are equal, you compare that.
Thanks for pointing this out. I believe it's easier than that: as we're
passed the offsets into the array (aka struct mpentry **), we can
simply compare p1 and p2 if the strings are equal.
Agree?
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
