On Mon, 2012-02-27 at 15:45 -0500, Jörn Engel wrote:
> On Mon, 27 February 2012 12:12:54 +0000, Nicholas A. Bellinger wrote:
> >
> > @@ -885,11 +851,11 @@ void qla_tgt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport)
> > mutex_lock(&ha->tgt_mutex);
> > sess = qla_tgt_create_sess(vha, fcport, false);
> > mutex_unlock(&ha->tgt_mutex);
> > -
> > - spin_lock_irqsave(&ha->hardware_lock, flags);
> > /* put the extra creation ref */
> > if (sess != NULL)
> > - __qla_tgt_sess_put(sess);
> > + if (ha->tgt_ops->put_sess(sess) != 0)
> > + return;
> > + spin_lock_irqsave(&ha->hardware_lock, flags);
>
> Can we get rid of the " != 0"? It does not add anything logically an
> to me personally it actually makes the code harder to read.
>
> Also, there is a subtle bug here. We used to have the second refcount
> to protect ourselves against freeing the session before we re-took
> ha->hardware_lock. Now we drop the second refcount, which, about half
> an hour of double-checking the code, is perfectly fine.
Thanks for having a look..
> In fact, there is no point in having the extra refcount at all anymore.
AFAICT the extra refcount is still required for qla_tgt_sess creation
case in order to prevent an external kref_put(se_sess->sess_kref) from
configfs context dropping sess_kref to zero + releasing before
->hardware_lock is done with qla_tgt_sess reference.
So your right that it might be OK for qla_tgt_fc_port_added() to not
need the extra refcount here because we don't expect qla_tgt_sess to
immediately fail once qla_tgt_create_sess() / qla_tgt_make_local_sess()
completes successfully.
But I' pretty sure it's possible for a session to immediately fail after
qla_tgt_sess is created from tgt_ops->find_sess_by_s_id() lookup
failure, which means the extra reference count is still required for
those cases.
I'm happy to be proved wrong here with a patch of course.. ;)
> Except that you snuck in the tiny semantic change of returning from
> this function in case the put_sess happens to drop the last reference.
> Now, if it makes sense to do so, we are racy here. But then again, if
> it makes sense to do so, it would have made sense with or without the
> conversion and your semantic change does not belong here in the first
> place.
>
Good catch. How about the following for qla_tgt_fc_port_added()..?
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index a08d4a8..5b44cc2 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -851,12 +851,11 @@ void qla_tgt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport)
mutex_lock(&ha->tgt_mutex);
sess = qla_tgt_create_sess(vha, fcport, false);
mutex_unlock(&ha->tgt_mutex);
- /* put the extra creation ref */
- if (sess != NULL)
- if (ha->tgt_ops->put_sess(sess) != 0)
- return;
+
spin_lock_irqsave(&ha->hardware_lock, flags);
} else {
+ kref_get(&sess->se_sess->sess_kref);
+
if (sess->deleted) {
qla_tgt_undelete_sess(sess);
@@ -888,6 +887,8 @@ void qla_tgt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport)
sess->local = 0;
}
spin_unlock_irqrestore(&ha->hardware_lock, flags);
+
+ ha->tgt_ops->put_sess(sess);
}
void qla_tgt_fc_port_deleted(struct scsi_qla_host *vha, fc_port_t *fcport)
--
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
