Re: [PATCH 04/30] multipath: Increase dev_loss_tmo prior to fast_io_fail

[Hannes Reinecke <hare@suse.de>]

On 07/19/2013 11:17 PM, Benjamin Marzinski wrote:
> On Tue, Jul 16, 2013 at 09:12:55AM +0200, Hannes Reinecke wrote:
>> There are several constraints when setting fast_io_fail and
>> dev_loss_tmo.
>> dev_loss_tmo will be capped automatically when fast_io_fail is
>> not set. And fast_io_fail can not be raised beyond dev_loss_tmo.
>>
>> So to increase dev_loss_tmo and fast_io_fail we first need
>> to increase dev_loss_tmo to the given fast_io_fail
>> setting, then set fast_io_fail, and then set dev_loss_tmo
>> to the final setting.
> 
> This patch seems kind of convoluted to me, and even with the fix to
> temporarily set dev_loss_tmo to one greater than fast_io_fail_tmo,
> there is still a broken case
> 
> If you currently have:
> fast_io_fail_tmo off
> dev_loss_tmo <something_less_than_600>
> 
> and you want
> 
> fast_io_fail_tmo 600
> dev_loss_tmo <something_greater_than_600>
> 
> This will fail, since it will try to set dev_loss_tmo to 601 with
> fast_io_fail_tmo set to off (Granted, I doubt that 600 is a
> popular fast_io_fail_tmo value).
> 
> But in the general case, If you aren't turning off fast_io_fail_tmo and
> the current dev_loss_tmo is greater than the target fast_io_fail_tmo
> (this seems like it is the most common case), you unecessarily go through
> the work of first setting dev_loss_tmo to its current value.
> 
>         if (mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET &&
>             mpp->fast_io_fail != MP_FAST_IO_FAIL_ZERO &&
>             mpp->fast_io_fail != MP_FAST_IO_FAIL_OFF) {
>                 /* Check if we need to temporarily increase dev_loss_tmo
>  * */
>                 ret = sysfs_attr_get_value(rport_dev, "dev_loss_tmo",
>                                            value, 16);
> 
> <SNIP>
> 
>         }
>         if (strlen(value)) {
>                 ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
>                                            value, strlen(value));
> 
> I have a patch that solves this issue differently.  It still checks
> dev_loss_tmo, but it always sets fast_io_fail_tmo first.  If the target
> fast_io_fail_tmo is greater than or equal to the current dev_loss_tmo,
> it sets fast_io_fail_tmo to one less than the current dev_loss_tmo.
> This always works, since the lowest value possible for dev_loss_tmo is 1
> and the lowest value possible for fast_io_fail_tmo is 0. Then it sets
> dev_loss_tmo. Finally, if necessary, it sets fast_io_fail_tmo to its
> final value.
> 
Ah.

So you're doing (worst case)
-> set fast_io_fail
-> set dev_loss_tmo
-> set fast_io_fail


and I'm doing
-> set dev_loss_tmo
-> set fast_io_fail
-> set dev_loss_tmo

So the only difference here is the '+- 1' offset to fast_io_fail or
dev_loss_tmo ...

Whatever.
Waiting for your patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)