Re: [RFC PATCH 19/20] multipathd: use foreign API

[Martin Wilck <mwilck@suse.com>]

On Wed, 2018-02-28 at 23:13 -0600, Benjamin Marzinski wrote:
> On Tue, Feb 20, 2018 at 02:26:57PM +0100, Martin Wilck wrote:
> > Call into the foreign library code when paths are discovered,
> > uevents
> > are received, and in the checker loop. Furthermore, use the foreign
> > code to print information in the "multipathd show paths",
> > "multipathd
> > show maps", and "multipathd show topology" client commands.
> > 
> > We don't support foreign data in the individual "show map" and
> > "show path"
> > commands, and neither in the "json" commands. The former is a
> > deliberate
> > decision, the latter could be added if desired.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/discovery.c  |  4 +++-
> >  multipathd/cli_handlers.c | 39 ++++++++++++++++++++++++++++++++++-
> > ----
> >  multipathd/main.c         | 43
> > ++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 77 insertions(+), 9 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index b900bb3ec2e3..e1d98861a841 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -84,6 +84,7 @@ static int use_watchdog;
> >  #include "waiter.h"
> >  #include "io_err_stat.h"
> >  #include "wwids.h"
> > +#include "foreign.h"
> >  #include "../third-party/valgrind/drd.h"
> >  
> >  #define FILE_NAME_SIZE 256
> > @@ -699,6 +700,15 @@ rescan:
> >  		mpp->action = ACT_RELOAD;
> >  		extract_hwe_from_path(mpp);
> >  	} else {
> > +		switch (add_foreign(pp->udev)) {
> 
> There's a rather convoluted issue here. This function will eventually
> call _find_slaves() in the nvme code, which calls glob(), which isn't
> strictly multithreading safe for a number of reasons. The only one
> that
> effects multipathd is its use of SIGALRM. Now assuming signal
> processing
> worked correctly in multipathd (it doesn't. commit 534ec4c broke it.
> I'm
> planning on fixing that shortly) there would still be a problem,
> because
> of the strict_timing option.  strict_timing calls setitimer(), which
> sets an alarm in checkerloop, without any locking.  strict_timing
> also
> messes with any call to lock_file(). lock_file() and _find_slaves()
> will
> never interfere with eachother, since both are only called with the
> vecs
> lock held (I think. See below). strict_timing probably needs to get
> reimplemented using nanosleep() or some other non-signal method, so
> that
> there aren't two threads setting alarms and waiting for SIGARLM at
> the
> same time.

Thanks for spotting this. It was lazyness on my side to use glob(). I
will reimplement this using safer, more elementary functions. 

> 
> > +		case FOREIGN_CLAIMED:
> > +		case FOREIGN_OK:
> > +			orphan_path(pp, "claimed by foreign
> > library");
> > +			return 0;
> > +		default:
> > +			break;
> 
> Is there a purpose to this break?

I thought this was common style. You find it the kernel's CodingStyle
document, too. I have no issues with removing it if you insist.

> > 	conf = get_multipath_config();
> >  	disable_changed_wwids = conf->disable_changed_wwids;
> >  	put_multipath_config(conf);
> > @@ -1122,8 +1149,13 @@ uev_trigger (struct uevent * uev, void *
> > trigger_data)
> >  	 * are not fully initialised then.
> >  	 */
> >  	if (!strncmp(uev->kernel, "dm-", 3)) {
> > -		if (!uevent_is_mpath(uev))
> > +		if (!uevent_is_mpath(uev)) {
> > +			if (!strncmp(uev->action, "change", 6))
> > +				(void)add_foreign(uev->udev);
> > +			else if (!strncmp(uev->action, "remove",
> > 6))
> > +				(void)delete_foreign(uev->udev);
> >  			goto out;
> > +		}
> 
> I'm confused. Should foreign maps ever have uev->kernel set to "dm-"?

Although I don't see a likely candidate, I didn't want to exclude it in
general.

> If this is correct, these calls probably need to get called with the
> vecs lock held, otherwise they can interfere with calls to
> lock_file().

Please explain. These devices are ignored by multipath-tools, because
!uevent_is_mpath(uev), so they won't be part of any of their data
structures. And lock_file() is only about fcntl, what does it have to
do with this code? 

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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