Please ignore this patch instead review ver-2 of this
patch
http://www.open-fcoe.org/pipermail/devel/2008-September/000700.html.
In ver-2, I change exch lock location in fc_exch_alloc() inside
mp lock since fc_exch_reset() can occur as soon as mp lock released in
this
function and also added check for lport reset state to prevent adding
new
ep in this state.
--Vasu
>-----Original Message-----
>From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]
On
>Behalf Of Vasu Dev
>Sent: Friday, September 05, 2008 11:29 AM
>To: [email protected]
>Subject: [Open-FCoE] [PATCH 2/2] libfc: fixed some more
>possiblefc_exch_reset() races
>
>Added exch lock for callers of exch_get()/fc_exch_alloc() to
>prevent fc_exch_reset() from freeing exch while these callers
>are is still working on just allocated exch.
>
>This change impacted fc_exch_resp() call flow also
>since fc_exch_resp() allocats new exchange, this patch
>made fc_exch_resp() similar to fc_exch_find()
>in terms of exch ref counting by having additional exch
>ref to caller of fc_exch_resp() before dropping exch lock
>from exchange allocation. This would allow fc_exch_reset()
>running in fc_exch_resp() callers without exch being freed
>by fc_exch_reset() while caller of fc_exch_resp() are still
>working on exch.
>
>This patch also associated exch hold from fc_exch_alloc()
>to exch pointer in mp and then called fc_exch_release()
>after removing exch pointer in mp to drop this hold
>in fc_exch_mgr_delete_ep(). This change eliminated need
>for extra exch hold in some error cases such as error
>handling case in fc_exch_seq_send() and also removed
>exch ref decrement without test in fc_exch_done_locked().
>
>Signed-off-by: Vasu Dev <[EMAIL PROTECTED]>
>---
>
> drivers/scsi/libfc/fc_exch.c | 28 +++++++++++++++++++---------
> 1 files changed, 19 insertions(+), 9 deletions(-)
>
>
>diff --git a/drivers/scsi/libfc/fc_exch.c
b/drivers/scsi/libfc/fc_exch.c
>index f307008..3a630c5 100644
>--- a/drivers/scsi/libfc/fc_exch.c
>+++ b/drivers/scsi/libfc/fc_exch.c
>@@ -322,7 +322,6 @@ static int fc_exch_done_locked(struct fc_exch *ep)
> ep->state |= FC_EX_DONE;
> if (del_timer(&ep->ex_timer))
> atomic_dec(&ep->ex_refcnt); /* drop hold for
timer */
>- atomic_dec(&ep->ex_refcnt); /* drop hold from alloc */
> rc = 0;
> }
> return rc;
>@@ -339,6 +338,7 @@ static void fc_exch_mgr_delete_ep(struct fc_exch
*ep)
> mp->exches[ep->xid - mp->min_xid] = NULL;
> list_del(&ep->ex_list);
> spin_unlock_bh(&mp->em_lock);
>+ fc_exch_release(ep); /* drop hold for exch in mp */
> }
>
> /*
>@@ -553,6 +553,7 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr
*mp,
>u16 xid)
> fc_seq_alloc(ep, ep->seq_id++);
> mp->total_exches++;
> spin_unlock_bh(&mp->em_lock);
>+ fc_exch_hold(ep); /* hold for exch in mp */
>
> /*
> * update exchange
>@@ -567,7 +568,12 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr
*mp,
>u16 xid)
> spin_lock_init(&ep->ex_lock);
> setup_timer(&ep->ex_timer, fc_exch_timeout, (unsigned long)ep);
>
>- fc_exch_hold(ep); /* hold for caller */
>+ /*
>+ * Hold exch lock for caller to prevent
>+ * fc_exch_reset() from releasing exch
>+ * while caller is still working on exch.
>+ */
>+ spin_lock_bh(&ep->ex_lock);
> out:
> return ep;
> }
>@@ -652,6 +658,8 @@ static struct fc_exch *fc_exch_resp(struct
fc_exch_mgr
>*mp, struct fc_frame *fp)
> WARN_ON(rxid != FC_XID_UNKNOWN);
> fh->fh_rx_id = htons(ep->rxid);
> }
>+ fc_exch_hold(ep); /* hold for caller */
>+ spin_unlock_bh(&ep->ex_lock); /* lock from exch_get */
> }
> return ep;
> }
>@@ -665,7 +673,7 @@ static enum fc_pf_rjt_reason
> fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame *fp)
> {
> struct fc_frame_header *fh = fc_frame_header_get(fp);
>- struct fc_exch *ep = NULL, *new_ep = NULL;
>+ struct fc_exch *ep = NULL;
> struct fc_seq *sp = NULL;
> enum fc_pf_rjt_reason reject = FC_RJT_NONE;
> u32 f_ctl;
>@@ -715,7 +723,7 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct
>fc_frame *fp)
> reject = FC_RJT_RX_ID;
> goto rel;
> }
>- new_ep = ep = fc_exch_resp(mp, fp);
>+ ep = fc_exch_resp(mp, fp);
> if (!ep) {
> reject = FC_RJT_EXCH_EST; /* XXX
*/
> goto out;
>@@ -758,9 +766,8 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct
>fc_frame *fp)
> out:
> return reject;
> rel:
>- fc_exch_release(ep);
>- if (new_ep)
>- fc_exch_release(new_ep);
>+ fc_exch_done(&ep->seq);
>+ fc_exch_release(ep); /* hold from fc_exch_find/fc_exch_resp
*/
> return reject;
> }
>
>@@ -1850,6 +1857,7 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport
*lp,
> struct fc_seq *sp = NULL;
> struct fc_frame_header *fh;
> u16 fill;
>+ int rc = 1;
>
> ep = lp->tt.exch_get(lp, fp);
> if (!ep) {
>@@ -1895,7 +1903,6 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport
*lp,
> if (unlikely(lp->tt.frame_send(lp, fp)))
> goto err;
>
>- spin_lock_bh(&ep->ex_lock);
> if (timer_msec)
> fc_exch_timer_set_locked(ep, timer_msec);
> sp->f_ctl = f_ctl; /* save for possible abort */
>@@ -1907,7 +1914,10 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport
*lp,
> spin_unlock_bh(&ep->ex_lock);
> return sp;
> err:
>- fc_exch_done(sp);
>+ rc = fc_exch_done_locked(ep);
>+ spin_unlock_bh(&ep->ex_lock);
>+ if (!rc)
>+ fc_exch_mgr_delete_ep(ep);
> return NULL;
> }
> EXPORT_SYMBOL(fc_exch_seq_send);
>
>_______________________________________________
>devel mailing list
>[email protected]
>http://www.open-fcoe.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
