* [PATCH 0/3] target: Sparse bugfixes and warnings/annotations
@ 2011-01-24 20:37 Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 1/3] target: Drop nacl->device_list_lock on core_update_device_list_for_node failure Nicholas A. Bellinger
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 20:37 UTC (permalink / raw)
To: linux-scsi, Fubo Chen
Cc: Christoph Hellwig, James Bottomley, Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
Greetings folks,
This series is a breakout of Fubo's recent sparse patches. The first
two patches fix real potential deadlocks for sanity check failures in
target_core_device.c, and the third contains the various non-critical
changes and locking annotations.
Thanks again to Fubo for his contribution!
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
Fubo Chen (3):
target: Drop nacl->device_list_lock on
core_update_device_list_for_node failure
target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports
continue
target: Minor sparse warning fixes and annotations
drivers/target/target_core_device.c | 5 +++++
drivers/target/target_core_fabric_lib.c | 1 +
drivers/target/target_core_mib.c | 20 ++++++++++++++++++++
drivers/target/target_core_pscsi.c | 3 +++
drivers/target/target_core_rd.h | 2 --
drivers/target/target_core_transport.c | 4 +---
include/target/target_core_base.h | 22 +++++++++++-----------
include/target/target_core_transport.h | 4 ++++
8 files changed, 45 insertions(+), 16 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] target: Drop nacl->device_list_lock on core_update_device_list_for_node failure
2011-01-24 20:37 [PATCH 0/3] target: Sparse bugfixes and warnings/annotations Nicholas A. Bellinger
@ 2011-01-24 20:37 ` Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 3/3] target: Minor sparse warning fixes and annotations Nicholas A. Bellinger
2 siblings, 0 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 20:37 UTC (permalink / raw)
To: linux-scsi
Cc: Christoph Hellwig, James Bottomley, Fubo Chen, Nicholas A. Bellinger
From: Fubo Chen <fubo.chen@gmail.com>
The struct se_node_acl->device_list_lock needs to be released if either
sanity check for struct se_dev_entry->se_lun_acl or deve->se_lun fails.
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 4c2bd9c..95dfe3a 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -391,12 +391,14 @@ int core_update_device_list_for_node(
printk(KERN_ERR "struct se_dev_entry->se_lun_acl"
" already set for demo mode -> explict"
" LUN ACL transition\n");
+ spin_unlock_irq(&nacl->device_list_lock);
return -1;
}
if (deve->se_lun != lun) {
printk(KERN_ERR "struct se_dev_entry->se_lun does"
" match passed struct se_lun for demo mode"
" -> explict LUN ACL transition\n");
+ spin_unlock_irq(&nacl->device_list_lock);
return -1;
}
deve->se_lun_acl = lun_acl;
--
1.5.6.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue
2011-01-24 20:37 [PATCH 0/3] target: Sparse bugfixes and warnings/annotations Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 1/3] target: Drop nacl->device_list_lock on core_update_device_list_for_node failure Nicholas A. Bellinger
@ 2011-01-24 20:37 ` Nicholas A. Bellinger
2011-01-25 0:08 ` Stefan Richter
2011-01-24 20:37 ` [PATCH 3/3] target: Minor sparse warning fixes and annotations Nicholas A. Bellinger
2 siblings, 1 reply; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 20:37 UTC (permalink / raw)
To: linux-scsi
Cc: Christoph Hellwig, James Bottomley, Fubo Chen, Nicholas A. Bellinger
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);
--
1.5.6.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 20:37 [PATCH 0/3] target: Sparse bugfixes and warnings/annotations Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 1/3] target: Drop nacl->device_list_lock on core_update_device_list_for_node failure Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue Nicholas A. Bellinger
@ 2011-01-24 20:37 ` Nicholas A. Bellinger
2011-01-24 20:56 ` James Bottomley
2 siblings, 1 reply; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 20:37 UTC (permalink / raw)
To: linux-scsi
Cc: Christoph Hellwig, James Bottomley, Fubo Chen, Nicholas A. Bellinger
From: Fubo Chen <fubo.chen@gmail.com>
This patch addresses the majority of sparse warnings and locking
annotations, and removes a handful of unnecessary struct accessor
macro casts in target_core_base.h.
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 | 1 +
drivers/target/target_core_fabric_lib.c | 1 +
drivers/target/target_core_mib.c | 20 ++++++++++++++++++++
drivers/target/target_core_pscsi.c | 3 +++
drivers/target/target_core_rd.h | 2 --
drivers/target/target_core_transport.c | 4 +---
include/target/target_core_base.h | 22 +++++++++++-----------
include/target/target_core_transport.h | 4 ++++
8 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 02b835f..1afddb5 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -586,6 +586,7 @@ static void core_export_port(
* Called with struct se_device->se_port_lock spinlock held.
*/
static void core_release_port(struct se_device *dev, struct se_port *port)
+ __releases(&dev->se_port_lock) __acquires(&dev->se_port_lock)
{
/*
* Wait for any port reference for PR ALL_TG_PT=1 operation
diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 2628564..bddab5d 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -35,6 +35,7 @@
#include <target/target_core_base.h>
#include <target/target_core_device.h>
#include <target/target_core_transport.h>
+#include <target/target_core_fabric_lib.h>
#include <target/target_core_fabric_ops.h>
#include <target/target_core_configfs.h>
diff --git a/drivers/target/target_core_mib.c b/drivers/target/target_core_mib.c
index d5a48aa..6098d9e 100644
--- a/drivers/target/target_core_mib.c
+++ b/drivers/target/target_core_mib.c
@@ -70,6 +70,7 @@ static inline int list_is_first(const struct list_head *list,
static void *locate_hba_start(
struct seq_file *seq,
loff_t *pos)
+ __acquires(&se_global->g_device_lock)
{
spin_lock(&se_global->g_device_lock);
return seq_list_start(&se_global->g_se_dev_list, *pos);
@@ -84,6 +85,7 @@ static void *locate_hba_next(
}
static void locate_hba_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->g_device_lock)
{
spin_unlock(&se_global->g_device_lock);
}
@@ -98,6 +100,7 @@ static void locate_hba_stop(struct seq_file *seq, void *v)
static void *scsi_inst_seq_start(
struct seq_file *seq,
loff_t *pos)
+ __acquires(&se_global->hba_lock)
{
spin_lock(&se_global->hba_lock);
return seq_list_start(&se_global->g_hba_list, *pos);
@@ -112,6 +115,7 @@ static void *scsi_inst_seq_next(
}
static void scsi_inst_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->hba_lock)
{
spin_unlock(&se_global->hba_lock);
}
@@ -154,6 +158,7 @@ static const struct file_operations scsi_inst_seq_fops = {
* SCSI Device Table
*/
static void *scsi_dev_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_hba_lock)
{
return locate_hba_start(seq, pos);
}
@@ -164,6 +169,7 @@ static void *scsi_dev_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void scsi_dev_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->hba_lock)
{
locate_hba_stop(seq, v);
}
@@ -235,6 +241,7 @@ static const struct file_operations scsi_dev_seq_fops = {
* SCSI Port Table
*/
static void *scsi_port_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_hba_lock)
{
return locate_hba_start(seq, pos);
}
@@ -245,6 +252,7 @@ static void *scsi_port_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void scsi_port_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_hba_lock)
{
locate_hba_stop(seq, v);
}
@@ -305,6 +313,7 @@ static const struct file_operations scsi_port_seq_fops = {
* SCSI Transport Table
*/
static void *scsi_transport_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_hba_lock)
{
return locate_hba_start(seq, pos);
}
@@ -315,6 +324,7 @@ static void *scsi_transport_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void scsi_transport_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_hba_lock)
{
locate_hba_stop(seq, v);
}
@@ -390,6 +400,7 @@ static const struct file_operations scsi_transport_seq_fops = {
* SCSI Target Device Table
*/
static void *scsi_tgt_dev_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_hba_lock)
{
return locate_hba_start(seq, pos);
}
@@ -400,6 +411,7 @@ static void *scsi_tgt_dev_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void scsi_tgt_dev_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_hba_lock)
{
locate_hba_stop(seq, v);
}
@@ -481,6 +493,7 @@ static const struct file_operations scsi_tgt_dev_seq_fops = {
* SCSI Target Port Table
*/
static void *scsi_tgt_port_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_hba_lock)
{
return locate_hba_start(seq, pos);
}
@@ -491,6 +504,7 @@ static void *scsi_tgt_port_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void scsi_tgt_port_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_hba_lock)
{
locate_hba_stop(seq, v);
}
@@ -575,6 +589,7 @@ static const struct file_operations scsi_tgt_port_seq_fops = {
* Iterates through all active TPGs and extracts the info from the ACLs
*/
static void *scsi_auth_intr_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_tpg_lock)
{
spin_lock_bh(&se_global->se_tpg_lock);
return seq_list_start(&se_global->g_se_tpg_list, *pos);
@@ -587,6 +602,7 @@ static void *scsi_auth_intr_seq_next(struct seq_file *seq, void *v,
}
static void scsi_auth_intr_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_tpg_lock)
{
spin_unlock_bh(&se_global->se_tpg_lock);
}
@@ -700,6 +716,7 @@ static const struct file_operations scsi_auth_intr_seq_fops = {
* to list the info fo this table.
*/
static void *scsi_att_intr_port_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_tpg_lock)
{
spin_lock_bh(&se_global->se_tpg_lock);
return seq_list_start(&se_global->g_se_tpg_list, *pos);
@@ -712,6 +729,7 @@ static void *scsi_att_intr_port_seq_next(struct seq_file *seq, void *v,
}
static void scsi_att_intr_port_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_tpg_lock)
{
spin_unlock_bh(&se_global->se_tpg_lock);
}
@@ -824,6 +842,7 @@ static const struct file_operations scsi_att_intr_port_seq_fops = {
* SCSI Logical Unit Table
*/
static void *scsi_lu_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(&se_global->se_hba_lock)
{
return locate_hba_start(seq, pos);
}
@@ -834,6 +853,7 @@ static void *scsi_lu_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void scsi_lu_seq_stop(struct seq_file *seq, void *v)
+ __releases(&se_global->se_hba_lock)
{
locate_hba_stop(seq, v);
}
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index e795f7d..40c2419 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -442,6 +442,7 @@ static struct se_device *pscsi_create_type_disk(
struct pscsi_dev_virt *pdv,
struct se_subsystem_dev *se_dev,
struct se_hba *hba)
+ __releases(sh->host_lock)
{
struct se_device *dev;
struct pscsi_hba_virt *phv = (struct pscsi_hba_virt *)pdv->pdv_se_hba->hba_ptr;
@@ -489,6 +490,7 @@ static struct se_device *pscsi_create_type_rom(
struct pscsi_dev_virt *pdv,
struct se_subsystem_dev *se_dev,
struct se_hba *hba)
+ __releases(sh->host_lock)
{
struct se_device *dev;
struct pscsi_hba_virt *phv = (struct pscsi_hba_virt *)pdv->pdv_se_hba->hba_ptr;
@@ -523,6 +525,7 @@ static struct se_device *pscsi_create_type_other(
struct pscsi_dev_virt *pdv,
struct se_subsystem_dev *se_dev,
struct se_hba *hba)
+ __releases(sh->host_lock)
{
struct se_device *dev;
struct pscsi_hba_virt *phv = (struct pscsi_hba_virt *)pdv->pdv_se_hba->hba_ptr;
diff --git a/drivers/target/target_core_rd.h b/drivers/target/target_core_rd.h
index 13badfb..3ea19e2 100644
--- a/drivers/target/target_core_rd.h
+++ b/drivers/target/target_core_rd.h
@@ -14,8 +14,6 @@
#define RD_BLOCKSIZE 512
#define RD_MAX_SECTORS 1024
-extern struct kmem_cache *se_mem_cache;
-
/* Used in target_core_init_configfs() for virtual LUN 0 access */
int __init rd_module_init(void);
void rd_module_exit(void);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8cb628b..5fa5f89 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -229,8 +229,6 @@ static void transport_remove_cmd_from_queue(struct se_cmd *cmd,
static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
static void transport_stop_all_task_timers(struct se_cmd *cmd);
-int transport_emulate_control_cdb(struct se_task *task);
-
int init_se_global(void)
{
struct se_global *global;
@@ -4371,7 +4369,7 @@ out:
return -1;
}
-extern u32 transport_calc_sg_num(
+u32 transport_calc_sg_num(
struct se_task *task,
struct se_mem *in_se_mem,
u32 task_offset)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index c071907..ba77c11 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -486,8 +486,8 @@ struct se_task {
struct list_head t_state_list;
} ____cacheline_aligned;
-#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
-#define TASK_DEV(task) ((struct se_device *)task->se_dev)
+#define TASK_CMD(task) ((task)->task_se_cmd)
+#define TASK_DEV(task) ((task)->se_dev)
struct se_cmd {
/* SAM response code being sent to initiator */
@@ -543,8 +543,8 @@ struct se_cmd {
void (*transport_complete_callback)(struct se_cmd *);
} ____cacheline_aligned;
-#define T_TASK(cmd) ((struct se_transport_task *)(cmd->t_task))
-#define CMD_TFO(cmd) ((struct target_core_fabric_ops *)cmd->se_tfo)
+#define T_TASK(cmd) ((cmd)->t_task)
+#define CMD_TFO(cmd) ((cmd)->se_tfo)
struct se_tmr_req {
/* Task Management function to be preformed */
@@ -611,8 +611,8 @@ struct se_session {
struct list_head sess_acl_list;
} ____cacheline_aligned;
-#define SE_SESS(cmd) ((struct se_session *)(cmd)->se_sess)
-#define SE_NODE_ACL(sess) ((struct se_node_acl *)(sess)->se_node_acl)
+#define SE_SESS(cmd) ((cmd)->se_sess)
+#define SE_NODE_ACL(sess) ((sess)->se_node_acl)
struct se_device;
struct se_transform_info;
@@ -799,8 +799,8 @@ struct se_device {
struct list_head g_se_dev_list;
} ____cacheline_aligned;
-#define SE_DEV(cmd) ((struct se_device *)(cmd)->se_lun->lun_se_dev)
-#define SU_DEV(dev) ((struct se_subsystem_dev *)(dev)->se_sub_dev)
+#define SE_DEV(cmd) ((cmd)->se_lun->lun_se_dev)
+#define SU_DEV(dev) ((dev)->se_sub_dev)
#define DEV_ATTRIB(dev) (&(dev)->se_sub_dev->se_dev_attrib)
#define DEV_T10_WWN(dev) (&(dev)->se_sub_dev->t10_wwn)
@@ -829,7 +829,7 @@ struct se_hba {
struct se_subsystem_api *transport;
} ____cacheline_aligned;
-#define SE_HBA(d) ((struct se_hba *)(d)->se_hba)
+#define SE_HBA(dev) ((dev)->se_hba)
struct se_lun {
/* See transport_lun_status_table */
@@ -849,7 +849,7 @@ struct se_lun {
struct se_port *lun_sep;
} ____cacheline_aligned;
-#define SE_LUN(c) ((struct se_lun *)(c)->se_lun)
+#define SE_LUN(cmd) ((cmd)->se_lun)
struct se_port {
/* RELATIVE TARGET PORT IDENTIFER */
@@ -909,7 +909,7 @@ struct se_portal_group {
struct config_group tpg_param_group;
} ____cacheline_aligned;
-#define TPG_TFO(se_tpg) ((struct target_core_fabric_ops *)(se_tpg)->se_tpg_tfo)
+#define TPG_TFO(se_tpg) ((se_tpg)->se_tpg_tfo)
struct se_wwn {
struct target_fabric_configfs *wwn_tf;
diff --git a/include/target/target_core_transport.h b/include/target/target_core_transport.h
index 66f44e5..feaf5d4 100644
--- a/include/target/target_core_transport.h
+++ b/include/target/target_core_transport.h
@@ -109,6 +109,8 @@
struct se_mem;
struct se_subsystem_api;
+extern struct kmem_cache *se_mem_cache;
+
extern int init_se_global(void);
extern void release_se_global(void);
extern void transport_init_queue_obj(struct se_queue_obj *);
@@ -186,6 +188,8 @@ extern void transport_generic_process_write(struct se_cmd *);
extern int transport_generic_do_tmr(struct se_cmd *);
/* From target_core_alua.c */
extern int core_alua_check_nonop_delay(struct se_cmd *);
+/* From target_core_cdb.c */
+extern int transport_emulate_control_cdb(struct se_task *);
/*
* Each se_transport_task_t can have N number of possible struct se_task's
--
1.5.6.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 20:37 ` [PATCH 3/3] target: Minor sparse warning fixes and annotations Nicholas A. Bellinger
@ 2011-01-24 20:56 ` James Bottomley
2011-01-24 21:33 ` Nicholas A. Bellinger
0 siblings, 1 reply; 16+ messages in thread
From: James Bottomley @ 2011-01-24 20:56 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: linux-scsi, Fubo Chen, Christoph Hellwig
On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> +#define TASK_CMD(task) ((task)->task_se_cmd)
> +#define TASK_DEV(task) ((task)->se_dev)
If sparse is objecting to things like this then sparse needs fixing:
It's decreasing typesafety. the things being cast are void * ... they'd
be depositable into any pointer whatsoever without the cast. With the
cast in the #define, we pick up pointer mismatches (as we should).
Without it, we don't. As long as the define is always a specific type,
it *should* cast to it.
James
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 20:56 ` James Bottomley
@ 2011-01-24 21:33 ` Nicholas A. Bellinger
2011-01-24 21:51 ` James Bottomley
2011-01-24 23:18 ` Joe Eykholt
0 siblings, 2 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 21:33 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, Fubo Chen, Christoph Hellwig
On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > +#define TASK_DEV(task) ((task)->se_dev)
>
> If sparse is objecting to things like this then sparse needs fixing:
> It's decreasing typesafety. the things being cast are void * ... they'd
> be depositable into any pointer whatsoever without the cast. With the
> cast in the #define, we pick up pointer mismatches (as we should).
> Without it, we don't. As long as the define is always a specific type,
> it *should* cast to it.
>
Hmmm, good point.. In that case I will go ahead and drop this part of
the patch.
Thanks!
--nab
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 21:33 ` Nicholas A. Bellinger
@ 2011-01-24 21:51 ` James Bottomley
2011-01-24 22:12 ` Nicholas A. Bellinger
2011-01-24 23:18 ` Joe Eykholt
1 sibling, 1 reply; 16+ messages in thread
From: James Bottomley @ 2011-01-24 21:51 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: linux-scsi, Fubo Chen, Christoph Hellwig
On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > +#define TASK_DEV(task) ((task)->se_dev)
> >
> > If sparse is objecting to things like this then sparse needs fixing:
> > It's decreasing typesafety. the things being cast are void * ... they'd
> > be depositable into any pointer whatsoever without the cast. With the
> > cast in the #define, we pick up pointer mismatches (as we should).
> > Without it, we don't. As long as the define is always a specific type,
> > it *should* cast to it.
> >
>
> Hmmm, good point.. In that case I will go ahead and drop this part of
> the patch.
Actually, I misspoke on this. They're not void *; they're defined as
struct pointers ... so the cast is actually a spurious double cast. As
long as the rest are, I'm fine with this.
James
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 21:51 ` James Bottomley
@ 2011-01-24 22:12 ` Nicholas A. Bellinger
2011-01-24 23:56 ` Stefan Richter
0 siblings, 1 reply; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 22:12 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, Fubo Chen, Christoph Hellwig
On Mon, 2011-01-24 at 15:51 -0600, James Bottomley wrote:
> On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > > +#define TASK_DEV(task) ((task)->se_dev)
> > >
> > > If sparse is objecting to things like this then sparse needs fixing:
> > > It's decreasing typesafety. the things being cast are void * ... they'd
> > > be depositable into any pointer whatsoever without the cast. With the
> > > cast in the #define, we pick up pointer mismatches (as we should).
> > > Without it, we don't. As long as the define is always a specific type,
> > > it *should* cast to it.
> > >
> >
> > Hmmm, good point.. In that case I will go ahead and drop this part of
> > the patch.
>
> Actually, I misspoke on this. They're not void *; they're defined as
> struct pointers ... so the cast is actually a spurious double cast. As
> long as the rest are, I'm fine with this.
>
Committed as seperate commit b58b76c -> lio-core-2.6.git/linus-38-rc2,
and picked into the mainline queue @ scsi-post-merge-2.6.git/for-jejb.
Thanks!
--nab
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 21:33 ` Nicholas A. Bellinger
2011-01-24 21:51 ` James Bottomley
@ 2011-01-24 23:18 ` Joe Eykholt
2011-01-24 23:25 ` Nicholas A. Bellinger
1 sibling, 1 reply; 16+ messages in thread
From: Joe Eykholt @ 2011-01-24 23:18 UTC (permalink / raw)
To: linux-iscsi-target-dev
Cc: Nicholas A. Bellinger, James Bottomley, linux-scsi, Fubo Chen,
Christoph Hellwig
On 1/24/11 1:33 PM, Nicholas A. Bellinger wrote:
> On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
>> On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
>>> -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
>>> -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
>>> +#define TASK_CMD(task) ((task)->task_se_cmd)
>>> +#define TASK_DEV(task) ((task)->se_dev)
Part of the problem with the old code is that task was not parenthesized,
so if TASK_CMD() were used with an expression, you might not get what
you want. If you did TASK_CMD(p + 5), for example, you would get
((struct se_cmd *)p + 5->task_se_cmd)
Which wouldn't compile, but maybe other examples would compile and
would cause strange problems. So, it's a bad macro as it is.
Just my 2 cents.
Cheers,
Joe
>>> If sparse is objecting to things like this then sparse needs fixing:
>>> It's decreasing typesafety. the things being cast are void * ... they'd
>>> be depositable into any pointer whatsoever without the cast. With the
>>> cast in the #define, we pick up pointer mismatches (as we should).
>>> Without it, we don't. As long as the define is always a specific type,
>>> it *should* cast to it.
>>>
>
> Hmmm, good point.. In that case I will go ahead and drop this part of
> the patch.
>
> Thanks!
>
> --nab
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 23:18 ` Joe Eykholt
@ 2011-01-24 23:25 ` Nicholas A. Bellinger
0 siblings, 0 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-24 23:25 UTC (permalink / raw)
To: Joe Eykholt
Cc: linux-iscsi-target-dev, James Bottomley, linux-scsi, Fubo Chen,
Christoph Hellwig
On Mon, 2011-01-24 at 15:18 -0800, Joe Eykholt wrote:
> On 1/24/11 1:33 PM, Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> >> On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> >>> -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> >>> -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> >>> +#define TASK_CMD(task) ((task)->task_se_cmd)
> >>> +#define TASK_DEV(task) ((task)->se_dev)
> Part of the problem with the old code is that task was not parenthesized,
> so if TASK_CMD() were used with an expression, you might not get what
> you want. If you did TASK_CMD(p + 5), for example, you would get
>
> ((struct se_cmd *)p + 5->task_se_cmd)
>
> Which wouldn't compile, but maybe other examples would compile and
> would cause strange problems. So, it's a bad macro as it is.
> Just my 2 cents.
>
Good point here Joe.. AFAIK there are no cases of this in actual usage,
but for good measure all of these within target_core_base.h where
converted to:
#define TCM_FOO(p) ((p)->member)
for:
commit b58b76cd21bf461308e7fba175931f8f8c089bd7
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Mon Jan 24 14:29:08 2011 -0800
target: Remove spurious double cast from structure macro accessors
Thanks for your comments!
--nab
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 22:12 ` Nicholas A. Bellinger
@ 2011-01-24 23:56 ` Stefan Richter
2011-01-25 0:37 ` Nicholas A. Bellinger
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Richter @ 2011-01-24 23:56 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: James Bottomley, linux-scsi, Fubo Chen, Christoph Hellwig
On Jan 24 Nicholas A. Bellinger wrote:
> On Mon, 2011-01-24 at 15:51 -0600, James Bottomley wrote:
> > On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> > > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > > > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > > > +#define TASK_DEV(task) ((task)->se_dev)
> > > >
> > > > If sparse is objecting to things like this then sparse needs fixing:
> > > > It's decreasing typesafety. the things being cast are void * ... they'd
> > > > be depositable into any pointer whatsoever without the cast. With the
> > > > cast in the #define, we pick up pointer mismatches (as we should).
> > > > Without it, we don't. As long as the define is always a specific type,
> > > > it *should* cast to it.
> > > >
> > >
> > > Hmmm, good point.. In that case I will go ahead and drop this part of
> > > the patch.
> >
> > Actually, I misspoke on this. They're not void *; they're defined as
> > struct pointers ... so the cast is actually a spurious double cast. As
> > long as the rest are, I'm fine with this.
> >
>
> Committed as seperate commit b58b76c -> lio-core-2.6.git/linus-38-rc2,
> and picked into the mainline queue @ scsi-post-merge-2.6.git/for-jejb.
Why do you provide macros for a simple structure member dereference?
And if you *really* need these helpers, why not
static inline struct se_cmd *cmd_of(struct se_task *task)
{
return task->task_se_cmd;
}
(But then, I see no reason at all not to write it as task->task_se_cmd at
the user sites.)
--
Stefan Richter
-=====-==-== ---= ==--=
http://arcgraph.de/sr/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue
2011-01-24 20:37 ` [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue Nicholas A. Bellinger
@ 2011-01-25 0:08 ` Stefan Richter
2011-01-25 1:20 ` Nicholas A. Bellinger
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Richter @ 2011-01-25 0:08 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: linux-scsi, Fubo Chen, Christoph Hellwig, James Bottomley
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/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations
2011-01-24 23:56 ` Stefan Richter
@ 2011-01-25 0:37 ` Nicholas A. Bellinger
0 siblings, 0 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-25 0:37 UTC (permalink / raw)
To: Stefan Richter; +Cc: James Bottomley, linux-scsi, Fubo Chen, Christoph Hellwig
On Tue, 2011-01-25 at 00:56 +0100, Stefan Richter wrote:
> On Jan 24 Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-24 at 15:51 -0600, James Bottomley wrote:
> > > On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> > > > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > > > > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > > > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > > > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > > > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > > > > +#define TASK_DEV(task) ((task)->se_dev)
> > > > >
> > > > > If sparse is objecting to things like this then sparse needs fixing:
> > > > > It's decreasing typesafety. the things being cast are void * ... they'd
> > > > > be depositable into any pointer whatsoever without the cast. With the
> > > > > cast in the #define, we pick up pointer mismatches (as we should).
> > > > > Without it, we don't. As long as the define is always a specific type,
> > > > > it *should* cast to it.
> > > > >
> > > >
> > > > Hmmm, good point.. In that case I will go ahead and drop this part of
> > > > the patch.
> > >
> > > Actually, I misspoke on this. They're not void *; they're defined as
> > > struct pointers ... so the cast is actually a spurious double cast. As
> > > long as the rest are, I'm fine with this.
> > >
> >
> > Committed as seperate commit b58b76c -> lio-core-2.6.git/linus-38-rc2,
> > and picked into the mainline queue @ scsi-post-merge-2.6.git/for-jejb.
>
> Why do you provide macros for a simple structure member dereference?
>
> And if you *really* need these helpers, why not
>
> static inline struct se_cmd *cmd_of(struct se_task *task)
> {
> return task->task_se_cmd;
> }
>
> (But then, I see no reason at all not to write it as task->task_se_cmd at
> the user sites.)
Most of these accessor macros originally came from the usage of more
than a single pointer deference. These days only DEV_ATTRIB and
DEV_T10_WWN still deference multiple pointers, so it's really just a
matter of style in modern v4.0 code.
So my perference would be against using inlined cmd_of() callers, and
just access things directly. Btw at this point this type of cleanup
would be considered for-39 material.
--nab
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue
2011-01-25 0:08 ` Stefan Richter
@ 2011-01-25 1:20 ` Nicholas A. Bellinger
2011-01-25 2:03 ` Nicholas A. Bellinger
2011-01-25 14:39 ` Stefan Richter
0 siblings, 2 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-25 1:20 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-scsi, Fubo Chen, Christoph Hellwig, James Bottomley
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.
Thanks for your comments!
--nab
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue
2011-01-25 1:20 ` Nicholas A. Bellinger
@ 2011-01-25 2:03 ` Nicholas A. Bellinger
2011-01-25 14:39 ` Stefan Richter
1 sibling, 0 replies; 16+ messages in thread
From: Nicholas A. Bellinger @ 2011-01-25 2:03 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-scsi, Fubo Chen, Christoph Hellwig, James Bottomley
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);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue
2011-01-25 1:20 ` Nicholas A. Bellinger
2011-01-25 2:03 ` Nicholas A. Bellinger
@ 2011-01-25 14:39 ` Stefan Richter
1 sibling, 0 replies; 16+ messages in thread
From: Stefan Richter @ 2011-01-25 14:39 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: linux-scsi, Fubo Chen, Christoph Hellwig, James Bottomley
On Jan 24 Nicholas A. Bellinger wrote:
> On Tue, 2011-01-25 at 01:08 +0100, Stefan Richter wrote:
> > void se_clear_dev_ports(struct se_device *dev)
[...]
> > 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.
I meant an on-stack throwaway list which you can change inside
se_clear_dev_ports() without any lock. But I didn't look whether this is
actually possible and beneficial.
> 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(),
[...]
> config_group structures, I think se_clear_dev_ports() should be able to
> be dropped all together now.
That of course sounds great.
> I will take a deeper look and see if this
> is really in fact safe for v4.0/for-38 code.
As a thought from an outsider: If drivers/target/ weren't entirely new in
2.6.38, then only a minimal cautious locking fix would be post -rc1
material. Linus releases so frequently that anything beyond essential
fixes can easily wait for regular merge windows. "Essential" = very low
risk : reward ratio && with reasonable reward. Those who directly work
with the code tend to underestimate risk and to overestimate reward. ;-)
--
Stefan Richter
-=====-==-== ---= ==--=
http://arcgraph.de/sr/
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-01-25 14:39 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-24 20:37 [PATCH 0/3] target: Sparse bugfixes and warnings/annotations Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 1/3] target: Drop nacl->device_list_lock on core_update_device_list_for_node failure Nicholas A. Bellinger
2011-01-24 20:37 ` [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue Nicholas A. Bellinger
2011-01-25 0:08 ` Stefan Richter
2011-01-25 1:20 ` Nicholas A. Bellinger
2011-01-25 2:03 ` Nicholas A. Bellinger
2011-01-25 14:39 ` Stefan Richter
2011-01-24 20:37 ` [PATCH 3/3] target: Minor sparse warning fixes and annotations Nicholas A. Bellinger
2011-01-24 20:56 ` James Bottomley
2011-01-24 21:33 ` Nicholas A. Bellinger
2011-01-24 21:51 ` James Bottomley
2011-01-24 22:12 ` Nicholas A. Bellinger
2011-01-24 23:56 ` Stefan Richter
2011-01-25 0:37 ` Nicholas A. Bellinger
2011-01-24 23:18 ` Joe Eykholt
2011-01-24 23:25 ` Nicholas A. Bellinger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.