On Mon, 2011-01-24 at 17:20 -0800, Nicholas A. Bellinger wrote:
> On Tue, 2011-01-25 at 01:08 +0100, Stefan Richter wrote:
> > On Jan 24 Nicholas A. Bellinger wrote:
> > > From: Fubo Chen <fubo.chen@xxxxxxxxx>
> > >
> > > This patch reaquires hba->device_lock and dev->se_port_lock in
> > > se_clear_dev_ports() if lun->lun_se_dev is NULL and we need
> > > to continue in dev->dev_sep_list.
> > >
> > > Signed-off-by: Fubo Chen <fubo.chen@xxxxxxxxx>
> > > Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/target/target_core_device.c | 2 ++
> > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> > > index 95dfe3a..02b835f 100644
> > > --- a/drivers/target/target_core_device.c
> > > +++ b/drivers/target/target_core_device.c
> > > @@ -796,6 +796,8 @@ void se_clear_dev_ports(struct se_device *dev)
> > > spin_lock(&lun->lun_sep_lock);
> > > if (lun->lun_se_dev == NULL) {
> > > spin_unlock(&lun->lun_sep_lock);
> > > + spin_lock(&hba->device_lock);
> > > + spin_lock(&dev->se_port_lock);
> > > continue;
> > > }
> > > spin_unlock(&lun->lun_sep_lock);
> >
> > The patch might be OK. But the code that it fixed is... stunning.
> >
> > void se_clear_dev_ports(struct se_device *dev)
> > {
> > struct se_hba *hba = dev->se_hba;
> > struct se_lun *lun;
> > struct se_portal_group *tpg;
> > struct se_port *sep, *sep_tmp;
> >
> > spin_lock(&dev->se_port_lock);
> > list_for_each_entry_safe(sep, sep_tmp, &dev->dev_sep_list, sep_list) {
> > spin_unlock(&dev->se_port_lock);
> > spin_unlock(&hba->device_lock);
> >
> > lun = sep->sep_lun;
> > tpg = sep->sep_tpg;
> > spin_lock(&lun->lun_sep_lock);
> > if (lun->lun_se_dev == NULL) {
> > spin_unlock(&lun->lun_sep_lock);
> > continue;
> > }
> > spin_unlock(&lun->lun_sep_lock);
> >
> > core_dev_del_lun(tpg, lun->unpacked_lun);
> >
> > spin_lock(&hba->device_lock);
> > spin_lock(&dev->se_port_lock);
> > }
> > spin_unlock(&dev->se_port_lock);
> >
> > return;
> > }
> >
> > Can this mess of releasing and reacquiring locks --- which looks all rather
> > dangerous --- be cleaned up if you move the logical units (?) to be deleted
> > over to a secondary list?
>
> Fair point on this one. Having to sleep in core_dev_del_lun() waiting
> for all outstanding struct se_cmd -> struct se_lun I/O descriptor
> mappings to be shutdown makes this look pretty ugly currently in
> se_clear_dev_ports(). However adding another list+lock to this mix
> really does not make it any less complex.
>
> Looking at se_clear_dev_ports() again in the two usage contexts
> target_core_device.c:se_free_virtual_device() and
> target_core_hba.c:core_delete_hba(), I am thinking now that
> se_clear_dev_ports() is in fact a genuine piece of left-over legacy
> shutdown (eg: IOCTL) cruft from the pre-configfs 'dark ages' where TCM
> backend device + port LUN removal was not guaranteed by Linux/VFS (and
> hence the extra shutdown logic hacks).
>
> Because TPG port / LUN shutdown from backend HBA+devices is *always*
> done via a configfs unlink syscall on parent/child protected struct
> config_group structures, I think se_clear_dev_ports() should be able to
> be dropped all together now. I will take a deeper look and see if this
> is really in fact safe for v4.0/for-38 code.
>
Ok, after a quick test w/o se_clear_dev_ports() @ struct se_device and
struct se_hba configfs context shutdown callers using iSCSI target
TargetName+TargetPortalGrouPTag endpoints with normal and demo mode TPG
LUNs, everything appears to be functioning as expected with the inline
patch below.
I am going to do some more testing and code review before pushing into
the upstream LIO tree, but I am pretty confident at this point with the
last assessment of this code being pre-configfs + pre-parent/child VFS
protected port shutdown cruft that can safely be dropped with modern
target_core_fabric_configfs.c code.
Best Regards,
--nab
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 1afddb5..9220dba 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -777,52 +777,12 @@ void se_release_vpd_for_dev(struct se_device *dev)
return;
}
-/*
- * Called with struct se_hba->device_lock held.
- */
-void se_clear_dev_ports(struct se_device *dev)
-{
- struct se_hba *hba = dev->se_hba;
- struct se_lun *lun;
- struct se_portal_group *tpg;
- struct se_port *sep, *sep_tmp;
-
- spin_lock(&dev->se_port_lock);
- list_for_each_entry_safe(sep, sep_tmp, &dev->dev_sep_list, sep_list) {
- spin_unlock(&dev->se_port_lock);
- spin_unlock(&hba->device_lock);
-
- lun = sep->sep_lun;
- tpg = sep->sep_tpg;
- spin_lock(&lun->lun_sep_lock);
- if (lun->lun_se_dev == NULL) {
- spin_unlock(&lun->lun_sep_lock);
- spin_lock(&hba->device_lock);
- spin_lock(&dev->se_port_lock);
- continue;
- }
- spin_unlock(&lun->lun_sep_lock);
-
- core_dev_del_lun(tpg, lun->unpacked_lun);
-
- spin_lock(&hba->device_lock);
- spin_lock(&dev->se_port_lock);
- }
- spin_unlock(&dev->se_port_lock);
-
- return;
-}
-
/* se_free_virtual_device():
*
* Used for IBLOCK, RAMDISK, and FILEIO Transport Drivers.
*/
int se_free_virtual_device(struct se_device *dev, struct se_hba *hba)
{
- spin_lock(&hba->device_lock);
- se_clear_dev_ports(dev);
- spin_unlock(&hba->device_lock);
-
core_alua_free_lu_gp_mem(dev);
se_release_device_for_hba(dev);
diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
index a99760a..1232da9 100644
--- a/drivers/target/target_core_hba.c
+++ b/drivers/target/target_core_hba.c
@@ -157,8 +157,6 @@ core_delete_hba(struct se_hba *hba)
spin_lock(&hba->device_lock);
list_for_each_entry_safe(dev, dev_tmp, &hba->hba_dev_list, dev_list) {
-
- se_clear_dev_ports(dev);
spin_unlock(&hba->device_lock);
se_release_device_for_hba(dev);
--
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
