From mboxrd@z Thu Jan 1 00:00:00 1970 From: The Lee-Man Subject: Re: iscsi: make mutex for target scanning and unbinding per-session Date: Thu, 10 Nov 2016 10:00:54 -0800 (PST) Message-ID: References: <1478542920-24460-1-git-send-email-cleech@redhat.com> Reply-To: open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_Part_856_1249682685.1478800854717" Return-path: Sender: open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org In-Reply-To: <1478542920-24460-1-git-send-email-cleech-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: open-iscsi Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, lduncan-IBi9RG/b67k@public.gmane.org List-Id: linux-scsi@vger.kernel.org ------=_Part_856_1249682685.1478800854717 Content-Type: multipart/alternative; boundary="----=_Part_857_995132725.1478800854718" ------=_Part_857_995132725.1478800854718 Content-Type: text/plain; charset=UTF-8 On Monday, November 7, 2016 at 11:22:23 AM UTC-7, 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. > I understood the reason for this mutex was to protect against the case where there are multiple paths to a target. In such cases, you can get simultaneous access to sysfs attributes (files), which can cause errors, i.e. two threads trying to write an attribute at the same time, or one changing an attribute while another reads or removes it. I worry that changing it will not address those issues. [Side note: we *really* need a test suite that somehow includes cases like this.] > > Signed-off-by: Chris Leech > --- > 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 42bca61..83c90fa 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -1568,7 +1568,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; > } > @@ -2001,26 +1995,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); > @@ -2053,6 +2045,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; > -- > 2.7.4 > > -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout. ------=_Part_857_995132725.1478800854718 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Monday, November 7, 2016 at 11:22:23 AM UTC-7, Chris Le= ech wrote:
Currently the iSCSI = transport class synchronises target scanning and
unbinding with a host level mutex. =C2=A0For multi-session hosts (offlo= ading
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. =C2=A0With slow enough
response times, this can cause scan requests initiated from userspace t= o
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 an= d let
the target scans run in parallel, speeding up connecting to a large
number of targets. =C2=A0Note that as iscsi_tcp creates a virtual host = for
each session, software iSCSI is effectively doing this already.

I understood the reason for this mutex was to pro= tect against the case
where there are multiple paths to a target. In suc= h cases, you can get
simultaneous access to sysfs attributes (files), wh= ich can cause errors,
i.e. two threads trying to write an attribute at t= he same time, or one
changing an attribute while another reads or remove= s it.

I worry that changing it will not address those issues.
[Side note: we *really* need a test suite that somehow includes
=C2=A0c= ases like this.]

Signed-off-by: Chris Leech <cleech-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
=C2=A0drivers/scsi/scsi_transport_iscsi.c | 19 ++++++-------------
=C2=A0include/scsi/scsi_transport_iscsi.h | =C2=A02 +-
=C2=A02 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/sc= si_transport_iscsi.c
index 42bca61..83c90fa 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1568,7 +1568,6 @@ static int iscsi_setup_host(struct transport_cont= ainer *tc, struct device *dev,
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0memset(ihost, 0, = sizeof(*ihost));
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0atomic_set(&i= host->nr_scans, 0);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_init(&ihost-= >mutex);
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iscsi_bsg_host_ad= d(shost, ihost);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* 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)
=C2=A0{
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct iscsi_scan= _data *scan_data =3D data;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct iscsi_cls_= session *session;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct Scsi_Host *shos= t;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct iscsi_cls_host = *ihost;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long fla= gs;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int id;
=C2=A0
@@ -1801,10 +1798,7 @@ static int iscsi_user_scan_session(struct device= *dev, void *data)
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ISCSI_DBG_TRANS_<= wbr>SESSION(session, "Scanning session\n");
=C2=A0
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0shost =3D iscsi_sessio= n_to_shost(session);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ihost =3D shost->sh= ost_data;
-
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&ihost-= >mutex);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&sessio= n->mutex);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave= (&session->lock, flags);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (session->s= tate !=3D ISCSI_SESSION_LOGGED_IN) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(&session-&= gt;lock, flags);
@@ -1823,7 +1817,7 @@ static int iscsi_user_scan_session(struct device = *dev, void *data)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
=C2=A0
=C2=A0user_scan_exit:
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&ihos= t->mutex);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&session->mutex);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ISCSI_DBG_TRANS_<= wbr>SESSION(session, "Completed session scan\n");
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
=C2=A0}
@@ -2001,26 +1995,24 @@ static void __iscsi_unbind_session(struct work_= struct *work)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct iscsi_cls_= session *session =3D
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0container_of(work, struct iscsi_cls_session,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0 =C2=A0 u= nbind_work);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct Scsi_Host *shos= t =3D iscsi_session_to_shost(session);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct iscsi_cls_host = *ihost =3D shost->shost_data;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long fla= gs;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int targ= et_id;
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ISCSI_DBG_TRANS_<= wbr>SESSION(session, "Unbinding session\n");
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Prevent new sc= ans and make sure scanning is not in progress */
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&ihost-= >mutex);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&sessio= n->mutex);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave= (&session->lock, flags);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (session->t= arget_id =3D=3D ISCSI_MAX_TARGET) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(&session-&= gt;lock, flags);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&ihost->mutex);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&session->mutex);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0target_id =3D ses= sion->target_id;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0session->targe= t_id =3D ISCSI_MAX_TARGET;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_= irqrestore(&session->lock, flags);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&ihos= t->mutex);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&session->mutex);
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (session->i= da_used)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ida_simple_remove(&iscsi_sess_ida= , target_id);
@@ -2053,6 +2045,7 @@ iscsi_alloc_session(struct Scsi_Host *shost, stru= ct iscsi_transport *transport,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0INIT_WORK(&se= ssion->unbind_work, __iscsi_unbind_session);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0INIT_WORK(&se= ssion->scan_work, iscsi_scan_session);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_init(&a= mp;session->lock);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_init(&sessio= n->mutex);
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* this is releas= ed in the dev's release function */
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0scsi_host_get(sho= st);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/sc= si_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 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct work_struc= t unblock_work;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct work_struc= t scan_work;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct work_struc= t unbind_work;
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct mutex mutex;
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* recovery field= s */
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int recovery_tmo;
@@ -272,7 +273,6 @@ struct iscsi_cls_session {
=C2=A0
=C2=A0struct iscsi_cls_host {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0atomic_t nr_scans= ;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct mutex mutex;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct request_qu= eue *bsg_q;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint32_t port_spe= ed;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint32_t port_sta= te;
--=20
2.7.4

--
You received this message because you are subscribed to the Google Groups &= quot;open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an e= mail to open-isc= si+unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org.
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org.
Visit this group at = https://groups.google.com/group/open-iscsi.
For more options, visit http= s://groups.google.com/d/optout.
------=_Part_857_995132725.1478800854718-- ------=_Part_856_1249682685.1478800854717--