[dm-devel] [PATCH 2/2] multipath: attempt at common multipath.rules

Fri Jun 30 16:48:25 UTC 2017

On Tue, Jun 27, 2017 at 11:41:32PM +0200, Martin Wilck wrote:
> Hello Ben,
> 
> Thanks again for your effort on this issue.
> I finally found time to look into this proposal more deeply, sorry that
> it took so long.
> 
> Please find my comments below.
> 
> Best regards,
> Martin
> 
> On Wed, 2017-04-12 at 17:15 -0500, Benjamin Marzinski wrote:
> > This is a proposal to try and bring the Redhat and SuSE
> > multipath.rules
> > closer. There are a couple of changes that I'd like some input on.
> > 
> > The big change is moving the kpartx call into the multipath
> > rules.  Half
> > of the current kpartx.rules file is about creating symlinks for
> > multiple
> > types of dm devices. The other half auto-creates kpartx devices on
> > top
> > of multipath devices. Since it is only creating kpartx devices on top
> > of
> > multipath devices, I've moved the these rules into multipath.rules,
> > or
> > rather, I've replaced them with the redhat rules in multipath.rules. 
> 
> I don't like the move of the rules too much, for two reasons:
> 
> 1) Even if multipath is the only use case today, I see no deeper reason
> why kpartx would be used only for multipath devices. It could be used
> to create partitions on top of other dm targets as well.
> 
> 2) multipath.rules now consists of two completely unrelated parts, one
> for detecting paths that are part of multipath maps, and one for
> running kpartx on the maps themselves. That's confusing and sort of
> against the "spirit" of udev rules, as far as I understand it.
> Logically, if you really want to merge this into another .rules file,
> the kpartx part would rather belong into 11-dm-mpath.rules (which deals
> with multipath maps) than in 56-multipath.rules (which deals with
> paths).

That's a fair complaint.  I'm not sure if the kpartx rules belong in
11-dm-mpath.rules. That works a lot line 11-dm-lvm.rules. Both are just
setting up flags for the other udev rules to use.  Perhaps the best
answer is to move what is the kpartx part of my multipath.rules to
kpartx.rules, and move the generic dm device labelling stuff from
kpartx.rules to some other rules file.

> > The
> > biggest difference is the kpartx isn't run on every reload.  It works
> > with the 11-dm-mpath.rules code to not run kpartx on multipathd
> > generated reloads or when there aren't any working paths. It does
> > remember if it didn't get to run kpartx when it was supposed to
> > (because
> > there were no valid paths or the device was suspended) and will make
> > sure to run it on the next possible uevent.
> 
> I like this part, but i have some suggestions, see below.
> 
> > The other change is the redhat multipath rules remove the partition
> > device nodes for devices claimed by multipath. The udev rule will
> > only
> > do this one time (both to keep from running partx on every event, and
> > so
> > that if users manually reread the partition table, we don't keep
> > removing them when clearly they are wanted). Redhat does this because
> > we
> > had multiple customer issues where they were using the scsi
> > partitions
> > instead of the kpartx devices. Obviously, with setting the partition
> > devices to not ready and clearing their fs_type, this isn't
> > essential,
> > but it has helped make customers do the right thing.
> 
> Isn't this racy? You call "partx -d" on the parent device (e.g. sda).
> At this point in time, the kernel will already have detected the
> partitions and "add" uevents for them are likely to follow up quickly.
> You generate "remove" events that may race with the "add" - or am I
> overlooking something?

Yes this is racey. However, when the partition devices do come up, the
are marked with ENV{SYSTEMD_READY}="0" and ENV{ID_FS_TYPE}="none". This
means that systemd isn't going to do anything with them. So the devices
may appear for an instant, and then get pulled, but nothing should
auto-anything on top of them.

> I'd feel better if we'd call "partx -d" while processing the "add"
> uevent for the _partitions_, ideally late in that process. That would
> make (almost) sure that "add" was finished when "remove" was generated.
> See below.

My goal was to pull them before they ever appeared at all.  I should
point out that although my way is racey insofar as the partition device
may appear for a brief moment or not. The partition device will always
get removed, regardless of whether the "partx -d" call comes before or
after it appears.

-Ben

