apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <ad...@rowe-clan.net>
Subject Re: sdbm locks, second try
Date Fri, 04 May 2001 05:41:46 GMT
From: "Greg Stein" <gstein@lyra.org>
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);



Mime
View raw message