From: "Greg Stein" <[EMAIL PROTECTED]>
Sent: Friday, May 04, 2001 12:17 AM
> On Wed, May 02, 2001 at 09:42:10PM -0500, William A. Rowe, Jr. wrote:
> >...
> > I explicitly decided not to do stacked locks/refcounting. Win32 will not
> > allow you to 'promote' a sharelock into an exclusivelock. Ergo, it's not
> > portable.
> >...
> > Index: dbm/sdbm/sdbm.c
> > retrieving revision 1.16
>
> oops! That isn't the HEAD. In fact, 1.16 has the original locking patch, so
> this is really just a diff of changes in how you do the lock. Thus, it isn't
> showing what is being added to get the fine-grained locking.
Outch ... very sorry!!! New patch attached (just copied code/headers from one
tree to a current one :-)
> I do see a number of changes regarding status handling and the use of
> ioerr(). However, every single use of ioerr() is *right* before returning
> the status code. I'd say the first change is to eliminate the ioerr() macro,
> and the apr_sdbm_error_get|clear functions. We give the caller the status,
> so there is no reason for apr_sdbm to remember it. (this may simplify the
> locking patch a bit) (btw, note that SDBM_IOERR is unused)
Yes. I don't believe there is any reason to promote more 'errno/h_errno' style
entities (c.f. OS2's approach to the world.) I'd be fine with doing away with
an error tracking mechanism _we_ never retest.
> Regarding the no-stack/refcount thing. That could be problematic. Why don't
> we just say that if the user calls apr_sdbm_lock, they get an exclusive
> lock. In other words, don't worry about promotion -- just give them the
> strongest lock possible.
Because in an application that is 98% tracking, and 2% recording, you have a
whole lot of children waiting for each other.
> The *current* apr_sdbm_(un)lock functions would be
> renamed and kept private -- the "exclusive" flag is for internal use.
Ok, so you are suggesting we leave things 'simple' internally (no extra lock
if it's already locked?)
> The external function just calls it with the "exclusive" flag enable. The lock
> function can do two things: record a refcount, and record the type (so we
> can fail if an exclusive was attempted, but a shared already exists (the
> promotion thing you mention)).
I think I have a simpler suggestion that I hate. If it's going to come down
to "We Must Have Refcounted Locks" (not necessarily a bad thing, but new to any
apr entity) then perhaps your suggestion not to always lock exclusive applies
to files that are opened for r/w. If the dbm is opened r/o, then every lock
is a shared lock. That would sort of lick all the problems at once.
I personally prefer caution on the part of the user, and not refcounting locks
(if the user wants to wrap the lock/unlock fn with refcounting themselves, then
great!) This allows a r/w opened file to have either type of lock, so they can
Sharelock what they want to check up on, and excllock when they need to look up,
make a judgement/calculation, and write back their opinion.
> Another comment on the patch: I think the invalidation needs to occur at
> unlock time. As I mentioned before, a readonly operation could see that the
> cache has a "valid" page, but the bits could be stale.
>
> Hmm. Maybe it would just be best to never use the "cache" when fine-grained
> locking is enabled. (as a data buffer, yes) Maybe that is as simple as
> saying that pagbno and dirbno never get set (other than to -1), so they
> always get read in (while holding a read and/or read/write lock).
Let's say I need to lock, lookup thirty random things, write a record, and then
release the lock. Caching is _still_ a very good thing in this situation.
> I'd need to see the full patch to see how you're currently handling
> invalidation, though, to be sure.
>
> Looks like it can be done... just some details and assurances to do. And a
> lot of the latter :-)
Fair enough, here's the proper patch. Now that I've merged against head, if you
want to apply the 'no more ioerr' patch, be my guest (++1 here!)
Bill
? aprutil.plg
? libaprutil.plg
Index: dbm/sdbm/sdbm.c
===================================================================
RCS file: /home/cvs/apr-util/dbm/sdbm/sdbm.c,v
retrieving revision 1.18
diff -u -r1.18 sdbm.c
--- dbm/sdbm/sdbm.c 2001/05/02 13:37:53 1.18
+++ dbm/sdbm/sdbm.c 2001/05/04 05:41:09
@@ -115,8 +115,9 @@
{
apr_sdbm_t *db = data;
+ if (db->flags & (SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK))
+ (void) apr_sdbm_unlock(db);
(void) apr_file_close(db->dirf);
- (void) sdbm_unlock(db);
(void) apr_file_close(db->pagf);
free(db);
@@ -153,15 +154,17 @@
* If we fail anywhere, undo everything, return NULL.
*/
- if ((status = apr_file_open(&db->pagf, pagname, flags, perms, p))
+ if ((status = apr_file_open(&db->dirf, dirname, flags, perms, p))
!= APR_SUCCESS)
goto error;
- if ((status = sdbm_lock(db, !(db->flags & SDBM_RDONLY)))
+ if ((status = apr_file_open(&db->pagf, pagname, flags, perms, p))
!= APR_SUCCESS)
goto error;
- if ((status = apr_file_open(&db->dirf, dirname, flags, perms, p))
+ if ((status = apr_sdbm_lock(db, (db->flags & SDBM_RDONLY)
+ ? APR_FLOCK_SHARED
+ : APR_FLOCK_EXCLUSIVE))
!= APR_SUCCESS)
goto error;
@@ -173,12 +176,17 @@
goto error;
/*
+ * if we are opened in SHARED mode, unlock ourself
+ */
+ if (db->flags & SDBM_SHARED)
+ if ((status = apr_sdbm_unlock(db)) != APR_SUCCESS)
+ goto error;
+
+ /*
* zero size: either a fresh database, or one with a single,
* unsplit data page: dirpage is all zeros.
*/
- db->dirbno = (!finfo.size) ? 0 : -1;
- db->pagbno = -1;
- db->maxbno = finfo.size * BYTESIZ;
+ SDBM_INVALIDATE_CACHE(db, finfo);
/* (apr_pcalloc zeroed the buffers) */
@@ -190,10 +198,11 @@
return APR_SUCCESS;
error:
+ if (db->dirf && db->pagf)
+ (void) apr_sdbm_unlock(db);
if (db->dirf != NULL)
(void) apr_file_close(db->dirf);
if (db->pagf != NULL) {
- (void) sdbm_unlock(db);
(void) apr_file_close(db->pagf);
}
free(db);
@@ -219,16 +228,26 @@
apr_sdbm_datum_t key)
{
apr_status_t status;
+ int impllock = !(db->flags & (SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK));
+
if (db == NULL || bad(key))
return APR_EINVAL;
+ if (impllock)
+ if ((status = apr_sdbm_lock(db, APR_FLOCK_SHARED)) != APR_SUCCESS)
+ return ioerr(db, status);
+
if ((status = getpage(db, exhash(key))) == APR_SUCCESS) {
*val = getpair(db->pagbuf, key);
/* ### do we want a not-found result? */
return APR_SUCCESS;
}
+ else
+ ioerr(db, status);
+
+ if (impllock)
+ (void) apr_sdbm_unlock(db);
- ioerr(db, status);
return status;
}
@@ -250,28 +269,31 @@
apr_status_t apr_sdbm_delete(apr_sdbm_t *db, const apr_sdbm_datum_t key)
{
apr_status_t status;
-
+ int impllock = !(db->flags & SDBM_EXCLUSIVE_LOCK);
+
if (db == NULL || bad(key))
return APR_EINVAL;
if (apr_sdbm_rdonly(db))
return APR_EINVAL;
+ if (db->flags & SDBM_SHARED_LOCK)
+ return APR_EINVAL;
+ if (impllock)
+ if ((status = apr_sdbm_lock(db, APR_FLOCK_EXCLUSIVE)) != APR_SUCCESS)
+ return ioerr(db, status);
if ((status = getpage(db, exhash(key))) == APR_SUCCESS) {
if (!delpair(db->pagbuf, key))
/* ### should we define some APRUTIL codes? */
- return APR_EGENERAL;
-
- /*
- * update the page file
- */
- if ((status = write_page(db, db->pagbuf, db->pagbno)) != APR_SUCCESS)
- return status;
-
- return APR_SUCCESS;
+ status = APR_EGENERAL;
+ else
+ status = write_page(db, db->pagbuf, db->pagbno);
}
+ else
+ ioerr(db, status);
+ if (impllock)
+ (void) apr_sdbm_unlock(db);
- ioerr(db, status);
- return APR_EACCES;
+ return status;
}
apr_status_t apr_sdbm_store(apr_sdbm_t *db, apr_sdbm_datum_t key,
@@ -280,12 +302,12 @@
int need;
register long hash;
apr_status_t status;
+ int impllock = !(db->flags & SDBM_EXCLUSIVE_LOCK);
if (db == NULL || bad(key))
return APR_EINVAL;
if (apr_sdbm_rdonly(db))
return APR_EINVAL;
-
need = key.dsize + val.dsize;
/*
* is the pair too big (or too small) for this database ??
@@ -293,6 +315,12 @@
if (need < 0 || need > PAIRMAX)
return APR_EINVAL;
+ if (db->flags & SDBM_SHARED_LOCK)
+ return APR_EINVAL;
+ if (impllock)
+ if ((status = apr_sdbm_lock(db, APR_FLOCK_EXCLUSIVE)) != APR_SUCCESS)
+ return ioerr(db, status);
+
if ((status = getpage(db, (hash = exhash(key)))) == APR_SUCCESS) {
/*
@@ -301,27 +329,31 @@
*/
if (flags == APR_SDBM_REPLACE)
(void) delpair(db->pagbuf, key);
- else if (!(flags & APR_SDBM_INSERTDUP) && duppair(db->pagbuf, key))
- return APR_EEXIST;
+ else if (!(flags & APR_SDBM_INSERTDUP) && duppair(db->pagbuf, key)) {
+ status = APR_EEXIST;
+ goto error;
+ }
/*
* if we do not have enough room, we have to split.
*/
if (!fitpair(db->pagbuf, need))
if ((status = makroom(db, hash, need)) != APR_SUCCESS)
- return status;
+ goto error:
/*
* we have enough room or split is successful. insert the key,
* and update the page file.
*/
(void) putpair(db->pagbuf, key, val);
-
- if ((status = write_page(db, db->pagbuf, db->pagbno)) != APR_SUCCESS)
- return status;
- return APR_SUCCESS;
+ status = write_page(db, db->pagbuf, db->pagbno);
}
-
- ioerr(db, status);
+
+error:
+ if (impllock)
+ (void) apr_sdbm_unlock(db);
+
+ if (status != APR_SUCCESS)
+ ioerr(db, status);
return status;
}
@@ -433,6 +465,13 @@
*/
apr_status_t apr_sdbm_firstkey(apr_sdbm_t *db, apr_sdbm_datum_t *key)
{
+ apr_status_t status;
+ int impllock = !(db->flags & (SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK));
+
+ if (impllock)
+ if ((status = apr_sdbm_lock(db, APR_FLOCK_SHARED)) != APR_SUCCESS)
+ return ioerr(db, status);
+
/*
* start at page 0
*/
@@ -443,16 +482,27 @@
return status;
}
- db->pagbno = 0;
- db->blkptr = 0;
- db->keyptr = 0;
+ if (impllock)
+ (void) apr_sdbm_unlock(db);
return getnext(key, db);
}
apr_status_t apr_sdbm_nextkey(apr_sdbm_t *db, apr_sdbm_datum_t *key)
{
- return getnext(key, db);
+ apr_status_t status;
+ int impllock = !(db->flags & (SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK));
+
+ if (impllock)
+ if ((status = apr_sdbm_lock(db, APR_FLOCK_SHARED)) != APR_SUCCESS)
+ return ioerr(db, status);
+
+ status = getnext(key, db);
+
+ if (impllock)
+ (void) apr_sdbm_unlock(db);
+
+ return status;
}
/*
Index: dbm/sdbm/sdbm_lock.c
===================================================================
RCS file: /home/cvs/apr-util/dbm/sdbm/sdbm_lock.c,v
retrieving revision 1.6
diff -u -r1.6 sdbm_lock.c
--- dbm/sdbm/sdbm_lock.c 2001/04/30 17:16:19 1.6
+++ dbm/sdbm/sdbm_lock.c 2001/05/04 05:41:09
@@ -52,25 +52,47 @@
* <http://www.apache.org/>.
*/
+#include "apr_file_info.h"
#include "apr_file_io.h"
#include "apr_sdbm.h"
#include "sdbm_private.h"
+#include "sdbm_tune.h"
/* NOTE: this function blocks until it acquires the lock */
-apr_status_t sdbm_lock(apr_sdbm_t *db, int exclusive)
+apr_status_t apr_sdbm_lock(apr_sdbm_t *db, int type)
{
- int type;
+ apr_status_t status;
- if (exclusive)
- type = APR_FLOCK_EXCLUSIVE;
- else
- type = APR_FLOCK_SHARED;
+ if (db->flags & (SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK))
+ return APR_EINVAL;
+ /*
+ * zero size: either a fresh database, or one with a single,
+ * unsplit data page: dirpage is all zeros.
+ */
+ if ((status = apr_file_lock(db->dirf, type)) == APR_SUCCESS)
+ {
+ apr_finfo_t finfo;
+ if ((status = apr_file_info_get(&finfo, APR_FINFO_SIZE, db->dirf))
+ != APR_SUCCESS) {
+ (void) apr_file_unlock(db->dirf);
+ return status;
+ }
- return apr_file_lock(db->pagf, type);
+ SDBM_INVALIDATE_CACHE(db, finfo);
+
+ if (type == APR_FLOCK_SHARED)
+ db->flags |= SDBM_SHARED_LOCK;
+ else if (type == APR_FLOCK_EXCLUSIVE)
+ db->flags |= SDBM_EXCLUSIVE_LOCK;
+ }
+ return status;
}
-apr_status_t sdbm_unlock(apr_sdbm_t *db)
+apr_status_t apr_sdbm_unlock(apr_sdbm_t *db)
{
- return apr_file_unlock(db->pagf);
+ if (!(db->flags & (SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK)))
+ return APR_EINVAL;
+ db->flags &= ~(SDBM_SHARED_LOCK | SDBM_EXCLUSIVE_LOCK);
+ return apr_file_unlock(db->dirf);
}
Index: dbm/sdbm/sdbm_private.h
===================================================================
RCS file: /home/cvs/apr-util/dbm/sdbm/sdbm_private.h,v
retrieving revision 1.5
diff -u -r1.5 sdbm_private.h
--- dbm/sdbm/sdbm_private.h 2001/04/30 17:16:23 1.5
+++ dbm/sdbm/sdbm_private.h 2001/05/04 05:41:09
@@ -79,8 +79,10 @@
#define SPLTMAX 10 /* maximum allowed splits */
/* for apr_sdbm_t.flags */
-#define SDBM_RDONLY 0x1 /* data base open read-only */
-#define SDBM_SHARED 0x4 /* data base locks for shared write */
+#define SDBM_RDONLY 0x1 /* data base open read-only */
+#define SDBM_SHARED 0x2 /* data base open for sharing */
+#define SDBM_SHARED_LOCK 0x4 /* data base locks for shared write */
+#define SDBM_EXCLUSIVE_LOCK 0x8 /* data base locks for shared write */
struct apr_sdbm_t {
apr_pool_t *pool;
@@ -100,11 +102,17 @@
apr_status_t status; /* track the specific last error */
};
-apr_status_t sdbm_lock(apr_sdbm_t *db, int exclusive);
-apr_status_t sdbm_unlock(apr_sdbm_t *db);
-
extern const apr_sdbm_datum_t sdbm_nullitem;
long sdbm_hash(const char *str, int len);
+
+/*
+ * zero the cache
+ */
+#define SDBM_INVALIDATE_CACHE(db, finfo) \
+ do { db->dirbno = (!finfo.size) ? 0 : -1; \
+ db->pagbno = -1; \
+ db->maxbno = finfo.size * BYTESIZ; \
+ } while (0);
#endif /* SDBM_PRIVATE_H */
Index: include/apr_sdbm.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_sdbm.h,v
retrieving revision 1.6
diff -u -r1.6 apr_sdbm.h
--- include/apr_sdbm.h 2001/05/02 13:37:52 1.6
+++ include/apr_sdbm.h 2001/05/04 05:41:09
@@ -88,6 +88,8 @@
apr_pool_t *p);
apr_status_t apr_sdbm_close(apr_sdbm_t *db);
+apr_status_t apr_sdbm_lock(apr_sdbm_t *db, int type);
+apr_status_t apr_sdbm_unlock(apr_sdbm_t *db);
apr_status_t apr_sdbm_fetch(apr_sdbm_t *db, apr_sdbm_datum_t *val,
apr_sdbm_datum_t key);
apr_status_t apr_sdbm_delete(apr_sdbm_t *db, const apr_sdbm_datum_t key);
