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

[Wuchongyun]

Hi Ben,
Thanks for your reply. 
The purpose to update the path's udev is that if we want to call interface sysfs_attr_set_value() to trigger uevent for this device might use the old udev_device
If this device already received a cheng uevent and updated. If we use the not update pp->udev in uev_update_path may get udev database's old value or status. 
Like we want to get the newest wwid get from udev.

On January 23, 2018 20:39PM, 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 affect 
> > 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?

> > The path may have gone R/O, or even have changed wwid. The safest 
> > course of action might be to delete and re-add the path if any 
> > important properties have changed. But blindly deleting and re-adding 
> > would be wrong as well...
> > 
> > Ben has recently done work in this area, so I'll leave the verdict to 
> > him for now.
> > 
> > @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, 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? It doesn't use the uevent's udev device. It uses the path device's udev device. Does it just rely on this being the first time
>you access DM_MULTIPATH_DEVICE_PATH, so it's not caching the old value?

AFAICS, this change won't hurt anything. But I we should probably update other values that are based on these attributes.

-Ben

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