Re: [PATCH] fc transport: resolve scan vs delete deadlocks

[Michael Reed <mdr@sgi.com>]

Will this patch be integrated into scsi-misc or other tree at some point?

Thanks,
 Mike


James Smart wrote:
> In a prior posting to linux-scsi on the fc transport and workq
> deadlocks, we noted a second error that did not have a patch:
>   http://marc.theaimsgroup.com/?l=linux-scsi&m=114467847711383&w=2
>   - There's a deadlock where scsi_remove_target() has to sit behind 
>     scsi_scan_target() due to contention over the scan_lock().
> 
> Subsequently we posted a request for comments about the deadlock:
>   http://marc.theaimsgroup.com/?l=linux-scsi&m=114469358829500&w=2
> 
> This posting resolves the second error. Here's what we now understand,
> and are implementing:
> 
>   If the lldd deletes the rport while a scan is active, the sdev's queue
>   is blocked which stops the issuing of commands associated with the scan.
>   At this point, the scan stalls, and does so with the shost->scan_mutex held.
>   If, at this point, if any scan or delete request is made on the host, it
>   will stall waiting for the scan_mutex.
>   
>   For the FC transport, we queue all delete work to a single workq.
>   So, things worked fine when competing with the scan, as long as the
>   target blocking the scan was the same target at the top of our delete
>   workq, as the delete workq routine always unblocked just prior to
>   requesting the delete.  Unfortunately, if the top of our delete workq
>   was for a different target, we deadlock.  Additionally, if the target
>   blocking scan returned, we were unblocking it in the scan workq routine,
>   which really won't execute until the existing stalled scan workq
>   completes (e.g. we're re-scheduling it while it is in the midst of its
>   execution).
> 
>   This patch moves the unblock out of the workq routines and moves it to
>   the context that is scheduling the work. This ensures that at some point,
>   we will unblock the target that is blocking scan.  Please note, however,
>   that the deadlock condition may still occur while it waits for the
>   transport to timeout an unblock on a target.  Worst case, this is bounded
>   by the transport dev_loss_tmo (default: 30 seconds).
> 
> 
> Finally, Michael Reed deserves the credit for the bulk of this patch,
> analysis, and it's testing. Thank you for your help.
> 
> -- james s
> 
> Note: this patch supercedes a previous preliminary post by Michael Reed
>   http://marc.theaimsgroup.com/?l=linux-scsi&m=114675967824701&w=2
> 
> Note: The request for comments statements about the gross-ness of the
>   scan_mutex still stand.
> 
> 
> 
> Signed-off-by: Michael Reed <mdr@sgi.com>
> Signed-off-by: James Smart <james.smart@emulex.com>
> 
> 
> diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> --- a/drivers/scsi/scsi_transport_fc.c	2006-05-11 10:43:31.000000000 -0400
> +++ b/drivers/scsi/scsi_transport_fc.c	2006-05-11 12:04:36.000000000 -0400
> @@ -1284,7 +1284,9 @@ EXPORT_SYMBOL(fc_release_transport);
>   * @work:	Work to queue for execution.
>   *
>   * Return value:
> - * 	0 on success / != 0 for error
> + * 	1 - work queued for execution
> + *	0 - work is already queued
> + *	-EINVAL - work queue doesn't exist
>   **/
>  static int
>  fc_queue_work(struct Scsi_Host *shost, struct work_struct *work)
> @@ -1434,8 +1436,6 @@ fc_starget_delete(void *data)
>  	struct Scsi_Host *shost = rport_to_shost(rport);
>  	unsigned long flags;
>  
> -	scsi_target_unblock(&rport->dev);
> -
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	if (rport->flags & FC_RPORT_DEVLOSS_PENDING) {
>  		spin_unlock_irqrestore(shost->host_lock, flags);
> @@ -1707,6 +1707,8 @@ fc_remote_port_add(struct Scsi_Host *sho
>  
>  				spin_unlock_irqrestore(shost->host_lock, flags);
>  
> +				scsi_target_unblock(&rport->dev);
> +
>  				return rport;
>  			}
>  		}
> @@ -1762,9 +1764,10 @@ fc_remote_port_add(struct Scsi_Host *sho
>  				/* initiate a scan of the target */
>  				rport->flags |= FC_RPORT_SCAN_PENDING;
>  				scsi_queue_work(shost, &rport->scan_work);
> -			}
> -
> -			spin_unlock_irqrestore(shost->host_lock, flags);
> +				spin_unlock_irqrestore(shost->host_lock, flags);
> +				scsi_target_unblock(&rport->dev);
> +			} else
> +				spin_unlock_irqrestore(shost->host_lock, flags);
>  
>  			return rport;
>  		}
> @@ -1938,6 +1941,7 @@ fc_remote_port_rolechg(struct fc_rport  
>  		rport->flags |= FC_RPORT_SCAN_PENDING;
>  		scsi_queue_work(shost, &rport->scan_work);
>  		spin_unlock_irqrestore(shost->host_lock, flags);
> +		scsi_target_unblock(&rport->dev);
>  	}
>  }
>  EXPORT_SYMBOL(fc_remote_port_rolechg);
> @@ -1970,8 +1974,9 @@ fc_timeout_deleted_rport(void  *data)
>  		dev_printk(KERN_ERR, &rport->dev,
>  			"blocked FC remote port time out: no longer"
>  			" a FCP target, removing starget\n");
> -		fc_queue_work(shost, &rport->stgt_delete_work);
>  		spin_unlock_irqrestore(shost->host_lock, flags);
> +		scsi_target_unblock(&rport->dev);
> +		fc_queue_work(shost, &rport->stgt_delete_work);
>  		return;
>  	}
>  
> @@ -2035,17 +2040,15 @@ fc_timeout_deleted_rport(void  *data)
>  	 * went away and didn't come back - we'll remove
>  	 * all attached scsi devices.
>  	 */
> -	fc_queue_work(shost, &rport->stgt_delete_work);
> -
>  	spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +	scsi_target_unblock(&rport->dev);
> +	fc_queue_work(shost, &rport->stgt_delete_work);
>  }
>  
>  /**
>   * fc_scsi_scan_rport - called to perform a scsi scan on a remote port.
>   *
> - * Will unblock the target (in case it went away and has now come back),
> - * then invoke a scan.
> - *
>   * @data:	remote port to be scanned.
>   **/
>  static void
> @@ -2057,7 +2060,6 @@ fc_scsi_scan_rport(void *data)
>  
>  	if ((rport->port_state == FC_PORTSTATE_ONLINE) &&
>  	    (rport->roles & FC_RPORT_ROLE_FCP_TARGET)) {
> -		scsi_target_unblock(&rport->dev);
>  		scsi_scan_target(&rport->dev, rport->channel,
>  			rport->scsi_target_id, SCAN_WILD_CARD, 1);
>  	}
> 
> 
> -
> 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
> 
>