Re: [PATCH 01/12] target: Convert se_node_acl->device_list[] to RCU hlist

[Christoph Hellwig]

On Tue, May 12, 2015 at 09:25:25AM +0000, Nicholas A. Bellinger wrote:
> @@ -240,18 +237,12 @@ int core_free_device_list_for_node(
>  {
>  	struct se_dev_entry *deve;
>  	struct se_lun *lun;
> -	u32 i;
> -
> -	if (!nacl->device_list)
> -		return 0;
> -
> -	spin_lock_irq(&nacl->device_list_lock);
> -	for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
> -		deve = nacl->device_list[i];
> +	u32 mapped_lun;
>  
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link) {
>  		if (!(deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS))
>  			continue;
> -
>  		if (!deve->se_lun) {
>  			pr_err("%s device entries device pointer is"
>  				" NULL, but Initiator has access.\n",
> @@ -259,16 +250,14 @@ int core_free_device_list_for_node(
>  			continue;
>  		}
>  		lun = deve->se_lun;
> +		mapped_lun = deve->mapped_lun;
> +		rcu_read_unlock();
>  
> -		spin_unlock_irq(&nacl->device_list_lock);
> -		core_disable_device_list_for_node(lun, NULL, deve->mapped_lun,
> -			TRANSPORT_LUNFLAGS_NO_ACCESS, nacl, tpg);
> -		spin_lock_irq(&nacl->device_list_lock);
> +		core_disable_device_list_for_node(lun, NULL, mapped_lun,
> +					TRANSPORT_LUNFLAGS_NO_ACCESS, nacl, tpg);

I don't think this change is a good idea.  Now that you've just switched
to a list call into core_disable_device_list_for_node with the lock
instead of retaking it and restart the list walk after it instead of
encoding the previous wrong behavior with the local mapped_lun
variable.  Note that this patter is the same for all all but one of the
callers, and even core_dev_del_initiator_node_lun_acl would benefit
from being called locked and with an already looked up dev entry.

Note that if you cherry picked this patch I posted a while ago
to be before the series one of the callers would already be gone:

http://git.infradead.org/users/hch/scsi.git/commitdiff/dfb7096ba5ea47cb5b7fb5b6e2f8d7d6436af24f

> +	spin_lock_irq(&nacl->lun_entry_lock);
> +	deve = target_nacl_find_deve(nacl, mapped_lun);
> +	if (deve) {
> +		if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) {
> +			deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_ONLY;
> +			deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
> +		} else {
> +			deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_WRITE;
> +			deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
> +		}
>  	}
> -	spin_unlock_irq(&nacl->device_list_lock);
> +	spin_unlock_irq(&nacl->lun_entry_lock);
> +
> +	synchronize_rcu();

This only updates scalar fields, the synchronize_rcu() calls isn't
going to buy you anything.

Btw, it would be good to always document what a synchronize_rcu()
call code is for.

> +
> +static void target_nacl_deve_callrcu(struct rcu_head *head)
> +{
> +	struct se_dev_entry *deve = container_of(head, struct se_dev_entry,
> +						 rcu_head);
> +	kfree(deve);
>  }

Just use kfree_rcu instead of open coding it.

> +/*
> + * Called with rcu_read_lock or nacl->device_list_lock held.
> + */

It would be good to assert that.  Paul, is there a good way to assert
we're called under rcu_read_lock?

> +	spin_lock_irq(&nacl->device_list_lock);
> +	orig = target_nacl_find_deve(nacl, mapped_lun);
> +	if (orig && orig->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) {
> +		BUG_ON(orig->se_lun_acl != NULL);
> +		BUG_ON(orig->se_lun != lun);
> +
> +		rcu_assign_pointer(new->se_lun, lun);
> +		rcu_assign_pointer(new->se_lun_acl, lun_acl);
> +		hlist_add_head_rcu(&new->link, &nacl->lun_entry_hlist);
>  		spin_unlock_irq(&nacl->device_list_lock);
> +		spin_lock_bh(&port->sep_alua_lock);
> +		list_del(&orig->alua_port_list);
> +		list_add_tail(&new->alua_port_list, &port->sep_alua_list);
> +		spin_unlock_bh(&port->sep_alua_lock);
>  
> +		return 0;
>  	}

The case where we have an original one is the demo mode -> explicit
change.  So I don't think we actually need the newly allocate dev
entry here.  Just change lun_flags like in core_update_device_list_access
and do an rcu_assign_pointer for the lun ACLs.

> -	deve->creation_time = get_jiffies_64();
> -	deve->attach_count++;
> +	rcu_assign_pointer(new->se_lun, lun);
> +	rcu_assign_pointer(new->se_lun_acl, lun_acl);
> +	hlist_add_head_rcu(&new->link, &nacl->lun_entry_hlist);
>  	spin_unlock_irq(&nacl->device_list_lock);
>  
>  	spin_lock_bh(&port->sep_alua_lock);
> -	list_add_tail(&deve->alua_port_list, &port->sep_alua_list);
> +	list_add_tail(&new->alua_port_list, &port->sep_alua_list);
>  	spin_unlock_bh(&port->sep_alua_lock);
>  
> +	synchronize_rcu();

Please add a comment why we need the synchronize_rcu here again.  Nothing
is delete from any list, and nothing is freed so I don't see any need
to wait for a grace period.

> +	core_scsi3_ua_release_all(orig);
> +	rcu_assign_pointer(orig->se_lun, NULL);
> +	rcu_assign_pointer(orig->se_lun_acl, NULL);

Can you document the life time rules that ensure ->se_lun and ->se_lun_acl
stay around while readers in the RCU grace period may still access them?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/