* [PATCH 0/2] RFC: multipath proposals @ 2017-04-12 22:15 Benjamin Marzinski 2017-04-12 22:15 ` [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs Benjamin Marzinski 2017-04-12 22:15 ` [PATCH 2/2] multipath: attempt at common multipath.rules Benjamin Marzinski 0 siblings, 2 replies; 11+ messages in thread From: Benjamin Marzinski @ 2017-04-12 22:15 UTC (permalink / raw) To: device-mapper development 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs 2017-04-12 22:15 [PATCH 0/2] RFC: multipath proposals Benjamin Marzinski @ 2017-04-12 22:15 ` Benjamin Marzinski 2017-04-13 8:44 ` Johannes Thumshirn ` (2 more replies) 2017-04-12 22:15 ` [PATCH 2/2] multipath: attempt at common multipath.rules Benjamin Marzinski 1 sibling, 3 replies; 11+ messages in thread From: Benjamin Marzinski @ 2017-04-12 22:15 UTC (permalink / raw) To: device-mapper development; +Cc: Xose Vazquez Perez 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs 2017-04-12 22:15 ` [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs Benjamin Marzinski @ 2017-04-13 8:44 ` Johannes Thumshirn 2017-04-13 15:45 ` Benjamin Marzinski 2017-04-13 14:59 ` Martin Wilck 2017-05-07 0:32 ` Xose Vazquez Perez 2 siblings, 1 reply; 11+ messages in thread From: Johannes Thumshirn @ 2017-04-13 8:44 UTC (permalink / raw) To: Benjamin Marzinski; +Cc: device-mapper development, Xose Vazquez Perez 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs 2017-04-13 8:44 ` Johannes Thumshirn @ 2017-04-13 15:45 ` Benjamin Marzinski 0 siblings, 0 replies; 11+ messages in thread From: Benjamin Marzinski @ 2017-04-13 15:45 UTC (permalink / raw) To: Johannes Thumshirn; +Cc: device-mapper development, Xose Vazquez Perez 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs 2017-04-12 22:15 ` [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs Benjamin Marzinski 2017-04-13 8:44 ` Johannes Thumshirn @ 2017-04-13 14:59 ` Martin Wilck 2017-05-07 0:32 ` Xose Vazquez Perez 2 siblings, 0 replies; 11+ messages in thread From: Martin Wilck @ 2017-04-13 14:59 UTC (permalink / raw) To: dm-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs 2017-04-12 22:15 ` [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs Benjamin Marzinski 2017-04-13 8:44 ` Johannes Thumshirn 2017-04-13 14:59 ` Martin Wilck @ 2017-05-07 0:32 ` Xose Vazquez Perez 2 siblings, 0 replies; 11+ messages in thread From: Xose Vazquez Perez @ 2017-05-07 0:32 UTC (permalink / raw) To: Benjamin Marzinski, device-mapper development Cc: Vijay Chauhan, Stewart, Sean, Schremmer, Steven 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, > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] multipath: attempt at common multipath.rules 2017-04-12 22:15 [PATCH 0/2] RFC: multipath proposals Benjamin Marzinski 2017-04-12 22:15 ` [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs Benjamin Marzinski @ 2017-04-12 22:15 ` Benjamin Marzinski 2017-04-13 15:27 ` Martin Wilck 2017-06-27 21:41 ` Martin Wilck 1 sibling, 2 replies; 11+ messages in thread From: Benjamin Marzinski @ 2017-04-12 22:15 UTC (permalink / raw) To: device-mapper development; +Cc: Martin Wilck 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] multipath: attempt at common multipath.rules 2017-04-12 22:15 ` [PATCH 2/2] multipath: attempt at common multipath.rules Benjamin Marzinski @ 2017-04-13 15:27 ` Martin Wilck 2017-06-27 21:41 ` Martin Wilck 1 sibling, 0 replies; 11+ messages in thread From: Martin Wilck @ 2017-04-13 15:27 UTC (permalink / raw) To: dm-devel, Benjamin Marzinski, Hannes Reinecke 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] multipath: attempt at common multipath.rules 2017-04-12 22:15 ` [PATCH 2/2] multipath: attempt at common multipath.rules Benjamin Marzinski 2017-04-13 15:27 ` Martin Wilck @ 2017-06-27 21:41 ` Martin Wilck 2017-06-28 13:00 ` Martin Wilck 2017-06-30 16:48 ` Benjamin Marzinski 1 sibling, 2 replies; 11+ messages in thread From: Martin Wilck @ 2017-06-27 21:41 UTC (permalink / raw) To: Benjamin Marzinski, device-mapper development 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] multipath: attempt at common multipath.rules 2017-06-27 21:41 ` Martin Wilck @ 2017-06-28 13:00 ` Martin Wilck 2017-06-30 16:48 ` Benjamin Marzinski 1 sibling, 0 replies; 11+ messages in thread From: Martin Wilck @ 2017-06-28 13:00 UTC (permalink / raw) To: Benjamin Marzinski, device-mapper development 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] multipath: attempt at common multipath.rules 2017-06-27 21:41 ` Martin Wilck 2017-06-28 13:00 ` Martin Wilck @ 2017-06-30 16:48 ` Benjamin Marzinski 1 sibling, 0 replies; 11+ messages in thread From: Benjamin Marzinski @ 2017-06-30 16:48 UTC (permalink / raw) To: Martin Wilck; +Cc: device-mapper development 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-06-30 16:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-12 22:15 [PATCH 0/2] RFC: multipath proposals Benjamin Marzinski 2017-04-12 22:15 ` [PATCH 1/2] multipath: Merge the DELL MD3xxx device configs Benjamin Marzinski 2017-04-13 8:44 ` Johannes Thumshirn 2017-04-13 15:45 ` Benjamin Marzinski 2017-04-13 14:59 ` Martin Wilck 2017-05-07 0:32 ` Xose Vazquez Perez 2017-04-12 22:15 ` [PATCH 2/2] multipath: attempt at common multipath.rules Benjamin Marzinski 2017-04-13 15:27 ` Martin Wilck 2017-06-27 21:41 ` Martin Wilck 2017-06-28 13:00 ` Martin Wilck 2017-06-30 16:48 ` Benjamin Marzinski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.