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

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.

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

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

Otherwise, +1.

Regards,
Malcolm