* [PATCH] Remove capping from dev_loss_tmo
@ 2009-12-15 8:26 Hannes Reinecke
2009-12-15 8:54 ` Mike Christie
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Hannes Reinecke @ 2009-12-15 8:26 UTC (permalink / raw)
To: James Bottomley; +Cc: dm-devel, linux-scsi
Currently dev_loss_tmo is capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
This causes problem with multipathing when the 'no_path_retry' setting
exceeds the dev_loss_tmo setting, as then the system might run into
a deadlock when all paths have been removed temporarily for longer
than dev_loss_tmo.
The principal reasons for the capping has been that we should
not allow a remote port to remain in status 'blocked' indefinitely,
so the capping is there to ensure that the port status is being reset
eventually.
However, the fast_io_fail_tmo will also move the remote port out of
the 'blocked' state, so for any HBA driver implementing both the
capping should really be on the fast_io_fail_tmo, and not on the
dev_loss_tmo.
This patch implements just that, ie the fast_io_fail_tmo is capped
to SCSI_DEVICE_BLOCK_TIMEOUT and the capping is removed from
dev_loss_tmo when fast_io_fail_tmo is set.
This allows us to synchronize the dev_loss_tmo setting to the
'no_path_retry' setting from multipathing thus avoiding the deadlock.
Signed-off-by: Hannes Reinecke <hare@suse.de>
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 573ce21..6f39bf4 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
"Maximum number of seconds that the FC transport should"
" insulate the loss of a remote port. Once this value is"
" exceeded, the scsi target is removed. Value should be"
- " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
+ " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
+ " fast_io_fail_tmo is not set.");
/*
* Netlink Infrastructure
@@ -831,9 +832,17 @@ store_fc_rport_dev_loss_tmo(struct device *dev, struct device_attribute *attr,
(rport->port_state == FC_PORTSTATE_NOTPRESENT))
return -EBUSY;
val = simple_strtoul(buf, &cp, 0);
- if ((*cp && (*cp != '\n')) ||
- (val < 0) || (val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT))
+ if ((*cp && (*cp != '\n')) || (val < 0))
return -EINVAL;
+
+ /*
+ * If fast_io_fail is off we have to cap
+ * dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT
+ */
+ if (rport->fast_io_fail_tmo == -1 &&
+ val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+ return -EINVAL;
+
i->f->set_rport_dev_loss_tmo(rport, val);
return count;
}
@@ -914,9 +923,16 @@ store_fc_rport_fast_io_fail_tmo(struct device *dev,
rport->fast_io_fail_tmo = -1;
else {
val = simple_strtoul(buf, &cp, 0);
- if ((*cp && (*cp != '\n')) ||
- (val < 0) || (val >= rport->dev_loss_tmo))
+ if ((*cp && (*cp != '\n')) || (val < 0))
return -EINVAL;
+ /*
+ * Cap fast_io_fail by dev_loss_tmo or
+ * SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
+ */
+ if ((val >= rport->dev_loss_tmo) ||
+ (val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT))
+ return -EINVAL;
+
rport->fast_io_fail_tmo = val;
}
return count;
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] Remove capping from dev_loss_tmo
2009-12-15 8:26 [PATCH] Remove capping from dev_loss_tmo Hannes Reinecke
@ 2009-12-15 8:54 ` Mike Christie
2009-12-15 9:53 ` Hannes Reinecke
2009-12-16 15:33 ` James Smart
2009-12-16 15:34 ` James Smart
2 siblings, 1 reply; 7+ messages in thread
From: Mike Christie @ 2009-12-15 8:54 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel, James Bottomley, linux-scsi
Hannes Reinecke wrote:
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 573ce21..6f39bf4 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
> "Maximum number of seconds that the FC transport should"
> " insulate the loss of a remote port. Once this value is"
> " exceeded, the scsi target is removed. Value should be"
> - " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
> + " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
> + " fast_io_fail_tmo is not set.");
>
What was the largest timeout value you tried? I think I am hitting a bug
with iscsi where if you pass queue_delayed_work a large enough value it
will overflow and instead of getting 100 minutes you get 1.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Remove capping from dev_loss_tmo
2009-12-15 8:54 ` Mike Christie
@ 2009-12-15 9:53 ` Hannes Reinecke
2009-12-17 2:40 ` Mike Christie
0 siblings, 1 reply; 7+ messages in thread
From: Hannes Reinecke @ 2009-12-15 9:53 UTC (permalink / raw)
To: Mike Christie; +Cc: James Bottomley, dm-devel, linux-scsi
Mike Christie wrote:
> Hannes Reinecke wrote:
>> diff --git a/drivers/scsi/scsi_transport_fc.c
>> b/drivers/scsi/scsi_transport_fc.c
>> index 573ce21..6f39bf4 100644
>> --- a/drivers/scsi/scsi_transport_fc.c
>> +++ b/drivers/scsi/scsi_transport_fc.c
>> @@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
>> "Maximum number of seconds that the FC transport should"
>> " insulate the loss of a remote port. Once this value is"
>> " exceeded, the scsi target is removed. Value should be"
>> - " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
>> + " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
>> + " fast_io_fail_tmo is not set.");
>>
>
> What was the largest timeout value you tried? I think I am hitting a bug
> with iscsi where if you pass queue_delayed_work a large enough value it
> will overflow and instead of getting 100 minutes you get 1.
Well, 0x7fffffff works, 0xffffffff is rejected with -EINVAL.
And so should every other value with the top bit set.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Remove capping from dev_loss_tmo
2009-12-15 9:53 ` Hannes Reinecke
@ 2009-12-17 2:40 ` Mike Christie
2009-12-17 2:43 ` Mike Christie
0 siblings, 1 reply; 7+ messages in thread
From: Mike Christie @ 2009-12-17 2:40 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel, James Bottomley, linux-scsi
Hannes Reinecke wrote:
> Mike Christie wrote:
>> Hannes Reinecke wrote:
>>> diff --git a/drivers/scsi/scsi_transport_fc.c
>>> b/drivers/scsi/scsi_transport_fc.c
>>> index 573ce21..6f39bf4 100644
>>> --- a/drivers/scsi/scsi_transport_fc.c
>>> +++ b/drivers/scsi/scsi_transport_fc.c
>>> @@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
>>> "Maximum number of seconds that the FC transport should"
>>> " insulate the loss of a remote port. Once this value is"
>>> " exceeded, the scsi target is removed. Value should be"
>>> - " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
>>> + " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
>>> + " fast_io_fail_tmo is not set.");
>>>
>> What was the largest timeout value you tried? I think I am hitting a bug
>> with iscsi where if you pass queue_delayed_work a large enough value it
>> will overflow and instead of getting 100 minutes you get 1.
>
> Well, 0x7fffffff works, 0xffffffff is rejected with -EINVAL.
> And so should every other value with the top bit set.
>
If someone did 0x7fffffff, then we do 0x7fffffff * HZ in
fc_queue_devloss_work(shost, &rport->dev_loss_work, timeout * HZ);
on a 32bit box, it won't overflow to the value the user wanted will it?
I think it will not happen normally, but users will try it like they
have with iscsi and they will end up not knowing how to set it really high.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Re: [PATCH] Remove capping from dev_loss_tmo
2009-12-17 2:40 ` Mike Christie
@ 2009-12-17 2:43 ` Mike Christie
0 siblings, 0 replies; 7+ messages in thread
From: Mike Christie @ 2009-12-17 2:43 UTC (permalink / raw)
To: device-mapper development; +Cc: James Bottomley, linux-scsi
Mike Christie wrote:
> Hannes Reinecke wrote:
>> Mike Christie wrote:
>>> Hannes Reinecke wrote:
>>>> diff --git a/drivers/scsi/scsi_transport_fc.c
>>>> b/drivers/scsi/scsi_transport_fc.c
>>>> index 573ce21..6f39bf4 100644
>>>> --- a/drivers/scsi/scsi_transport_fc.c
>>>> +++ b/drivers/scsi/scsi_transport_fc.c
>>>> @@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
>>>> "Maximum number of seconds that the FC transport should"
>>>> " insulate the loss of a remote port. Once this value is"
>>>> " exceeded, the scsi target is removed. Value should be"
>>>> - " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
>>>> + " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
>>>> + " fast_io_fail_tmo is not set.");
>>>>
>>> What was the largest timeout value you tried? I think I am hitting a bug
>>> with iscsi where if you pass queue_delayed_work a large enough value it
>>> will overflow and instead of getting 100 minutes you get 1.
>>
>> Well, 0x7fffffff works, 0xffffffff is rejected with -EINVAL.
>> And so should every other value with the top bit set.
>>
>
> If someone did 0x7fffffff, then we do 0x7fffffff * HZ in
>
> fc_queue_devloss_work(shost, &rport->dev_loss_work, timeout * HZ);
>
> on a 32bit box, it won't overflow to the value the user wanted will it?
> I think it will not happen normally, but users will try it like they
> have with iscsi and they will end up not knowing how to set it really high.
>
Oh yeah, I think the idea of the patch is right. If I am reading the
code right, I was just worried about how to handle that issue.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Remove capping from dev_loss_tmo
2009-12-15 8:26 [PATCH] Remove capping from dev_loss_tmo Hannes Reinecke
2009-12-15 8:54 ` Mike Christie
@ 2009-12-16 15:33 ` James Smart
2009-12-16 15:34 ` James Smart
2 siblings, 0 replies; 7+ messages in thread
From: James Smart @ 2009-12-16 15:33 UTC (permalink / raw)
To: device-mapper development; +Cc: James Bottomley, linux-scsi@vger.kernel.org
Hannes,
Not sure I quite agree with your description of the two timers - but
relative to the goal of
allowing a larger dev_loss_tmo when fast_io_fail_tmo is enabled, it's
fine. Interestingly, I
was looking at when it's disabled, does the cap kick back in - and
realized that once we
enable fast fail, we don't allow it to be turned off.
Looks good.
Acked-by: James Smart <james.smart@emulex.com>
-- james s
Hannes Reinecke wrote:
> Currently dev_loss_tmo is capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
> This causes problem with multipathing when the 'no_path_retry' setting
> exceeds the dev_loss_tmo setting, as then the system might run into
> a deadlock when all paths have been removed temporarily for longer
> than dev_loss_tmo.
> The principal reasons for the capping has been that we should
> not allow a remote port to remain in status 'blocked' indefinitely,
> so the capping is there to ensure that the port status is being reset
> eventually.
> However, the fast_io_fail_tmo will also move the remote port out of
> the 'blocked' state, so for any HBA driver implementing both the
> capping should really be on the fast_io_fail_tmo, and not on the
> dev_loss_tmo.
> This patch implements just that, ie the fast_io_fail_tmo is capped
> to SCSI_DEVICE_BLOCK_TIMEOUT and the capping is removed from
> dev_loss_tmo when fast_io_fail_tmo is set.
> This allows us to synchronize the dev_loss_tmo setting to the
> 'no_path_retry' setting from multipathing thus avoiding the deadlock.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
>
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 573ce21..6f39bf4 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
> "Maximum number of seconds that the FC transport should"
> " insulate the loss of a remote port. Once this value is"
> " exceeded, the scsi target is removed. Value should be"
> - " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
> + " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
> + " fast_io_fail_tmo is not set.");
>
> /*
> * Netlink Infrastructure
> @@ -831,9 +832,17 @@ store_fc_rport_dev_loss_tmo(struct device *dev, struct device_attribute *attr,
> (rport->port_state == FC_PORTSTATE_NOTPRESENT))
> return -EBUSY;
> val = simple_strtoul(buf, &cp, 0);
> - if ((*cp && (*cp != '\n')) ||
> - (val < 0) || (val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT))
> + if ((*cp && (*cp != '\n')) || (val < 0))
> return -EINVAL;
> +
> + /*
> + * If fast_io_fail is off we have to cap
> + * dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT
> + */
> + if (rport->fast_io_fail_tmo == -1 &&
> + val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
> + return -EINVAL;
> +
> i->f->set_rport_dev_loss_tmo(rport, val);
> return count;
> }
> @@ -914,9 +923,16 @@ store_fc_rport_fast_io_fail_tmo(struct device *dev,
> rport->fast_io_fail_tmo = -1;
> else {
> val = simple_strtoul(buf, &cp, 0);
> - if ((*cp && (*cp != '\n')) ||
> - (val < 0) || (val >= rport->dev_loss_tmo))
> + if ((*cp && (*cp != '\n')) || (val < 0))
> return -EINVAL;
> + /*
> + * Cap fast_io_fail by dev_loss_tmo or
> + * SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
> + */
> + if ((val >= rport->dev_loss_tmo) ||
> + (val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT))
> + return -EINVAL;
> +
> rport->fast_io_fail_tmo = val;
> }
> return count;
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Remove capping from dev_loss_tmo
2009-12-15 8:26 [PATCH] Remove capping from dev_loss_tmo Hannes Reinecke
2009-12-15 8:54 ` Mike Christie
2009-12-16 15:33 ` James Smart
@ 2009-12-16 15:34 ` James Smart
2 siblings, 0 replies; 7+ messages in thread
From: James Smart @ 2009-12-16 15:34 UTC (permalink / raw)
To: device-mapper development; +Cc: James Bottomley, linux-scsi@vger.kernel.org
Hannes,
Not sure I quite agree with your description of the two timers - but relative
to the goal of
allowing a larger dev_loss_tmo when fast_io_fail_tmo is enabled, it's fine.
Interestingly, I
was looking at when it's disabled, does the cap kick back in - and realized
that once we
enable fast fail, we don't allow it to be turned off.
Looks good.
Acked-by: James Smart <james.smart@emulex.com>
-- james s
Hannes Reinecke wrote:
> Currently dev_loss_tmo is capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
> This causes problem with multipathing when the 'no_path_retry' setting
> exceeds the dev_loss_tmo setting, as then the system might run into
> a deadlock when all paths have been removed temporarily for longer
> than dev_loss_tmo.
> The principal reasons for the capping has been that we should
> not allow a remote port to remain in status 'blocked' indefinitely,
> so the capping is there to ensure that the port status is being reset
> eventually.
> However, the fast_io_fail_tmo will also move the remote port out of
> the 'blocked' state, so for any HBA driver implementing both the
> capping should really be on the fast_io_fail_tmo, and not on the
> dev_loss_tmo.
> This patch implements just that, ie the fast_io_fail_tmo is capped
> to SCSI_DEVICE_BLOCK_TIMEOUT and the capping is removed from
> dev_loss_tmo when fast_io_fail_tmo is set.
> This allows us to synchronize the dev_loss_tmo setting to the
> 'no_path_retry' setting from multipathing thus avoiding the deadlock.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
>
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 573ce21..6f39bf4 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -475,7 +475,8 @@ MODULE_PARM_DESC(dev_loss_tmo,
> "Maximum number of seconds that the FC transport should"
> " insulate the loss of a remote port. Once this value is"
> " exceeded, the scsi target is removed. Value should be"
> - " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT.");
> + " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
> + " fast_io_fail_tmo is not set.");
>
> /*
> * Netlink Infrastructure
> @@ -831,9 +832,17 @@ store_fc_rport_dev_loss_tmo(struct device *dev, struct device_attribute *attr,
> (rport->port_state == FC_PORTSTATE_NOTPRESENT))
> return -EBUSY;
> val = simple_strtoul(buf, &cp, 0);
> - if ((*cp && (*cp != '\n')) ||
> - (val < 0) || (val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT))
> + if ((*cp && (*cp != '\n')) || (val < 0))
> return -EINVAL;
> +
> + /*
> + * If fast_io_fail is off we have to cap
> + * dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT
> + */
> + if (rport->fast_io_fail_tmo == -1 &&
> + val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
> + return -EINVAL;
> +
> i->f->set_rport_dev_loss_tmo(rport, val);
> return count;
> }
> @@ -914,9 +923,16 @@ store_fc_rport_fast_io_fail_tmo(struct device *dev,
> rport->fast_io_fail_tmo = -1;
> else {
> val = simple_strtoul(buf, &cp, 0);
> - if ((*cp && (*cp != '\n')) ||
> - (val < 0) || (val >= rport->dev_loss_tmo))
> + if ((*cp && (*cp != '\n')) || (val < 0))
> return -EINVAL;
> + /*
> + * Cap fast_io_fail by dev_loss_tmo or
> + * SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
> + */
> + if ((val >= rport->dev_loss_tmo) ||
> + (val > SCSI_DEVICE_BLOCK_MAX_TIMEOUT))
> + return -EINVAL;
> +
> rport->fast_io_fail_tmo = val;
> }
> return count;
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-12-17 2:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-12-15 8:26 [PATCH] Remove capping from dev_loss_tmo Hannes Reinecke
2009-12-15 8:54 ` Mike Christie
2009-12-15 9:53 ` Hannes Reinecke
2009-12-17 2:40 ` Mike Christie
2009-12-17 2:43 ` Mike Christie
2009-12-16 15:33 ` James Smart
2009-12-16 15:34 ` James Smart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox