[Scst-svn] SF.net SVN: scst: [199] trunk

[<vl...@us...>]

Revision: 199
          http://scst.svn.sourceforge.net/scst/?rev=199&view=rev
Author:   vlnb
Date:     2007-09-25 02:52:53 -0700 (Tue, 25 Sep 2007)

Log Message:
-----------
 - Fixes memory leaks in scst_user spotted by new SGV cache backend
 - Version changed on -pre3
 - Minor fixes and cosmetics

Modified Paths:
--------------
    trunk/scst/include/scsi_tgt.h
    trunk/scst/src/dev_handlers/scst_user.c
    trunk/scst/src/scst_mem.c
    trunk/usr/fileio/common.c
    trunk/usr/fileio/fileio.c

Modified: trunk/scst/include/scsi_tgt.h
===================================================================
--- trunk/scst/include/scsi_tgt.h	2007-09-25 09:46:36 UTC (rev 198)
+++ trunk/scst/include/scsi_tgt.h	2007-09-25 09:52:53 UTC (rev 199)
@@ -39,7 +39,7 @@
 /* Version numbers, the same as for the kernel */
 #define SCST_VERSION_CODE 0x000906
 #define SCST_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
-#define SCST_VERSION_STRING "0.9.6-pre2"
+#define SCST_VERSION_STRING "0.9.6-pre3"
 
 /*************************************************************
  ** States of command processing state machine

Modified: trunk/scst/src/dev_handlers/scst_user.c
===================================================================
--- trunk/scst/src/dev_handlers/scst_user.c	2007-09-25 09:46:36 UTC (rev 198)
+++ trunk/scst/src/dev_handlers/scst_user.c	2007-09-25 09:52:53 UTC (rev 199)
@@ -361,7 +361,7 @@
 {
 	TRACE_ENTRY();
 
-	TRACE_DBG("Preparing ON_CACHED_MEM_FREE (ucmd %p, h %d, ubuff %lx)",
+	TRACE_MEM("Preparing ON_CACHED_MEM_FREE (ucmd %p, h %d, ubuff %lx)",
 		ucmd, ucmd->h, ucmd->ubuff);
 
 	ucmd->user_cmd.cmd_h = ucmd->h;
@@ -400,18 +400,14 @@
 	return;
 }
 
-static void dev_user_free_sg_entries(struct scatterlist *sg, int sg_count,
-	void *priv)
+static void __dev_user_free_sg_entries(struct dev_user_cmd *ucmd)
 {
-	struct dev_user_cmd *ucmd = (struct dev_user_cmd*)priv;
-
 	TRACE_ENTRY();
 
 	sBUG_ON(ucmd->data_pages == NULL);
 
-	TRACE_MEM("Freeing data pages (ucmd=%p, ubuff=%lx, sg=%p, sg_count=%d, "
-		"buff_cached=%d)", ucmd, ucmd->ubuff, sg, sg_count,
-		ucmd->buff_cached);
+	TRACE_MEM("Freeing data pages (ucmd=%p, ubuff=%lx, buff_cached=%d)",
+		ucmd, ucmd->ubuff, ucmd->buff_cached);
 
 	dev_user_unmap_buf(ucmd);
 
@@ -424,6 +420,19 @@
 	return;
 }
 
+static void dev_user_free_sg_entries(struct scatterlist *sg, int sg_count,
+	void *priv)
+{
+	struct dev_user_cmd *ucmd = (struct dev_user_cmd*)priv;
+
+	TRACE_MEM("Freeing data pages (sg=%p, sg_count=%d, priv %p)", sg,
+		sg_count, ucmd);
+
+	__dev_user_free_sg_entries(ucmd);
+
+	return;
+}
+
 static inline int is_buff_cached(struct dev_user_cmd *ucmd)
 {
 	int mem_reuse_type = ucmd->dev->memory_reuse_type;
@@ -510,9 +519,8 @@
 					cmd->tgt->sg_tablesize);
 				ll++;
 			}
-			sgv_pool_free(ucmd->sgv);
-			ucmd->sgv = NULL;
 			cmd->sg = NULL;
+			/* sgv will be freed in dev_user_free_sgv() */
 			res = -1;
 		}
 	} else {
@@ -520,9 +528,9 @@
 			"sg_cnt %d, ubuff %lx, sgv %p", ucmd, ucmd->h,
 			ucmd->buff_cached, cmd->sg_cnt,	ucmd->ubuff, ucmd->sgv);
 		if (unlikely(cmd->sg_cnt == 0)) {
+			TRACE_MEM("Refused allocation (ucmd %p)", ucmd);
+			sBUG_ON(ucmd->sgv != NULL);
 			res = -1;
-			if (ucmd->data_pages != NULL)
-				dev_user_unmap_buf(ucmd);
 		} else {
 			switch(ucmd->state & ~UCMD_STATE_MASK) {
 			case UCMD_STATE_BUF_ALLOCING:
@@ -530,8 +538,6 @@
 				break;
 			case UCMD_STATE_EXECING:
 				res = -1;
-				if (ucmd->data_pages != NULL)
-					dev_user_unmap_buf(ucmd);
 				break;
 			default:
 				sBUG();
@@ -810,7 +816,12 @@
 	if (ucmd->sgv != NULL) {
 		sgv_pool_free(ucmd->sgv);
 		ucmd->sgv = NULL;
+	} else if (ucmd->data_pages != NULL) {
+		/* We mapped pages, but for some reason didn't allocate them */
+		ucmd_get(ucmd, 0);
+		__dev_user_free_sg_entries(ucmd);
 	}
+	return;
 }
 
 static void dev_user_on_free_cmd(struct scst_cmd *cmd)
@@ -822,7 +833,7 @@
 	if (unlikely(ucmd == NULL))
 		goto out;
 
-	TRACE_DBG("ucmd %p, cmd %p, buff_cached %d, ubuff %lx", ucmd, ucmd->cmd,
+	TRACE_MEM("ucmd %p, cmd %p, buff_cached %d, ubuff %lx", ucmd, ucmd->cmd,
 		ucmd->buff_cached, ucmd->ubuff);
 
 	ucmd->cmd = NULL;
@@ -998,6 +1009,8 @@
 	if (unlikely(ubuff == 0))
 		goto out_nomem;
 
+	sBUG_ON(ucmd->data_pages != NULL);
+
 	ucmd->num_data_pages = num_pg;
 
 	ucmd->data_pages = kzalloc(sizeof(*ucmd->data_pages)*ucmd->num_data_pages,

Modified: trunk/scst/src/scst_mem.c
===================================================================
--- trunk/scst/src/scst_mem.c	2007-09-25 09:46:36 UTC (rev 198)
+++ trunk/scst/src/scst_mem.c	2007-09-25 09:52:53 UTC (rev 199)
@@ -503,6 +503,8 @@
 	int no_cached = flags & SCST_POOL_ALLOC_NO_CACHED;
 	int no_fail = ((gfp_mask & __GFP_NOFAIL) == __GFP_NOFAIL);
 
+	TRACE_ENTRY();
+
 	sBUG_ON(size == 0);
 
 	pages = ((size + PAGE_SIZE - 1) >> PAGE_SHIFT);
@@ -519,13 +521,8 @@
 		EXTRACHECKS_BUG_ON(obj->sg_count != 0);
 		pages_to_alloc = (1 << order);
 		cache = pool->caches[obj->order];
-		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0) {
-			obj->sg_count = 0;
-			if ((flags & SCST_POOL_RETURN_OBJ_ON_ALLOC_FAIL))
-				goto out_return1;
-			else
-				goto out_fail_free_sg_entries;
-		}
+		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0)
+			goto out_fail_free_sg_entries;
 	} else if ((order < SGV_POOL_ELEMENTS) && !no_cached) {
 		cache = pool->caches[order];
 		obj = sgv_pool_cached_get(pool, order, gfp_mask);
@@ -579,13 +576,8 @@
 			goto out_return;
 			
 		obj->allocator_priv = priv;
-		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0) {
-			obj->sg_count = 0;
-			if ((flags & SCST_POOL_RETURN_OBJ_ON_ALLOC_FAIL))
-				goto out_return1;
-			else
-				goto out_fail_free_sg_entries;
-		}
+		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0)
+			goto out_fail_free_sg_entries;
 	} else {
 		int sz;
 		pages_to_alloc = pages;
@@ -604,10 +596,8 @@
 		obj->sg_entries = obj->sg_entries_data;
 		obj->allocator_priv = priv;
 		
-		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0) {
-			obj->sg_count = 0;
+		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0)
 			goto out_fail_free_sg_entries;
