multipathd:checkerloop() - why not check path state before calling pathinfo(), which calls path priority callouts?

multipathd:checkerloop() - why not check path state before calling pathinfo(), which calls path priority callouts?

[David Wysochanski]

Looking through the mulitpathd code, in particular checkerloop() and the
below snippit.  I'm wondering why the path state isn't checked and then the
pathinfo() not called if the path state is PATH_DOWN (or maybe !PATH_UP).
Seems like we shouldn't be trying to refresh the priority if we know the
path is bad anyway, since the callouts all issue scsi passthru commands,
which will always fail.  Is this right or am I missing something?

Thanks.



            /*
             * path prio refreshing
             */
            condlog(4, "path prio refresh");
            pathinfo(pp, conf->hwtable, DI_PRIO);

            if (need_switch_pathgroup(pp->mpp, 0)) {
                if (pp->mpp->pgfailback > 0 &&
                    pp->mpp->failback_tick <= 0)
                    pp->mpp->failback_tick =
                        pp->mpp->pgfailback + 1;
                else if (pp->mpp->pgfailback ==
                        -FAILBACK_IMMEDIATE)
                    switch_pathgroup(pp->mpp);
            }

RE: multipathd:checkerloop() - why not check path stat e before calling pathinfo(), which calls path priority callouts?

[egoggin]

On Wednesday, March 01, 2006 2:55 PM David Wysochans wrote

> 
> Looking through the mulitpathd code, in particular 
> checkerloop() and the
> below snippit.  I'm wondering why the path state isn't 
> checked and then the
> pathinfo() not called if the path state is PATH_DOWN (or 
> maybe !PATH_UP).
> Seems like we shouldn't be trying to refresh the priority if 
> we know the
> path is bad anyway, since the callouts all issue scsi 
> passthru commands,
> which will always fail.  Is this right or am I missing something?

I think you are definitely right, but ...

Keep in mind that some of the multipath path checkers set the
multipath user state to PATH_DOWN even though the scsi pass
through io succeeded.  So some scsi pass through commands may
succeed, including path priority checking, path health testing,
uid generation, and serial number retrieval even though the
path's multipath state is PATH_DOWN.  So I think another reason
to not issue the path priority update for a path in the PATH_DOWN
state is that the updated priority value isn't even used.

The calculation of a path group's priority does not include paths
in a PATH_DOWN state.  See select_path_group() in
libmultipath/switchgroup.c.  So there is no need to check if the
priority of a path in a PATH_DOWN state has changed.

But, I would instead patch pathinfo() in libmultipath/discovery.c
to not call get_prio() if the path's state is PATH_DOWN.  Doing
so will catch all other cases of calls to pathinfo() with
DI_PRIO (like in need_switch_pathgroup() in multipathd/main.c).
 
> 
> Thanks.
> 
> 
> 
>             /*
>              * path prio refreshing
>              */
>             condlog(4, "path prio refresh");
>             pathinfo(pp, conf->hwtable, DI_PRIO);
> 
>             if (need_switch_pathgroup(pp->mpp, 0)) {
>                 if (pp->mpp->pgfailback > 0 &&
>                     pp->mpp->failback_tick <= 0)
>                     pp->mpp->failback_tick =
>                         pp->mpp->pgfailback + 1;
>                 else if (pp->mpp->pgfailback ==
>                         -FAILBACK_IMMEDIATE)
>                     switch_pathgroup(pp->mpp);
>             }

Re: multipathd:checkerloop() - why not check path state before calling pathinfo(), which calls path priority callouts?

[David Wysochanski]

[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]

egoggin@emc.com wrote:
>
> On Wednesday, March 01, 2006 2:55 PM David Wysochans wrote
>
> >
> > Looking through the mulitpathd code, in particular
> > checkerloop() and the
> > below snippit.  I'm wondering why the path state isn't
> > checked and then the
> > pathinfo() not called if the path state is PATH_DOWN (or
> > maybe !PATH_UP).
> > Seems like we shouldn't be trying to refresh the priority if
> > we know the
> > path is bad anyway, since the callouts all issue scsi
> > passthru commands,
> > which will always fail.  Is this right or am I missing something?
>
> I think you are definitely right, but ...
>
> Keep in mind that some of the multipath path checkers set the
> multipath user state to PATH_DOWN even though the scsi pass
> through io succeeded.  So some scsi pass through commands may
> succeed, including path priority checking, path health testing,
> uid generation, and serial number retrieval even though the
> path's multipath state is PATH_DOWN.  So I think another reason
> to not issue the path priority update for a path in the PATH_DOWN
> state is that the updated priority value isn't even used.
>
> The calculation of a path group's priority does not include paths
> in a PATH_DOWN state.  See select_path_group() in
> libmultipath/switchgroup.c.  So there is no need to check if the
> priority of a path in a PATH_DOWN state has changed.
>
> But, I would instead patch pathinfo() in libmultipath/discovery.c
> to not call get_prio() if the path's state is PATH_DOWN.  Doing
> so will catch all other cases of calls to pathinfo() with
> DI_PRIO (like in need_switch_pathgroup() in multipathd/main.c).
>  
>
Ok, attached is the 1-line patch against libmultipath/discovery.c
Should apply cleanly against latest source.


> >
> > Thanks.
> >
> >
> >
> >             /*
> >              * path prio refreshing
> >              */
> >             condlog(4, "path prio refresh");
> >             pathinfo(pp, conf->hwtable, DI_PRIO);
> >
> >             if (need_switch_pathgroup(pp->mpp, 0)) {
> >                 if (pp->mpp->pgfailback > 0 &&
> >                     pp->mpp->failback_tick <= 0)
> >                     pp->mpp->failback_tick =
> >                         pp->mpp->pgfailback + 1;
> >                 else if (pp->mpp->pgfailback ==
> >                         -FAILBACK_IMMEDIATE)
> >                     switch_pathgroup(pp->mpp);
> >             }
>
> -- 
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>


[-- Attachment #2: dont-call-get-prio-if-path-down.patch --]
[-- Type: text/x-patch, Size: 347 bytes --]

--- libmultipath/discovery.c.orig	2006-03-01 18:29:38.000000000 -0500
+++ libmultipath/discovery.c	2006-03-01 18:33:10.000000000 -0500
@@ -652,7 +652,7 @@ pathinfo (struct path *pp, vector hwtabl
 	/*
 	 * get path prio
 	 */
-	if (mask & DI_PRIO) {
+	if (mask & DI_PRIO && pp->state != PATH_DOWN) {
 		if (!pp->getprio)
 			select_getprio(pp);
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]