[dm-devel] [PATCH 0/3] handle transitioning devices in TUR checker

[dm-devel] [PATCH 0/3] handle transitioning devices in TUR checker

[Benjamin Marzinski]

This patchset is based on Brian Bunker's "libmultipath: return
'ghost' state when port is in transition" patch:

https://listman.redhat.com/archives/dm-devel/2023-February/053344.html
https://github.com/opensvc/multipath-tools/pull/60

Instead of setting the state to PATH_GHOST, it uses PATH_PENDING. The
other two patches are small cleanups to the TUR checker that I noticed
while writing the first patch.

Benjamin Marzinski (3):
  libmultipath: return 'pending' state when port is in transition
  libmultipath: set init failure message when init fails
  libmultipath: reset nr_timeouts if we freed the context

 libmultipath/checkers/tur.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 1/3] libmultipath: return 'pending' state when port is in transition

[Benjamin Marzinski]

The tur checker should not return that a path is down when it is in the
transitioning state. Instead, it should return PATH_PENDING, so that
the path retains its current state, and multipathd can react quickly
when it moves out of the transitioning state.

The code needs to be careful to differentiate between when the checker
thread has finished and returned PATH_PENDING, and when it is still
running.

Reported-by: Brian Bunker <brian@purestorage.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 551dc4f0..a5045f10 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -32,6 +32,7 @@ enum {
 	MSG_TUR_RUNNING = CHECKER_FIRST_MSGID,
 	MSG_TUR_TIMEOUT,
 	MSG_TUR_FAILED,
+	MSG_TUR_TRANSITIONING,
 };
 
 #define _IDX(x) (MSG_ ## x - CHECKER_FIRST_MSGID)
@@ -39,6 +40,7 @@ const char *libcheck_msgtable[] = {
 	[_IDX(TUR_RUNNING)] = " still running",
 	[_IDX(TUR_TIMEOUT)] = " timed out",
 	[_IDX(TUR_FAILED)] = " failed to initialize",
+	[_IDX(TUR_TRANSITIONING)] = " reports path is transitioning",
 	NULL,
 };
 
@@ -186,6 +188,13 @@ retry:
 				 */
 				*msgid = CHECKER_MSGID_GHOST;
 				return PATH_GHOST;
+			} else if (asc == 0x04 && ascq == 0x0a) {
+				/*
+				 * LOGICAL UNIT NOT ACCESSIBLE,
+				 * ASYMMETRIC ACCESS STATE TRANSITION
+				 */
+				*msgid = MSG_TUR_TRANSITIONING;
+				return PATH_PENDING;
 			}
 		}
 		*msgid = CHECKER_MSGID_DOWN;
@@ -350,6 +359,7 @@ int libcheck_check(struct checker * c)
 			condlog(3, "%d:%d : tur checker not finished",
 				major(ct->devt), minor(ct->devt));
 			tur_status = PATH_PENDING;
+			c->msgid = MSG_TUR_RUNNING;
 		} else {
 			/* TUR checker done */
 			ct->thread = 0;
@@ -404,7 +414,7 @@ int libcheck_check(struct checker * c)
 		/* Start new TUR checker */
 		pthread_mutex_lock(&ct->lock);
 		tur_status = ct->state = PATH_PENDING;
-		ct->msgid = CHECKER_MSGID_NONE;
+		c->msgid = ct->msgid = MSG_TUR_RUNNING;
 		pthread_mutex_unlock(&ct->lock);
 		ct->fd = c->fd;
 		ct->timeout = c->timeout;
@@ -424,7 +434,7 @@ int libcheck_check(struct checker * c)
 		}
 		tur_timeout(&tsp);
 		pthread_mutex_lock(&ct->lock);
-		if (ct->state == PATH_PENDING)
+		if (ct->state == PATH_PENDING && ct->msgid == MSG_TUR_RUNNING)
 			r = pthread_cond_timedwait(&ct->active, &ct->lock,
 						   &tsp);
 		if (!r) {
@@ -432,7 +442,7 @@ int libcheck_check(struct checker * c)
 			c->msgid = ct->msgid;
 		}
 		pthread_mutex_unlock(&ct->lock);
-		if (tur_status == PATH_PENDING) {
+		if (tur_status == PATH_PENDING && c->msgid == MSG_TUR_RUNNING) {
 			condlog(4, "%d:%d : tur checker still running",
 				major(ct->devt), minor(ct->devt));
 		} else {
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 2/3] libmultipath: set init failure message when init fails

[Benjamin Marzinski]

The tur checker has a message for initialization failure. We might as
well use it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a5045f10..a19becf5 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -400,8 +400,10 @@ int libcheck_check(struct checker * c)
 			 * It fails only in OOM situations. In this case, return
 			 * PATH_UNCHECKED to avoid prematurely failing the path.
 			 */
-			if (libcheck_init(c) != 0)
+			if (libcheck_init(c) != 0) {
+				c->msgid = MSG_TUR_FAILED;
 				return PATH_UNCHECKED;
+			}
 			((struct tur_checker_context *)c->context)->nr_timeouts = ct->nr_timeouts;
 
 			if (!uatomic_sub_return(&ct->holders, 1))
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 3/3] libmultipath: reset nr_timeouts if we freed the context

