The previous cache locking was insufficient. eet_clearcache() used no
locking at all. eet_open() locked the cache to find an existing entry,
but then unlocked the cache before incrementing the file's reference
count and using the file. eet_close() also did not hold a lock while
decrementing the reference count and checking to see if a file was
unreferenced.
These changes update eet_open(), eet_clearcache(), and
eet_internal_close() so that they always hold the lock while updating
file references and modifying the cache.
This does result in the cache lock being held during some file
operations that ideally should be done outside of a critical section.
However, this seems like the easiest fix for now without re-ordering any
of the existing logic.
---
eet/src/lib/eet_lib.c | 63 +++++++++++++++++++++++++++++--------------
eet/src/tests/eet_suite.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 106 insertions(+), 21 deletions(-)
diff --git a/eet/src/lib/eet_lib.c b/eet/src/lib/eet_lib.c
index 8fed189..4ed8432 100644
--- a/eet/src/lib/eet_lib.c
+++ b/eet/src/lib/eet_lib.c
@@ -280,7 +280,7 @@ eet_test_close(int test, Eet_File *ef)
if (test)
{
ef->delete_me_now = 1;
- eet_internal_close(ef, 0);
+ eet_internal_close(ef, 1);
}
return test;
}
@@ -307,6 +307,7 @@ eet_cache_find(const char *path, Eet_File **cache, int
cache_num)
}
/* add to end of cache */
+/* this should only be called when the cache lock is already held */
static void
eet_cache_add(Eet_File *ef, Eet_File ***cache, int *cache_num, int
*cache_alloc)
{
@@ -358,6 +359,7 @@ eet_cache_add(Eet_File *ef, Eet_File ***cache, int
*cache_num, int *cache_alloc)
}
/* delete from cache */
+/* this should only be called when the cache lock is already held */
static void
eet_cache_del(Eet_File *ef, Eet_File ***cache, int *cache_num, int
*cache_alloc)
{
@@ -849,6 +851,7 @@ eet_clearcache(void)
* We need to compute the list of eet file to close separately from the
cache,
* due to eet_close removing them from the cache after each call.
*/
+ LOCK_CACHE;
for (i = 0; i < eet_writers_num; i++)
{
if (eet_writers[i]->references <= 0) num++;
@@ -887,9 +890,10 @@ eet_clearcache(void)
for (i = 0; i < num; i++)
{
- eet_close(closelist[i]);
+ eet_internal_close(closelist[i], 1);
}
}
+ UNLOCK_CACHE;
}
/* FIXME: MMAP race condition in READ_WRITE_MODE */
@@ -1281,6 +1285,12 @@ eet_internal_read1(Eet_File *ef)
}
#endif
+/*
+ * this should only be called when the cache lock is already held
+ * (We could drop this restriction if we add a parameter to eet_test_close
+ * that indicates if the lock is held or not. For now it is easiest
+ * to just require that it is always held.)
+ */
static Eet_File *
eet_internal_read(Eet_File *ef)
{
@@ -1317,6 +1327,9 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked)
/* check to see its' an eet file pointer */
if (eet_check_pointer(ef))
return EET_ERROR_BAD_OBJECT;
+
+ if (!locked) LOCK_CACHE;
+
/* deref */
ef->references--;
/* if its still referenced - dont go any further */
@@ -1329,17 +1342,18 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked)
/* if not urgent to delete it - dont free it - leave it in cache */
if ((!ef->delete_me_now) && (ef->mode == EET_FILE_MODE_READ))
- return EET_ERROR_NONE;
-
- /* remove from cache */
- if (!locked)
{
- LOCK_CACHE;
+ if (!locked) UNLOCK_CACHE;
+ return EET_ERROR_NONE;
}
+
+ /* remove from cache */
if (ef->mode == EET_FILE_MODE_READ)
eet_cache_del(ef, &eet_readers, &eet_readers_num, &eet_readers_alloc);
else if ((ef->mode == EET_FILE_MODE_WRITE) || (ef->mode ==
EET_FILE_MODE_READ_WRITE))
eet_cache_del(ef, &eet_writers, &eet_writers_num, &eet_writers_alloc);
+
+ /* we can unlock the cache now */
if (!locked)
{
UNLOCK_CACHE;
@@ -1425,7 +1439,11 @@ eet_memopen_read(const void *data, size_t size)
ef->sha1 = NULL;
ef->sha1_length = 0;
- return eet_internal_read(ef);
+ /* eet_internal_read expects the cache lock to be held when it is called */
+ LOCK_CACHE;
+ ef = eet_internal_read(ef);
+ UNLOCK_CACHE;
+ return ef;
}
EAPI Eet_File *
@@ -1466,7 +1484,6 @@ eet_open(const char *file, Eet_File_Mode mode)
}
ef = eet_cache_find((char *)file, eet_writers, eet_writers_num);
}
- UNLOCK_CACHE;
/* try open the file based on mode */
if ((mode == EET_FILE_MODE_READ) || (mode == EET_FILE_MODE_READ_WRITE))
@@ -1475,23 +1492,23 @@ eet_open(const char *file, Eet_File_Mode mode)
file_stat.st_mtime = 0;
fp = fopen(file, "rb");
- if (!fp) goto on_error;
+ if (!fp) goto open_error;
if (fstat(fileno(fp), &file_stat))
{
fclose(fp);
fp = NULL;
- goto on_error;
+ goto open_error;
}
if ((mode == EET_FILE_MODE_READ) &&
(file_stat.st_size < ((int) sizeof(int) * 3)))
{
fclose(fp);
fp = NULL;
- goto on_error;
+ goto open_error;
}
- on_error:
- if (fp == NULL && mode == EET_FILE_MODE_READ) return NULL;
+ open_error:
+ if (fp == NULL && mode == EET_FILE_MODE_READ) goto on_error;
}
else
{
@@ -1503,7 +1520,7 @@ eet_open(const char *file, Eet_File_Mode mode)
unlink(file);
fd = open(file, O_CREAT | O_TRUNC | O_RDWR, S_IRUSR | S_IWUSR);
fp = fdopen(fd, "wb");
- if (!fp) return NULL;
+ if (!fp) goto on_error;
}
/* We found one */
@@ -1520,6 +1537,7 @@ eet_open(const char *file, Eet_File_Mode mode)
/* reference it up and return it */
if (fp != NULL) fclose(fp);
ef->references++;
+ UNLOCK_CACHE;
return ef;
}
@@ -1528,7 +1546,7 @@ eet_open(const char *file, Eet_File_Mode mode)
/* Allocate struct for eet file and have it zero'd out */
ef = malloc(sizeof(Eet_File) + file_len);
if (!ef)
- return NULL;
+ goto on_error;
/* fill some of the members */
INIT_FILE(ef);
@@ -1557,7 +1575,7 @@ eet_open(const char *file, Eet_File_Mode mode)
/* if we can't open - bail out */
if (eet_test_close(!ef->fp, ef))
- return NULL;
+ goto on_error;
fcntl(fileno(ef->fp), F_SETFD, FD_CLOEXEC);
/* if we opened for read or read-write */
@@ -1567,10 +1585,10 @@ eet_open(const char *file, Eet_File_Mode mode)
ef->data = mmap(NULL, ef->data_size, PROT_READ,
MAP_SHARED, fileno(ef->fp), 0);
if (eet_test_close((ef->data == MAP_FAILED), ef))
- return NULL;
+ goto on_error;
ef = eet_internal_read(ef);
if (!ef)
- return NULL;
+ goto on_error;
}
empty_file:
@@ -1584,16 +1602,19 @@ eet_open(const char *file, Eet_File_Mode mode)
/* add to cache */
if (ef->references == 1)
{
- LOCK_CACHE;
if (ef->mode == EET_FILE_MODE_READ)
eet_cache_add(ef, &eet_readers, &eet_readers_num, &eet_readers_alloc);
else
if ((ef->mode == EET_FILE_MODE_WRITE) || (ef->mode ==
EET_FILE_MODE_READ_WRITE))
eet_cache_add(ef, &eet_writers, &eet_writers_num,
&eet_writers_alloc);
- UNLOCK_CACHE;
}
+ UNLOCK_CACHE;
return ef;
+
+on_error:
+ UNLOCK_CACHE;
+ return NULL;
}
EAPI Eet_File_Mode
diff --git a/eet/src/tests/eet_suite.c b/eet/src/tests/eet_suite.c
index a41e8f7..0faa01b 100644
--- a/eet/src/tests/eet_suite.c
+++ b/eet/src/tests/eet_suite.c
@@ -6,6 +6,7 @@
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
+#include <pthread.h>
#include <check.h>
@@ -1410,6 +1411,65 @@ START_TEST(eet_cipher_decipher_simple)
}
END_TEST
+static Eina_Bool open_worker_stop;
+static void*
+open_close_worker(void* path)
+{
+ while (!open_worker_stop)
+ {
+ Eet_File* ef = eet_open((char const*)path, EET_FILE_MODE_READ);
+ if (ef == NULL)
+ {
+ pthread_exit("eet_open() failed");
+ }
+ else
+ {
+ Eet_Error err_code = eet_close(ef);
+ if (err_code != EET_ERROR_NONE)
+ pthread_exit("eet_close() failed");
+ }
+ }
+
+ pthread_exit(NULL);
+}
+
+START_TEST(eet_cache_concurrency)
+{
+ char *file = strdup("/tmp/eet_suite_testXXXXXX");
+ const char *buffer = "test data";
+ Eet_File *ef;
+ void *thread_ret;
+ unsigned int n;
+
+ eet_init();
+
+ /* create a file to test with */
+ fail_if(!mktemp(file));
+ ef = eet_open(file, EET_FILE_MODE_WRITE);
+ fail_if(!ef);
+ fail_if(!eet_write(ef, "keys/tests", buffer, strlen(buffer) + 1, 0));
+
+ /* start a thread that repeatedly opens and closes a file */
+ open_worker_stop = 0;
+ pthread_t thread;
+ pthread_create(&thread, NULL, open_close_worker, file);
+
+ /* clear the cache repeatedly in this thread */
+ for (n = 0; n < 100000; ++n)
+ {
+ eet_clearcache();
+ }
+
+ /* join the other thread, and fail if it returned an error message */
+ open_worker_stop = 1;
+ fail_if(pthread_join(thread, &thread_ret) != 0);
+ fail_unless(thread_ret == NULL, (char const*)thread_ret);
+
+ fail_if(unlink(file) != 0);
+ eet_shutdown();
+}
+END_TEST
+
Suite *
eet_suite(void)
@@ -1455,6 +1515,10 @@ eet_suite(void)
suite_add_tcase(s, tc);
#endif
+ tc = tcase_create("Eet Cache");
+ tcase_add_test(tc, eet_cache_concurrency);
+ suite_add_tcase(s, tc);
+
return s;
}
--
1.6.0.4
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
