[dm-devel] [RFC PATCH v3] dm mpath: add a queue_if_no_path timeout

[Junichi Nomura]

On 11/07/13 04:21, Mike Snitzer wrote:
> On Wed, Nov 06 2013 at 10:43am -0500,
> Frank Mayhar <fmayhar at google.com> wrote:
> 
>> On Wed, 2013-11-06 at 07:54 +0100, Hannes Reinecke wrote:
>>> On 11/05/2013 05:02 PM, Frank Mayhar wrote:
>>>> This is the patch submitted by Jun'ichi Nomura, originally based on
>>>> Mike's patch with some small changes by me.  Jun'ichi's description
>>>> follows, along with my changes:
>>>>
>>>> On Tue, 2013-11-05 at 07:18 -0800, Frank Mayhar wrote:
>>>>> On Fri, 2013-11-01 at 04:17 +0000, Junichi Nomura wrote:
>>>>>> I slightly modified the patch:
>>>>>>   - fixed the timeout handler to correctly find
>>>>>>     clone request and "struct multipath"
>>>>>>   - the timeout handler just disables "queue_if_no_path"
>>>>>>     instead of killing the request directly
>>>>>>   - "dmsetup status" to show the parameter
>>>>>>   - changed an interface between dm core and target
>>>>>>   - added some debugging printk (you can remove them)
>>>>>> and checked the timeout occurs, at least.
>>>>>>
>>>>>> I'm not sure if this feature is good or not though.
>>>>>> (The timer behavior is not intuitive, I think)
>>>>> Thanks!  I integrated your new patch and tested it.  Sure enough, it
>>>>> seems to work.  I've made a few tweaks (added a module tunable and
>>>>> support for setting the timer in multipath_message(), removed your debug
>>>>> printks) and will submit the modified patch for discussion shortly.
>>>>
>>>> Comments?
>>>>
>>> Yeah. Seems to be my eternal fate; initiating fixes and not getting
>>> mentioned at all.
>>> Sigh.
>>>
>>> I dimly remember having sent the original patch for the blk timeout
>>> function ... hence a short notice would've been nice.
>>
>> Sorry, I did ding you early on (and I think Mike dinged you as well),
>> but you were apparently busy with other things at the time.
> 
> Hi Frank,
> 
> I wouldn't worry about this.  You didn't even supply a patch header.. so
> it isn't like Hannes was obviously left out.  Fact is this patch has had
> 4 iterations, the first of which from Hannes didn't compile or even make
> sense.  Anyway, he'll get attribution through Suggested-by unless he
> wins the race to produces the first upstream-worthy variant of this line
> of work.
> 
> So far it has all been RFC-style patches.. your most recent one that
> builds on Jun'ichi's patch with a module param and timeout default:  We
> generally don't add module params to targets (but I can appreciate why
> you might want that.. just not seeing the need).  And having a message
> to change the timeout conflicts with my desire to conditionally
> establish a timed_out method only if a timeout was specified (like my
> last reply in this thread suggested).

BTW, this approach of using blk timer might conflict with
possible future removal of multipath internal queue:
  https://www.redhat.com/archives/dm-devel/2013-November/msg00025.html
because the timer is stopped if the request is queued to block layer.
To cope with that, we might have to extend block layer to run timer
for queued request, etc.

-- 
Jun'ichi Nomura, NEC Corporation