[Benjamin Marzinski]

If a the tur checker creates a new context because an old thread is
still running, but the old thread finishes before the checker drops
the old context, the checker should reset nr_timeouts to 0, since
the old thread did complete in time.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a19becf5..fe6a2f14 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -406,9 +406,11 @@ int libcheck_check(struct checker * c)
 			}
 			((struct tur_checker_context *)c->context)->nr_timeouts = ct->nr_timeouts;
 
-			if (!uatomic_sub_return(&ct->holders, 1))
+			if (!uatomic_sub_return(&ct->holders, 1)) {
 				/* It did terminate, eventually */
 				cleanup_context(ct);
+				((struct tur_checker_context *)c->context)->nr_timeouts = 0;
+			}
 
 			ct = c->context;
 		} else
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] libmultipath: reset nr_timeouts if we freed the context

[Martin Wilck]

On Tue, 2023-03-07 at 16:49 -0600, Benjamin Marzinski wrote:
> If a the tur checker creates a new context because an old thread is
> still running, but the old thread finishes before the checker drops
> the old context, the checker should reset nr_timeouts to 0, since
> the old thread did complete in time.

This looks strange to me. First of all, the old thread _did_ timeout,
otherwise we wouldn't be in this code path at all. And even if you say
this timeout shouldn't count, as the thread isn't in stalled state any
more, shouldn't you just decrease nr_timeouts by 1? The fact that
_this_ thread terminated doesn't tell us anything about other hanging
threads.

Martin




> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/checkers/tur.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/checkers/tur.c
> b/libmultipath/checkers/tur.c
> index a19becf5..fe6a2f14 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -406,9 +406,11 @@ int libcheck_check(struct checker * c)
>                         }
>                         ((struct tur_checker_context *)c->context)-
> >nr_timeouts = ct->nr_timeouts;
>  
> -                       if (!uatomic_sub_return(&ct->holders, 1))
> +                       if (!uatomic_sub_return(&ct->holders, 1)) {
>                                 /* It did terminate, eventually */
>                                 cleanup_context(ct);
> +                               ((struct tur_checker_context *)c-
> >context)->nr_timeouts = 0;
> +                       }
>  
>                         ct = c->context;
>                 } else

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] libmultipath: reset nr_timeouts if we freed the context

[Benjamin Marzinski]

On Wed, Mar 08, 2023 at 06:17:05PM +0000, Martin Wilck wrote:
> On Tue, 2023-03-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > If a the tur checker creates a new context because an old thread is
> > still running, but the old thread finishes before the checker drops
> > the old context, the checker should reset nr_timeouts to 0, since
> > the old thread did complete in time.
> 
> This looks strange to me. First of all, the old thread _did_ timeout,
> otherwise we wouldn't be in this code path at all. And even if you say
> this timeout shouldn't count, as the thread isn't in stalled state any
> more, shouldn't you just decrease nr_timeouts by 1? The fact that
> _this_ thread terminated doesn't tell us anything about other hanging
> threads.

We can't track the old threads once we drop the old context.  So
instead, we just assume that if the last thread we created did complete
we're back to a normal state, and reset nr_timeouts to 0. This change is
just extending that same logic to the case where the old thread didn't
complete until the last possible moment where we could find that out.

The MAX_NR_TIMEOUTS code was added because a user ran into a situation
where a scsi kernel issue made TUR commands block uninterruptibly
forever, and multipathd kept making more threads, tens of thousnds of
them. The goal was to stop creating threads if two in a row haven't
completed, and wait for the last thread to complete. It's not the most
robust solution possible, but I think it's good enough. We could revist
the whole solution, but the way things are, nr_timeouts = 1 was supposed
to mean that, as far as we know, the last thread was cancelled but
didn't complete, and we are going to try one more thread before stopping
and waiting. I'm just fixing to code to take care of the corner case
where we find out that the last thread did complete when we drop the old
context. 

-Ben
 
> Martin
> 
> 
> 
> 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/checkers/tur.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index a19becf5..fe6a2f14 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -406,9 +406,11 @@ int libcheck_check(struct checker * c)
> >                         }
> >                         ((struct tur_checker_context *)c->context)-
> > >nr_timeouts = ct->nr_timeouts;
> >  
> > -                       if (!uatomic_sub_return(&ct->holders, 1))
> > +                       if (!uatomic_sub_return(&ct->holders, 1)) {
> >                                 /* It did terminate, eventually */
> >                                 cleanup_context(ct);
> > +                               ((struct tur_checker_context *)c-
> > >context)->nr_timeouts = 0;
> > +                       }
> >  
> >                         ct = c->context;
> >                 } else
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] handle transitioning devices in TUR checker

