[dm-devel] [PATCH v2 3/3] multipathd: add recheck_wwid option to verify the path wwid

[Martin Wilck]

On Thu, 2021-03-11 at 15:33 -0600, Benjamin Marzinski wrote:
> On Thu, Mar 11, 2021 at 08:30:51PM +0000, Martin Wilck wrote:
> > On Wed, 2021-02-24 at 00:33 -0600, Benjamin Marzinski wrote:
> > > There are cases where the wwid of a path changes due to LUN
> > > remapping
> > > without triggering uevent for the changed path. Multipathd has no
> > > method
> > > for trying to catch these cases, and corruption has resulted
> > > because
> > > of
> > > it.
> > > 
> > > In order to have a better chance at catching these cases,
> > > multipath
> > > now
> > > has a recheck_wwid option. If this is set to "yes", when a failed
> > > path
> > > has become active again, multipathd will recheck its wwid. If
> > > multipathd
> > > notices that a path's wwid has changed, it will remove and re-add
> > > the
> > > path, just like the existing wwid checking code for change events
> > > does.
> > > In cases where the no uevent occurs, both the udev database entry
> > > and
> > > sysfs will have the old wwid, so the only way to get a current
> > > wwid
> > > is
> > > to ask the device directly. Currently multipath only has code to
> > > directly get the wwid for scsi devices, so this option only
> > > effects
> > > scsi
> > > devices, and they must be configured to be able to use the
> > > uid_fallback
> > > methods. To make sure both the sysfs and udev database values are
> > > updated, multipathd triggers a both a rescan of the device and a
> > > udev
> > > add event.
> > > 
> > > Co-developed-by: Chongyun Wu <wucy11 at chinatelecom.cn>
> > > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > 
> > Two minor remarks below, but:
> > 
> > Reviewed-by: Martin Wilck <mwilck at suse.com>
> > 
> > >  
> > > +static void
> > > +rescan_path(struct udev_device *parent)
> > > +{
> > > +       while(parent) {
> > > +               const char *subsys =
> > > udev_device_get_subsystem(parent);
> > > +               if (subsys && !strncmp(subsys, "scsi", 4))
> > > +                       break;
> > > +               parent = udev_device_get_parent(parent);
> > > +       }
> > 
> > You could have used udev_device_get_parent_with_subsystem_devtype()
> > here. 
> 
> fair enough.
>  
> > > +       if (parent)
> > > +               sysfs_attr_set_value(parent, "rescan", "1",
> > > strlen("1"));
> > > +}
> > > +
> > > +void
> > > +handle_path_wwid_change(struct path *pp, struct vectors *vecs)
> > > +{
> > > +       struct udev_device *udd;
> > > +
> > > +       if (!pp || !pp->udev)
> > > +               return;
> > > +
> > > +       udd = udev_device_ref(pp->udev);
> > > +       if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) {
> > > +               pp->dmstate = PSTATE_FAILED;
> > > +               dm_fail_path(pp->mpp->alias, pp->dev_t);
> > > +       }
> > > +       rescan_path(udd);
> > > +       sysfs_attr_set_value(udd, "uevent", "add",
> > > strlen("add"));
> > > +       trigger_partitions_udev_change(udd, "add",
> > > strlen("add"));
> > 
> > Why do you need to do this for the partitions?
> 
> When I tested this before, it didn't appear that an add event ever
> got
> retrigger for any existing partitions. But since the device got
> remapped, the udev information about those partitions most also be
> wrong. The idea was to get them to be using better data. Admittedly,
> this isn't really necessary for kpartx. It just seems like since
> multipath noticed it, it should try to clean it up.  But actually,
> there
> might not even be partitions on the device anymore, so perhaps it's
> best
> to just leave them alone.

It seems to me that we're "papering over" a kernel deficiency here. IMO
multipathd isn't responsible for partititions on path devices. Doing so
may come as a surprise for other actors in the system (mind you how
much confusion udev's "watch" functionality for block devices has
wrought in various areas, even though it's a good idea in general).
OTOH, nobody uses these partitions; we even have "del-part-nodes.rules"
to make them disappear altogether.

My understanding was that when the kernel revalidates a gendisk (as it
should after a "rescan"), it would also revalidate the partitions and
add/remove them as appropriate. I haven't digged through the details,
but if these uevents don't occur, the rescan may have been ineffective
or incomplete?

>From past experience with rescan-scsi-bus.sh, just a "rescan" may
actually not always be sufficient. rescan-scsi-bus.sh removed devices
before rescanning them in "--forcerescan" mode:

(https://github.com/hreinecke/sg3_utils/blob/d82f040c69689305ca1d318d3dc0e1e42ab6ffa3/scripts/rescan-scsi-bus.sh#L1237)

The delete/rescan combination basically adds a new device, which should
provide us with every uevent we need.

That's of course highly dangerous (although not quite as dangerous as
combining different disks in a multipath map), not sure if we should
automate it.

> I can respin this using
> udev_device_get_parent_with_subsystem_devtype(),
> and removing the udev triggers on the partitions, if you want.

I already committed this to "queue". I thought that you'd been waiting
long enough for my review, and these were nitpicks, but now I'm not so
sure any more. Given the considerations above, it'd perhaps really be
better not to trigger partition uevents. Please think about it, and
submit a fixup if you agree. I'll squash it. If we don't need that any
more, we can revert the part that exports
trigger_partitions_udev_change() in libmultipath.version. In that case
I may have to force-push to "queue". Still learning proper branch
maintenance :-/

Thanks,
Martin