-		}
 		TRACE_MEM("Big or no_cached sgv_obj %p (size %d)", obj,	sz);		
 	}
 
@@ -662,6 +652,7 @@
 		obj->sg_count, *count, obj->sg_entries[obj->orig_sg].length);
 
 out:
+	TRACE_EXIT_HRES(res);
 	return res;
 
 out_return:

Modified: trunk/usr/fileio/common.c
===================================================================
--- trunk/usr/fileio/common.c	2007-09-25 09:46:36 UTC (rev 198)
+++ trunk/usr/fileio/common.c	2007-09-25 09:52:53 UTC (rev 199)
@@ -663,9 +663,9 @@
 
 	vcmd.fd = open_dev_fd(dev);
 	if (vcmd.fd < 0) {
-		res = vcmd.fd;
+		res = -errno;
 		PRINT_ERROR_PR("Unable to open file %s (%s)", dev->file_name,
-			strerror(res));
+			strerror(-res));
 		goto out;
 	}
 

Modified: trunk/usr/fileio/fileio.c
===================================================================
--- trunk/usr/fileio/fileio.c	2007-09-25 09:46:36 UTC (rev 198)
+++ trunk/usr/fileio/fileio.c	2007-09-25 09:52:53 UTC (rev 199)
@@ -277,9 +277,9 @@
 	TRACE_DBG("Opening file %s", dev.file_name);
 	fd = open(dev.file_name, O_RDONLY|O_LARGEFILE);
 	if (fd < 0) {
-		res = errno;
+		res = -errno;
 		PRINT_ERROR_PR("Unable to open file %s (%s)", dev.file_name,
-			strerror(res));
+			strerror(-res));
 		goto out_done;
 	}
 
@@ -368,9 +368,9 @@
 	dev.scst_usr_fd = open(DEV_USER_PATH DEV_USER_NAME, O_RDWR | 
 		(dev.non_blocking ? O_NONBLOCK : 0));
 	if (dev.scst_usr_fd < 0) {
-		res = dev.scst_usr_fd;
+		res = -errno;
 		PRINT_ERROR_PR("Unable to open SCST device %s (%s)",
-			DEV_USER_PATH DEV_USER_NAME, strerror(res));
+			DEV_USER_PATH DEV_USER_NAME, strerror(-res));
 		goto out_done;
 	}
 


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.