From: antirez <antirez@gmail.com>
Date: Thu, 14 Jan 2010 22:18:27 +0000 (-0500)
Subject: Fixed a never experienced, theoretical bug that can actually happen in practice.... 
X-Git-Url: https://git.saurik.com/redis.git/commitdiff_plain/2e111efe5a8d5092a4a6d391096ff6712cf6f162

Fixed a never experienced, theoretical bug that can actually happen in practice. Basically when a thread is working on a I/O Job we need to wait it to finish before to cancel the Job in vmCancelThreadedIOJob(), otherwise the thread may mess with an object that is being manipulated by the main thread as well.
---

diff --git a/TODO b/TODO
index 1ddf6f5d..5e592c99 100644
--- a/TODO
+++ b/TODO
@@ -18,6 +18,7 @@ Virtual Memory sub-TODO:
 * it should be possible to give the vm-max-memory option in megabyte, gigabyte, ..., just using 2GB, 100MB, and so forth.
 * Make sure to wait all the IO threads are done before to fork() for BGSAVE and BGREWRITEAOF
 * Enlarge the stack of threads, to problems when calling LZF lib.
+* redis-cli vmstat, calling INFO every second and printing VM stats ala vmstat.
 
 VERSION 1.6 TODO (Virtual memory)
 =================================
diff --git a/redis.c b/redis.c
index ce047a40..f1ce18fb 100644
--- a/redis.c
+++ b/redis.c
@@ -7573,6 +7573,7 @@ static void vmCancelThreadedIOJob(robj *o) {
     int i;
 
     assert(o->storage == REDIS_VM_LOADING || o->storage == REDIS_VM_SWAPPING);
+again:
     lockThreadedIO();
     /* Search for a matching key in one of the queues */
     for (i = 0; i < 3; i++) {
@@ -7585,8 +7586,8 @@ static void vmCancelThreadedIOJob(robj *o) {
 
             if (job->canceled) continue; /* Skip this, already canceled. */
             if (compareStringObjects(job->key,o) == 0) {
-                redisLog(REDIS_DEBUG,"*** CANCELED %p (%s)\n",
-                    (void*)job, (char*)o->ptr);
+                redisLog(REDIS_DEBUG,"*** CANCELED %p (%s) (LIST ID %d)\n",
+                    (void*)job, (char*)o->ptr, i);
                 /* Mark the pages as free since the swap didn't happened
                  * or happened but is now discarded. */
                 if (job->type == REDIS_IOJOB_DO_SWAP)
@@ -7601,7 +7602,27 @@ static void vmCancelThreadedIOJob(robj *o) {
                     listDelNode(lists[i],ln);
                     break;
                 case 1: /* io_processing */
+                    /* Oh Shi- the thread is messing with the Job, and
+                     * probably with the object if this is a
+                     * PREPARE_SWAP or DO_SWAP job. Better to wait for the
+                     * job to move into the next queue... */
+                    if (job->type != REDIS_IOJOB_LOAD) {
+                        /* Yes, we try again and again until the job
+                         * is completed. */
+                        unlockThreadedIO();
+                        /* But let's wait some time for the I/O thread
+                         * to finish with this job. After all this condition
+                         * should be very rare. */
+                        usleep(1);
+                        goto again;
+                    } else {
+                        job->canceled = 1;
+                        break;
+                    }
                 case 2: /* io_processed */
+                    /* The job was already processed, that's easy...
+                     * just mark it as canceled so that we'll ignore it
+                     * when processing completed jobs. */
                     job->canceled = 1;
                     break;
                 }