[PATCH 2 of 2 RFC] localrepo: make it possible to use flock in a repo

Gregory Szorc gregory.szorc at gmail.com
Fri May 19 21:03:58 EDT 2017


On Fri, May 19, 2017 at 9:33 AM, Jun Wu <quark at fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark at fb.com>
> # Date 1495051320 25200
> #      Wed May 17 13:02:00 2017 -0700
> # Node ID e59c24bd146290c910945e84ac27a7cb4edbf4dd
> # Parent  d1ddcac58ac7085b1b0238b74e38871343730c91
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> e59c24bd1462
> localrepo: make it possible to use flock in a repo
>
> This patch adds a "flock" repo requirement, once set, Mercurial will
> attempt
> to use flock to lock the repo and store on supported platforms.
>
> The flock will be performed on directory, so no extra files will be
> created:
>
>   wlock: flock(".hg")
>   lock:  flock(".hg/store")
>
> This makes it impossible to have deadlock or repo corruption (no /proc
> mount) when running hg inside different Linux pid namespaces with a shared
> filesystem.
>
> For concerns about repo corruption on network filesystem or portability,
> this should be safe because util.makelock will double-check filesystem type
> so nfs/cifs/sshfs and Windows will just error out correctly.
>
> Note: in weird setups, like .hg/store lives on NFS when .hg is on local
> disk, the lock is not safe. But we have broken locks on that setup already,
> so this patch is at least not making things worse. It even makes things
> better in local shared repo use-case [1].
>
> [1]: Consider two repos sharing a store: repo1/.hg and repo2/.hg and
> shared/.hg/store. We should really make "repo.lock()" write a lock file at
> "shared/.hg/store/lock" but not "repo1/.hg/lock" or "repo2/.hg/lock".  With
> this patch, "shared/.hg/store" gets locked properly and there is no such
> problem.
>

>From a high level, this approach seems reasonable. There are some obvious
missing components to the implementation:

* Docs for the new requirement in internals/requirements.txt
* Ability to create new repos with the requirement

I initially thought it surprising that we didn't check for flock support at
repo open time as part of validating requirements. But your commit message
explains that we'll error at lock time upon missing flock support. Since we
don't have reader locks and I think it is user hostile to lock readers out
of repos no longer supporting flock, I think this is fine. But there should
be a test demonstrating the behavior of a flock requirement without flock
support.

Also, I'm going to bikeshed the requirement name. Requirements are global
and will persist for all of time. With that context in mind, "flock" is
arguably too generic. What this feature/requirement actually resembles is
"use flock() for the locking strategy used by the 'store'
layout/requirement which implies use of flock() on the lock and wlock
files." The meaning of the "flock" requirement can thus only be interpreted
in the presence of another requirement, such as "store." After all, if
there is a "store2" requirement, then "flock" likely takes on new meaning
(since we'd likely use a different locking strategy). I believe
requirements should be narrowly tailored to the exact feature they support.
Otherwise if their meaning changes over time, that opens us up to bugs and
possibly repo corruption. So I think a more appropriate requirement name is
something like "store-flock" so the limits of its application (to the lock
and wlock files for the "store" requirement) are explicit and not subject
to reinterpretation. Semantically, this is probably ideally expressed as a
sub-requirement. e.g. "store+flock." But I'd rather punt on that discussion
until we have a few more requirements and the relationship between them is
less clear.


>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -260,4 +260,5 @@ class localrepository(object):
>          'relshared',
>          'dotencode',
> +        'flock',
>      }
>      openerreqs = {
> @@ -367,4 +368,9 @@ class localrepository(object):
>                  self.requirements, self.sharedpath, vfsmod.vfs)
>          self.spath = self.store.path
> +
> +        if 'flock' in self.requirements and self.spath == self.path:
> +            raise error.RepoError(
> +                _('flock requires store path and repo path to be
> different'))
> +
>          self.svfs = self.store.vfs
>          self.sjoin = self.store.join
> @@ -1335,5 +1341,5 @@ class localrepository(object):
>
>      def _lock(self, vfs, lockname, wait, releasefn, acquirefn, desc,
> -              inheritchecker=None, parentenvvar=None):
> +              inheritchecker=None, parentenvvar=None, flockpath=None):
>          parentlock = None
>          # the contents of parentenvvar are used by the underlying lock to
> @@ -1345,5 +1351,5 @@ class localrepository(object):
>                               acquirefn=acquirefn, desc=desc,
>                               inheritchecker=inheritchecker,
> -                             parentlock=parentlock)
> +                             parentlock=parentlock, flockpath=flockpath)
>          except error.LockHeld as inst:
>              if not wait:
> @@ -1391,6 +1397,12 @@ class localrepository(object):
>              return l
>
> +        if 'flock' in self.requirements:
> +            flockpath = ''
> +        else:
> +            flockpath = None
> +
>          l = self._lock(self.svfs, "lock", wait, None,
> -                       self.invalidate, _('repository %s') %
> self.origroot)
> +                       self.invalidate, _('repository %s') %
> self.origroot,
> +                       flockpath=flockpath)
>          self._lockref = weakref.ref(l)
>          return l
> @@ -1429,9 +1441,14 @@ class localrepository(object):
>              self._filecache['dirstate'].refresh()
>
> +        if 'flock' in self.requirements:
> +            flockpath = ''
> +        else:
> +            flockpath = None
> +
>          l = self._lock(self.vfs, "wlock", wait, unlock,
>                         self.invalidatedirstate, _('working directory of
> %s') %
>                         self.origroot,
>                         inheritchecker=self._wlockchecktransaction,
> -                       parentenvvar='HG_WLOCK_LOCKER')
> +                       parentenvvar='HG_WLOCK_LOCKER',
> flockpath=flockpath)
>          self._wlockref = weakref.ref(l)
>          return l
> diff --git a/tests/hghave.py b/tests/hghave.py
> --- a/tests/hghave.py
> +++ b/tests/hghave.py
> @@ -217,4 +217,18 @@ def has_fifo():
>          return False
>
> + at check("flock", "flock on whitelisted filesystems")
> +def has_flock():
> +    try:
> +        import fcntl
> +        fcntl.flock
> +    except ImportError:
> +        return False
> +    from mercurial import util
> +    try:
> +        fstype = util.getfstype('.')
> +    except OSError:
> +        return False
> +    return fstype in util._flockfswhitelist
> +
>  @check("killdaemons", 'killdaemons.py support')
>  def has_killdaemons():
> diff --git a/tests/test-lock-badness.t b/tests/test-lock-badness.t
> --- a/tests/test-lock-badness.t
> +++ b/tests/test-lock-badness.t
> @@ -1,7 +1,15 @@
> +#testcases default useflock
>  #require unix-permissions no-root no-windows
>
> +#if useflock
> +#require flock
> +#endif
> +
>  Prepare
>
>    $ hg init a
> +#if useflock
> +  $ echo flock >> a/.hg/requires
> +#endif
>    $ echo a > a/a
>    $ hg -R a ci -A -m a
> @@ -11,4 +19,7 @@ Prepare
>    updating to branch default
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +#if useflock
> +  $ echo flock >> b/.hg/requires
> +#endif
>
>  Test that raising an exception in the release function doesn't cause the
> lock to choke
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170519/6eb0d543/attachment.html>


More information about the Mercurial-devel mailing list