Patchwork [5,of,7,V2] fncache: move fncache writing to be in a transaction

login
register
mail settings
Submitter Durham Goode
Date March 31, 2014, 11:19 p.m.
Message ID <23c4a802f5179a96dcc0.1396307987@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/4172/
State Superseded
Commit cd443c7589cca16a7fbcb3a7526ac10bd773dbda
Headers show

Comments

Durham Goode - March 31, 2014, 11:19 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1395700933 25200
#      Mon Mar 24 15:42:13 2014 -0700
# Node ID 23c4a802f5179a96dcc0ae26ce97671b675d87e3
# Parent  498a1087dd60ec7234c4352e566abe977eb63332
fncache: move fncache writing to be in a transaction

Previously the fncache was written at lock.release time. This meant it was not
tracked by a transaction, and if an error occurred during the fncache write it
would fail to update the fncache, but would not rollback the transaction,
resulting in an fncache that was not in sync with the files on disk (which
causes verify to fail, and causes streaming clones to not copy all the revlogs).

This uses the new transaction backup mechanism to make the fncache transacted.
It also moves the fncache from being written at lock.release time, to being
written at transaction.close time.
Pierre-Yves David - April 1, 2014, 1:25 a.m.
On 03/31/2014 04:19 PM, Durham Goode wrote:
> -    def _write(self, files, atomictemp):
> -        fp = self.vfs('fncache', mode='wb', atomictemp=atomictemp)
> -        if files:
> -            fp.write(encodedir('\n'.join(files) + '\n'))
> -        fp.close()
> -        self._dirty = False
> -
> -    def write(self):
> +    def write(self, tr):
>           if self._dirty:
> -            self._write(self.entries, True)
> +            tr.addbackup('fncache')
> +            fp = self.vfs('fncache', mode='wb', atomictemp=True)
> +            if self.entries:
> +                fp.write(encodedir('\n'.join(self.entries) + '\n'))
> +            fp.close()
> +            self._dirty = False


