On Fri, Oct 05, 2007 at 03:17:53PM -0700, David Glasser wrote:
> On 10/5/07, Malcolm Rowe <firstname.lastname@example.org> wrote:
> > On Thu, Oct 04, 2007 at 09:22:49AM -0700, David Glasser wrote:
> > > The following patch locks transaction-current itself instead of using
> > > the write-lock. (I assume it's OK to lock the file even if we're
> > > changing it by overwriting, right? that is, there's no need to
> > > introduce transaction-current.lock?)
> > Correct, _as long as_ we're locking it in an
> > open-lock-read-write-unlock-close cycle. We only need separate
> > lockfiles if we're doing lockless reads.
> I think the read and writes are separate open()s (maybe that could be
> fixed), so ok, I'll use a transaction.lock file.
Ah, I wasn't clear.
Currently, we do:
rename to transaction-current
We could switch the lockfile to a new transaction-current-lock, sure,
though it'd be much better if we switched to:
(What we can't ever do is allow an unblocked read (actually, close()) on
a file that's locked for write, since the reader's close() will unlock
the writer's lock()).
> > > epg also suggested locking
> > > revprop files individually instead of using write-lock there; maybe
> > > I'll do that later.
> > Sorry, that won't work, for two reasons:
> > 1. Older clients use the write-lock. You'd need to make the locking
> > dependent on the fs format or something. But, more importantly,
> > 2. We read the revprops files without locking them. POSIX locks for a
> > file are released when _any thread in the process closes any fd opened
> > on that file_. This means that you can't do write locks without a
> > separate lockfile. (Yes, this is insane).
> Hmm, "older clients use the write-lock" is true, but admittedly
> revprops were changed *without any locking at all* until sometime this
> year. We backported that fix to the 1.4.x line, but I'm not convinced
> that changing it in 1.5 would be that horrible.
Okay, that's a fair enough point, I suppose.
> And OK, I guess
> instead of locking the revprop file itself we'd lock an auxillary file
> (either revprops/0/123.lock or just revprops.lock). If we have one
> lock file per revprop I think we'd want to delete the file afterwards;
> the amount of disk space wasted by the tiny revprop files themselves
> is big enough.
Right, you wouldn't want an empty file per rev - we have enough tiny
files already :-). If you really wanted to create the lockfile on
demand, I suspect you'd have to do something careful to make sure it
worked on NFS, as creat(O_EXCL) doesn't work on NFS; I'm also not
entirely sure whether creat/fcntl works as you'd expect if two clients
create a file.
[ The only way I can see that's guaranteed to work is to do the dot-lock
dance: create a uniquely-named file in the relevant directory, then
hardlink to the lockfile. You'd need to detect stale locks though, and
you'd also need a solution for filesystems that can't do hardlinks. I'd
recommend using a single pre-created lockfile :-). ]
But really, is the lock that contended? We aren't planning on changing
revprops that frequently, are we?
Received on Mon Oct 8 10:18:53 2007
- application/pgp-signature attachment: stored