> > 
> > Cc: Martin Wilck <mwilck at suse.com>
> > Cc: Hannes Reinecke <hare at suse.de>
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > ---
> >  kpartx/kpartx.rules       |  8 --------
> >  multipath/multipath.rules | 27 ++++++++++++++++++++++++---
> >  2 files changed, 24 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
> > index 48a4d6c..906e320 100644
> > --- a/kpartx/kpartx.rules
> > +++ b/kpartx/kpartx.rules
> > @@ -34,12 +34,4 @@ ENV{ID_FS_LABEL_ENC}=="?*",
> > IMPORT{db}="ID_FS_LABEL_ENC"
> >  ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
> >  	SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
> >  
> > -# Create dm tables for partitions
> > -ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
> > -ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end"
> > -ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1",
> > IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
> > -ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end"
> > -ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \
> > -	RUN+="/sbin/kpartx -u -p -part /dev/$name"
> > -
> >  LABEL="kpartx_end"
> > diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> > index 86defc0..10b9b2b 100644
> > --- a/multipath/multipath.rules
> > +++ b/multipath/multipath.rules
> > @@ -1,13 +1,13 @@
> >  # Set DM_MULTIPATH_DEVICE_PATH if the device should be handled by
> > multipath
> >  SUBSYSTEM!="block", GOTO="end_mpath"
> >  ACTION!="add|change", GOTO="end_mpath"
> > -KERNEL!="sd*|dasd*", GOTO="end_mpath"
> > -
> > +KERNEL!="sd*|dasd*|rbd*|dm-*", GOTO="end_mpath"
> >  IMPORT{cmdline}="nompath"
> >  ENV{nompath}=="?*", GOTO="end_mpath"
> >  IMPORT{cmdline}="multipath"
> >  ENV{multipath}=="off", GOTO="end_mpath"
> >  
> > +KERNEL=="dm-*", GOTO="check_kpartx"
> >  ENV{DEVTYPE}!="partition", GOTO="test_dev"
> >  IMPORT{parent}="DM_MULTIPATH_DEVICE_PATH"
> >  ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="none", \
> > @@ -21,7 +21,28 @@ TEST!="$env{MPATH_SBIN_PATH}/multipath",
> > ENV{MPATH_SBIN_PATH}="/usr/sbin"
> >  
> >  ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
> >  	PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
> > -	ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="none", \
> > +	ENV{DM_MULTIPATH_DEVICE_PATH}="1",
> > ENV{ID_FS_TYPE}="mpath_member", \
> >  	ENV{SYSTEMD_READY}="0"
> >  
> > +ENV{DM_MULTIPATH_DEVICE_PATH}!="1", GOTO="end_mpath"
> > +
> > +IMPORT{db}="DM_MULTIPATH_WIPE_PARTS"
> > +ENV{DM_MULTIPATH_WIPE_PARTS}!="1", ENV{DM_MULTIPATH_WIPE_PARTS}="1",
> > \
> > +	RUN+="/sbin/partx -d --nr 1-1024 $env{DEVNAME}"
> > +GOTO="end_mpath"
> 
> I could imagine that this functionality, in general, might be useful
> for members of other dm targets besides multipath as well. I suggest to
> create a new, separate rules file for it. Moreover, I think it would be
> better to act on the partitions rather than on disks (see above).
> 
> Here's a draft attempt at such a new rules file, please tell me what
> you think:
> 
> # 68-del-parts.rules
> # Delete partitions on disks which are members of dm (multipath) maps
> 
> SUBSYSTEM!="block", GOTO="end_del_parts"
> ACTION!="add", GOTO="end_del_parts"
> KERNEL!="sd*|dasd*|rbd*", GOTO="end_del_parts"
> 
> # possibly add other DM member types here
> ENV{DM_MULTIPATH_DEVICE_PATH}=="1", GOTO="wipe_parts"
> GOTO="end_del_parts"
> 
> LABEL="wipe_parts"
> ENV{DEVTYPE}=="partition", GOTO="del_partition"
> ENV{DEVTYPE}!="disk", GOTO="end_del_parts"
> 
> # Handle disks
> # DM_WIPE_PARTS is set to "1" at first processing, "2" later
> # Only if it's "1", partitions will be deleted
> IMPORT{db}="DM_WIPE_PARTS"
> ENV{DM_WIPE_PARTS}=="1", ENV{DM_WIPE_PARTS}="2"
> ENV{DM_WIPE_PARTS}!="?*", ENV{DM_WIPE_PARTS}="1"
> GOTO="end_del_parts"
> 
> LABEL="del_partition"
> IMPORT{parent}="DM_WIPE_PARTS"
> ENV{DM_WIPE_PARTS}=="1", ENV{SYSTEMD_READY}="0", RUN+="/sbin/partx -d $env{DEVNAME}", OPTIONS:="nowatch"
> 
> LABEL="end_del_parts"
> # end 68-del-parts.rules
> 
> > +LABEL="check_kpartx"
> > +
> > +IMPORT{db}="DM_MULTIPATH_NEED_KPARTX"
> As remarked above, I'd vote for removing "MULTIPATH" from this property
> name.
> 
> > +ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1",
> > IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
> > +ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="end_mpath"
> > +ACTION!="change", GOTO="end_mpath"
> > +ENV{DM_UUID}!="mpath-?*", GOTO="end_mpath"
> > +ENV{DM_ACTIVATION}=="1", ENV{DM_MULTIPATH_NEED_KPARTX}="1"
> > +ENV{DM_SUSPENDED}=="1", GOTO="end_mpath"
> > +ENV{DM_ACTION}=="PATH_FAILED", GOTO="end_mpath"
> 
> The previous code had ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED",
> why did you drop the latter? 
> Anyway, I think both aren't needed because in 11-dm-mpath.rules,
> "DM_ACTIVATION" is set to "0" in the "reload if paths are
> lost/recovered" case. I think the cleaner way to express the logic here
> would be:
> 
> # don't rerun kpartx on "reload" events (see 11-dm-mpath.rules)
> ENV{DM_ACTIVATION}=="0", GOTO="end_mpath"
> # don't run kpartx when there are no paths
> ENV{MPATH_DEVICE_READY}=="0", GOTO="end_mpath"
> 
> > +ENV{DM_ACTIVATION}!="1", ENV{DM_MULTIPATH_NEED_KPARTX}!="1",
> > GOTO="end_mpath"
> > +RUN+="/sbin/kpartx -u -p -part /dev/$name"
> > +ENV{DM_MULTIPATH_NEED_KPARTX}=""
> > +
> >  LABEL="end_mpath"
> 
> In general, it seems to me that both the addition and removal of
> partition device nodes is not specific to multipath and, once we agree
> on a set of rules, we should forward this to the udev and libdevmapper
> community (reading this already? thanks!).
> 
> -- 
> 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)