Quantcast

[PATCH 1 of 2 RFC] lock: support flock

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 1 of 2 RFC] lock: support flock

Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1495048786 25200
#      Wed May 17 12:19:46 2017 -0700
# Node ID d1ddcac58ac7085b1b0238b74e38871343730c91
# Parent  763d7292569138886c3fdf179f7e688351bfb212
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r d1ddcac58ac7
lock: support flock

This patch makes it possible to use flock internally.

flock is more reliable in Linux's pid namespace use-case than file-existence
test, because it works without a /proc filesystem and does not have deadlock
issue if an hg process is killed.

diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -48,5 +48,10 @@ class lock(object):
     Typically used via localrepository.lock() to lock the repository
     store (.hg/store/) or localrepository.wlock() to lock everything
-    else under .hg/.'''
+    else under .hg/.
+
+    If flockpath is not None, flock will be used instead of file-existence
+    lock. The lock file will still be used to provide lock information but it
+    is not effective for locking purpose.
+    '''
 
     # lock is symlink on platforms that support it, file on others.
@@ -61,5 +66,6 @@ class lock(object):
 
     def __init__(self, vfs, file, timeout=-1, releasefn=None, acquirefn=None,
-                 desc=None, inheritchecker=None, parentlock=None):
+                 desc=None, inheritchecker=None, parentlock=None,
+                 flockpath=None):
         self.vfs = vfs
         self.f = file
@@ -72,4 +78,6 @@ class lock(object):
         self.parentlock = parentlock
         self._parentheld = False
+        self._flockpath = flockpath
+        self._flockfd = None
         self._inherited = False
         self.postrelease  = []
@@ -97,4 +105,11 @@ class lock(object):
         self.release()
 
+    def _getflockfd(self):
+        if self._flockfd is None:
+            if self._flockpath is not None:
+                self._flockfd = os.open(self.vfs.join(self._flockpath),
+                                        os.O_RDONLY)
+        return self._flockfd
+
     def _getpid(self):
         # wrapper around util.getpid() to make testing easier
@@ -127,9 +142,14 @@ class lock(object):
             retry -= 1
             try:
-                self.vfs.makelock(lockname, self.f)
+                self.vfs.makelock(lockname, self.f, self._getflockfd())
                 self.held = 1
             except (OSError, IOError) as why:
-                if why.errno == errno.EEXIST:
+                # EAGAIN: flock fails, EEXIST: non-flock fails
+                if why.errno in (errno.EEXIST, errno.EAGAIN):
                     locker = self._readlock()
+                    # for flock case, locker may be outdated but surely some
+                    # process is alive and taking the lock
+                    if why.errno == errno.EAGAIN and locker is None:
+                        locker = 'unknown'
                     if locker is None:
                         continue
@@ -163,4 +183,8 @@ class lock(object):
         Returns None if no lock exists, pid for old-style locks, and host:pid
         for new-style locks.
+
+        Note: in flock's case, _readlock() is a best-effort and could be
+        non-reliable after a makelock failure. It's still reliable if makelock
+        succeeded.
         """
         try:
@@ -172,4 +196,8 @@ class lock(object):
 
     def _testlock(self, locker):
+        if self._flockpath is not None:
+            # when flock is used, locker couldn't be dead (if it were, the OS
+            # will release its flock and we should not hit this code path)
+            return locker
         if locker is None:
             return None
@@ -260,4 +288,8 @@ class lock(object):
                     except OSError:
                         pass
+                    # release flock
+                    if self._flockfd is not None:
+                        os.close(self._flockfd)
+                        self._flockfd = None
             # The postrelease functions typically assume the lock is not held
             # at all.
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1080,4 +1080,7 @@ def checksignature(func):
 }
 
+# a whitelist of known filesystems where flock works reliably
+_flockfswhitelist = _hardlinkfswhitelist
+
 def copyfile(src, dest, hardlink=False, copystat=False, checkambig=False):
     '''copy a file, preserving mode and optionally other stat info like
@@ -1233,16 +1236,40 @@ if safehasattr(time, "perf_counter"):
     timer = time.perf_counter
 
-def makelock(info, pathname):
+def makelock(info, pathname, flockfd=None):
+    openflags = os.O_CREAT | os.O_WRONLY
+    # if flockfd is provided, use it as the source of truth of locking. flockfd
+    # should be on a same filesystem of pathname so the sanity check about
+    # filesystem type works.
+    if flockfd is not None:
+        fstype = getfstype(os.path.dirname(pathname)) or _('(unknown)')
+        if fstype not in _flockfswhitelist:
+            raise error.Abort(_('filesystem %s is not allowed to use flock')
+                              % fstype)
+        import fcntl # not available on all platforms / code paths
+        fcntl.flock(flockfd, fcntl.LOCK_NB | fcntl.LOCK_EX) # may raise EAGAIN
+        symname = '%s.tmp%d' % (pathname, os.getpid())
+    else:
+        openflags |= os.O_EXCL
+        symname = pathname
+
     try:
-        return os.symlink(info, pathname)
-    except OSError as why:
-        if why.errno == errno.EEXIST:
-            raise
-    except AttributeError: # no symlink in os
-        pass
-
-    ld = os.open(pathname, os.O_CREAT | os.O_WRONLY | os.O_EXCL)
-    os.write(ld, info)
-    os.close(ld)
+        try:
+            os.symlink(info, symname)
+            if symname != pathname:
+                os.rename(symname, pathname)
+            return
+        except OSError as why:
+            if why.errno == errno.EEXIST:
+                raise
+        except AttributeError: # no symlink in os
+            pass
+
+        ld = os.open(pathname, openflags)
+        os.write(ld, info)
+        os.close(ld)
+    except Exception:
+        if flockfd is not None:
+            fcntl.flock(flockfd, fcntl.LOCK_UN)
+        raise
 
 def readlock(pathname):
diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -147,6 +147,6 @@ class abstractvfs(object):
         return util.makedirs(self.join(path), mode)
 
-    def makelock(self, info, path):
-        return util.makelock(info, self.join(path))
+    def makelock(self, info, path, flockfd=None):
+        return util.makelock(info, self.join(path), flockfd)
 
     def mkdir(self, path=None):
diff --git a/tests/test-lock.py b/tests/test-lock.py
--- a/tests/test-lock.py
+++ b/tests/test-lock.py
@@ -33,5 +33,5 @@ class lockwrapper(lock.lock):
 
 class teststate(object):
-    def __init__(self, testcase, dir, pidoffset=0):
+    def __init__(self, testcase, dir, pidoffset=0, useflock=False):
         self._testcase = testcase
         self._acquirecalled = False
@@ -40,6 +40,9 @@ class teststate(object):
         self.vfs = vfsmod.vfs(dir, audit=False)
         self._pidoffset = pidoffset
+        self._useflock = useflock
 
     def makelock(self, *args, **kwargs):
+        if self._useflock:
+            kwargs['flockpath'] = self.vfs.base
         l = lockwrapper(self._pidoffset, self.vfs, testlockname,
                         releasefn=self.releasefn, acquirefn=self.acquirefn,
@@ -106,6 +109,9 @@ class teststate(object):
 
 class testlock(unittest.TestCase):
+    useflock = False
+
     def testlock(self):
-        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()))
+        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()),
+                          useflock=self.useflock)
         lock = state.makelock()
         state.assertacquirecalled(True)
@@ -116,5 +122,6 @@ class testlock(unittest.TestCase):
 
     def testrecursivelock(self):
-        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()))
+        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()),
+                          useflock=self.useflock)
         lock = state.makelock()
         state.assertacquirecalled(True)
@@ -136,5 +143,6 @@ class testlock(unittest.TestCase):
 
     def testlockfork(self):
-        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()))
+        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()),
+                          useflock=self.useflock)
         lock = state.makelock()
         state.assertacquirecalled(True)
@@ -156,5 +164,5 @@ class testlock(unittest.TestCase):
     def testinheritlock(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        parentstate = teststate(self, d)
+        parentstate = teststate(self, d, useflock=self.useflock)
         parentlock = parentstate.makelock()
         parentstate.assertacquirecalled(True)
@@ -166,5 +174,5 @@ class testlock(unittest.TestCase):
             parentstate.assertlockexists(True)
 
-            childstate = teststate(self, d, pidoffset=1)
+            childstate = teststate(self, d, pidoffset=1, useflock=self.useflock)
             childlock = childstate.makelock(parentlock=lockname)
             childstate.assertacquirecalled(True)
@@ -186,5 +194,5 @@ class testlock(unittest.TestCase):
     def testmultilock(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        state0 = teststate(self, d)
+        state0 = teststate(self, d, useflock=self.useflock)
         lock0 = state0.makelock()
         state0.assertacquirecalled(True)
@@ -195,5 +203,5 @@ class testlock(unittest.TestCase):
             state0.assertlockexists(True)
 
-            state1 = teststate(self, d, pidoffset=1)
+            state1 = teststate(self, d, pidoffset=1, useflock=self.useflock)
             lock1 = state1.makelock(parentlock=lock0name)
             state1.assertacquirecalled(True)
@@ -205,5 +213,5 @@ class testlock(unittest.TestCase):
                 self.assertEqual(lock0name, lock1name)
 
-                state2 = teststate(self, d, pidoffset=2)
+                state2 = teststate(self, d, pidoffset=2, useflock=self.useflock)
                 lock2 = state2.makelock(parentlock=lock1name)
                 state2.assertacquirecalled(True)
@@ -227,5 +235,5 @@ class testlock(unittest.TestCase):
     def testinheritlockfork(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        parentstate = teststate(self, d)
+        parentstate = teststate(self, d, useflock=self.useflock)
         parentlock = parentstate.makelock()
         parentstate.assertacquirecalled(True)
@@ -233,5 +241,5 @@ class testlock(unittest.TestCase):
         # set up lock inheritance
         with parentlock.inherit() as lockname:
-            childstate = teststate(self, d, pidoffset=1)
+            childstate = teststate(self, d, pidoffset=1, useflock=self.useflock)
             childlock = childstate.makelock(parentlock=lockname)
             childstate.assertacquirecalled(True)
@@ -255,5 +263,5 @@ class testlock(unittest.TestCase):
     def testinheritcheck(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        state = teststate(self, d)
+        state = teststate(self, d, useflock=self.useflock)
         def check():
             raise error.LockInheritanceContractViolation('check failed')
@@ -275,5 +283,5 @@ class testlock(unittest.TestCase):
 
         d = tempfile.mkdtemp(dir=os.getcwd())
-        state = teststate(self, d)
+        state = teststate(self, d, useflock=self.useflock)
 
         def emulatefrequentlock(*args):
@@ -293,4 +301,7 @@ class testlock(unittest.TestCase):
             state.assertlockexists(False)
 
+class testflock(testlock):
+    useflock = True
+
 if __name__ == '__main__':
     silenttestrunner.main(__name__)
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# 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.

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
 
+@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
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jun Wu
Excerpts from Jun Wu's message of 2017-05-19 09:33:12 -0700:

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

Sorry but I realized this is not true. I thought both wlock and lock are
using vfs, but one of them uses svfs. So it should just work as expected.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Gregory Szorc
In reply to this post by Jun Wu
On Fri, May 19, 2017 at 9:33 AM, Jun Wu <[hidden email]> wrote:
# HG changeset patch
# User Jun Wu <[hidden email]>
# 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

+@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
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jun Wu
Thanks for the detailed comment! Replied inline.

Excerpts from Gregory Szorc's message of 2017-05-19 18:03:58 -0700:

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

Lacking of tests, docs, debug commands (to turn this on and off) will be
addressed once this graduates from RFC.

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

More precisely, the requirement is "svfs.base" being a different directory
from "vfs.base".

> 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

If "store2" provides a directory, like repo.svfs = vfs('.hg/store2').
The existing code will just work without modification.

So I don't think it's necessary to distinguish "store-flock" and
"store2-flock".

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

With a second thought, I think it may be possible to just unify the locks if
svfs.base == vfs.base. That could be done by letting repo.lock returns
repo.wlock() in this special case (no store + flock).
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Gregory Szorc
On Fri, May 19, 2017 at 8:26 PM, Jun Wu <[hidden email]> wrote:
Thanks for the detailed comment! Replied inline.

Excerpts from Gregory Szorc's message of 2017-05-19 18:03:58 -0700:
> 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.

Lacking of tests, docs, debug commands (to turn this on and off) will be
addressed once this graduates from RFC.

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

More precisely, the requirement is "svfs.base" being a different directory
from "vfs.base".

> 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

If "store2" provides a directory, like repo.svfs = vfs('.hg/store2').
The existing code will just work without modification.

OK. I failed to understand that it is locking directories and not files.

I don't want to go down this rat hole, but since it may help my point...

The next store format will likely use some form of "generational store." Files protected by transactions that aren't append only (like bookmarks and phases) will exist in a directory corresponding to their transaction/generation number. e.g. .hg/store/txn0/bookmarks, .hg/store/txn1500/phaseroots. This allows readers to have consistent snapshots since we never rewrite existing bytes: we just copy or create files and have a pointer to the "current" generation. In this model you have reader locks so you know when there are no more active consumers for a generation and can GC its files.

Anyway, the locking model may be radically different. You can't rely on it being based on whole directories. So the semantics of a "flock" requirement could vary drastically depending on the store and other repo features in use. And I think having the semantics of a requirement vary depending on the presence of other requirements is not ideal. You want a requirement to mean one thing and one thing only and not be subject to multiple interpretations.
 

So I don't think it's necessary to distinguish "store-flock" and
"store2-flock".

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

With a second thought, I think it may be possible to just unify the locks if
svfs.base == vfs.base. That could be done by letting repo.lock returns
repo.wlock() in this special case (no store + flock).

The "store" requirement was added in Mercurial 0.9.2. The chances of this special case of
no store + flock occurring in 2017 are approximately 0%. I dare say we should add code to
prevent combinations of requirements like this because they make no sense and are a footgun waiting to fire. To prove my point, I just created a repo with format.usestore=false and experimental.treemanifest=true. It did seem to work, surprisingly. But it would be foolish to run such a config given the pre-store format has been out of style for over 10 years.

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jun Wu
Excerpts from Gregory Szorc's message of 2017-05-19 20:49:47 -0700:

> OK. I failed to understand that it is locking directories and not files.
>
> I don't want to go down this rat hole, but since it may help my point...
>
> The next store format will likely use some form of "generational store."
> Files protected by transactions that aren't append only (like bookmarks and
> phases) will exist in a directory corresponding to their
> transaction/generation number. e.g. .hg/store/txn0/bookmarks,
> .hg/store/txn1500/phaseroots. This allows readers to have consistent
> snapshots since we never rewrite existing bytes: we just copy or create
> files and have a pointer to the "current" generation. In this model you
> have reader locks so you know when there are no more active consumers for a
> generation and can GC its files.
>
> Anyway, the locking model may be radically different. You can't rely on it
> being based on whole directories. So the semantics of a "flock" requirement
> could vary drastically depending on the store and other repo features in
> use. And I think having the semantics of a requirement vary depending on
> the presence of other requirements is not ideal. You want a requirement to
> mean one thing and one thing only and not be subject to multiple
> interpretations.

How about creating files then? For every existing file existence lock at
$PATH, create $PATH.flock first (but never delete or replace it), and then
flock it. That'll be compatible with every file-existence lock (like two
locks under a same directory).

> The "store" requirement was added in Mercurial 0.9.2. The chances of this
> special case of
> no store + flock occurring in 2017 are approximately 0%. I dare say we
> should add code to
> prevent combinations of requirements like this because they make no sense
> and are a footgun waiting to fire. To prove my point, I just created a repo
> with format.usestore=false and experimental.treemanifest=true. It did seem
> to work, surprisingly. But it would be foolish to run such a config given
> the pre-store format has been out of style for over 10 years.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Pierre-Yves David-2
In reply to this post by Gregory Szorc


On 05/20/2017 03:03 AM, Gregory Szorc wrote:

> On Fri, May 19, 2017 at 9:33 AM, Jun Wu <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     # HG changeset patch
>     # User Jun Wu <[hidden email] <mailto:[hidden email]>>
>     # 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
>     <https://bitbucket.org/quark-zju/hg-draft>
>     #              hg pull https://bitbucket.org/quark-zju/hg-draft
>     <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

+1. The usual way to do this is to add a 'format.usexyz' config option.
Adding support for upgrading the repository in `hg debugupgraderepo`
would be nice too.

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Loading...