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-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
