Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Christoph Hellwig <hch@lst.de>]

>  	mutex_lock(&tpg->acl_node_mutex);
>  	acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
> +	/*
> +	 * Obtain the acl_kref now, which will be dropped upon the
> +	 * release of se_sess memory within transport_free_session().
> +	 */
> +	if (acl)
> +		kref_get(&acl->acl_kref);

І think the comment is highly confusing as it's about one of the
callers, while the function has many.

I'd suggest you move it to core_tpg_check_initiator_node_acl instead.

Also I think iscsit_build_sendtargets_response will need a put on
the nacl, otherwise you'll leak references.

While we're at it - is there any god reason to keep acl_pr_ref_count
as a separate entity from acl_kref?

>  void transport_free_session(struct se_session *se_sess)
>  {
> +	struct se_node_acl *se_nacl = se_sess->se_node_acl;
> +	/*
> +	 * Drop the se_node_acl->nacl_kref obtained from within
> +	 * core_tpg_get_initiator_node_acl().
> +	 */
> +	if (se_nacl) {
> +		se_sess->se_node_acl = NULL;

Whats the point of zeroing se_node_acl just before freeing se_sess?

>  	/*
>  	 * If last kref is dropping now for an explicit NodeACL, awake sleeping
>  	 * ->acl_free_comp caller to wakeup configfs se_node_acl->acl_group
> -	 * removal context.
> +	 * removal context from within transport_free_session() code.
>  	 */

The comment neds to move to transport_free_session.  Or maybe just removed
given that it's obvious from the code flow.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html