Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

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@gmail.com>
> > > 
> > > 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@gmail.com>
> > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > > ---
> > >  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);