Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua

[Benjamin Marzinski]

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 

Re: [dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua

[Xose Vazquez Perez]

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

[Benjamin Marzinski]

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

[Xose Vazquez Perez]

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

[Xose Vazquez Perez]

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

[Hannes Reinecke]

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

[Christophe Varoqui]

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

[dm-devel] [PATCH] multipath-tools: remove hwhandler when it's alua

[Xose Vazquez Perez]

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