there was both a `_write` and a `write` function before. It not very 
clear why you are droping it (nor it is very clear why it existed in the 
first place.

Also, not that you looked at it for one week, consider documenting the 
on disk format.
Durham Goode - April 1, 2014, 1:28 a.m.
On 3/31/14 6:25 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
>On 03/31/2014 04:19 PM, Durham Goode wrote:
>> -    def _write(self, files, atomictemp):
>> -        fp = self.vfs('fncache', mode='wb', atomictemp=atomictemp)
>> -        if files:
>> -            fp.write(encodedir('\n'.join(files) + '\n'))
>> -        fp.close()
>> -        self._dirty = False
>> -
>> -    def write(self):
>> +    def write(self, tr):
>>           if self._dirty:
>> -            self._write(self.entries, True)
>> +            tr.addbackup('fncache')
>> +            fp = self.vfs('fncache', mode='wb', atomictemp=True)
>> +            if self.entries:
>> +                fp.write(encodedir('\n'.join(self.entries) + '\n'))
>> +            fp.close()
>> +            self._dirty = False
>
>
>there was both a `_write` and a `write` function before. It not very
>clear why you are droping it (nor it is very clear why it existed in the
>first place.
>
>Also, not that you looked at it for one week, consider documenting the
>on disk format.

It existed because write() and rewrite() both used to call _write().  Now
that rewrite is gone, I dropped _write() the next time a patch touched it.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -823,13 +823,17 @@ 
             raise error.RepoError(
                 _("abandoned transaction found - run hg recover"))
 
+        def onclose():
+            self.store.write(tr)
+
         self._writejournal(desc)
         renames = [(vfs, x, undoname(x)) for vfs, x in self._journalfiles()]
         rp = report and report or self.ui.warn
         tr = transaction.transaction(rp, self.sopener,
                                      "journal",
                                      aftertrans(renames),
-                                     self.store.createmode)
+                                     self.store.createmode,
+                                     onclose)
         self._transref = weakref.ref(tr)
         return tr
 
@@ -1037,7 +1041,6 @@ 
             return l
 
         def unlock():
-            self.store.write()
             if hasunfilteredcache(self, '_phasecache'):
                 self._phasecache.write()
             for k, ce in self._filecache.items():
diff --git a/mercurial/store.py b/mercurial/store.py
--- a/mercurial/store.py
+++ b/mercurial/store.py
@@ -337,7 +337,7 @@ 
     def copylist(self):
         return ['requires'] + _data.split()
 
-    def write(self):
+    def write(self, tr):
         pass
 
     def __contains__(self, path):
@@ -402,16 +402,14 @@ 
                     raise util.Abort(t)
         fp.close()
 
-    def _write(self, files, atomictemp):
-        fp = self.vfs('fncache', mode='wb', atomictemp=atomictemp)
-        if files:
-            fp.write(encodedir('\n'.join(files) + '\n'))
-        fp.close()
-        self._dirty = False
-
-    def write(self):
+    def write(self, tr):
         if self._dirty:
-            self._write(self.entries, True)
+            tr.addbackup('fncache')
+            fp = self.vfs('fncache', mode='wb', atomictemp=True)
+            if self.entries:
+                fp.write(encodedir('\n'.join(self.entries) + '\n'))
+            fp.close()
+            self._dirty = False
 
     def add(self, fn):
         if self.entries is None:
@@ -488,8 +486,8 @@ 
         return (['requires', '00changelog.i'] +
                 ['store/' + f for f in d.split()])
 
-    def write(self):
-        self.fncache.write()
+    def write(self, tr):
+        self.fncache.write(tr)
 
     def _exists(self, f):
         ef = self.encode(f)
diff --git a/tests/test-fncache.t b/tests/test-fncache.t
--- a/tests/test-fncache.t
+++ b/tests/test-fncache.t
@@ -177,4 +177,62 @@ 
 
   $ cd ..
 
+Aborting lock does not prevent fncache writes
 
+  $ cat > exceptionext.py <<EOF
+  > import os
+  > from mercurial import commands, util
+  > from mercurial.extensions import wrapfunction
+  > 
+  > def lockexception(orig, vfs, lockname, wait, releasefn, acquirefn, desc):
+  >     def releasewrap():
+  >         raise util.Abort("forced lock failure")
+  >     return orig(vfs, lockname, wait, releasewrap, acquirefn, desc)
+  > 
+  > def reposetup(ui, repo):
+  >     wrapfunction(repo, '_lock', lockexception)
+  > 
+  > cmdtable = {}
+  > 
+  > EOF
+  $ extpath=`pwd`/exceptionext.py
+  $ hg init fncachetxn
+  $ cd fncachetxn
+  $ printf "[extensions]\nexceptionext=$extpath\n" >> .hg/hgrc
+  $ touch y
+  $ hg ci -qAm y
+  abort: forced lock failure
+  [255]
+  $ cat .hg/store/fncache
+  data/y.i
+
+Aborting tranaction prevents fncache change
+
+  $ cat > ../exceptionext.py <<EOF
+  > import os
+  > from mercurial import commands, util, transaction
+  > from mercurial.extensions import wrapfunction
+  > 
+  > def wrapper(orig, self, *args, **kwargs):
+  >     origonclose = self.onclose
+  >     def onclose():
+  >         origonclose()
+  >         raise util.Abort("forced transaction failure")
+  >     self.onclose = onclose
+  >     return orig(self, *args, **kwargs)
+  > 
+  > def uisetup(ui):
+  >     wrapfunction(transaction.transaction, 'close', wrapper)
+  > 
+  > cmdtable = {}
+  > 
+  > EOF
+  $ rm "${extpath}c"
+  $ touch z
+  $ hg ci -qAm z
+  transaction abort!
+  rollback completed
+  abort: forced transaction failure
+  [255]
+  $ cat .hg/store/fncache
+  data/y.i