| Message ID | CEB9978CF3252343BE3C67AC9F0086A3429585CA@H3CMLB14-EX.srv.huawei-3com.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
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@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. 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? Martin > -- > 1.7.9.5 > > Regards > Chongyun
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@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@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
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@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
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];
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@h3c.com> --- multipathd/main.c | 3 +++ 1 file changed, 3 insertions(+)