Re: [PATCH] lock transaction-current separately from current

On Fri, Oct 05, 2007 at 03:17:53PM -0700, David Glasser wrote:
> On 10/5/07, Malcolm Rowe <malcolm-svn-dev@farside.org.uk> 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:
  open write-lock
  lock write-lock
  open transaction-current
  read
  close
  open temporary
  write
  fflush
  close
  rename to transaction-current
  open directory
  fflush directory
  close
  unlock write-lock
  close write-lock

We could switch the lockfile to a new transaction-current-lock, sure,
though it'd be much better if we switched to:

  open transaction-current
  lock
  read
  seek
  write
  fflush
  close

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

Regards,
Malcolm