[PATCH 0/2] RFC: multipath proposals

[PATCH 0/2] RFC: multipath proposals

[Benjamin Marzinski]

I'm interested if anyone has any opinions on either of these two patches.

Benjamin Marzinski (2):
  multipath: Merge the DELL MD3xxx device configs
  multipath: attempt at common multipath.rules

 kpartx/kpartx.rules       |  8 --------
 libmultipath/hwtable.c    | 26 +-------------------------
 multipath/multipath.rules | 27 ++++++++++++++++++++++++---
 3 files changed, 25 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] multipath: Merge the DELL MD3xxx device configs

[Benjamin Marzinski]

All of the Dell MD3xxx configs are identical, so we can't just use
one config for them.

Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/hwtable.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index c944015..54309ef 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -227,31 +227,7 @@ static struct hwentry default_hw[] = {
 		/* MD Series */
 	{
 		.vendor        = "DELL",
-		.product       = "MD3000",
-		.bl_product    = "Universal Xport",
-		.pgpolicy      = GROUP_BY_PRIO,
-		.checker_name  = RDAC,
-		.features      = "2 pg_init_retries 50",
-		.hwhandler     = "1 rdac",
-		.prio_name     = PRIO_RDAC,
-		.pgfailback    = -FAILBACK_IMMEDIATE,
-		.no_path_retry = 30,
-	},
-	{
-		.vendor        = "DELL",
-		.product       = "(MD32xx|MD36xx)",
-		.bl_product    = "Universal Xport",
-		.pgpolicy      = GROUP_BY_PRIO,
-		.checker_name  = RDAC,
-		.features      = "2 pg_init_retries 50",
-		.hwhandler     = "1 rdac",
-		.prio_name     = PRIO_RDAC,
-		.pgfailback    = -FAILBACK_IMMEDIATE,
-		.no_path_retry = 30,
-	},
-	{
-		.vendor        = "DELL",
-		.product       = "(MD34xx|MD38xx)",
+		.product       = "^MD3",
 		.bl_product    = "Universal Xport",
 		.pgpolicy      = GROUP_BY_PRIO,
 		.checker_name  = RDAC,
-- 
1.8.3.1

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

[Benjamin Marzinski]

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. 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.

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.

Cc: Martin Wilck <mwilck@suse.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Benjamin Marzinski <bmarzins@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"
+
+LABEL="check_kpartx"
+
+IMPORT{db}="DM_MULTIPATH_NEED_KPARTX"
+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"
+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"
-- 
1.8.3.1

Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs

[Johannes Thumshirn]

Hi Benjamin,

On Wed, Apr 12, 2017 at 05:15:09PM -0500, Benjamin Marzinski wrote:
> All of the Dell MD3xxx configs are identical, so we can't just use
                                                 can? ^
> one config for them.
> 
> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs

[Martin Wilck]

On Wed, 2017-04-12 at 17:15 -0500, Benjamin Marzinski wrote:
> All of the Dell MD3xxx configs are identical, so we can't just use
> one config for them.
> 
> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>
-- 
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

Re: [PATCH 2/2] multipath: attempt at common multipath.rules

[Martin Wilck]

Hi Ben,

> 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.

Thanks for making this so explicit. Perhaps we should have done the
same for our recent changes, in particular d7188fcd "multipathd: start
daemon after udev trigger".

> 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.
> 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.
> 
> 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.
> 
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  kpartx/kpartx.rules       |  8 --------
>  multipath/multipath.rules | 27 ++++++++++++++++++++++++---
>  2 files changed, 24 insertions(+), 11 deletions(-)
> 

All of this makes sense to me. But I may not overlook all the possible
pitfalls in the udev rules, so please wait for Hannes' comments, too.

Regards
Martin

-- 
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

Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs

[Benjamin Marzinski]

On Thu, Apr 13, 2017 at 10:44:43AM +0200, Johannes Thumshirn wrote:
> Hi Benjamin,
> 
> On Wed, Apr 12, 2017 at 05:15:09PM -0500, Benjamin Marzinski wrote:
> > All of the Dell MD3xxx configs are identical, so we can't just use
>                                                  can? ^

Oops. Yes. That's what I meant.  I can fix the message if people are
happy with the idea.

-Ben


> > one config for them.
> > 
> > Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> 
> -- 
> Johannes Thumshirn                                          Storage
> jthumshirn@suse.de                                +49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs

[Xose Vazquez Perez]

On 04/13/2017 12:15 AM, Benjamin Marzinski wrote:

> All of the Dell MD3xxx configs are identical, so we can't just use
> one config for them.

If you do that to simplify the config file or minimize the number of
RDAC entries. More of them, from (NETAPP|LSI|ENGENIO) IBM SGI STK SUN,
can be consolidated: grep -B7 -A2 "PRIO_RDAC" libmultipath/hwtable.c


BTW, I did send time ago a kernel patch related to RDAC arrays and
it's still waiting for ACK: https://marc.info/?l=linux-scsi&m=147585935707470

> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/hwtable.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> index c944015..54309ef 100644
> --- a/libmultipath/hwtable.c
> +++ b/libmultipath/hwtable.c
> @@ -227,31 +227,7 @@ static struct hwentry default_hw[] = {
>  		/* MD Series */
>  	{
>  		.vendor        = "DELL",
> -		.product       = "MD3000",
> -		.bl_product    = "Universal Xport",
> -		.pgpolicy      = GROUP_BY_PRIO,
> -		.checker_name  = RDAC,
> -		.features      = "2 pg_init_retries 50",
> -		.hwhandler     = "1 rdac",
> -		.prio_name     = PRIO_RDAC,
> -		.pgfailback    = -FAILBACK_IMMEDIATE,
> -		.no_path_retry = 30,
> -	},
> -	{
> -		.vendor        = "DELL",
> -		.product       = "(MD32xx|MD36xx)",
> -		.bl_product    = "Universal Xport",
> -		.pgpolicy      = GROUP_BY_PRIO,
> -		.checker_name  = RDAC,
> -		.features      = "2 pg_init_retries 50",
> -		.hwhandler     = "1 rdac",
> -		.prio_name     = PRIO_RDAC,
> -		.pgfailback    = -FAILBACK_IMMEDIATE,
> -		.no_path_retry = 30,
> -	},
> -	{
> -		.vendor        = "DELL",
> -		.product       = "(MD34xx|MD38xx)",
> +		.product       = "^MD3",
>  		.bl_product    = "Universal Xport",
>  		.pgpolicy      = GROUP_BY_PRIO,
>  		.checker_name  = RDAC,
>

Re: [PATCH 2/2] multipath: attempt at common multipath.rules

[Martin Wilck]

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).

> 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?

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.

> 
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Benjamin Marzinski <bmarzins@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@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

Re: [PATCH 2/2] multipath: attempt at common multipath.rules

[Martin Wilck]

On Tue, 2017-06-27 at 23:41 +0200, Martin Wilck wrote:
> 
> > 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?
> 
> 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.

It turns out that my idea doesn't work quite like your approach,
because users can't simply run "partx -a /dev/sd$X" to get the deleted
partitions back. They need an additional, manual "echo change
>/sys/class/block/sd$X/uevent" first to make "partx -a work".
So, if you can confirm that uevent races are no problem, your technique
seems to be more user-friendly.


> > +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"

The ENV{DM_ACTIVATION}=="0" can be left out, too, I think. I'll post a
patch with what I think the setup should be soon.

Best
Martin
-- 
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

Re: [PATCH 2/2] multipath: attempt at common multipath.rules

[Benjamin Marzinski]

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@suse.com>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Benjamin Marzinski <bmarzins@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@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)