lib/tdb: fix OpenBSD incoherent mmap.

This comment appears in two places in the code (commit
4c6a8273c6dd3e2aeda5a63c4a62aa55bc133099 from 2001):

	/*
	 * We must ensure the file is unmapped before doing this
	 * to ensure consistency with systems like OpenBSD where
	 * writes and mmaps are not consistent.
	 */

But this doesn't help, because if one process is using mmap and another
using pwrite, we get incoherent results.  As demonstrated by OpenBSD's
failure on the tdb unit tests.

Rather than disable mmap on OpenBSD, we test for this issue and force mmap
to be enabled.  This means that we will fail on very large TDBs on 32-bit
systems, but it's better than the horrendous performance penalty on every
OpenBSD system.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/lib/tdb/common/io.c b/lib/tdb/common/io.c
index ac21e3f67a1b00c30f0cb354e72a46ca52a015ac..24c2d615aba14fb73deecb619659b7419893636d 100644 (file)
--- a/lib/tdb/common/io.c
+++ b/lib/tdb/common/io.c
@@ -88,8 +88,7 @@ static int tdb_oob(struct tdb_context *tdb, tdb_off_t off, tdb_len_t len,
                return -1;
        }
        tdb->map_size = st.st_size;
-       tdb_mmap(tdb);
-       return 0;
+       return tdb_mmap(tdb);
 }
 
 /* write a lump of data at a specified offset */
@@ -111,6 +110,10 @@ static int tdb_write(struct tdb_context *tdb, tdb_off_t off,
        if (tdb->map_ptr) {
                memcpy(off + (char *)tdb->map_ptr, buf, len);
        } else {
+#ifdef HAVE_INCOHERENT_MMAP
+               tdb->ecode = TDB_ERR_IO;
+               return -1;
+#else
                ssize_t written = pwrite(tdb->fd, buf, len, off);
                if ((written != (ssize_t)len) && (written != -1)) {
                        /* try once more */
@@ -135,6 +138,7 @@ static int tdb_write(struct tdb_context *tdb, tdb_off_t off,
                                 len, off));
                        return -1;
                }
+#endif
        }
        return 0;
 }
@@ -160,6 +164,10 @@ static int tdb_read(struct tdb_context *tdb, tdb_off_t off, void *buf,
        if (tdb->map_ptr) {
                memcpy(buf, off + (char *)tdb->map_ptr, len);
        } else {
+#ifdef HAVE_INCOHERENT_MMAP
+               tdb->ecode = TDB_ERR_IO;
+               return -1;
+#else
                ssize_t ret = pread(tdb->fd, buf, len, off);
                if (ret != (ssize_t)len) {
                        /* Ensure ecode is set for log fn. */
@@ -170,6 +178,7 @@ static int tdb_read(struct tdb_context *tdb, tdb_off_t off, void *buf,
                                 (int)tdb->map_size));
                        return -1;
                }
+#endif
        }
        if (cv) {
                tdb_convert(buf, len);
@@ -222,13 +231,23 @@ int tdb_munmap(struct tdb_context *tdb)
        return 0;
 }
 
-void tdb_mmap(struct tdb_context *tdb)
+/* If mmap isn't coherent, *everyone* must always mmap. */
+static bool should_mmap(const struct tdb_context *tdb)
+{
+#ifdef HAVE_INCOHERENT_MMAP
+       return true;
+#else
+       return !(tdb->flags & TDB_NOMMAP);
+#endif
+}
+
+int tdb_mmap(struct tdb_context *tdb)
 {
        if (tdb->flags & TDB_INTERNAL)
                return;
 
 #ifdef HAVE_MMAP
-       if (!(tdb->flags & TDB_NOMMAP)) {
+       if (should_mmap(tdb)) {
                tdb->map_ptr = mmap(NULL, tdb->map_size, 
                                    PROT_READ|(tdb->read_only? 0:PROT_WRITE), 
                                    MAP_SHARED|MAP_FILE, tdb->fd, 0);
@@ -241,6 +260,10 @@ void tdb_mmap(struct tdb_context *tdb)
                        tdb->map_ptr = NULL;
                        TDB_LOG((tdb, TDB_DEBUG_WARNING, "tdb_mmap failed for size %d (%s)\n", 
                                 tdb->map_size, strerror(errno)));
+#ifdef HAVE_INCOHERENT_MMAP
+                       tdb->ecode = TDB_ERR_IO;
+                       return -1;
+#endif
                }
        } else {
                tdb->map_ptr = NULL;
@@ -248,6 +271,7 @@ void tdb_mmap(struct tdb_context *tdb)
 #else
        tdb->map_ptr = NULL;
 #endif
+       return 0;
 }
 
 /* expand a file.  we prefer to use ftruncate, as that is what posix
@@ -360,12 +384,6 @@ int tdb_expand(struct tdb_context *tdb, tdb_off_t size)
        if (!(tdb->flags & TDB_INTERNAL))
                tdb_munmap(tdb);
 
-       /*
-        * We must ensure the file is unmapped before doing this
-        * to ensure consistency with systems like OpenBSD where
-        * writes and mmaps are not consistent.
-        */
-
        /* expand the file itself */
        if (!(tdb->flags & TDB_INTERNAL)) {
                if (tdb->methods->tdb_expand_file(tdb, tdb->map_size, size) != 0)
@@ -383,14 +401,9 @@ int tdb_expand(struct tdb_context *tdb, tdb_off_t size)
                }
                tdb->map_ptr = new_map_ptr;
        } else {
-               /*
-                * We must ensure the file is remapped before adding the space
-                * to ensure consistency with systems like OpenBSD where
-                * writes and mmaps are not consistent.
-                */
-
-               /* We're ok if the mmap fails as we'll fallback to read/write */
-               tdb_mmap(tdb);
+               if (tdb_mmap(tdb) != 0) {
+                       goto fail;
+               }
        }
 
        /* form a new freelist record */

diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index 2965ea77b5a26c4cd12c60a548b69a218012e54c..8836f8442ee72c0c61bd971639e7a00396145a1c 100644 (file)
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -587,7 +587,9 @@ static int tdb_reopen_internal(struct tdb_context *tdb, bool active_lock)
                TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_reopen: file dev/inode has changed!\n"));
                goto fail;
        }
-       tdb_mmap(tdb);
+       if (tdb_mmap(tdb) != 0) {
+               goto fail;
+       }
 #endif /* fake pread or pwrite */
 
        /* We may still think we hold the active lock. */

diff --git a/lib/tdb/common/tdb_private.h b/lib/tdb/common/tdb_private.h
index 9913284f3c7bcad8fc0dbc6dd15ca1c6e2465743..0441fb287fdf067645f4ce0d2845b111a554705e 100644 (file)
--- a/lib/tdb/common/tdb_private.h
+++ b/lib/tdb/common/tdb_private.h
@@ -222,7 +222,7 @@ struct tdb_context {
   internal prototypes
 */
 int tdb_munmap(struct tdb_context *tdb);
-void tdb_mmap(struct tdb_context *tdb);
+int tdb_mmap(struct tdb_context *tdb);
 int tdb_lock(struct tdb_context *tdb, int list, int ltype);
 int tdb_lock_nonblock(struct tdb_context *tdb, int list, int ltype);
 int tdb_nest_lock(struct tdb_context *tdb, uint32_t offset, int ltype,

diff --git a/lib/tdb/test/run-3G-file.c b/lib/tdb/test/run-3G-file.c
index cc700c267655efa494ff1fcd2d0583147adff7c5..3b4e45739a115c7cac484fbe1b24ee184b700018 100644 (file)
--- a/lib/tdb/test/run-3G-file.c
+++ b/lib/tdb/test/run-3G-file.c
@@ -70,6 +70,7 @@ int main(int argc, char *argv[])
        uint32_t hash;
        tdb_off_t rec_ptr;
        struct tdb_record rec;
+       int ret;
 
        plan_tests(24);
        tdb = tdb_open_ex("run-36-file.tdb", 1024, TDB_CLEAR_IF_FIRST,
@@ -78,15 +79,33 @@ int main(int argc, char *argv[])
        ok1(tdb);
        tdb->methods = &large_io_methods;
 
-       /* Enlarge the file (internally multiplies by 2). */
-       ok1(tdb_expand(tdb, 1500000000) == 0);
-
-       /* Put an entry in, and check it. */
        key.dsize = strlen("hi");
        key.dptr = (void *)"hi";
        orig_data.dsize = strlen("world");
        orig_data.dptr = (void *)"world";
 
+       /* Enlarge the file (internally multiplies by 2). */
+       ret = tdb_expand(tdb, 1500000000);
+#ifdef HAVE_INCOHERENT_MMAP
+       /* This can fail due to mmap failure on 32 bit systems. */
+       if (ret == -1) {
+               /* These should now fail. */
+               ok1(tdb_store(tdb, key, orig_data, TDB_INSERT) == -1);
+               data = tdb_fetch(tdb, key);
+               ok1(data.dptr == NULL);
+               ok1(tdb_traverse(tdb, test_traverse, &orig_data) == -1);
+               ok1(tdb_delete(tdb, key) == -1);
+               ok1(tdb_traverse(tdb, test_traverse, NULL) == -1);
+               /* Skip the rest... */
+               for (ret = 0; ret < 24 - 6; ret++)
+                       ok1(1);
+               tdb_close(tdb);
+               return exit_status();
+       }
+#endif
+       ok1(ret == 0);
+
+       /* Put an entry in, and check it. */
        ok1(tdb_store(tdb, key, orig_data, TDB_INSERT) == 0);
 
        data = tdb_fetch(tdb, key);