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

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

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?
-- 
Stefan Richter
-=====-==-== ---= ==--=
http://arcgraph.de/sr/