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

Stefan Richter
2011-01-25T00:08:25Z

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
-=====-==-== ---= ==--=