[mwilck]

Brian,

could you give this patch set a test in your environment?

On Tue, 2023-03-07 at 16:49 -0600, Benjamin Marzinski wrote:
> This patchset is based on Brian Bunker's "libmultipath: return
> 'ghost' state when port is in transition" patch:
> 
> https://listman.redhat.com/archives/dm-devel/2023-February/053344.html
> https://github.com/opensvc/multipath-tools/pull/60
> 
> Instead of setting the state to PATH_GHOST, it uses PATH_PENDING. The
> other two patches are small cleanups to the TUR checker that I
> noticed
> while writing the first patch.
> 
> Benjamin Marzinski (3):
>   libmultipath: return 'pending' state when port is in transition
>   libmultipath: set init failure message when init fails
>   libmultipath: reset nr_timeouts if we freed the context
> 
>  libmultipath/checkers/tur.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 

I'd like to wait for Brian's results. From code inspection, and with
Ben's explanation for the 3/3 logic,

For the set:
Reviewed-by: Martin Wilck <mwilck@suse.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] handle transitioning devices in TUR checker

[Brian Bunker]

> On Mar 9, 2023, at 12:38 AM, mwilck@googlemail.com wrote:
> 
> Brian,
> 
> could you give this patch set a test in your environment?
> 
> On Tue, 2023-03-07 at 16:49 -0600, Benjamin Marzinski wrote:
>> This patchset is based on Brian Bunker's "libmultipath: return
>> 'ghost' state when port is in transition" patch:
>> 
>> https://listman.redhat.com/archives/dm-devel/2023-February/053344.html
>> https://github.com/opensvc/multipath-tools/pull/60
>> 
>> Instead of setting the state to PATH_GHOST, it uses PATH_PENDING. The
>> other two patches are small cleanups to the TUR checker that I
>> noticed
>> while writing the first patch.
>> 
>> Benjamin Marzinski (3):
>>   libmultipath: return 'pending' state when port is in transition
>>   libmultipath: set init failure message when init fails
>>   libmultipath: reset nr_timeouts if we freed the context
>> 
>>  libmultipath/checkers/tur.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>> 
> 
> I'd like to wait for Brian's results. From code inspection, and with
> Ben's explanation for the 3/3 logic,
> 
> For the set:
> Reviewed-by: Martin Wilck <mwilck@suse.com>
Ben and Martin,

This works well for me against my array. A couple of things in this part

Can you make the above match your formatting since it is better:
if( asc == 0x04 && ascq == 0x0b){

if (asc == 0x04 && ascq == 0x0a) {

And I don’t think you need else if since the line above it returns in its
case, so else is not hit if the first if is true:

    return PATH_GHOST;
} else if (asc == 0x04 && ascq == 0x0a) {

Thanks,
Brian



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] handle transitioning devices in TUR checker

[Benjamin Marzinski]

On Fri, Mar 10, 2023 at 11:55:19AM -0800, Brian Bunker wrote:
> > On Mar 9, 2023, at 12:38 AM, mwilck@googlemail.com wrote:
> > 
> > Brian,
> > 
> > could you give this patch set a test in your environment?
> > 
> > On Tue, 2023-03-07 at 16:49 -0600, Benjamin Marzinski wrote:
> >> This patchset is based on Brian Bunker's "libmultipath: return
> >> 'ghost' state when port is in transition" patch:
> >> 
> >> https://listman.redhat.com/archives/dm-devel/2023-February/053344.html
> >> https://github.com/opensvc/multipath-tools/pull/60
> >> 
> >> Instead of setting the state to PATH_GHOST, it uses PATH_PENDING. The
> >> other two patches are small cleanups to the TUR checker that I
> >> noticed
> >> while writing the first patch.
> >> 
> >> Benjamin Marzinski (3):
> >>   libmultipath: return 'pending' state when port is in transition
> >>   libmultipath: set init failure message when init fails
> >>   libmultipath: reset nr_timeouts if we freed the context
> >> 
> >>  libmultipath/checkers/tur.c | 24 +++++++++++++++++++-----
> >>  1 file changed, 19 insertions(+), 5 deletions(-)
> >> 
> > 
> > I'd like to wait for Brian's results. From code inspection, and with
> > Ben's explanation for the 3/3 logic,
> > 
> > For the set:
> > Reviewed-by: Martin Wilck <mwilck@suse.com>
> Ben and Martin,
> 
> This works well for me against my array. A couple of things in this part
> 
> Can you make the above match your formatting since it is better:
> if( asc == 0x04 && ascq == 0x0b){
> 
> if (asc == 0x04 && ascq == 0x0a) {
> 
> And I don’t think you need else if since the line above it returns in its
> case, so else is not hit if the first if is true:
> 
>     return PATH_GHOST;
> } else if (asc == 0x04 && ascq == 0x0a) {
> 

Sure.

-Ben

> Thanks,
> Brian
> 
> 
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel