apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: apr-util/dbm/sdbm sdbm.c
Date Wed, 02 May 2001 02:00:56 GMT
On Tue, May 01, 2001 at 05:35:47PM -0500, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <gstein@lyra.org>
> Sent: Tuesday, May 01, 2001 12:45 AM
> 
> > On Mon, Apr 30, 2001 at 09:50:25PM -0500, William A. Rowe, Jr. wrote:
> > > 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...
> > 
> > Sorry. In this case, that would not be prudent. Please back out.
> > 
> > The situation is that the SDBM is ten *years* old. It doesn't have bugs. It
> > simply is not something that I worry about. These kinds of changes are
> > destabilizing, in an area where I think we can't afford it.
> 
> Greg, now you have me confused (unless I've missed a post on this subject !?!)
> 
> I noted your commits (all look good) and have some additional thoughts below,
> but the question in my mind is ... do you want me to unroll these patches?

Yes, please back out the lock/unlock patch. The first three (namespace
protection, re-indenting, and status code returns) all are fine. It was only
the portion that introduced the per-operation lock/unlock.

The small tweaks that I made should not interfere with backing out the
lock/unlock.

[ just do a "cvs diff -r1.N -r1.N-1" to generate a reverse patch, then apply
  to head, then check in ]

If you have any problem, then I'll do it tonite. I just wanted to be a bit
more courteous and ask you (personally, it feels more polite to /not/ back
out somebody's change on them).

>...
> I also realized that not all Win based systems can be trusted to even update the
> mtime upon every update.  I only see one answer;
> 
>   * expose lock/unlock for multiple operations that take advantage of the cache

Hmm. This would certainly help an app who is doing multiple operations, but
if an app forgets, it could lead to corruption. Another possibility is to
"count" the locks. The caller could:

- Lock
- Store
  - lock
  - operation
  - unlock
- Store again
- Unlock

The sub-locks in the Store operation would just incr a count and return
(thus they'd be fast). If the app makes a mistake in their lock/unlock, or
simply doesn't care to do it, then each operation is protected.

>   * flush the cache upon granting every lock

Hmm. Need to think on it. I imagined doing it on the unlock. We don't want a
read to say, "oh. that page is in my cache, so I don't have to hit the file
to read it in [thus avoiding acquiring a lock to do the read]."

>   * be sure we aren't using buffered writes on either the pagf or dirf

Yah. That would suck. :-)

> This is certainly sub-optimal, but still significantly faster than closing and
> opening the two files for every request.

Absolutely!

>...
> Good patches, as I said.  I need to vet this code too, and I'll have more
> comments in a couple of hours, and add the invalidation (not optimized by
> mtime stamps) at the same time.

Okay. I'll look for them tonite.

>...
> > sdbm is already cross-process. It arbitrates access to the DBM quite fine.
> > You're simply trying to make the locking more fine-grained. That's okay, but
> > not at the expense of trust in this code.
> 
> Huh?  By closing and reopening the file (exclusively)?  I don't follow.

If you create/open an SDBM, then it takes out the appropriater reader/writer
lock. Granted, you don't want to leave it open longer than your request for
fear of keeping others out.

My point was that SDBM currently arbitrates across processes today. It just
doesn't have the granularity you are seeking.

Cheers,
-g

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

Mime
View raw message