Remove capping from dev_loss_tmo

PATCH In review Hannes Reinecke 3 participants 7 messages Dec 15, 2009 – Dec 17, 2009

Subsystems (2)

SCSI Device mapper

lore

Share & tools

lore

Hannes Reinecke

author

2009-12-15 08:26 UTC #0 lore raw

PATCH 1/1 Remove capping from dev_loss_tmo

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 <[email protected]>

drivers/scsi/scsi_transport_fc.c +21 -5

index 573ce21..6f39bf4 100644
@@ -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;

Mike Christie

2009-12-15 08:54 UTC #1 lore raw

drivers/scsi/scsi_transport_fc.c MODULE_PARM_DESC(dev_loss_tmo, · line 480

Quoted 12 lines · Hannes Reinecke

drivers/scsi/scsi_transport_fc.c +2 -1

index 573ce21..6f39bf4 100644
@@ -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.

Hannes Reinecke

author

2009-12-15 09:53 UTC #2 lore raw

Quoted 10 lines · Mike Christie

Quoted 9 lines · Hannes Reinecke

drivers/scsi/scsi_transport_fc.c +0 -0

index 573ce21..6f39bf4 100644
@@ -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"

Quoted 8 lines

drivers/scsi/scsi_transport_fc.c MODULE_PARM_DESC(dev_loss_tmo, · line 480
Quoted 4 lines
drivers/scsi/scsi_transport_fc.c +2 -1
-         " 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

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Christie

2009-12-17 02:40 UTC #3 lore raw

Quoted 19 lines · Hannes Reinecke

Quoted 17 lines · Mike Christie
drivers/scsi/scsi_transport_fc.c MODULE_PARM_DESC(dev_loss_tmo, · line 480
Quoted 13 lines · Hannes Reinecke
drivers/scsi/scsi_transport_fc.c +2 -1
index 573ce21..6f39bf4 100644
@@ -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.

Quoted 3 lines

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.

Mike Christie

2009-12-17 02:43 UTC #4 lore raw

Quoted 24 lines · Mike Christie

Quoted 19 lines · Hannes Reinecke
Quoted 17 lines · Mike Christie
drivers/scsi/scsi_transport_fc.c MODULE_PARM_DESC(dev_loss_tmo, · line 480
Quoted 13 lines · Hannes Reinecke
drivers/scsi/scsi_transport_fc.c +2 -1
index 573ce21..6f39bf4 100644
@@ -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.
Quoted 3 lines
Well, 0x7fffffff works, 0xffffffff is rejected with -EINVAL. And so should every other value with the top bit set.

Quoted 6 lines

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.

James Smart

Acked-by on v1 1/1

2009-12-16 15:33 UTC #5 lore raw

In reply to Hannes Reinecke

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 <[email protected]>

-- james s

Quoted 81 lines · Hannes Reinecke

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 <[email protected]>
drivers/scsi/scsi_transport_fc.c +21 -6
index 573ce21..6f39bf4 100644
@@ -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
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

James Smart

Acked-by on v1 1/1

2009-12-16 15:34 UTC #6 lore raw

In reply to Hannes Reinecke

Hannes,

Looks good.

Acked-by James Smart <[email protected]>

-- james s

Quoted 80 lines · Hannes Reinecke

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 <[email protected]>
drivers/scsi/scsi_transport_fc.c +21 -6
index 573ce21..6f39bf4 100644
@@ -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
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel