[PATCH 0/4] path latency prio fixes

[PATCH 0/4] path latency prio fixes

[Martin Wilck]

Hi Christophe and Guan,

I was hoping to get down to the review before Christophe committed
your your patch "calculate standard deviation on a logarithmic scale for
prioritizer path_latency". I failed to do this in timely manner, sorry.

But I still have some issues with the patch. Please check these fixes.
I also have remaining issues with the "intermittent IO accounting"
patch, will send these later.

Martin Wilck (4):
  libmultipath: path latency: fix default base num
  libmultipath: path latency: log threshold with p2
  libmultipath: path latency: simplify getprio()
  libmultipath: path latency: remove warnings

 libmultipath/prioritizers/path_latency.c | 112 ++++++++++---------------------
 1 file changed, 34 insertions(+), 78 deletions(-)

-- 
2.15.0

Re: [PATCH 0/4] path latency prio fixes

[Martin Wilck]

Hi Guan,

while staring at this code the other day, I realized another possible
issue with your latency prioritizer. 

It will cause significant IO to every path of a map during multipath /
multipathd startup. If any paths really have latencies as long as your
patch considers (up to 100s), or worse if they don't respond at all,
startup may be *massively* delayed or may even never complete. So if we
a storage with two mirrors with a fast and a slow leg (I reckon that's
the scenario this patch was made for), and if we're out of luck and the
slow leg is probed first, we may end up in a situation where the fast
leg, which may be fully up and healthy, is never set up (or with big
delay) because multipathd keeps waiting for the slow leg to respond.

Similar delays can occur whenever pathinfo(..., DI_PRIO) is called.
Unless I'm overlooking something essential here, that's a really
dangerous thing to do. I believe that before activating this prio
checker for  everyone, we need find a way to avoid this scenario. 

By using aio with a reasonable timeout for the latency check rather
then sync IO, we could at least set an upper limit for the time
get_prio takes. That would be a first step. But I don't think that
would be sufficient. 

What we'd really need is an asynchronous priority checker, similar to
the asynchronous path checker. The get_prio() call would return
immediately with some special return code indicating to the caller that
a priority check is running the background. A preliminary prio would be
set for the path in pathinfo(), and multipathd would re-check later (or
get some sort of event) when the priority check has actually been done.
An open question is what multipathd should do wrt path grouping if it
only has preliminary prio values, in particular with group_by_prio.

Putting Hannes and Ben on CC because I'd like to get their opinion,
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

Re: [PATCH 0/4] path latency prio fixes

[Guan Junxiong]

Hi Martin,

Sorry for the late reply. I have recovered from a busy week.
Please find my comments below.

On 2017/11/23 10:10, Martin wrote:
> Hi Guan,
> 
> while staring at this code the other day, I realized another possible
> issue with your latency prioritizer. 
> 
> It will cause significant IO to every path of a map during multipath /
> multipathd startup. If any paths really have latencies as long as your
> patch considers (up to 100s), or worse if they don't respond at all,
> startup may be *massively* delayed or may even never complete. So if we
> a storage with two mirrors with a fast and a slow leg (I reckon that's
> the scenario this patch was made for), and if we're out of luck and the
> slow leg is probed first, we may end up in a situation where the fast
> leg, which may be fully up and healthy, is never set up (or with big
> delay) because multipathd keeps waiting for the slow leg to respond.
> 

Yes, we should take of the above case.  The prio_args num_io is not the
essential method to avoid this.

> Similar delays can occur whenever pathinfo(..., DI_PRIO) is called.
> Unless I'm overlooking something essential here, that's a really
> dangerous thing to do. I believe that before activating this prio
> checker for  everyone, we need find a way to avoid this scenario. 
> 
> By using aio with a reasonable timeout for the latency check rather
> then sync IO, we could at least set an upper limit for the time
> get_prio takes. That would be a first step. But I don't think that
> would be sufficient. 
> 
> What we'd really need is an asynchronous priority checker, similar to
> the asynchronous path checker. The get_prio() call would return
> immediately with some special return code indicating to the caller that
> a priority check is running the background. A preliminary prio would be

Aligned with this. An asynchronous priority checker is helpful.

> set for the path in pathinfo(), and multipathd would re-check later (or
> get some sort of event) when the priority check has actually been done.
> An open question is what multipathd should do wrt path grouping if it
> only has preliminary prio values, in particular with group_by_prio.

