[t:3970] closes #3970 Manually edit changeset 35079 into cachetable.c, add...

[t:3970] closes #3970 Manually edit changeset 35079 into cachetable.c, add newbrt/tests/cachetable-unpin-remove-and-checkpoint.c (written by Zardosht with minor edits by Barry).

git-svn-id: file:///svn/toku/tokudb@35435 c7de825b-a66e-492c-adef-691d508d4ae1

newbrt/cachetable.c

@@ -1072,7 +1072,8 @@ static void cachetable_remove_pair (CACHETABLE ct, PAIR p) {
 // or not there are any threads interested in the pair.  The flush callback
 // is called with write_me and keep_me both false, and the pair is destroyed.
 
-static void cachetable_maybe_remove_and_free_pair (CACHETABLE ct, PAIR p) {
+static void cachetable_maybe_remove_and_free_pair (CACHETABLE ct, PAIR p, BOOL* destroyed) {
+    *destroyed = FALSE;
     if (rwlock_users(&p->rwlock) == 0) {
         cachetable_remove_pair(ct, p);
 
@@ -1094,6 +1095,7 @@ static void cachetable_maybe_remove_and_free_pair (CACHETABLE ct, PAIR p) {
         rwlock_read_unlock(&cachefile->fdlock);
 
         ctpair_destroy(p);
+        *destroyed = TRUE;
     }
 }
 
@@ -1164,7 +1166,7 @@ static int cachetable_fetch_pair(
     }
 }
 
-static void cachetable_complete_write_pair (CACHETABLE ct, PAIR p, BOOL do_remove);
+static void cachetable_complete_write_pair (CACHETABLE ct, PAIR p, BOOL do_remove, BOOL* destroyed);
 
 // Write a pair to storage
 // Effects: an exclusive lock on the pair is obtained, the write callback is called,
@@ -1216,21 +1218,24 @@ static void cachetable_write_pair(CACHETABLE ct, PAIR p, BOOL remove_me) {
     // otherwise complete the write now
     if (p->cq)
         workqueue_enq(p->cq, &p->asyncwork, 1);
-    else
-        cachetable_complete_write_pair(ct, p, remove_me);
+    else {
+        BOOL destroyed;
+        cachetable_complete_write_pair(ct, p, p->remove_me, &destroyed);
+    }
 }
 
 // complete the write of a pair by reseting the writing flag, and 
 // maybe removing the pair from the cachetable if there are no
 // references to it
 
-static void cachetable_complete_write_pair (CACHETABLE ct, PAIR p, BOOL do_remove) {
+static void cachetable_complete_write_pair (CACHETABLE ct, PAIR p, BOOL do_remove, BOOL* destroyed) {
     p->cq = 0;
     p->state = CTPAIR_IDLE;
     
     rwlock_write_unlock(&p->rwlock);
-    if (do_remove)
-        cachetable_maybe_remove_and_free_pair(ct, p);
+    if (do_remove) {
+        cachetable_maybe_remove_and_free_pair(ct, p, destroyed);
+    }
 }
 
 
@@ -2376,7 +2381,8 @@ static int cachetable_flush_cachefile(CACHETABLE ct, CACHEFILE cf) {
         p->cq = 0;
         if (p->state == CTPAIR_READING || p->state == CTPAIR_WRITING) { //Some other thread owned the lock, but transferred ownership to the thread executing this function
             rwlock_write_unlock(&p->rwlock);  //Release the lock, no one has a pin.
-            cachetable_maybe_remove_and_free_pair(ct, p);
+            BOOL destroyed;
+            cachetable_maybe_remove_and_free_pair(ct, p, &destroyed);
         } else if (p->state == CTPAIR_INVALID) {
             abort_fetch_pair(p);
         } else
@@ -2478,13 +2484,19 @@ int toku_cachetable_unpin_and_remove (CACHEFILE cachefile, CACHEKEY key) {
                     assert(rwlock_writers(&p->rwlock) == 1);
                     assert(rwlock_readers(&p->rwlock) == 0);
                     assert(rwlock_blocked_readers(&p->rwlock) == 0);
-                    cachetable_complete_write_pair(ct, p, TRUE);
+                    BOOL destroyed = FALSE;
+                    cachetable_complete_write_pair(ct, p, TRUE, &destroyed);
+                    if (destroyed) {
+                        break;
+                    }
                 }
                 workqueue_destroy(&cq);
             }
             else {
                 //Remove pair.
-                cachetable_maybe_remove_and_free_pair(ct, p);
+                BOOL destroyed = FALSE;;
+                cachetable_maybe_remove_and_free_pair(ct, p, &destroyed);
+                assert(destroyed);
             }
             r = 0;
 	    goto done;

newbrt/tests/cachetable-unpin-remove-and-checkpoint.c

+#ident "$Id: cachetable-simple-verify.c 34757 2011-09-14 19:12:42Z leifwalsh $"
+#ident "Copyright (c) 2007-2011 Tokutek Inc.  All rights reserved."
+#include "includes.h"
+#include "test.h"
+
+CACHETABLE ct;
+
+//
+// This test exposed a bug (#3970) caught only by Valgrind.
+// freed memory was being accessed by toku_cachetable_unpin_and_remove
+//
+
+static void
+flush (CACHEFILE f __attribute__((__unused__)),
+       int UU(fd),
+       CACHEKEY k  __attribute__((__unused__)),
+       void *v     __attribute__((__unused__)),
+       void *e     __attribute__((__unused__)),
+       long s      __attribute__((__unused__)),
+       BOOL w      __attribute__((__unused__)),
+       BOOL keep   __attribute__((__unused__)),
+       BOOL c      __attribute__((__unused__))
+       ) {
+}
+
+static int
+fetch (CACHEFILE f        __attribute__((__unused__)),
+       int UU(fd),
+       CACHEKEY k         __attribute__((__unused__)),
+       u_int32_t fullhash __attribute__((__unused__)),
+       void **value       __attribute__((__unused__)),
+       long *sizep        __attribute__((__unused__)),
+       int  *dirtyp,
+       void *extraargs    __attribute__((__unused__))
+       ) {
+  *dirtyp = 0;
+  *value = NULL;
+  *sizep = 8;
+  return 0;
+}
+
+static void 
+pe_est_callback(
+    void* UU(brtnode_pv), 
+    long* bytes_freed_estimate, 
+    enum partial_eviction_cost *cost, 
+    void* UU(write_extraargs)
+    )
+{
+    *bytes_freed_estimate = 0;
+    *cost = PE_CHEAP;
+}
+
+static int 
+pe_callback (
+    void *brtnode_pv __attribute__((__unused__)), 
+    long bytes_to_free __attribute__((__unused__)), 
+    long* bytes_freed, 
+    void* extraargs __attribute__((__unused__))
+    ) 
+{
+    *bytes_freed = bytes_to_free;
+    return 0;
+}
+static BOOL pf_req_callback(void* UU(brtnode_pv), void* UU(read_extraargs)) {
+  return FALSE;
+}
+
+static int pf_callback(void* UU(brtnode_pv), void* UU(read_extraargs), int UU(fd), long* UU(sizep)) {
+  assert(FALSE);
+}
+
+static void fake_ydb_lock(void) {
+}
+
+static void fake_ydb_unlock(void) {
+}
+
+static void *run_end_chkpt(void *arg) {
+    assert(arg == NULL);
+    int r = toku_cachetable_end_checkpoint(
+        ct, 
+        NULL, 
+        fake_ydb_lock,
+        fake_ydb_unlock,
+        NULL,
+        NULL
+        );
+    assert(r==0);
+    return arg;
+}
+
+static int
+dummy_note_pin_by_checkpoint(CACHEFILE UU(cf), void* UU(extra)){
+  return 0;
+}
+
+static void
+run_test (void) {
+    const int test_limit = 12;
+    int r;
+    ct = NULL;
+    r = toku_create_cachetable(&ct, test_limit, ZERO_LSN, NULL_LOGGER); assert(r == 0);
+    char fname1[] = __FILE__ "test1.dat";
+    unlink(fname1);
+    CACHEFILE f1;
+    r = toku_cachetable_openf(&f1, ct, fname1, O_RDWR|O_CREAT, S_IRWXU|S_IRWXG|S_IRWXO); assert(r == 0);
+
+    toku_cachefile_set_userdata (f1,
+				 NULL,
+				 NULL,
+				 NULL,
+				 NULL,
+				 NULL,
+				 NULL,
+				 NULL,
+				 dummy_note_pin_by_checkpoint,
+				 dummy_note_pin_by_checkpoint
+                                 );
+
+    void* v1;
+    //void* v2;
+    long s1;
+    //long s2;
+    r = toku_cachetable_get_and_pin(f1, make_blocknum(1), toku_cachetable_hash(f1, make_blocknum(1)), &v1, &s1, flush, fetch, 
+				    pe_est_callback, pe_callback, pf_req_callback, pf_callback, 0, 0);
+    CKERR(r);
+    toku_cachetable_unpin(
+        f1, 
+        make_blocknum(1), 
+        toku_cachetable_hash(f1, make_blocknum(1)),
+        CACHETABLE_DIRTY,
+        8
+        );
+
+    // now this should mark the pair for checkpoint
+    r = toku_cachetable_begin_checkpoint(ct, NULL);
+
+    r = toku_cachetable_get_and_pin(f1, make_blocknum(1), toku_cachetable_hash(f1, make_blocknum(1)), &v1, &s1, flush, fetch,
+				    pe_est_callback, pe_callback, pf_req_callback, pf_callback, 0, 0);
+    CKERR(r);
+
+    toku_cachetable_unpin(
+        f1, 
+        make_blocknum(1), 
+        toku_cachetable_hash(f1, make_blocknum(1)),
+        CACHETABLE_DIRTY,
+        8
+        );
+    r = toku_cachetable_get_and_pin(f1, make_blocknum(1), toku_cachetable_hash(f1, make_blocknum(1)), &v1, &s1, flush, fetch,
+				    pe_est_callback, pe_callback, pf_req_callback, pf_callback, 0, 0);
+    CKERR(r);
+
+    toku_pthread_t mytid;
+    r = toku_pthread_create(&mytid, NULL, run_end_chkpt, NULL);
+    assert(r==0);
+
+    // give checkpoint thread a chance to start waiting on lock
+    sleep(1);
+    r = toku_cachetable_unpin_and_remove(f1, make_blocknum(1));
+    assert(r==0);
+
+    void* ret;
+    r = toku_pthread_join(mytid, &ret);
+    assert(r==0);
+    
+    toku_cachetable_verify(ct);
+    r = toku_cachefile_close(&f1, 0, FALSE, ZERO_LSN); assert(r == 0 && f1 == 0);
+    r = toku_cachetable_close(&ct); lazy_assert_zero(r);
+    
+    
+}
+
+int
+test_main(int argc, const char *argv[]) {
+  default_parse_args(argc, argv);
+  run_test();
+  return 0;
+}