svn commit: r1643139 - /subversion/trunk/subversion/libsvn_fs_fs/caching.c

[st...@apache.org]

Author: stefan2
Date: Fri Dec  5 00:01:54 2014
New Revision: 1643139

URL: http://svn.apache.org/viewvc?rev=1643139&view=rev
Log:
Fix the FSFS-specific txn root cleanup misbehavior demonstrated by r16.
Neither BDB nor FSX use txn-local caches.

The solution implemented here is simple: Both, FS->POOL and the pool owning
the txn, must be limit the lifetime for the TXN_DIR_CACHE to be valid.  If
one of them gets cleaned up, the cache needs to be removed as well.  So,
register one cleanup function per pool and, when one of the gets triggered,
unregister the respective other one.

* subversion/libsvn_fs_fs/caching.c
  (txn_cleanup_baton_t): Remember the pools that we registered cleanup on.
  (remove_txn_cache): Duplicate and rename to ...
  (remove_txn_cache_fs,
   remove_txn_cache_txn): ... these. Unregister the respective other cleanup.
  (init_txn_callbacks): Add FS parameter to have access to its pool.
  (svn_fs_fs__initialize_txn_caches): Update caller.

Modified:
    subversion/trunk/subversion/libsvn_fs_fs/caching.c

Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1643139&r1=1643138&r2=1643139&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Fri Dec  5 00:01:54 2014
@@ -676,12 +676,44 @@ struct txn_cleanup_baton_t
 
   /* the position where to reset it */
   svn_cache__t **to_reset;
+
+  /* pool that TXN_CACHE was allocated in */
+  apr_pool_t *txn_pool;
+
+  /* pool that the FS containing the TO_RESET pointer was allocator */
+  apr_pool_t *fs_pool;
 };
 
+/* Forward declaration. */
+static apr_status_t
+remove_txn_cache_fs(void *baton_void);
+
+/* APR pool cleanup handler that will reset the cache pointer given in
+   BATON_VOID when the TXN_POOL gets cleaned up. */
+static apr_status_t
+remove_txn_cache_txn(void *baton_void)
+{
+  struct txn_cleanup_baton_t *baton = baton_void;
+
+  /* be careful not to hurt performance by resetting newer txn's caches. */
+  if (*baton->to_reset == baton->txn_cache)
+    {
+      /* This is equivalent to calling svn_fs_fs__reset_txn_caches(). */
+      *baton->to_reset  = NULL;
+
+      /* It's cleaned up now. Prevent double cleanup. */
+      apr_pool_cleanup_kill(baton->fs_pool,
+                            baton,
+                            remove_txn_cache_fs);
+    }
+
+  return  APR_SUCCESS;
+}
+
 /* APR pool cleanup handler that will reset the cache pointer given in
-   BATON_VOID. */
+   BATON_VOID when the FS_POOL gets cleaned up. */
 static apr_status_t
-remove_txn_cache(void *baton_void)
+remove_txn_cache_fs(void *baton_void)
 {
   struct txn_cleanup_baton_t *baton = baton_void;
 
@@ -690,18 +722,24 @@ remove_txn_cache(void *baton_void)
     {
      /* This is equivalent to calling svn_fs_fs__reset_txn_caches(). */
       *baton->to_reset  = NULL;
+
+      /* It's cleaned up now. Prevent double cleanup. */
+      apr_pool_cleanup_kill(baton->txn_pool,
+                            baton,
+                            remove_txn_cache_txn);
     }
 
   return  APR_SUCCESS;
 }
 
 /* This function sets / registers the required callbacks for a given
- * transaction-specific *CACHE object, if CACHE is not NULL and a no-op
- * otherwise. In particular, it will ensure that *CACHE gets reset to NULL
- * upon POOL destruction latest.
+ * transaction-specific *CACHE object in FS, if CACHE is not NULL and
+ * a no-op otherwise. In particular, it will ensure that *CACHE gets
+ * reset to NULL upon POOL or FS->POOL destruction latest.
  */
 static void
-init_txn_callbacks(svn_cache__t **cache,
+init_txn_callbacks(svn_fs_t *fs,
+                   svn_cache__t **cache,
                    apr_pool_t *pool)
 {
   if (*cache != NULL)
@@ -711,10 +749,20 @@ init_txn_callbacks(svn_cache__t **cache,
       baton = apr_palloc(pool, sizeof(*baton));
       baton->txn_cache = *cache;
       baton->to_reset = cache;
+      baton->txn_pool = pool;
+      baton->fs_pool = fs->pool;
 
+      /* If any of these pools gets cleaned, we must reset the cache.
+       * We don't know which one will get cleaned up first, so register
+       * cleanup actions for both and during the cleanup action, unregister
+       * the respective other action. */
       apr_pool_cleanup_register(pool,
                                 baton,
-                                remove_txn_cache,
+                                remove_txn_cache_txn,
+                                apr_pool_cleanup_null);
+      apr_pool_cleanup_register(fs->pool,
+                                baton,
+                                remove_txn_cache_fs,
                                 apr_pool_cleanup_null);
     }
 }
@@ -764,7 +812,7 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
                        pool, pool));
 
   /* reset the transaction-specific cache if the pool gets cleaned up. */
-  init_txn_callbacks(&(ffd->txn_dir_cache), pool);
+  init_txn_callbacks(fs, &(ffd->txn_dir_cache), pool);
 
   return SVN_NO_ERROR;
 }