On Wed, 2015-05-13 at 07:46 +0200, Christoph Hellwig wrote:
> 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.
>
Ugh, yes. Fixing up clear_lun_from_tpg + free_device_list_for_node to
use a common caller acquiring se_node_acl->lun_entry_mutex during
se_dev_entry release.
Fixing up target_fabric_mappedlun_unlink() as well.
> 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.
<nod>, dropping synchronize_rcu() here
>
> > +
> > +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.
>
Done
> > +/*
> > + * 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.
Will take a look at this.
>
> > - 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.
>
I don't think it's required either. Dropping.
> > + 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?
Will do.
Thanks HCH.
--nab
--
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
