This patch should address the mmap problems. What it does is ditch the
ownership flag and replace it with something resembling a reference count.
It's not actually a refcount because .. where would you store the
refcount? I talked with a few of the guys about that possibility but
everything we came up with was kind of ugly. So an alternative that
somebody (Sander?) came up with was to put all of the dup'ed apr_mmap_t's
that refer to the same mmap'ed region in a ring with each other... that
way you don't have to have a single centralized location for the refcount
and you don't have lifetime issues to worry about anymore.
Greg Ames did some testing on this and says it looks good as far as he can
tell... additional eyes would be appreciated.
--Cliff
Index: srclib/apr/include/apr_mmap.h
===================================================================
RCS file: /home/cvs/apr/include/apr_mmap.h,v
retrieving revision 1.33
diff -u -d -r1.33 apr_mmap.h
--- srclib/apr/include/apr_mmap.h 10 Nov 2002 08:35:16 -0000 1.33
+++ srclib/apr/include/apr_mmap.h 21 Nov 2002 19:52:30 -0000
@@ -58,6 +58,7 @@
#include "apr.h"
#include "apr_pools.h"
#include "apr_errno.h"
+#include "apr_ring.h"
#include "apr_file_io.h" /* for apr_file_t */
#ifdef BEOS
@@ -115,8 +116,12 @@
void *mm;
/** The amount of data in the mmap */
apr_size_t size;
- /** Whether this object is reponsible for doing the munmap */
- int is_owner;
+ /** @deprecated this field is no longer used and will be removed
+ * in APR 1.0 */
+ int unused;
+ /** ring of apr_mmap_t's that reference the same
+ * mmap'ed region; acts in place of a reference count */
+ APR_RING_ENTRY(apr_mmap_t) link;
};
#if APR_HAS_MMAP || defined(DOXYGEN)
@@ -174,8 +179,8 @@
* @param new_mmap The structure to duplicate into.
* @param old_mmap The mmap to duplicate.
* @param p The pool to use for new_mmap.
- * @param transfer_ownership Whether responsibility for destroying
- * the memory-mapped segment is transferred from old_mmap to new_mmap
+ * @param transfer_ownership DEPRECATED: this param is now ignored
+ * and should be removed prior to APR 1.0
*/
APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
apr_mmap_t *old_mmap,
@@ -185,10 +190,11 @@
#if defined(DOXYGEN)
/**
* Transfer the specified MMAP to a different pool
- * @param new_mmap The structure to duplicate into.
+ * @param new_mmap The structure to duplicate into.
* @param old_mmap The file to transfer.
* @param p The pool to use for new_mmap.
- * @remark After this call, old_mmap cannot be used
+ * @deprecated Just use apr_mmap_dup(). The transfer_ownership flag will
+ * go away soon anyway.
*/
APR_DECLARE(apr_status_t) apr_mmap_setaside(apr_mmap_t **new_mmap,
apr_mmap_t *old_mmap,
@@ -196,7 +202,6 @@
#else
#define apr_mmap_setaside(new_mmap, old_mmap, p) apr_mmap_dup(new_mmap,
old_mmap, p, 1)
#endif /* DOXYGEN */
-
/**
* Remove a mmap'ed.
Index: srclib/apr/mmap/unix/mmap.c
===================================================================
RCS file: /home/cvs/apr/mmap/unix/mmap.c,v
retrieving revision 1.43
diff -u -d -r1.43 mmap.c
--- srclib/apr/mmap/unix/mmap.c 16 Apr 2002 20:25:57 -0000 1.43
+++ srclib/apr/mmap/unix/mmap.c 21 Nov 2002 19:52:30 -0000
@@ -83,13 +83,17 @@
static apr_status_t mmap_cleanup(void *themmap)
{
apr_mmap_t *mm = themmap;
- int rv;
+ apr_mmap_t *next = APR_RING_NEXT(mm,link);
+ int rv = 0;
- if (!mm->is_owner) {
- return APR_EINVAL;
- }
- else if (mm->mm == (void *)-1) {
- return APR_ENOENT;
+ /* we no longer refer to the mmaped region */
+ APR_RING_REMOVE(mm,link);
+ APR_RING_NEXT(mm,link) = NULL;
+ APR_RING_PREV(mm,link) = NULL;
+
+ if (next != mm) {
+ /* more references exist, so we're done */
+ return APR_SUCCESS;
}
#ifdef BEOS
@@ -163,7 +167,7 @@
(*new)->mm = mm;
(*new)->size = size;
(*new)->cntxt = cont;
- (*new)->is_owner = 1;
+ APR_RING_ELEM_INIT(*new, link);
/* register the cleanup... */
apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
@@ -179,21 +183,10 @@
*new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
(*new_mmap)->cntxt = p;
- /* The old_mmap can transfer ownership only if the old_mmap itself
- * is an owner of the mmap'ed segment.
- */
- if (old_mmap->is_owner) {
- if (transfer_ownership) {
- (*new_mmap)->is_owner = 1;
- old_mmap->is_owner = 0;
- apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
- apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
- apr_pool_cleanup_null);
- }
- else {
- (*new_mmap)->is_owner = 0;
- }
- }
+ APR_RING_INSERT_AFTER(old_mmap, *new_mmap, link);
+
+ apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
+ apr_pool_cleanup_null);
return APR_SUCCESS;
}
Index: srclib/apr/mmap/win32/mmap.c
===================================================================
RCS file: /home/cvs/apr/mmap/win32/mmap.c,v
retrieving revision 1.14
diff -u -d -r1.14 mmap.c
--- srclib/apr/mmap/win32/mmap.c 18 Apr 2002 20:26:40 -0000 1.14
+++ srclib/apr/mmap/win32/mmap.c 21 Nov 2002 19:52:30 -0000
@@ -66,13 +66,17 @@
static apr_status_t mmap_cleanup(void *themmap)
{
apr_mmap_t *mm = themmap;
+ apr_mmap_t *next = APR_RING_NEXT(mm,link);
apr_status_t rv = 0;
- if (!mm->is_owner) {
- return APR_EINVAL;
- }
- else if (!mm->mhandle) {
- return APR_ENOENT;
+ /* we no longer refer to the mmaped region */
+ APR_RING_REMOVE(mm,link);
+ APR_RING_NEXT(mm,link) = NULL;
+ APR_RING_PREV(mm,link) = NULL;
+
+ if (next != mm) {
+ /* more references exist, so we're done */
+ return APR_SUCCESS;
}
if (mm->mv) {
@@ -163,7 +167,7 @@
(*new)->mm = (char*)((*new)->mv) + (*new)->poffset;
(*new)->size = size;
(*new)->cntxt = cont;
- (*new)->is_owner = 1;
+ APR_RING_ELEM_INIT(*new, link);
/* register the cleanup... */
apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
@@ -179,21 +183,10 @@
*new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
(*new_mmap)->cntxt = p;
- /* The old_mmap can transfer ownership only if the old_mmap itself
- * is an owner of the mmap'ed segment.
- */
- if (old_mmap->is_owner) {
- if (transfer_ownership) {
- (*new_mmap)->is_owner = 1;
- old_mmap->is_owner = 0;
- apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
- apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
- apr_pool_cleanup_null);
- }
- else {
- (*new_mmap)->is_owner = 0;
- }
- }
+ APR_RING_INSERT_AFTER(old_mmap, *new_mmap, link);
+
+ apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
+ apr_pool_cleanup_null);
return APR_SUCCESS;
}
Index: srclib/apr-util/buckets/apr_buckets_mmap.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_mmap.c,v
retrieving revision 1.52
diff -u -d -r1.52 apr_buckets_mmap.c
--- srclib/apr-util/buckets/apr_buckets_mmap.c 2 Jun 2002 19:54:49 -0000
1.52
+++ srclib/apr-util/buckets/apr_buckets_mmap.c 21 Nov 2002 19:52:30 -0000
@@ -62,9 +62,9 @@
apr_bucket_mmap *m = b->data;
apr_status_t ok;
void *addr;
-
+
if (!m->mmap) {
- /* due to cleanup issues we have no choice but to check this */
+ /* the apr_mmap_t was already cleaned up out from under us */
return APR_EINVAL;
}
@@ -79,10 +79,11 @@
static apr_status_t mmap_bucket_cleanup(void *data)
{
- /* the mmap has disappeared out from under us. we have no choice
- * but to invalidate this bucket. from now on, all you can do to this
- * bucket is destroy it, not read from it.
- */
+ /* the apr_mmap_t is about to disappear out from under us, so we
+ * have no choice but to pretend it doesn't exist anymore. the
+ * refcount is now useless because there's nothing to refer to
+ * anymore. so the only valid action on any remaining referrer
+ * is to delete it. no more reads, no more anything. */
apr_bucket_mmap *m = data;
m->mmap = NULL;
@@ -94,7 +95,7 @@
apr_bucket_mmap *m = data;
if (apr_bucket_shared_destroy(m)) {
- if (m->mmap && m->mmap->is_owner) {
+ if (m->mmap) {
apr_pool_cleanup_kill(m->mmap->cntxt, m, mmap_bucket_cleanup);
apr_mmap_delete(m->mmap);
}
@@ -114,10 +115,8 @@
m = apr_bucket_alloc(sizeof(*m), b->list);
m->mmap = mm;
- if (mm->is_owner) {
- apr_pool_cleanup_register(mm->cntxt, m, mmap_bucket_cleanup,
- apr_pool_cleanup_null);
- }
+ apr_pool_cleanup_register(mm->cntxt, m, mmap_bucket_cleanup,
+ apr_pool_cleanup_null);
b = apr_bucket_shared_make(b, m, start, length);
b->type = &apr_bucket_type_mmap;
@@ -139,33 +138,44 @@
return apr_bucket_mmap_make(b, mm, start, length);
}
-static apr_status_t mmap_bucket_setaside(apr_bucket *data, apr_pool_t *p)
+static apr_status_t mmap_bucket_setaside(apr_bucket *b, apr_pool_t *p)
{
- apr_bucket_mmap *m = data->data;
+ apr_bucket_mmap *m = b->data;
apr_mmap_t *mm = m->mmap;
+ apr_bucket_mmap *new_m;
apr_mmap_t *new_mm;
apr_status_t ok;
if (!mm) {
- /* due to cleanup issues we have no choice but to check this */
+ /* the apr_mmap_t was already cleaned up out from under us */
return APR_EINVAL;
}
+ /* shortcut if possible */
if (apr_pool_is_ancestor(mm->cntxt, p)) {
return APR_SUCCESS;
}
+ /* duplicate apr_mmap_t into new pool */
+ /* XXX: the transfer_ownership flag on this call
+ * will go away soon.. it's ignored right now. */
ok = apr_mmap_dup(&new_mm, mm, p, 1);
if (ok != APR_SUCCESS) {
return ok;
}
-
- m->mmap = new_mm;
- if (new_mm->is_owner) {
- apr_pool_cleanup_kill(mm->cntxt, m, mmap_bucket_cleanup);
- apr_pool_cleanup_register(new_mm->cntxt, m, mmap_bucket_cleanup,
- apr_pool_cleanup_null);
- }
+
+ /* decrement refcount on old apr_bucket_mmap */
+ mmap_bucket_destroy(mm);
+
+ /* create new apr_bucket_mmap pointing to new apr_mmap_t */
+ new_m = apr_bucket_alloc(sizeof(*new_m), b->list);
+ new_m->refcount.refcount = 1;
+ new_m->mmap = new_mm;
+ apr_pool_cleanup_register(new_mm->cntxt, new_m, mmap_bucket_cleanup,
+ apr_pool_cleanup_null);
+
+ /* this bucket uses the new apr_mmap_t and its apr_bucket_mmap now */
+ b->data = new_m;
return APR_SUCCESS;
}
Index: modules/cache/mod_file_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/cache/mod_file_cache.c,v
retrieving revision 1.73
diff -u -d -r1.73 mod_file_cache.c
--- modules/cache/mod_file_cache.c 9 Jun 2002 03:44:03 -0000 1.73
+++ modules/cache/mod_file_cache.c 21 Nov 2002 19:54:05 -0000
@@ -249,14 +249,12 @@
return;
}
apr_file_close(fd);
- /* We want to cache an apr_mmap_t that's marked as "non-owner"
- * to pass to each request so that mmap_setaside()'s call to
- * apr_mmap_dup() will never try to move the apr_mmap_t to a
- * different pool. This apr_mmap_t is already going to live
- * longer than any request, but mmap_setaside() has no way to
- * know that because it's allocated out of cmd->pool,
- * which is disjoint from r->pool.
- */
+ /* We want to cache a duplicate apr_mmap_t to pass to each
+ * request so that nothing in the request will ever think that
+ * it's allowed to delete the mmap, since the "refcount" will
+ * never reach zero. */
+ /* XXX: the transfer_ownership flag on this call
+ * will go away soon.. it's ignored right now. */
apr_mmap_dup(&new_file->mm, mm, cmd->pool, 0);
new_file->is_mmapped = TRUE;
}