For the open question, in this situation, IMO  it's reasonable to overwriting
the preliminary prio values if we let the user/admin know : "this is an
asynchronous priority checkers and it has high priority to the other synchronous
prioritizer.

> Putting Hannes and Ben on CC because I'd like to get their opinion,
> too.
Me too.


> Regards
> Martin

Re: [PATCH 0/4] path latency prio fixes

[Martin Wilck]

On Mon, 2017-12-04 at 22:23 +0800, Guan Junxiong wrote:
> 
> > An open question is what multipathd should do wrt path grouping if
> > it
> > only has preliminary prio values, in particular with group_by_prio.
> 
> For the open question, in this situation, IMO  it's reasonable to
> overwriting
> the preliminary prio values if we let the user/admin know : "this is
> an
> asynchronous priority checkers and it has high priority to the other
> synchronous
> prioritizer.

This is not what I meant. Once we have the final prio values, it's of
course the right thing to overwrite anything preliminary. 

The "open question" arises at the time when we only have the
preliminary values: We may group paths wrongly. Really bad example: we
might be putting active and passive paths of a storage system into the
same path group, causing endless trespassing... we should probably use
"failover" policy if "group_by_prio" is selected but no reliable
priorities are available.

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

Re: [PATCH 0/4] path latency prio fixes

[Guan Junxiong]

On 2017/12/4 23:11, Martin Wilck wrote:
> On Mon, 2017-12-04 at 22:23 +0800, Guan Junxiong wrote:
>>> An open question is what multipathd should do wrt path grouping if
>>> it
>>> only has preliminary prio values, in particular with group_by_prio.
>> For the open question, in this situation, IMO  it's reasonable to
>> overwriting
>> the preliminary prio values if we let the user/admin know : "this is
>> an
>> asynchronous priority checkers and it has high priority to the other
>> synchronous
>> prioritizer.
> This is not what I meant. Once we have the final prio values, it's of
> course the right thing to overwrite anything preliminary. 
> 
> The "open question" arises at the time when we only have the
> preliminary values: We may group paths wrongly. Really bad example: we
> might be putting active and passive paths of a storage system into the
> same path group, causing endless trespassing... we should probably use
> "failover" policy if "group_by_prio" is selected but no reliable
> priorities are available.

Thanks for your clarification. Agreed.

Re: [PATCH 0/4] path latency prio fixes

[Benjamin Marzinski]

On Mon, Nov 20, 2017 at 10:11:02AM +0100, Martin Wilck wrote:
> Hi Guan,
> 
> while staring at this code the other day, I realized another possible
> issue with your latency prioritizer. 
> 
> It will cause significant IO to every path of a map during multipath /
> multipathd startup. If any paths really have latencies as long as your
> patch considers (up to 100s), or worse if they don't respond at all,
> startup may be *massively* delayed or may even never complete. So if we
> a storage with two mirrors with a fast and a slow leg (I reckon that's
> the scenario this patch was made for), and if we're out of luck and the
> slow leg is probed first, we may end up in a situation where the fast
> leg, which may be fully up and healthy, is never set up (or with big
> delay) because multipathd keeps waiting for the slow leg to respond.
> 
> Similar delays can occur whenever pathinfo(..., DI_PRIO) is called.
> Unless I'm overlooking something essential here, that's a really
> dangerous thing to do. I believe that before activating this prio
> checker for  everyone, we need find a way to avoid this scenario. 
> 
> By using aio with a reasonable timeout for the latency check rather
> then sync IO, we could at least set an upper limit for the time
> get_prio takes. That would be a first step. But I don't think that
> would be sufficient. 
> 
> What we'd really need is an asynchronous priority checker, similar to
> the asynchronous path checker. The get_prio() call would return
> immediately with some special return code indicating to the caller that
> a priority check is running the background. A preliminary prio would be
> set for the path in pathinfo(), and multipathd would re-check later (or
> get some sort of event) when the priority check has actually been done.
> An open question is what multipathd should do wrt path grouping if it
> only has preliminary prio values, in particular with group_by_prio.

Yeah, you're right. Something like this is necessary. We could have the
prioritizers work like the checkers and have them include a context,
that the checkers can use to save data.  Then could make this work like
the directio checker, with its async calls.

> Putting Hannes and Ben on CC because I'd like to get their opinion,
> 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)