[dm-devel] [RFC PATCH v2 0/3] multipath: new path validation library

[Martin Wilck]

On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> This patchset is for a new library that programs can use to determine
> if a device belongs to multipath.  The primary user that this is
> intended for is SID, the Storage Instantiation Daemon
> 
> https://github.com/sid-project
> 
> Right now, this doesn't change our existing code to determine path
> ownership, and it doesn't do the exact same steps, although it is
> very
> close.  In the future, it would be possible to pull most of this code
> entirely into libmultipath, except for some wrappers, and use it for
> both methods.

I'd like to see how that's done. To reach that goal, we'd have to
eliminate the differences in the function's logic this way or that way.
Readability-wise, your new code is way better.

>   Obviously, this still needs man pages, and there are some
> helper functions for things like controlling multipath's logging that
> are missing, but I want to see if anyone has strong feelings about
> what
> this looks like.

As you're asking for it, I don't like the static linking linking of
libmultipath, which IMO unnecessarily complicates the multipath-tools
build. If this is what you need, why don't you simply pull multipath-
tools as-is into the SID source tree, e.g. with "git submodule", and
rebuild it there to you suit your needs? It's rather unlikely that
there will be other users of libmpathvalid besides SID any time time
soon. 

To put it more provocatively: I can see the benefit of this patch set
for SID, but what's the benefit for multipath-tools?

OTOH, multipath-tools *would* benefit if we used this as an incentive
to cleanup our libraries, first by cleaning up the "multipath -u"
logic, and later perhaps even so that SID (and other applications)
could simply link with -lmultipath without polluting its namespace in
inacceptible ways.

> I also have two more changes that I want to make to the multipath
> code,
> to make path validation do less unnecessary work, which aren't in
> this
> patchset.
> 
> 1. I want to remove the lock file from the failed wwids code. I don't
> see how it actually stops any races, and it means that simply reading
> a file, can trigger delays and writes (albeit to a memory base fs).

The main idea of the "locking" was that I wanted to create the actual
failed_wwids file atomically, using link(2). open_file() is
unfortunately not atomical at all. If we look into these issues, we
should put open_file() on the table, IMO.

Rather than creating that "lock" file and linking to it, we might 
create "/dev/shm/multipath/failed_wwids/$PID" (just as a 0-byte file,
not with open_file()) and rename it to $WWID atomically.

Moreover, it's possible (though not common) that multipath and
multipathd simultaneously try to set up a certain map. In that case,
one process would likely get an error. But you are right, the actual
race isn't prevented; for that we'd need to handle EEXIST in
dm_addmap_create(). 

> 2. I want to deprecate getuid_callout.  Once this is gone, you will
> be
> able to call pathinfo and get a path's WWID, without ever needing to
> open the path.

It's already been deprecated since 2013. Unfortunately I used it for
the hwtable test because it takes a free-form string argument; so if we
rip it out, we need to rewrite that test.

Best Regards,
Martin


> 
> changes in v2:
> 0002: make sysfs_is_multipathed only read the sysfs file once, as
> suggested by Martin.
> 
> 0003: dm_is_mpath_uuid() is now dm_map_present_by_uuid(). The library
> includes a new function mpath_get_mode(), to get the find_multipaths
> mode, and the modes now include MPATH_FIND. mpath_is_path() now
> accepts
> an array of mpath_infos, which the caller can use to pass the
> previous
> path wwids. This allows mpath_is_path() to return MPATH_IS_VALID for
> paths if there already is another path with that wwid.
> 
> However, mpath_is_path() still treats MPATH_SMART and MPATH_FIND the
> same.  I tried to make them work differently, but I realized that I
> need
> a way to signal that the MPATH_FIND path didn't fail because it was
> blacklisted, but instead because it needed another paths. Otherwise
> the
> caller won't know that it needs to save the wwid to check when later
> paths appear. This is exactly what MPATH_IS_MAYBE_VALID means. In the
> multipath -u code, the only difference between the find_multipaths
> "on"
> and "smart" case is what to do when a path that needs another path
> appears for the first time.  Dealing with this difference is the
> responsiblity of the caller of the mpathvalid library.
> mpath_get_mode(),
> will let it know what the configured find_multipaths mode is.
> 
> Benjamin Marzinski (3):
>   libmultipath: make libmp_dm_init optional
>   libmultipath: make sysfs_is_multipathed able to return wwid
>   multipath: add libmpathvalid library
> 
>  Makefile                            |   1 +
>  Makefile.inc                        |   1 +
>  libmpathvalid/Makefile              |  38 ++++++
>  libmpathvalid/libmpathvalid.version |   7 +
>  libmpathvalid/mpath_valid.c         | 198
> ++++++++++++++++++++++++++++
>  libmpathvalid/mpath_valid.h         |  56 ++++++++
>  libmultipath/Makefile               |   1 +
>  libmultipath/devmapper.c            |  66 +++++++++-
>  libmultipath/devmapper.h            |   4 +-
>  libmultipath/sysfs.c                |  24 +++-
>  libmultipath/sysfs.h                |   2 +-
>  multipath/main.c                    |   7 +-
>  12 files changed, 391 insertions(+), 14 deletions(-)
>  create mode 100644 libmpathvalid/Makefile
>  create mode 100644 libmpathvalid/libmpathvalid.version
>  create mode 100644 libmpathvalid/mpath_valid.c
>  create mode 100644 libmpathvalid/mpath_valid.h
> 

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer