| Message ID | 1442513809-11467-1-git-send-email-cleech@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 9/17/15, 1:16 PM, Chris Leech wrote: > Currently the iSCSI transport class synchronises target scanning and > unbinding with a host level mutex. For multi-session hosts (offloading > iSCSI HBAs) connecting to storage arrays that may implement one > target-per-lun, this can result in the target scan work for hundreds of > sessions being serialized behind a single mutex. With slow enough > response times, this can cause scan requests initiated from userspace to > block on the mutex long enough to trigger 120 sec hung task warnings. > > I can't see any reason not to move this to a session level mutex and let > the target scans run in parallel, speeding up connecting to a large > number of targets. Note that as iscsi_tcp creates a virtual host for > each session, software iSCSI is effectively doing this already. > > Signed-off-by: Chris Leech <cleech@redhat.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 19 ++++++------------- > include/scsi/scsi_transport_iscsi.h | 2 +- > 2 files changed, 7 insertions(+), 14 deletions(-) > > diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c > index e4b3d8f..9daeb7f 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -1569,7 +1569,6 @@ static int iscsi_setup_host(struct transport_container *tc, struct device *dev, > > memset(ihost, 0, sizeof(*ihost)); > atomic_set(&ihost->nr_scans, 0); > - mutex_init(&ihost->mutex); > > iscsi_bsg_host_add(shost, ihost); > /* ignore any bsg add error - we just can't do sgio */ > @@ -1789,8 +1788,6 @@ static int iscsi_user_scan_session(struct device *dev, void *data) > { > struct iscsi_scan_data *scan_data = data; > struct iscsi_cls_session *session; > - struct Scsi_Host *shost; > - struct iscsi_cls_host *ihost; > unsigned long flags; > unsigned int id; > > @@ -1801,10 +1798,7 @@ static int iscsi_user_scan_session(struct device *dev, void *data) > > ISCSI_DBG_TRANS_SESSION(session, "Scanning session\n"); > > - shost = iscsi_session_to_shost(session); > - ihost = shost->shost_data; > - > - mutex_lock(&ihost->mutex); > + mutex_lock(&session->mutex); > spin_lock_irqsave(&session->lock, flags); > if (session->state != ISCSI_SESSION_LOGGED_IN) { > spin_unlock_irqrestore(&session->lock, flags); > @@ -1823,7 +1817,7 @@ static int iscsi_user_scan_session(struct device *dev, void *data) > } > > user_scan_exit: > - mutex_unlock(&ihost->mutex); > + mutex_unlock(&session->mutex); > ISCSI_DBG_TRANS_SESSION(session, "Completed session scan\n"); > return 0; > } > @@ -1999,26 +1993,24 @@ static void __iscsi_unbind_session(struct work_struct *work) > struct iscsi_cls_session *session = > container_of(work, struct iscsi_cls_session, > unbind_work); > - struct Scsi_Host *shost = iscsi_session_to_shost(session); > - struct iscsi_cls_host *ihost = shost->shost_data; > unsigned long flags; > unsigned int target_id; > > ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n"); > > /* Prevent new scans and make sure scanning is not in progress */ > - mutex_lock(&ihost->mutex); > + mutex_lock(&session->mutex); > spin_lock_irqsave(&session->lock, flags); > if (session->target_id == ISCSI_MAX_TARGET) { > spin_unlock_irqrestore(&session->lock, flags); > - mutex_unlock(&ihost->mutex); > + mutex_unlock(&session->mutex); > return; > } > > target_id = session->target_id; > session->target_id = ISCSI_MAX_TARGET; > spin_unlock_irqrestore(&session->lock, flags); > - mutex_unlock(&ihost->mutex); > + mutex_unlock(&session->mutex); > > if (session->ida_used) > ida_simple_remove(&iscsi_sess_ida, target_id); > @@ -2051,6 +2043,7 @@ iscsi_alloc_session(struct Scsi_Host *shost, struct iscsi_transport *transport, > INIT_WORK(&session->unbind_work, __iscsi_unbind_session); > INIT_WORK(&session->scan_work, iscsi_scan_session); > spin_lock_init(&session->lock); > + mutex_init(&session->mutex); > > /* this is released in the dev's release function */ > scsi_host_get(shost); > diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h > index 6183d20..acf9d9d 100644 > --- a/include/scsi/scsi_transport_iscsi.h > +++ b/include/scsi/scsi_transport_iscsi.h > @@ -238,6 +238,7 @@ struct iscsi_cls_session { > struct work_struct unblock_work; > struct work_struct scan_work; > struct work_struct unbind_work; > + struct mutex mutex; > > /* recovery fields */ > int recovery_tmo; > @@ -272,7 +273,6 @@ struct iscsi_cls_session { > > struct iscsi_cls_host { > atomic_t nr_scans; > - struct mutex mutex; > struct request_queue *bsg_q; > uint32_t port_speed; > uint32_t port_state; Reviewed-by: Mike Christie <michaelc@cs.wisc.edu> -- 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
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c index e4b3d8f..9daeb7f 100644 --- a/drivers/scsi/scsi_transport_iscsi.c +++ b/drivers/scsi/scsi_transport_iscsi.c @@ -1569,7 +1569,6 @@ static int iscsi_setup_host(struct transport_container *tc, struct device *dev, memset(ihost, 0, sizeof(*ihost)); atomic_set(&ihost->nr_scans, 0); - mutex_init(&ihost->mutex); iscsi_bsg_host_add(shost, ihost); /* ignore any bsg add error - we just can't do sgio */ @@ -1789,8 +1788,6 @@ static int iscsi_user_scan_session(struct device *dev, void *data) { struct iscsi_scan_data *scan_data = data; struct iscsi_cls_session *session; - struct Scsi_Host *shost; - struct iscsi_cls_host *ihost; unsigned long flags; unsigned int id; @@ -1801,10 +1798,7 @@ static int iscsi_user_scan_session(struct device *dev, void *data) ISCSI_DBG_TRANS_SESSION(session, "Scanning session\n"); - shost = iscsi_session_to_shost(session); - ihost = shost->shost_data; - - mutex_lock(&ihost->mutex); + mutex_lock(&session->mutex); spin_lock_irqsave(&session->lock, flags); if (session->state != ISCSI_SESSION_LOGGED_IN) { spin_unlock_irqrestore(&session->lock, flags); @@ -1823,7 +1817,7 @@ static int iscsi_user_scan_session(struct device *dev, void *data) } user_scan_exit: - mutex_unlock(&ihost->mutex); + mutex_unlock(&session->mutex); ISCSI_DBG_TRANS_SESSION(session, "Completed session scan\n"); return 0; } @@ -1999,26 +1993,24 @@ static void __iscsi_unbind_session(struct work_struct *work) struct iscsi_cls_session *session = container_of(work, struct iscsi_cls_session, unbind_work); - struct Scsi_Host *shost = iscsi_session_to_shost(session); - struct iscsi_cls_host *ihost = shost->shost_data; unsigned long flags; unsigned int target_id; ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n"); /* Prevent new scans and make sure scanning is not in progress */ - mutex_lock(&ihost->mutex); + mutex_lock(&session->mutex); spin_lock_irqsave(&session->lock, flags); if (session->target_id == ISCSI_MAX_TARGET) { spin_unlock_irqrestore(&session->lock, flags); - mutex_unlock(&ihost->mutex); + mutex_unlock(&session->mutex); return; } target_id = session->target_id; session->target_id = ISCSI_MAX_TARGET; spin_unlock_irqrestore(&session->lock, flags); - mutex_unlock(&ihost->mutex); + mutex_unlock(&session->mutex); if (session->ida_used) ida_simple_remove(&iscsi_sess_ida, target_id); @@ -2051,6 +2043,7 @@ iscsi_alloc_session(struct Scsi_Host *shost, struct iscsi_transport *transport, INIT_WORK(&session->unbind_work, __iscsi_unbind_session); INIT_WORK(&session->scan_work, iscsi_scan_session); spin_lock_init(&session->lock); + mutex_init(&session->mutex); /* this is released in the dev's release function */ scsi_host_get(shost); diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h index 6183d20..acf9d9d 100644 --- a/include/scsi/scsi_transport_iscsi.h +++ b/include/scsi/scsi_transport_iscsi.h @@ -238,6 +238,7 @@ struct iscsi_cls_session { struct work_struct unblock_work; struct work_struct scan_work; struct work_struct unbind_work; + struct mutex mutex; /* recovery fields */ int recovery_tmo; @@ -272,7 +273,6 @@ struct iscsi_cls_session { struct iscsi_cls_host { atomic_t nr_scans; - struct mutex mutex; struct request_queue *bsg_q; uint32_t port_speed; uint32_t port_state;
Currently the iSCSI transport class synchronises target scanning and unbinding with a host level mutex. For multi-session hosts (offloading iSCSI HBAs) connecting to storage arrays that may implement one target-per-lun, this can result in the target scan work for hundreds of sessions being serialized behind a single mutex. With slow enough response times, this can cause scan requests initiated from userspace to block on the mutex long enough to trigger 120 sec hung task warnings. I can't see any reason not to move this to a session level mutex and let the target scans run in parallel, speeding up connecting to a large number of targets. Note that as iscsi_tcp creates a virtual host for each session, software iSCSI is effectively doing this already. Signed-off-by: Chris Leech <cleech@redhat.com> --- drivers/scsi/scsi_transport_iscsi.c | 19 ++++++------------- include/scsi/scsi_transport_iscsi.h | 2 +- 2 files changed, 7 insertions(+), 14 deletions(-)