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: cvs commit: apr-util/dbm/sdbm sdbm.c
Date Tue, 01 May 2001 02:50:25 GMT
From: "Greg Stein" <gstein@lyra.org>
Sent: Monday, April 30, 2001 8:49 PM

> -1. Please back out.

I'd rather push forward and address your other concerns...

> You cannot lock the file only during write operations. See sdbm_private.h --
> copies of the file's data sits in memory. If User B goes and works with the
> file, then the cached block for User A becomes invalid. Now, if User A goes
> and does some work against the invalid block and writes that back out, then
> you've got corruption.

I'm well aware of this.  That's why I posited the question to the list about
the cache invalidation semantic.  The other option, of course, is to drop cache 
entirely in a shared open.

Please note we continue to lock the file for it's lifetime iff the user doesn't
pass the new APR_SHARELOCK flag.  See the snippets I grabbed from the commit 
log below.

> I am mildly against the concept, in general, for SDBM (since people can turn
> to other DBM implementations), but if you come up with a patch that solves
> the issues, then we could certainly discuss it.

I'm not proposing we provide the most optimized solution in the world.  Only a
straightforward functional build.  The mechansims I'm building on only affect
shared opens, not traditional opens we do today.  We are _cross_ platforms, while
there are many dbm implementations, this simplifies a baseline.  I _hope_ others
can be implemented with the APR_SHARELOCK flag, very simply and with better 

> The previous changes have looked good since there was no real functional or
> semantic change. This one isn't going to work, unfortunately.

Again, it shouldn't change if SDBM_SHARED isn't toggled.  If it has, that's my bad.

> A client app could open/close the SDBM to keep the contention duration
> shorter. Another alternative (ugly) would be to flush the cache after all
> write operations (thus losing its entire benefit).

What benefit?  If you want exclusive access, open APR_WRITE without the share flag,
as you probably do now.  And nothing changed

> There are some other issues with this patch, but it's kind of moot...

Bring them on.  Again, I prefer to work forward, since I mentioned in my other
comment, the auth_dbm, auth_digest, and soon ssl will need at least a bare-bones
cross-process dbm.  

I'd prefer to dump caching entirely for APR_SHARELOCK access, rather than revert
to the prior code, but if we can find a better way I'd like that.  Dumping caching
can't be worse than closing up and reopening an sdbm on every request.


> On Mon, Apr 30, 2001 at 07:03:50PM -0000, wrowe@apache.org wrote:
> > wrowe       01/04/30 12:03:49
> > 
> >   Modified:    include  apr_file_io.h
> >                .        CHANGES
> >                dbm/sdbm sdbm.c
> >   Log:
> >     shared sdbm file locking patch, writes/deletes require an excl lock,
> >     read/getkeys require a shared lock

>   -    (void) sdbm_unlock(db);
>   +    if (!(db->flags & SDBM_SHARED))
>   +        (void) sdbm_unlock(db);

>   +    /*
>   +     * adjust user flags so that SHARELOCK isn't used within
>   +     * APR (if it's ever implemented) since we will perform
>   +     * that locking here.
>   +     */
>   +    if (flags & APR_SHARELOCK) {
>   +        flags &= ~APR_SHARELOCK;
>   +        db->flags |= SDBM_SHARED;
>   +    }

>        /*
>   +     * if we are opened in SHARED mode, unlock ourself 
>   +     */
>   +    if (db->flags & SDBM_SHARED)
>   +        if ((status = sdbm_unlock(db)) != APR_SUCCESS)
>   +            goto error;

op begin
>   +    if (db->flags & SDBM_SHARED)
>   +        if ((status = sdbm_lock(db, 0)) != APR_SUCCESS)
>   +            return ioerr(db, status);

op end
>   +    if (db->flags & SDBM_SHARED)
>   +        if ((status = sdbm_unlock(db)) != APR_SUCCESS)
>   +            goto error;

View raw message