Hi Andy,
On Thu, 2012-07-12 at 17:34 -0700, Andy Grover wrote:
> Code was almost entirely divided based on value of bool param "enable".
>
> Split it into two functions.
>
> Signed-off-by: Andy Grover <agrover@xxxxxxxxxx>
> ---
> drivers/target/target_core_device.c | 166 +++++++++++++++++++--------------
> drivers/target/target_core_internal.h | 6 +-
> drivers/target/target_core_tpg.c | 8 +-
> 3 files changed, 103 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 00753bc..617a59f 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
<SNIP>
> +/* core_disable_device_list_for_node():
> + *
> + *
> + */
> +int core_disable_device_list_for_node(
> + struct se_lun *lun,
> + struct se_lun_acl *lun_acl,
> + u32 mapped_lun,
> + u32 lun_access,
> + struct se_node_acl *nacl,
> + struct se_portal_group *tpg)
> +{
> + struct se_port *port = lun->lun_sep;
> + struct se_dev_entry *deve;
> +
> + spin_lock_irq(&nacl->device_list_lock);
> +
> + deve = nacl->device_list[mapped_lun];
> +
> + /*
> + * If the MappedLUN entry is being disabled, the entry in
> + * port->sep_alua_list must be removed now before clearing the
> + * struct se_dev_entry pointers below as logic in
> + * core_alua_do_transition_tg_pt() depends on these being present.
> + *
> + * deve->se_lun_acl will be NULL for demo-mode created LUNs
> + * that have not been explicitly converted to MappedLUNs ->
> + * struct se_lun_acl, but we remove deve->alua_port_list from
> + * port->sep_alua_list. This also means that active UAs and
> + * NodeACL context specific PR metadata for demo-mode
> + * MappedLUN *deve will be released below..
> + */
> + spin_lock_bh(&port->sep_alua_lock);
> + list_del(&deve->alua_port_list);
> + spin_unlock_bh(&port->sep_alua_lock);
> +
> /*
> * Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE
> * PR operation to complete.
So after kicking this patch around a bit more, a tcm_qla2xxx LUN unlink
OP has generated the following warning:
[ 50.386625] qla2xxx [0000:07:00.0]-00af:0: Performing ISP error recovery - ha=ffff880263774000.
[ 70.572988] qla2xxx [0000:07:00.0]-8038:0: Cable is unplugged...
[ 126.527531] ------------[ cut here ]------------
[ 126.532677] WARNING: at kernel/softirq.c:159 local_bh_enable_ip+0x41/0x8c()
[ 126.540433] Hardware name: S5520HC
[ 126.544248] Modules linked in: tcm_vhost ib_srpt ib_cm ib_sa ib_mad ib_core tcm_qla2xxx tcm_loop tcm_fc libfc iscsi_target_mod target_core_pscsi target_core_file target_core_iblock target_core_mod configfs ipv6 iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi dm_round_robin dm_multipath scsi_dh loop i2c_i801 kvm_intel kvm crc32c_intel i2c_core microcode joydev button iomemory_vsl(O) pcspkr ext3 jbd uhci_hcd lpfc ata_piix libata ehci_hcd qla2xxx mlx4_core scsi_transport_fc scsi_tgt igb [last unloaded: scsi_wait_scan]
[ 126.595567] Pid: 3283, comm: unlink Tainted: G O 3.5.0-rc2+ #33
[ 126.603128] Call Trace:
[ 126.605853] [<ffffffff81026b91>] ? warn_slowpath_common+0x78/0x8c
[ 126.612737] [<ffffffff8102c342>] ? local_bh_enable_ip+0x41/0x8c
[ 126.619433] [<ffffffffa03582a2>] ? core_disable_device_list_for_node+0x70/0xe3 [target_core_mod]
[ 126.629323] [<ffffffffa035849f>] ? core_clear_lun_from_tpg+0x88/0xeb [target_core_mod]
[ 126.638244] [<ffffffffa0362ec1>] ? core_tpg_post_dellun+0x17/0x48 [target_core_mod]
[ 126.646873] [<ffffffffa03575ee>] ? core_dev_del_lun+0x26/0x8c [target_core_mod]
[ 126.655114] [<ffffffff810bcbd1>] ? dput+0x27/0x154
[ 126.660549] [<ffffffffa0359aa0>] ? target_fabric_port_unlink+0x3b/0x41 [target_core_mod]
[ 126.669661] [<ffffffffa034a698>] ? configfs_unlink+0xfc/0x14a [configfs]
[ 126.677224] [<ffffffff810b5979>] ? vfs_unlink+0x58/0xb7
[ 126.683141] [<ffffffff810b6ef3>] ? do_unlinkat+0xbb/0x142
[ 126.689253] [<ffffffff81330c75>] ? page_fault+0x25/0x30
[ 126.695170] [<ffffffff81335df9>] ? system_call_fastpath+0x16/0x1b
[ 126.702053] ---[ end trace 2f8e5b0a9ec797ef ]---
[ 126.756336] qla2xxx [0000:07:00.0]-00af:0: Performing ISP error recovery - ha=ffff880263774000.
[ 146.942414] qla2xxx [0000:07:00.0]-8038:0: Cable is unplugged...
So this warning triggered because device_list disable logic is now
holding nacl->device_list_lock w/ spin_lock_irqsave before obtaining
port->sep_alua_lock with only spin_lock_bh..
The original disable logic obtains *deve ahead of dropping the entry
from deve->alua_port_list and then obtains ->device_list_lock to do the
remaining work. Also, I'm pretty sure this particular warning is being
generated by a demo-mode session in tcm_qla2xxx, and not by explicit
NodeACL MappedLUNs. The Initiator MappedLUNs are already protected by a
seperate configfs symlink reference back se_lun->lun_group, and the
demo-mode se_node_acl (and the associated ->device_list[] is released
during se_portal_group->tpg_group shutdown.
So that said, I'm now committing the following patch to drop the extra
functional change to disable logic post re-factoring. Please have a
look.
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 0a1c907..0048d0a 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -432,11 +432,7 @@ int core_disable_device_list_for_node(
struct se_portal_group *tpg)
{
struct se_port *port = lun->lun_sep;
- struct se_dev_entry *deve;
-
- spin_lock_irq(&nacl->device_list_lock);
-
- deve = nacl->device_list[mapped_lun];
+ struct se_dev_entry *deve = nacl->device_list[mapped_lun];
/*
* If the MappedLUN entry is being disabled, the entry in
@@ -454,14 +450,13 @@ int core_disable_device_list_for_node(
spin_lock_bh(&port->sep_alua_lock);
list_del(&deve->alua_port_list);
spin_unlock_bh(&port->sep_alua_lock);
-
/*
* Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE
* PR operation to complete.
*/
- spin_unlock_irq(&nacl->device_list_lock);
while (atomic_read(&deve->pr_ref_count) != 0)
cpu_relax();
+
spin_lock_irq(&nacl->device_list_lock);
/*
* Disable struct se_dev_entry LUN ACL mapping
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
