Re: [dm-devel] [PATCH 18/21] libmultipath: keep bindings in memory

[Martin Wilck <mwilck@suse.com>]

On Thu, 2023-09-07 at 15:02 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 07, 2023 at 02:14:04PM -0500, Benjamin Marzinski wrote:
> > On Thu, Sep 07, 2023 at 12:30:53PM +0200, Martin Wilck wrote:
> > > On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote:
> > > > On Fri, Sep 01, 2023 at 08:02:31PM +0200,
> > > > mwilck@suse.com wrote:
> > > > > From: Martin Wilck <mwilck@suse.com>
> > > > > 
> > 
> > > > > +       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:
> > > 
> > > That's true. Unfortunately, we can't ensure ordering "by alias
> > > order".
> > > The reason is that our configuration allows using different alias
> > > prefixes for different devices. The current sorting is simply
> > > alphabetical. It detects duplicates, but it ensures "alias order"
> > > only
> > > between "mpatha" and "mpathz".
> > > 
> > > I've thought of just removing the "different aliases for
> > > different
> > > devices" feature. I don't know if any users out there actually
> > > set
> > > device-specific alias_prefix values in the devices section of
> > > multipath.conf. I don't recall having seen such a configuration
> > > so far;
> > > almost every config I have seen simply uses "mpath" everywhere.
> > > But I
> > > recognize that it may feel tempting in some cases. One could use
> > > the
> > > "NETAPP" prefix for some arrays and the "EMC" prefix for others,
> > > for
> > > example, making it easier to see which is what. And we simply
> > > don't
> > > know if we'd break existing user setups with such a change. So if
> > > at
> > > all, we can't do it in a minor release without deprecating it
> > > first.
> > > 
> > > When we add a binding in add_binding(), we don't know which
> > > alias_prefix is configured for a given WWID, and we have no easy
> > > way to
> > > figure it out. We know the alias, but we don't know which portion
> > > of it
> > > is the prefix and which is the ID (it's not forbidden to use
> > > "aaaa" as
> > > alias_prefix). I wouldn't want to start guessing.
> > > 
> > > If you can think of a way to keep the bindings cleanly sorted,
> > > please
> > > let me know.
> > 
> > Doh! You even mentioned this issue in your "fix alias tests"
> > commit. I
> > just didn't pay enough attention to it. But I believe there is a
> > way to
> > make sure that we are dealing with a correctly sorted list of
> > potential
> > bindings when calling get_free_id(), if we decide it's worth the
> > extra
> > work.
> > 
> > The global_bindings isn't guaranteed to have all the bindings for
> > our
> > prefix correctly sorted, but they will all be in a group.  If we
> > wanted,
> > we could create a new vector that just included these bindings, and
> > sort
> > it using the current prefix and a comparison function to do alias-
> > sytle
> > sorting. the msort_r() function takes an extra argument to pass to
> > the
> > compar() function, which could be the prefix.
> > 
> > It would be great if we could just sort part of the global_bindings
> > vector in place, once we knew the prefix.  Unfortunately, that
> > would
> > only work if we could know that no prefix can ever match a starting
> > substring of another prefix. Otherwise, sorting using one can make
> > the
> > other not be in a single continuous group.  For instance, if some
> > device
> > configs used "mpatha" as a prefix, they would expect "mpathaaa" to
> > follow immediately after "mapthaz", but if the bindings file had
> > already
> > been sorted using the "mpath" prefix, then "mpathba" would follow
> > "mpathaz", and "mpathaaa" would come much later. Keeping the global
> > file
> > alphabetically sorted guarantees that no matter the prefix that
> > devices
> > were added under, all device aliases that are valid with that
> > prefix
> > will be in a group together.
> >  
> > So, is it worth it to make another vector, copy the alises which
> > are
> > valid with the current prefix into it, and then sort it?
> > get_free_id()
> > will be cleaner, and gap detection won't break down after you get
> > past
> > mpathaa, which it currently does with an alpahbetically sorted
> > bindings
> > list.
> > 
> > Alternatively, we could just decide that setting a prefix in a
> > device
> > config that matches the starting substring of a another prefix is a
> > config error (obviously using the exact same prefix is fine), and
> > ignore
> > that prefix from the device config when we parse the configuration.
> > Then
> > we could read in the bindings alphabetically like we currently do,
> > find
> > the prefix groups in them, and sort them once, in-place. When
> > allocating
> > a binding, we would need to search backwards through the bindings
> > till
> > we found the end of the group matching our prefix (or an alias that
> > comes alphabetically before our prefix). Then we would have to
> > search
> > backwards through our prefix group using the id, till we found a
> > binding
> > with a smaller id.
> 
> So, thinking about this a little more, assuming we would be writing
> out
> the bindings file in full alias sorted order, it's stupid to sort it
> alphabetically, and then resort it back to alias order.  We should
> obviously read it in using the same method as when allocating a
> binding.
> Where you search backwards till you find your prefix group, and then
> find the proper spot in the prefix group using alias sorting. 
> Aliases
> that aren't valid for any configured prefix (these could have been
> added
> by hand to the bindings file, or possibly they were created with a
> different multipath configuration) will just get sorted
> alphabetically
> into the bindings list, between the prefix groups.
> 
> Also, it's not enough to just ignore any device config prefix that is
> a
> starting substring of another prefix. The device config prefixes also
> cannot be superstrings of the default prefix, so if the default
> prefix
> is "mpath", both of the device config prefixes "mp" and "mpathdev"
> would
> need to be ignored.

I don't quite understand yet. Assume we read the bindings file and
encounter an alias "mpathab". Is this alias #28 with prefix "mpath", or
is it #2 with prefix "mpatha", or perhaps alias guess-what  (order
"thab") with prefix "mpa"? How would we know at this point in the code?
Are you suggesting that we create a list of all configured prefixes and
compare the bindings we read with each of them?

Our bindings list is now partially sorted, which is an improvement wrt
the previous situation. "missing the gap" is not really an awful
problem [*]. Perhaps we could postpone this for after this patch set,
and give it some more time to sink in?

Martin

[*] I admit that with my patch, we _know_ now that the bindings list
will be sub-optimally sorted as soon as mpathaa is reached, whereas
before the ordering might be perfect even with a large number of
aliases, depending on the history of the bindings file. That's not a
change for the better; it will cause the gap to be missed in some
situations where we don't miss it now. I am not sure how bad this is.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel