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.
> > 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. 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.
> > To get and increment the FSFS transaction-current file, just lock
> > 'transaction-current' itself instead of 'write-lock', so that new
> > transactions and commits don't block each other. In order to do so,
> > abstract out the locking code a little more.
> >
>
> > +/* Get a lock on file LOCK_FILENAME, creating it in POOL. */
> > +static svn_error_t *
> > +get_lock_on_filesystem(const char *lock_filename,
> > + apr_pool_t *pool)
> > +{
> > + svn_node_kind_t kind;
> > +
> > + /* svn 1.1.1 and earlier deferred lock file creation (for the global
> > + repository 'write-lock') to the first commit. So in case the
> > + repository was created by an earlier version of svn, check the
> > + lock file here. */
> > + SVN_ERR(svn_io_check_path(lock_filename, &kind, pool));
> > + if ((kind == svn_node_unknown) || (kind == svn_node_none))
> > + SVN_ERR(svn_io_file_create(lock_filename, "", pool));
> > +
> > + SVN_ERR(svn_io_file_lock2(lock_filename, TRUE, FALSE, pool));
>
> Is there any way that we can just go ahead and lock, and handle the
> ENOENT error if it turns up?
>
> [Actually, this is wrong for transaction-current, where 'not exists'
> should be a hard error. As glasser points out on IRC:
>
> [17:47] <malcolmr> One thing I spotted so far: instead of stat()-ing the
> lock file before locking it (in case it wasn't created yet; for very old
> repositories), could you change to lock-and-handle error?
> [17:48] <glasser> malcolmr: Actually come to think of it, what I have
> there is kind of wrong
> [17:49] <glasser> since for transaction-current I'm locking the
> transaction-current file itself
> [17:49] <glasser> this will create it empty if it doesn't exist, which
> is bad, it should be '1' or something
> [17:49] <glasser> I think I'll move the existence check into the
> 'write-lock' specific function (svn_fs_fs__with_write_lock)
> [17:50] <glasser> and make with_some_lock/get_lock_on_filesystem just
> assume it exists and throw the error
> ]
>
> I'd like it if svn_fs_fs__with_write_lock() did
> lock,(error:create,lock) instead of stat,(missing:create),lock, if possible.
Sounds reasonable.
--dave
--
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 6 00:18:04 2007