On Mon, 2012-02-27 at 09:25 -0500, Jörn Engel wrote:
> On Mon, 27 February 2012 12:12:51 +0000, Nicholas A. Bellinger wrote:
> > @@ -466,18 +468,17 @@ int core_tpg_del_initiator_node_acl(
> > &tpg->tpg_sess_list, sess_list) {
> > if (sess->se_node_acl != acl)
> > continue;
> > - /*
> > - * Determine if the session needs to be closed by our context.
> > - */
> > - if (!tpg->se_tpg_tfo->shutdown_session(sess))
> > - continue;
> >
> > + kref_get(&sess->sess_kref);
> > spin_unlock_irqrestore(&tpg->session_lock, flags);
> > - /*
> > - * If the $FABRIC_MOD session for the Initiator Node ACL exists,
> > - * forcefully shutdown the $FABRIC_MOD session/nexus.
> > - */
> > - tpg->se_tpg_tfo->close_session(sess);
> > +
> > + rc = tpg->se_tpg_tfo->shutdown_session(sess);
> > + target_put_session(sess);
> > + if (!rc)
> > + continue;
> > + target_put_session(sess);
>
> This looks wrong. If we are guaranteed to still hold a refcount,
> there is no point in the kref_get(), target_put_session() pair.
AFAICT that is not guaranteed here..
> If not, how do we protect against the refcount being 0 at kref_get()
> time?
>
Mmmmm, good catch. I think that ->shutdown_session() probably needs to
make that decision (on a per fabric basis, possibly under hardware_lock)
to determine when to obtain extra ref here from session shutdown invoked
from explicit NodeACL group removal.
I need to think about this some more, and will respin a patch soon.
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
