On Wed, 2016-01-13 at 09:15 +0100, Christoph Hellwig wrote:
> On Wed, Jan 13, 2016 at 12:00:28AM -0800, Nicholas A. Bellinger wrote:
> > diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> > index 52ae8ba..62f02ea 100644
> > --- a/drivers/target/target_core_tpg.c
> > +++ b/drivers/target/target_core_tpg.c
> > @@ -75,16 +75,9 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
> > unsigned char *initiatorname)
> > {
> > struct se_node_acl *acl;
> > - /*
> > - * Obtain the acl_kref now, which will be dropped upon the
> > - * release of se_sess memory within transport_free_session().
> > - */
> > +
> > mutex_lock(&tpg->acl_node_mutex);
> > acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
> > - if (acl) {
> > - if (!kref_get_unless_zero(&acl->acl_kref))
> > - acl = NULL;
> > - }
>
> nah, please keep the kref_get_unless_zero here, it's needed. It's
> just the misleding comment I was complaining about :)
Ok. How about the following comment instead..?
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 62f02ea..1984bb2 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -75,9 +75,21 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
unsigned char *initiatorname)
{
struct se_node_acl *acl;
-
+ /*
+ * Obtain se_node_acl->acl_kref using fabric driver provided
+ * initiatorname[] during node acl endpoint lookup driven by
+ * new se_session login.
+ *
+ * The reference is held until se_session shutdown -> release
+ * occurs via fabric driver invoked transport_deregister_session()
+ * or transport_free_session() code.
+ */
>
> > @@ -3435,10 +3436,14 @@ iscsit_build_sendtargets_response(struct iscsi_cmd *cmd,
> >
> > if ((tpg->tpg_attrib.generate_node_acls == 0) &&
> > (tpg->tpg_attrib.demo_mode_discovery == 0) &&
> > - (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
> > - cmd->conn->sess->sess_ops->InitiatorName))) {
> > + (!(se_acl = core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
> > + cmd->conn->sess->sess_ops->InitiatorName)))) {
> > continue;
> > }
> > + if (se_acl) {
> > + target_put_nacl(se_acl);
> > + se_acl = NULL;
> > + }
>
>
> Seems like with the current code we don't need to keep the ACL,
> so a new helper like:
>
> bool target_tpg_has_node_acl(struct se_portal_group *tpg,
> const char *initiatorname)
> {
> struct se_node_acl *acl;
>
> acl = core_tpg_get_initiator_node_acl(tpg, initiatorname);
> if (acl) {
> target_put_nacl(se_acl);
> return true;
> }
>
> return false;
> }
>
> or fully open coded as:
>
> bool target_tpg_has_node_acl(struct se_portal_group *tpg,
> const char *initiatorname)
> {
> struct se_node_acl *acl;
> bool found = false;
>
> mutex_lock(&tpg->acl_node_mutex);
> list_for_each_entry(acl, &tpg->acl_node_list, acl_list) {
> if (!strcmp(acl->initiatorname, initiatorname)) {
> found = true;
> break;
> }
> }
> mutex_unlock(&tpg->acl_node_mutex);
>
> return found;
> }
>
> might be better to keep the code readable.
Seems reasonable. Adding this instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
