cache, GC: improve handling of LMDB maxsize

This version seems to work OK.  Unfortunately we had to resort to
an extra write and cache reopening when attempting to set cache size.
And even so, decreasing the size can't really be done, so we only warn
about failing to do that.

diff --git a/lib/cache/api.c b/lib/cache/api.c
index cb0ff392c1777979d4e64cf5bb6db83c9db3a619..1a9781bf1ebfc7e8e0a4dd611442c60983cc42e7 100644 (file)
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -63,7 +63,7 @@ static ssize_t stash_rrset(struct kr_cache *cache, const struct kr_query *qry,
 /** Preliminary checks before stash_rrset().  Don't call if returns <= 0. */
 static int stash_rrset_precond(const knot_rrset_t *rr, const struct kr_query *qry/*logs*/);
 
-/** @internal Open cache db transaction and check internal data version. */
+/** @internal Ensure the cache version is right, possibly by clearing it. */
 static int assert_right_version(struct kr_cache *cache)
 {
        /* Check cache ABI version. */
@@ -93,12 +93,14 @@ static int assert_right_version(struct kr_cache *cache)
                        ret = cache_op(cache, clear);
                }
                /* Either purged or empty. */
-               if (ret == 0) {
-                       /* Key/Val is invalidated by cache purge, recreate it */
-                       val.data = /*const-cast*/(void *)&CACHE_VERSION;
-                       val.len = sizeof(CACHE_VERSION);
-                       ret = cache_op(cache, write, &key, &val, 1);
-               }
+       }
+       /* Rewrite the entry even if it isn't needed.  Because of cache-size-changing
+        * possibility it's good to always perform some write during opening of cache. */
+       if (ret == 0) {
+               /* Key/Val is invalidated by cache purge, recreate it */
+               val.data = /*const-cast*/(void *)&CACHE_VERSION;
+               val.len = sizeof(CACHE_VERSION);
+               ret = cache_op(cache, write, &key, &val, 1);
        }
        kr_cache_commit(cache);
        return ret;
@@ -117,14 +119,33 @@ int kr_cache_open(struct kr_cache *cache, const struct kr_cdb_api *api, struct k
        }
        cache->api = api;
        int ret = cache->api->open(&cache->db, &cache->stats, opts, mm);
+       if (ret == 0) {
+               ret = assert_right_version(cache);
+               // The included write also committed maxsize increase to the file.
+       }
+       if (ret == 0 && opts->maxsize) {
+               /* If some maxsize is requested and it's smaller than in-file maxsize,
+                * LMDB only restricts our env without changing the in-file maxsize.
+                * That is worked around by reopening (found no other reliable way). */
+               cache->api->close(cache->db, &cache->stats);
+               struct kr_cdb_opts opts2;
+               memcpy(&opts2, opts, sizeof(opts2));
+               opts2.maxsize = 0;
+               ret = cache->api->open(&cache->db, &cache->stats, &opts2, mm);
+       }
+       if (ret == 0 && opts->maxsize) {
+               size_t maxsize = cache->api->get_maxsize(cache->db);
+               if (maxsize > opts->maxsize) kr_log_info(
+                       "[cache] Warning: cache size %zu instead of %zu."
+                       "  To reduce the size you need to remove the file by hand.\n",
+                       maxsize, opts->maxsize);
+       }
        if (ret != 0) {
                return ret;
        }
        cache->ttl_min = KR_CACHE_DEFAULT_TTL_MIN;
        cache->ttl_max = KR_CACHE_DEFAULT_TTL_MAX;
-       /* Check cache ABI version */
        kr_cache_make_checkpoint(cache);
-       (void)assert_right_version(cache);
 
        char *fpath;
        ret = asprintf(&fpath, "%s/data.mdb", opts->path);

