apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: sdbm locks, second try
Date Sat, 05 May 2001 07:38:29 GMT
On Fri, May 04, 2001 at 12:41:46AM -0500, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <gstein@lyra.org>:
>...
> > 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.

Can we do this first to simplify the next steps? As I mentioned, your
patches are already needing to deal with the ioerr() stuff. And the
introduction of get_err and clear_err was kind of a mistake given that you
did the Right Thing with returning the status codes in the first place.

> > 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.

Hmm. I'm a bit skeptical that an app with that ratio would want to "pre
lock" the dbm, rather than simply open it "normally" and let the lock be
outstanding.

But that really depends on the app, and I can't truly judge that.

Okay... so let's say that the app can (externally) establish the lock. And
that the app should be able to establish either kind: shared (read only) or
exclusive (read/write).

Now...

> > 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?)

Yup. Simple code. Remember: I don't want to see any chance that we break
this stuff. Everything might look fine, but then two years from now a bug
will crop up in what we did. The existing SDBM implementation is ten years
old. I suspect it is entirely bug free, and it would be shame for us to
start dorking around and introducing bugs.

So... the simple approach. Record the type of lock: shared/exclusive. Record
the number of times that type was locked. Bail if some requests an exclusive
lock and all we have is shared (i.e. no promotion). Asking for shared when
an exclusive is held is fine.

The reason for refcounting: the app is going to call apr_sdbm_lock(). And
then apr_sdbm_store() is going to call it, too. We don't want the second to
fail. And we don't want apr_sdbm_store's unlock to toss what the app did.

And we don't want apr_sdbm_store to mess around with bit flags as you've
done in your patch. It ought to just call apr_sdbm_lock(db, EXCLUSIVE).
Internally, that function can Do The Right Thing.

Consider: if you open the dbm "old style", then a lock will be taken out
right away, of the right type. Whenever store() goes to get a lock
(unconditionally!), it will already exist, so the "lock" is fast. Since it
is refcounting, the unlock in store() can be simple, too.

IOW, put the logic into apr_sdbm_(un)lock. That reduces the chances that the
rest of the code will get the flags wrong. The refcounting will ensure that
we get proper lock/unlock patterns.

> > 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'm not sure what you're saying here. I'm talking about refcounting in SDBM,
*NOT* in the apr_file or locking code. The apr_sdbm_t grows two fields:

    int lock_count;
    int lock_type;  /* shared or exclusive */

This is very simple, and we don't have to mess around with APR locks or
files at all. This is also simple and straight-forward for SDBM.

> 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.

The user can't do this properly. If they have a "big lock" that they wrap
around all the operations, the first store() is going to toast their lock on
exit. The locks need to be refcounted.

[ and store must lock/unlock, rather than depend upon the app. ]

>...
> > 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.

Okay. Good point. So we should do the cache wipe at unlock time. We still
have to consider the case of a read that decides it doesn't have to hit the
disk, so it doesn't bother to take out a lock. Since it never goes for a
lock, we can't toss the cache at lock time.

>...
> 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!)

I'm not sure that I could remove ioerr() without making your patch
inapplicable. That was why I suggested doing it beforehand: get it out of
our way and simplify some bits in your patch.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message