[stgt] [PATCH] Put locking around each entry in cmd_hash_list

[Andy Grover]

Both the receiving thread and the bs worker threads access the hash of
lists at it_nexus->cmd_hash_list. It may be the case that this is causing
list corruption. Lock each individual list in the hash with a mutex.

Using a recursive mutex, since abort_task_set both needs to hold the mutex,
and __cmd_done (called from abort_cmd) also is calling cmd_hlist_remove,
which also takes the mutex.

Signed-off-by: Andy Grover <agrover at redhat.com>
---

Hi Kiefer, does this make a difference? Thanks -- Andy

 usr/scsi_cmnd.h |    3 +++
 usr/target.c    |   46 ++++++++++++++++++++++++++++++++++++++--------
 usr/target.h    |    7 ++++++-
 3 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/usr/scsi_cmnd.h b/usr/scsi_cmnd.h
index c764c4e..cd9272e 100644
--- a/usr/scsi_cmnd.h
+++ b/usr/scsi_cmnd.h
@@ -49,6 +49,9 @@ struct scsi_cmd {
 
 	struct it_nexus *it_nexus;
 	struct it_nexus_lu_info *itn_lu_info;
+
+	/* backptr to what cmd_hash_entry we're on so we can lock properly */
+	struct cmd_hash_entry *cur_hash_entry;
 };
 
 #define scsi_cmnd_accessor(field, type)						\
diff --git a/usr/target.c b/usr/target.c
index 42443d0..ac26a99 100644
--- a/usr/target.c
+++ b/usr/target.c
@@ -29,6 +29,7 @@
 #include <unistd.h>
 #include <sys/socket.h>
 #include <sys/time.h>
+#include <pthread.h>
 
 #include "list.h"
 #include "util.h"
@@ -269,6 +270,7 @@ int it_nexus_create(int tid, uint64_t itn_id, int host_no, char *info)
 	struct scsi_lu *lu;
 	struct it_nexus_lu_info *itn_lu;
 	struct timeval tv;
+	pthread_mutexattr_t mutex_attr;
 
 	dprintf("%d %" PRIu64 " %d\n", tid, itn_id, host_no);
 	/* for reserve/release code */
@@ -308,8 +310,15 @@ int it_nexus_create(int tid, uint64_t itn_id, int host_no, char *info)
 			 &itn->it_nexus_lu_info_list);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(itn->cmd_hash_list); i++)
-		INIT_LIST_HEAD(&itn->cmd_hash_list[i]);
+	pthread_mutexattr_init(&mutex_attr);
+	pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE);
+
+	for (i = 0; i < ARRAY_SIZE(itn->cmd_hash_list); i++) {
+		INIT_LIST_HEAD(&itn->cmd_hash_list[i].head);
+		pthread_mutex_init(&itn->cmd_hash_list[i].lock, &mutex_attr);
+	}
+
+	pthread_mutexattr_destroy(&mutex_attr);
 
 	list_add_tail(&itn->nexus_siblings, &target->it_nexus_list);
 
@@ -324,6 +333,7 @@ int it_nexus_destroy(int tid, uint64_t itn_id)
 	int i;
 	struct it_nexus *itn;
 	struct scsi_lu *lu;
+	struct cmd_hash_entry *entry;
 
 	dprintf("%d %" PRIu64 "\n", tid, itn_id);
 
@@ -331,9 +341,18 @@ int it_nexus_destroy(int tid, uint64_t itn_id)
 	if (!itn)
 		return -ENOENT;
 
-	for (i = 0; i < ARRAY_SIZE(itn->cmd_hash_list); i++)
-		if (!list_empty(&itn->cmd_hash_list[i]))
+	for (i = 0; i < ARRAY_SIZE(itn->cmd_hash_list); i++) {
+		entry = &itn->cmd_hash_list[i];
+		pthread_mutex_lock(&entry->lock);
+		if (!list_empty(&entry->head)) {
+			pthread_mutex_unlock(&entry->lock);
 			return -EBUSY;
+		}
+		pthread_mutex_unlock(&entry->lock);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(itn->cmd_hash_list); i++)
+		pthread_mutex_destroy(&itn->cmd_hash_list[i].lock);
 
 	list_for_each_entry(lu, &itn->nexus_target->device_list, device_siblings)
 		device_release(tid, itn_id, lu->lun, 0);
@@ -357,13 +376,22 @@ static struct scsi_lu *device_lookup(struct target *target, uint64_t lun)
 
 static void cmd_hlist_insert(struct it_nexus *itn, struct scsi_cmd *cmd)
 {
-	struct list_head *list = &itn->cmd_hash_list[hashfn(cmd->tag)];
-	list_add(&cmd->c_hlist, list);
+	struct cmd_hash_entry *entry = &itn->cmd_hash_list[hashfn(cmd->tag)];
+
+	pthread_mutex_lock(&entry->lock);
+	cmd->cur_hash_entry = entry;
+	list_add(&cmd->c_hlist, &entry->head);
+	pthread_mutex_unlock(&entry->lock);
 }
 
 static void cmd_hlist_remove(struct scsi_cmd *cmd)
 {
+	struct cmd_hash_entry *entry = cmd->cur_hash_entry;
+
+	pthread_mutex_lock(&entry->lock);
+	cmd->cur_hash_entry = NULL;
 	list_del(&cmd->c_hlist);
+	pthread_mutex_unlock(&entry->lock);
 }
 
 static void tgt_cmd_queue_init(struct tgt_cmd_queue *q)
@@ -1116,8 +1144,9 @@ static int abort_task_set(struct mgmt_req *mreq, struct target* target,
 
 	list_for_each_entry(itn, &target->it_nexus_list, nexus_siblings) {
 		for (i = 0; i < ARRAY_SIZE(itn->cmd_hash_list); i++) {
-			struct list_head *list = &itn->cmd_hash_list[i];
-			list_for_each_entry_safe(cmd, tmp, list, c_hlist) {
+			struct cmd_hash_entry *entry = &itn->cmd_hash_list[i];
+			pthread_mutex_lock(&entry->lock);
+			list_for_each_entry_safe(cmd, tmp, &entry->head, c_hlist) {
 				if ((all && itn->itn_id == itn_id) ||
 				    (cmd->tag == tag && itn->itn_id == itn_id) ||
 				    (lun && !memcmp(cmd->lun, lun, sizeof(cmd->lun)))) {
@@ -1127,6 +1156,7 @@ static int abort_task_set(struct mgmt_req *mreq, struct target* target,
 					count++;
 				}
 			}
+			pthread_mutex_unlock(&entry->lock);
 		}
 	}
 	return count;
diff --git a/usr/target.h b/usr/target.h
index f1e51f3..23e149d 100644
--- a/usr/target.h
+++ b/usr/target.h
@@ -48,11 +48,16 @@ struct target {
 	struct tgt_account account;
 };
 
+struct cmd_hash_entry {
+	struct list_head head;
+	pthread_mutex_t lock;
+};
+
 struct it_nexus {
 	uint64_t itn_id;
 	long ctime;
 
-	struct list_head cmd_hash_list[1 << HASH_ORDER];
+	struct cmd_hash_entry cmd_hash_list[1 << HASH_ORDER];
 
 	struct target *nexus_target;
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html