diff --git a/lib/cache/cdb_api.h b/lib/cache/cdb_api.h
index 935945f4261c5a75d06fb824d40b5cde0206c8a2..546964fde9a871bc0d5fccd7fd8f687e372e30d0 100644 (file)
--- a/lib/cache/cdb_api.h
+++ b/lib/cache/cdb_api.h
@@ -11,7 +11,7 @@
 /* Cache options. */
 struct kr_cdb_opts {
        const char *path; /*!< Cache URI path. */
-       size_t maxsize;   /*!< Suggested cache size in bytes. */
+       size_t maxsize;   /*!< Suggested cache size in bytes; pass 0 to keep unchanged/default. */
 };
 
 struct kr_cdb_stats {
@@ -79,6 +79,9 @@ struct kr_cdb_api {
 
        double (*usage_percent)(knot_db_t *db);
 
+       /** Return the current cache size limit in bytes; could be cached by check_health(). */
+       size_t (*get_maxsize)(knot_db_t *db);
+
        /** Perform maintenance.
         * In LMDB case it checks whether data.mdb is still the same
         * and reopens it if it isn't; it errors out if the file doesn't exist anymore.

diff --git a/lib/cache/cdb_lmdb.c b/lib/cache/cdb_lmdb.c
index 073305671d6d584956245dc3a160fe299b809c9a..685f2e215dda18645e014c7413b9270bb6a5b1da 100644 (file)
--- a/lib/cache/cdb_lmdb.c
+++ b/lib/cache/cdb_lmdb.c
@@ -60,6 +60,8 @@ struct libknot_lmdb_env {
 };
 
 
+static int cdb_commit(knot_db_t *db, struct kr_cdb_stats *stats);
+
 /** @brief Convert LMDB error code. */
 static int lmdb_error(int error)
 {
@@ -88,6 +90,26 @@ static inline MDB_val val_knot2mdb(knot_db_val_t v)
        return (MDB_val){ .mv_size = v.len, .mv_data = v.data };
 }
 
+/** Refresh mapsize value from file, including env->mapsize.
+ * It's much lighter than reopen_env(). */
+static int refresh_mapsize(struct lmdb_env *env)
+{
+       int ret = cdb_commit(env, NULL);
+       if (!ret) ret = lmdb_error(mdb_env_set_mapsize(env->env, 0));
+       if (ret) return ret;
+
+       MDB_envinfo info;
+       ret = lmdb_error(mdb_env_info(env->env, &info));
+       if (ret) return ret;
+
+       env->mapsize = info.me_mapsize;
+       if (env->mapsize != env->st_size) {
+               kr_log_info("[cache] suspicious size of cache file: %zu != %zu\n",
+                               (size_t)env->st_size, env->mapsize);
+       }
+       return kr_ok();
+}
+
 #define FLAG_RENEW (2*MDB_RDONLY)
 /** mdb_txn_begin or _renew + handle retries in some situations
  *
@@ -114,10 +136,9 @@ retry:
 
        if (unlikely(ret == MDB_MAP_RESIZED)) {
                kr_log_info("[cache] detected size increased by another process\n");
-               ret = mdb_env_set_mapsize(env->env, 0);
-               if (ret == MDB_SUCCESS) {
+               ret = refresh_mapsize(env);
+               if (ret == 0)
                        goto retry;
-               }
        } else if (unlikely(ret == MDB_READERS_FULL)) {
                int cleared;
                ret = mdb_reader_check(env->env, &cleared);
@@ -179,7 +200,7 @@ static int cdb_commit(knot_db_t *db, struct kr_cdb_stats *stats)
        struct lmdb_env *env = db;
        int ret = kr_ok();
        if (env->txn.rw) {
-               stats->commit++;
+               if (stats) stats->commit++;
                ret = lmdb_error(mdb_txn_commit(env->txn.rw));
                env->txn.rw = NULL; /* the transaction got freed even in case of errors */
        } else if (env->txn.ro && env->txn.ro_active) {
@@ -265,7 +286,7 @@ static void cdb_close_env(struct lmdb_env *env, struct kr_cdb_stats *stats)
 }
 
 /** We assume that *env is zeroed and we return it zeroed on errors. */
-static int cdb_open_env(struct lmdb_env *env, const char *path, size_t mapsize,
+static int cdb_open_env(struct lmdb_env *env, const char *path, const size_t mapsize,
                struct kr_cdb_stats *stats)
 {
        int ret = mkdir(path, LMDB_DIR_MODE);
@@ -288,10 +309,13 @@ static int cdb_open_env(struct lmdb_env *env, const char *path, size_t mapsize,
                ret = errno;
                goto error_sys;
        }
-       mapsize = (mapsize / pagesize) * pagesize;
-       ret = mdb_env_set_mapsize(env->env, mapsize);
-       if (ret != MDB_SUCCESS) goto error_mdb;
-       env->mapsize = mapsize;
+
+       const bool size_requested = mapsize;
+       if (size_requested) {
+               env->mapsize = (mapsize / pagesize) * pagesize;
+               ret = mdb_env_set_mapsize(env->env, env->mapsize);
+               if (ret != MDB_SUCCESS) goto error_mdb;
+       }
 
        /* Cache doesn't require durability, we can be
         * loose with the requirements as a tradeoff for speed. */
@@ -311,9 +335,13 @@ static int cdb_open_env(struct lmdb_env *env, const char *path, size_t mapsize,
        env->st_dev = st.st_dev;
        env->st_ino = st.st_ino;
        env->st_size = st.st_size;
-       if (env->st_size != env->mapsize)
-               kr_log_verbose("[cache] suspicious size of cache file: %zu != %zu\n",
-                               (size_t)env->st_size, env->mapsize);
+
+       /* Get the real mapsize.  Shrinking can be restricted, etc.
+        * Unfortunately this is only reliable when not setting the size explicitly. */
+       if (!size_requested) {
+               ret = refresh_mapsize(env);
+               if (ret) goto error_sys;
+       }
 
        /* Open the database. */
        MDB_txn *txn = NULL;
@@ -327,7 +355,11 @@ static int cdb_open_env(struct lmdb_env *env, const char *path, size_t mapsize,
        }
 
 #if !defined(__MACOSX__) && !(defined(__APPLE__) && defined(__MACH__))
-       ret = posix_fallocate(fd, 0, mapsize);
+       if (size_requested) {
+               ret = posix_fallocate(fd, 0, MAX(env->mapsize, env->st_size));
+       } else {
+               ret = 0;
+       }
        if (ret == EINVAL) {
                /* POSIX says this can happen when the feature isn't supported by the FS.
                 * We haven't seen this happen on Linux+glibc but it was reported on FreeBSD.*/
@@ -416,7 +448,7 @@ static int cdb_count(knot_db_t *db, struct kr_cdb_stats *stats)
        }
 }
 
-static int reopen_env(struct lmdb_env *env, struct kr_cdb_stats *stats)
+static int reopen_env(struct lmdb_env *env, struct kr_cdb_stats *stats, const size_t mapsize)
 {
        /* Keep copy as it points to current handle internals. */
        const char *path;
@@ -425,7 +457,6 @@ static int reopen_env(struct lmdb_env *env, struct kr_cdb_stats *stats)
                return lmdb_error(ret);
        }
        auto_free char *path_copy = strdup(path);
-       size_t mapsize = env->mapsize;
        cdb_close_env(env, stats);
        return cdb_open_env(env, path_copy, mapsize, stats);
 }
@@ -439,20 +470,21 @@ static int cdb_check_health(knot_db_t *db, struct kr_cdb_stats *stats)
                int ret = errno;
                return kr_error(ret);
        }
-       if (st.st_dev == env->st_dev && st.st_ino == env->st_ino) {
-               if (st.st_size == env->st_size)
-                       return kr_ok();
-               kr_log_info("[cache] detected size change by another process: %zu -> %zu\n",
-                               (size_t)env->st_size, (size_t)st.st_size);
-               int ret = cdb_commit(db, stats);
-               if (!ret) ret = lmdb_error(mdb_env_set_mapsize(env->env, st.st_size));
-               if (!ret) env->mapsize = st.st_size;
-               env->st_size = st.st_size; // avoid retrying in cycle even if it failed
-               return kr_error(ret);
+
+       if (st.st_dev != env->st_dev || st.st_ino != env->st_ino) {
+               kr_log_verbose("[cache] cache file has been replaced, reopening\n");
+               int ret = reopen_env(env, stats, 0); // we accept mapsize from the new file
+               return ret == 0 ? 1 : ret;
        }
-       kr_log_verbose("[cache] cache file has been replaced, reopening\n");
-       int ret = reopen_env(env, stats);
-       return ret == 0 ? 1 : ret;
+
+       /* Cache check through file size works OK without reopening,
+        * contrary to methods based on mdb_env_info(). */
+       if (st.st_size == env->st_size)
+               return kr_ok();
+       kr_log_info("[cache] detected file size change by another process: %zu -> %zu\n",
+                       (size_t)env->st_size, (size_t)st.st_size);
+       env->st_size = st.st_size; // avoid retrying in cycle even if we fail
+       return refresh_mapsize(env);
 }
 
 static int cdb_clear(knot_db_t *db, struct kr_cdb_stats *stats)
@@ -521,7 +553,7 @@ static int cdb_clear(knot_db_t *db, struct kr_cdb_stats *stats)
        // coverity[toctou]
        unlink(env->mdb_data_path);
        unlink(mdb_lockfile);
-       ret = reopen_env(env, stats);
+       ret = reopen_env(env, stats, env->mapsize);
        /* Environment updated, release lockfile. */
        unlink(lockfile);
        return ret;
@@ -752,6 +784,11 @@ static double cdb_usage(knot_db_t *db)
        return db_usage;
 }
 
+static size_t cdb_get_maxsize(knot_db_t *db)
+{
+       struct lmdb_env *env = db;
+       return env->mapsize;
+}
 
 /** Conversion between knot and lmdb structs. */
 knot_db_t *knot_db_t_kres2libknot(const knot_db_t * db)
@@ -777,6 +814,7 @@ const struct kr_cdb_api *kr_cdb_lmdb(void)
                cdb_match,
                cdb_read_leq,
                cdb_usage,
+               cdb_get_maxsize,
                cdb_check_health,
        };
 

diff --git a/utils/cache_gc/db.c b/utils/cache_gc/db.c
index 612316589271e208f0beae469e76592ef288b12b..573f2ba0b07bd6219ad56496d462931a9bdb3f77 100644 (file)
--- a/utils/cache_gc/db.c
+++ b/utils/cache_gc/db.c
@@ -22,9 +22,7 @@ int kr_gc_cache_open(const char *cache_path, struct kr_cache *kres_db,
                return -ENOENT;
        }
 
-       size_t cache_size = st.st_size;
-
-       struct kr_cdb_opts opts = { cache_path, cache_size };
+       struct kr_cdb_opts opts = { .path = cache_path, .maxsize = 0/*don't resize*/ };
 
        int ret = kr_cache_open(kres_db, NULL, &opts, NULL);
        if (ret || kres_db->db == NULL) {