[dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
Autodetected. Cc: Hannes Reinecke Cc: Christophe Varoqui Cc: device-mapper development Signed-off-by: Xose Vazquez Perez --- libmultipath/hwtable.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c index 367aa0a..1fc2f24 100644 --- a/libmultipath/hwtable.c +++ b/libmultipath/hwtable.c @@ -46,7 +46,6 @@ static struct hwentry default_hw[] = { .product = "VV", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, - .hwhandler = "1 alua", .prio_name = PRIO_ALUA, .no_path_retry = 18, }, @@ -124,7 +123,6 @@ static struct hwentry default_hw[] = { /* SAN Virtualization Services Platform */ .vendor= "HP", .product = "HSVX700", - .hwhandler = "1 alua", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .no_path_retry = 12, @@ -504,7 +502,6 @@ static struct hwentry default_hw[] = { .vendor= "IBM", .product = "^IPR", .no_path_retry = NO_PATH_RETRY_QUEUE, - .hwhandler = "1 alua", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .prio_name = PRIO_ALUA, @@ -546,7 +543,6 @@ static struct hwentry default_hw[] = { { .vendor= "AIX", .product = "NVDISK", - .hwhandler = "1 alua", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .no_path_retry = (300 / DEFAULT_CHECKINT), @@ -654,7 +650,6 @@ static struct hwentry default_hw[] = { /* M-Series */ .vendor= "NEC", .product = "DISK ARRAY", - .hwhandler = "1 alua", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .prio_name = PRIO_ALUA, @@ -780,7 +775,6 @@ static struct hwentry default_hw[] = { { .vendor= "(Intel|INTEL)", .product = "Multi-Flex", - .hwhandler = "1 alua", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .no_path_retry = NO_PATH_RETRY_QUEUE, @@ -792,7 +786,6 @@ static struct hwentry default_hw[] = { { .vendor= "(LIO-ORG|SUSE)", .product = "RBD", - .hwhandler = "1 alua", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .no_path_retry = 12, -- 2.10.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
I'm not comfortable with this patch that induces a less predictable behaviour. We have that retain hardware handler switch that users can activate to achieve the purpose. Hannes, Ben, would you support that patch ? Best Regards, Christophe On Thu, Oct 6, 2016 at 8:50 PM, Xose Vazquez Perez wrote: > Autodetected. > > Cc: Hannes Reinecke > Cc: Christophe Varoqui > Cc: device-mapper development > Signed-off-by: Xose Vazquez Perez > --- > libmultipath/hwtable.c | 7 --- > 1 file changed, 7 deletions(-) > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > index 367aa0a..1fc2f24 100644 > --- a/libmultipath/hwtable.c > +++ b/libmultipath/hwtable.c > @@ -46,7 +46,6 @@ static struct hwentry default_hw[] = { > .product = "VV", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > - .hwhandler = "1 alua", > .prio_name = PRIO_ALUA, > .no_path_retry = 18, > }, > @@ -124,7 +123,6 @@ static struct hwentry default_hw[] = { > /* SAN Virtualization Services Platform */ > .vendor= "HP", > .product = "HSVX700", > - .hwhandler = "1 alua", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > .no_path_retry = 12, > @@ -504,7 +502,6 @@ static struct hwentry default_hw[] = { > .vendor= "IBM", > .product = "^IPR", > .no_path_retry = NO_PATH_RETRY_QUEUE, > - .hwhandler = "1 alua", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > .prio_name = PRIO_ALUA, > @@ -546,7 +543,6 @@ static struct hwentry default_hw[] = { > { > .vendor= "AIX", > .product = "NVDISK", > - .hwhandler = "1 alua", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > .no_path_retry = (300 / DEFAULT_CHECKINT), > @@ -654,7 +650,6 @@ static struct hwentry default_hw[] = { > /* M-Series */ > .vendor= "NEC", > .product = "DISK ARRAY", > - .hwhandler = "1 alua", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > .prio_name = PRIO_ALUA, > @@ -780,7 +775,6 @@ static struct hwentry default_hw[] = { > { > .vendor= "(Intel|INTEL)", > .product = "Multi-Flex", > - .hwhandler = "1 alua", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > .no_path_retry = NO_PATH_RETRY_QUEUE, > @@ -792,7 +786,6 @@ static struct hwentry default_hw[] = { > { > .vendor= "(LIO-ORG|SUSE)", > .product = "RBD", > - .hwhandler = "1 alua", > .pgpolicy = GROUP_BY_PRIO, > .pgfailback= -FAILBACK_IMMEDIATE, > .no_path_retry = 12, > -- > 2.10.1 > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
On 10/11/2016 08:02 AM, Christophe Varoqui wrote: > I'm not comfortable with this patch that induces a less predictable > behaviour. > We have that retain hardware handler switch that users can activate to > achieve the purpose. > > Hannes, Ben, would you support that patch ? > > Best Regards, > Christophe > Please don't apply this. Autodetection will only work if the hardware handler is loaded. If the handler is _not_ loaded autodetection won't work, either. And if we add this patch multipath will never load the modules, neither. So we need this functionality as a fallback if autodetect does not work. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
Right, so Xosé, can you confirm the last introduced hwtable entries don't need the alua hwhandler : - nexsan - tegile - nimble - nfinidat Thanks. On Tue, Oct 11, 2016 at 8:22 AM, Hannes Reinecke wrote: > On 10/11/2016 08:02 AM, Christophe Varoqui wrote: > > I'm not comfortable with this patch that induces a less predictable > > behaviour. > > We have that retain hardware handler switch that users can activate to > > achieve the purpose. > > > > Hannes, Ben, would you support that patch ? > > > > Best Regards, > > Christophe > > > Please don't apply this. > Autodetection will only work if the hardware handler is loaded. > If the handler is _not_ loaded autodetection won't work, either. > And if we add this patch multipath will never load the modules, neither. > > So we need this functionality as a fallback if autodetect does not work. > > Cheers, > > Hannes > -- > Dr. Hannes ReineckeTeamlead Storage & Networking > h...@suse.de +49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton > HRB 21284 (AG Nürnberg) > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
On 10/11/2016 08:22 AM, Hannes Reinecke wrote: > Autodetection will only work if the hardware handler is loaded. > If the handler is _not_ loaded autodetection won't work, either. > And if we add this patch multipath will never load the modules, neither. > So we need this functionality as a fallback if autodetect does not work. I'm pretty sure all works flawlessly with this patch. Because I use similar options for EMC VNX7500(CLARiiON) configured as ALUA "Failover Mode 4 (Asymmetric Active/Active)", with SLES-11/12 and RHEL-6 booting from SAN. For SLES it was added to DGC config: retain_attached_hw_handler yes detect_prio yes In RHEL it's included by default: http://pkgs.fedoraproject.org/cgit/rpms/device-mapper-multipath.git/plain/0117-RHBZ-1198424-autodetect-clariion-alua.patch And the modules are preloaded by multipathd/multipathd.service: ExecStartPre=/sbin/modprobe -a scsi_dh_alua scsi_dh_emc scsi_dh_rdac dm-multipath RDAC devices also depend *exclusively* on "retain_attached_hw_handler" and "detect_prio" to run in ALUA mode. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
On 10/11/2016 08:43 AM, Christophe Varoqui wrote: > so Xosé, can you confirm the last introduced hwtable entries don't need the > alua hwhandler : Yes, defined: > - tegile > - nimble No needed/defined: > - nexsan > - nfinidat - NexGen|Pivot3 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
On Wed, Oct 12, 2016 at 01:06:10PM +0200, Xose Vazquez Perez wrote: > On 10/11/2016 08:22 AM, Hannes Reinecke wrote: > > > Autodetection will only work if the hardware handler is loaded. > > If the handler is _not_ loaded autodetection won't work, either. > > And if we add this patch multipath will never load the modules, neither. > > So we need this functionality as a fallback if autodetect does not work. > > I'm pretty sure all works flawlessly with this patch. Because I use similar > options for EMC VNX7500(CLARiiON) configured as ALUA "Failover Mode 4 > (Asymmetric Active/Active)", > with SLES-11/12 and RHEL-6 booting from SAN. > > For SLES it was added to DGC config: > retain_attached_hw_handler yes > detect_prio yes > > In RHEL it's included by default: > http://pkgs.fedoraproject.org/cgit/rpms/device-mapper-multipath.git/plain/0117-RHBZ-1198424-autodetect-clariion-alua.patch > > And the modules are preloaded by multipathd/multipathd.service: > ExecStartPre=/sbin/modprobe -a scsi_dh_alua scsi_dh_emc scsi_dh_rdac > dm-multipath > > > RDAC devices also depend *exclusively* on "retain_attached_hw_handler" and > "detect_prio" > to run in ALUA mode. The purpose of the the "retain_attached_hw_handler" and "detect_prio" options were to allow devices that support ALUA and non-ALUA modes, to detect which mode the device was in, so that the builtin configuration would work correctly for both modes. If a device is ALUA only, I don't see any benefit to not hardcoding that. Simply from a ease-of-use persepective, having ALUA in the configuration make it obvious how the device is supposed to be configured. But more importantly, as has already been metioned, it's more robust to hardcode the ALUA handler for the devices that need it in case the handler was not attached before multipath started working on the device. Also, if I recall correctly, for the device handler to get attached correctly, we don't just need the device handler module isntalled before multipathd starts. We need it installed when the path device is initially discovered. The more I think about this, the more I think we might want to revisit some to the configuration simplifications that have been made. In some cases, I think they went too far. For instance, most devices will work correctly regardless of the values for things like "rr_minio_rq" and "path_selector", so I have no objection to these values being removed from the device configuartion, if they are the same as the default values. On the other hand, we have things like "hardware_handler" and "path_checker", where the device simply will not function correctly with certain values. For these attributes, it makes sense to leave them in the device configuration, even if they are the same as the default values. The goal should be that you can't break any device configurations by changing the default values. -Ben -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
On 10/12/2016 06:20 PM, Benjamin Marzinski wrote: > On Wed, Oct 12, 2016 at 01:06:10PM +0200, Xose Vazquez Perez wrote: >> RDAC devices also depend *exclusively* on "retain_attached_hw_handler" >> and "detect_prio" to run in ALUA mode. > The purpose of the the "retain_attached_hw_handler" and "detect_prio" > options were to allow devices that support ALUA and non-ALUA modes, to > detect which mode the device was in, so that the builtin configuration > would work correctly for both modes. Then that is validating my point, hwhandler="1 alua" is superfluous. Because it's autodetected and autoconfigured from the array, first from the kernel and later from multipath-tools. > If a device is ALUA only, I don't see any benefit to not hardcoding > that. Simply from a ease-of-use persepective, having ALUA in the > configuration make it obvious how the device is supposed to be > configured. It's redundant having RETAIN_HWHANDLER_ON and hwhandler="1 alua" defined. > But more importantly, as has already been metioned, it's > more robust to hardcode the ALUA handler for the devices that need it in > case the handler was not attached before multipath started working on > the device. Why you do not trust the SCSI subsystem? It seems a bit paranoid. Anyway, if you are really uncomfortable with the change, this patch should be dropped. > Also, if I recall correctly, for the device handler to get > attached correctly, we don't just need the device handler module > isntalled before multipathd starts. We need it installed when the path > device is initially discovered. This is a kernel modules dependency bug. Distributions can bypass it by including these modules in the initrd image. > The more I think about this, the more I think we might want to revisit > some to the configuration simplifications that have been made. Could you please review all commits? git log -p --reverse --since="Mon Jun 13 16:38:50 2016" libmultipath/hwtable.c What should be removed? > In some cases, I think they went too far. For instance, most devices will work > correctly regardless of the values for things like "rr_minio_rq" and > "path_selector", so I have no objection to these values being removed > from the device configuartion, if they are the same as the default > values. On the other hand, we have things like "hardware_handler" and > "path_checker", where the device simply will not function correctly with > certain values. For these attributes, it makes sense to leave them in > the device configuration, even if they are the same as the default > values. Related to path_checker, only two changes were done: 6019c4e0 replace TUR by RDAC in two RDAC devices 40fd537c replace DIRECTIO by TUR(default) Related to hardware_handler, no relevant changes were done. > The goal should be that you can't break any device > configurations by changing the default values. It is going to be too verbose, you should send a patch to prove your thoughts. Thank you. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua
On Sat, Oct 22, 2016 at 07:58:29PM +0200, Xose Vazquez Perez wrote: > On 10/12/2016 06:20 PM, Benjamin Marzinski wrote: > > > On Wed, Oct 12, 2016 at 01:06:10PM +0200, Xose Vazquez Perez wrote: > > >> RDAC devices also depend *exclusively* on "retain_attached_hw_handler" > >> and "detect_prio" to run in ALUA mode. > > > The purpose of the the "retain_attached_hw_handler" and "detect_prio" > > options were to allow devices that support ALUA and non-ALUA modes, to > > detect which mode the device was in, so that the builtin configuration > > would work correctly for both modes. > > Then that is validating my point, hwhandler="1 alua" is superfluous. > Because it's autodetected and autoconfigured from the array, first from > the kernel and later from multipath-tools. > > > If a device is ALUA only, I don't see any benefit to not hardcoding > > that. Simply from a ease-of-use persepective, having ALUA in the > > configuration make it obvious how the device is supposed to be > > configured. > > It's redundant having RETAIN_HWHANDLER_ON and hwhandler="1 alua" defined. > I'm not denying that it is reduntant if the hardware handler gets attached by the scsi subsystem. I'm objecting because I've seen cases where the hardware handler does not get attached by the scsi subsystem. Also, it means that the device will still be configured correctly if the user disables "retain_attached_hw_handler" in the defaults section. > > But more importantly, as has already been metioned, it's > > more robust to hardcode the ALUA handler for the devices that need it in > > case the handler was not attached before multipath started working on > > the device. > > Why you do not trust the SCSI subsystem? It seems a bit paranoid. > Anyway, if you are really uncomfortable with the change, this patch > should be dropped > Because I've seen cases where it doesn't work. They were always dealing with devices not getting correctly set up at boot, and they are solvable, but there is no reason why we need to make solving them necessary in these cases. > > Also, if I recall correctly, for the device handler to get > > attached correctly, we don't just need the device handler module > > isntalled before multipathd starts. We need it installed when the path > > device is initially discovered. > > This is a kernel modules dependency bug. Distributions can bypass it > by including these modules in the initrd image. Sure, but why should we not just be convenient and include the hardware handler in the device config, to avoid this necessity? > > The more I think about this, the more I think we might want to revisit > > some to the configuration simplifications that have been made. > > Could you please review all commits? > git log -p --reverse --since="Mon Jun 13 16:38:50 2016" libmultipath/hwtable.c > What should be removed? Here are some ones that demonstrate what I'm talking about f568ca361a614389b316707091708ba1be972588 92ea4f9e61216a066b9637386142c3f0a054dbae af186a82594248160dff5d759f9dbdea6a42befb (except minio/minio_rq) 1db20948432b75dad1ba13daa50b9f66e8eb78b2 40fd537c4966ec195641e004b756127e94d5e817 In these patches we are removing configuration parameters that are necessary for the device to function correctly. If the user changes these values in the defaults section, it will break the builtin device configurations. The idea for the defaults section values is that they are used for cases where there isn't a builtin configuration for the device. But by making the device configuration depend on the defaults section, this is no longer the case. > > In some cases, I think they went too far. For instance, most devices will > > work > > correctly regardless of the values for things like "rr_minio_rq" and > > "path_selector", so I have no objection to these values being removed > > from the device configuartion, if they are the same as the default > > values. On the other hand, we have things like "hardware_handler" and > > "path_checker", where the device simply will not function correctly with > > certain values. For these attributes, it makes sense to leave them in > > the device configuration, even if they are the same as the default > > values. > > Related to path_checker, only two changes were done: > 6019c4e0 replace TUR by RDAC in two RDAC devices > 40fd537c replace DIRECTIO by TUR(default) > > Related to hardware_handler, no relevant changes were done. I don't have issues with these at all. > > The goal should be that you can't break any device > > configurations by changing the default values. > > It is going to be too verbose, you should send a patch to prove your thoughts. huh? We used to do this. In fact, we also had a bunch of parameters that weren't necessary for the devices to work correctly in almost every device configuration. I am fully in support of removing the unnecessary parameters from the config configuration section, like "rr_minio_rq", "path_selector", etc. I would argue that all builtin