[dm-devel] [PATCH] multipathd: update path's udev in uev_update_path

[Martin Wilck]

On Tue, 2018-01-23 at 06:39 -0600, Benjamin Marzinski wrote:
> On Mon, Jan 22, 2018 at 11:34:15AM +0100, Martin Wilck wrote:
> > On Sat, 2018-01-20 at 02:30 +0000, Wuchongyun wrote:
> > > Hi Martin and Ben,
> > > Could you help to review this patch, thanks.
> > > 
> > > When receiving a change uevent and calling uev_update_path,
> > > current
> > > code
> > > not update the path's udev, if use pp->udev to query the udev
> > > database
> > > info will get the old data. So it should update the path's udev
> > > with
> > > the
> > > new udev from the new uevent in uev_update_path also.
> > > 
> > > Signed-off-by: Chongyun Wu <wu.chongyun at h3c.com>
> > > ---
> > >  multipathd/main.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > index 27cf234..e7a4f4b 100644
> > > --- a/multipathd/main.c
> > > +++ b/multipathd/main.c
> > > @@ -937,6 +937,9 @@ uev_update_path (struct uevent *uev, struct
> > > vectors * vecs)
> > >  	if (pp) {
> > >  		struct multipath *mpp = pp->mpp;
> > >  
> > > +		udev_device_unref(pp->udev);
> > > +		pp->udev = udev_device_ref(uev->udev);
> > > +
> > >  		if (disable_changed_wwids &&
> > >  		    (strlen(pp->wwid) || pp->wwid_changed)) {
> > >  			char wwid[WWID_SIZE];
> > 
> > The general idea is correct, but I fear we need to do more.
> > 
> > Basically, if relevant udev properties have changed, we need to
> > call
> > pathinfo() on the changed path. Getting this right is subtle, I
> > guess.
> > We can't simply change the path properties in the pathvec, because
> > there'll be locking issues, and changed path properties might
> > affectssmutex
> > how the path is handled.
> 
> We do call pathinfo to update paths in a number of places, and as
> long
> as we do it while holding the vecs lock, I thought that was safe. Am
> I
> missing something here?  

AFAICT vecs->log protects the vectors themselves, not the properties of
every member. But probably it's me who's missing something.
 
> > @Ben, AFAICS this patch shows that disable_changed_wwids won't work
> > correctly as long as the WWID is derived from udev, which is the
> > default case these days. Or am I overlooking something?
> 
> We build the "struct uevent *uev" up from the specific uevent and we
> check the R/O value from that, so it will be correct based on the
> actual
> event.  We also check the wwid from the udev device linked from the
> uev,

That't it, yes.

> so that is also from the actual current device state.  I don't
> remember
> offhand why I didn't just update the udev device for the path in
> uev_update_path (or even if I had a good reason for not doing so. I'm
> very jetlagged right now). However, does your update to
> trigger_paths_udev_change() work correctly in this case? 

I have to admit I haven't thought about that. I guess it would if we'd
make sure that pp->udev and derived path properties are updated in
uev_update_path...

Anywy, if we can trust cached udev properties at all, we should be able
to do so in trigger_paths_udev_change(), too. But in order to be able
to trust cached properties, we should make sure they're updated when
events arrive.

Martin

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