revlog: improve the robustness of the splitting process
The previous "in-place" splitting, preserving the splitting on transaction
failure had a couple of issue in case of transaction rollback:
- a race windows that could still lead to a crash and data loss
- it corrupted the `fncache`.
So instead, we use a new approach that we summarized as "we do a backup of the
inline revlog pre-split, and we restore this in case of failure".
To make readers live easier, we don't overwrite the inline index file until
transaction finalization. (once the transaction get into its finalization phase,
it is not expected to rollback, unless some crash happens).
To do so, we write the index of the split index in a temporary file that we use
until transaction finalization. We also keep a backup of the initial inline file
to be able to rollback the split if needed.
As a result, transaction rollback cancel the split and no longer corrupt
fncache. We also no longer have a small inconsistency windows where the
transaction could be unrecoverable.
--- a/mercurial/changelog.py Mon Mar 20 11:40:18 2023 +0100
+++ b/mercurial/changelog.py Mon Mar 20 11:52:17 2023 +0100
@@ -481,7 +481,7 @@
self._delaybuf = None
self._divert = False
# split when we're done
- self._enforceinlinesize(tr)
+ self._enforceinlinesize(tr, side_write=False)
def _writepending(self, tr):
"""create a file containing the unfinalized state for
@@ -512,9 +512,9 @@
return False
- def _enforceinlinesize(self, tr):
+ def _enforceinlinesize(self, tr, side_write=True):
if not self._delayed:
- revlog.revlog._enforceinlinesize(self, tr)
+ revlog.revlog._enforceinlinesize(self, tr, side_write=side_write)
def read(self, nodeorrev):
"""Obtain data from a parsed changelog revision.
--- a/mercurial/filelog.py Mon Mar 20 11:40:18 2023 +0100
+++ b/mercurial/filelog.py Mon Mar 20 11:52:17 2023 +0100
@@ -25,7 +25,7 @@
@interfaceutil.implementer(repository.ifilestorage)
class filelog:
- def __init__(self, opener, path):
+ def __init__(self, opener, path, try_split=False):
self._revlog = revlog.revlog(
opener,
# XXX should use the unencoded path
@@ -33,6 +33,7 @@
radix=b'/'.join((b'data', path)),
censorable=True,
canonical_parent_order=False, # see comment in revlog.py
+ try_split=try_split,
)
# Full name of the user visible file, relative to the repository root.
# Used by LFS.
@@ -256,8 +257,8 @@
class narrowfilelog(filelog):
"""Filelog variation to be used with narrow stores."""
- def __init__(self, opener, path, narrowmatch):
- super(narrowfilelog, self).__init__(opener, path)
+ def __init__(self, opener, path, narrowmatch, try_split=False):
+ super(narrowfilelog, self).__init__(opener, path, try_split=try_split)
self._narrowmatch = narrowmatch
def renamed(self, node):
--- a/mercurial/localrepo.py Mon Mar 20 11:40:18 2023 +0100
+++ b/mercurial/localrepo.py Mon Mar 20 11:52:17 2023 +0100
@@ -1240,7 +1240,12 @@
if path.startswith(b'/'):
path = path[1:]
- return filelog.filelog(self.svfs, path)
+ try_split = (
+ self.currenttransaction() is not None
+ or txnutil.mayhavepending(self.root)
+ )
+
+ return filelog.filelog(self.svfs, path, try_split=try_split)
@interfaceutil.implementer(repository.ilocalrepositoryfilestorage)
@@ -1251,7 +1256,13 @@
if path.startswith(b'/'):
path = path[1:]
- return filelog.narrowfilelog(self.svfs, path, self._storenarrowmatch)
+ try_split = (
+ self.currenttransaction() is not None
+ or txnutil.mayhavepending(self.root)
+ )
+ return filelog.narrowfilelog(
+ self.svfs, path, self._storenarrowmatch, try_split=try_split
+ )
def makefilestorage(requirements, features, **kwargs):
--- a/mercurial/revlog.py Mon Mar 20 11:40:18 2023 +0100
+++ b/mercurial/revlog.py Mon Mar 20 11:52:17 2023 +0100
@@ -302,6 +302,7 @@
persistentnodemap=False,
concurrencychecker=None,
trypending=False,
+ try_split=False,
canonical_parent_order=True,
):
"""
@@ -328,6 +329,7 @@
self._nodemap_file = None
self.postfix = postfix
self._trypending = trypending
+ self._try_split = try_split
self.opener = opener
if persistentnodemap:
self._nodemap_file = nodemaputil.get_nodemap_file(self)
@@ -511,6 +513,8 @@
entry_point = b'%s.i.%s' % (self.radix, self.postfix)
elif self._trypending and self.opener.exists(b'%s.i.a' % self.radix):
entry_point = b'%s.i.a' % self.radix
+ elif self._try_split and self.opener.exists(b'%s.i.s' % self.radix):
+ entry_point = b'%s.i.s' % self.radix
else:
entry_point = b'%s.i' % self.radix
@@ -2015,7 +2019,7 @@
raise error.CensoredNodeError(self.display_id, node, text)
raise
- def _enforceinlinesize(self, tr):
+ def _enforceinlinesize(self, tr, side_write=True):
"""Check if the revlog is too big for inline and convert if so.
This should be called after revisions are added to the revlog. If the
@@ -2032,7 +2036,8 @@
raise error.RevlogError(
_(b"%s not found in the transaction") % self._indexfile
)
- trindex = None
+ if troffset:
+ tr.addbackup(self._indexfile, for_offset=True)
tr.add(self._datafile, 0)
existing_handles = False
@@ -2048,6 +2053,29 @@
# No need to deal with sidedata writing handle as it is only
# relevant with revlog-v2 which is never inline, not reaching
# this code
+ if side_write:
+ old_index_file_path = self._indexfile
+ new_index_file_path = self._indexfile + b'.s'
+ opener = self.opener
+
+ fncache = getattr(opener, 'fncache', None)
+ if fncache is not None:
+ fncache.addignore(new_index_file_path)
+
+ # the "split" index replace the real index when the transaction is finalized
+ def finalize_callback(tr):
+ opener.rename(
+ new_index_file_path,
+ old_index_file_path,
+ checkambig=True,
+ )
+
+ tr.registertmp(new_index_file_path)
+ if self.target[1] is not None:
+ finalize_id = b'000-revlog-split-%d-%s' % self.target
+ else:
+ finalize_id = b'000-revlog-split-%d' % self.target[0]
+ tr.addfinalize(finalize_id, finalize_callback)
new_dfh = self._datafp(b'w+')
new_dfh.truncate(0) # drop any potentially existing data
@@ -2055,17 +2083,10 @@
with self._indexfp() as read_ifh:
for r in self:
new_dfh.write(self._getsegmentforrevs(r, r, df=read_ifh)[1])
- if (
- trindex is None
- and troffset
- <= self.start(r) + r * self.index.entry_size
- ):
- trindex = r
new_dfh.flush()
- if trindex is None:
- trindex = 0
-
+ if side_write:
+ self._indexfile = new_index_file_path
with self.__index_new_fp() as fp:
self._format_flags &= ~FLAG_INLINE_DATA
self._inline = False
@@ -2079,16 +2100,9 @@
if self._docket is not None:
self._docket.index_end = fp.tell()
- # There is a small transactional race here. If the rename of
- # the index fails, we should remove the datafile. It is more
- # important to ensure that the data file is not truncated
- # when the index is replaced as otherwise data is lost.
- tr.replace(self._datafile, self.start(trindex))
-
- # the temp file replace the real index when we exit the context
- # manager
-
- tr.replace(self._indexfile, trindex * self.index.entry_size)
+ # If we don't use side-write, the temp file replace the real
+ # index when we exit the context manager
+
nodemaputil.setup_persistent_nodemap(tr, self)
self._segmentfile = randomaccessfile.randomaccessfile(
self.opener,
--- a/tests/test-transaction-rollback-on-revlog-split.t Mon Mar 20 11:40:18 2023 +0100
+++ b/tests/test-transaction-rollback-on-revlog-split.t Mon Mar 20 11:52:17 2023 +0100
@@ -9,12 +9,12 @@
> from mercurial import extensions, util
>
> def extsetup(ui):
- > def close(orig, *args, **kwargs):
- > path = util.normpath(args[0]._atomictempfile__name)
- > if path.endswith(b'/.hg/store/data/file.i'):
+ > def rename(orig, src, dest, *args, **kwargs):
+ > path = util.normpath(dest)
+ > if path.endswith(b'data/file.i'):
> os.kill(os.getpid(), signal.SIGKILL)
- > return orig(*args, **kwargs)
- > extensions.wrapfunction(util.atomictempfile, 'close', close)
+ > return orig(src, dest, *args, **kwargs)
+ > extensions.wrapfunction(util, 'rename', rename)
> EOF
$ cat > $TESTTMP/intercept_after_rename.py << EOF
@@ -30,6 +30,14 @@
> os.kill(os.getpid(), signal.SIGKILL)
> return r
> extensions.wrapfunction(util.atomictempfile, 'close', close)
+ > def extsetup(ui):
+ > def rename(orig, src, dest, *args, **kwargs):
+ > path = util.normpath(dest)
+ > r = orig(src, dest, *args, **kwargs)
+ > if path.endswith(b'data/file.i'):
+ > os.kill(os.getpid(), signal.SIGKILL)
+ > return r
+ > extensions.wrapfunction(util, 'rename', rename)
> EOF
$ cat > $TESTTMP/killme.py << EOF
@@ -75,7 +83,7 @@
$ printf '%20d' '1' > file
$ hg commit -Aqmc
$ dd if=/dev/zero of=file bs=1k count=128 > /dev/null 2>&1
- $ hg commit -AqmD
+ $ hg commit -AqmD --traceback
Reference size:
$ f -s file
@@ -127,32 +135,33 @@
#endif
-The revlog have been split on disk
+The inline revlog still exist, but a split version exist next to it
$ f -s .hg/store/data/file*
.hg/store/data/file.d: size=132139
- .hg/store/data/file.i: size=256
+ .hg/store/data/file.i: size=132395
+ .hg/store/data/file.i.s: size=256
- $ cat .hg/store/journal | tr -s '\000' ' ' | grep data/file | tail -1
- data/file.i 128
The first file.i entry should match the "Reference size" above.
The first file.d entry is the temporary record during the split,
-The second entry after the split happened. The sum of the second file.d
-and the second file.i entry should match the first file.i entry.
+A "temporary file" entry exist for the split index.
$ cat .hg/store/journal | tr -s '\000' ' ' | grep data/file
data/file.i 1174
data/file.d 0
- data/file.d 1046
- data/file.i 128
+ $ cat .hg/store/journal.backupfiles | tr -s '\000' ' ' | tr -s '\00' ' '| grep data/file
+ data/file.i data/journal.backup.file.i 0
+ data/file.i.s 0
+
+recover is rolling the split back, the fncache is still valid
+
$ hg recover
rolling back interrupted transaction
(verify step skipped, run `hg verify` to check your repository content)
$ f -s .hg/store/data/file*
- .hg/store/data/file.d: size=1046
- .hg/store/data/file.i: size=128
+ .hg/store/data/file.i: size=1174
$ hg tip
changeset: 1:cfa8d6e60429
tag: tip
@@ -161,12 +170,8 @@
summary: b
$ hg verify -q
- warning: revlog 'data/file.d' not in fncache!
- 1 warnings encountered!
- hint: run "hg debugrebuildfncache" to recover from corrupt fncache
$ hg debugrebuildfncache --only-data
- adding data/file.d
- 1 items added, 0 removed from fncache
+ fncache already up to date
$ hg verify -q
$ cd ..
@@ -189,36 +194,43 @@
$ cat > .hg/hgrc <<EOF
> [extensions]
> intercept_rename = $TESTTMP/intercept_before_rename.py
- > [hooks]
- > pretxnchangegroup = python:$TESTTMP/killme.py:killme
> EOF
#if chg
$ hg pull ../troffset-computation
pulling from ../troffset-computation
+ searching for changes
+ adding changesets
+ adding manifests
+ adding file changes
[255]
#else
$ hg pull ../troffset-computation
pulling from ../troffset-computation
+ searching for changes
+ adding changesets
+ adding manifests
+ adding file changes
Killed
[137]
#endif
-The data file is created, but the revlog is still inline
+The inline revlog still exist, but a split version exist next to it
$ f -s .hg/store/data/file*
.hg/store/data/file.d: size=132139
.hg/store/data/file.i: size=132395
+ .hg/store/data/file.i.s: size=256
$ cat .hg/store/journal | tr -s '\000' ' ' | grep data/file
data/file.i 1174
data/file.d 0
- data/file.d 1046
+
+recover is rolling the split back, the fncache is still valid
$ hg recover
rolling back interrupted transaction
(verify step skipped, run `hg verify` to check your repository content)
$ f -s .hg/store/data/file*
- .hg/store/data/file.d: size=1046
.hg/store/data/file.i: size=1174
$ hg tip
changeset: 1:cfa8d6e60429
@@ -235,8 +247,6 @@
Now retry the procedure but intercept the rename of the index.
-Things get corrupted /o\
-
$ hg clone --quiet --rev 1 troffset-computation troffset-computation-crash-after-rename
$ cd troffset-computation-crash-after-rename
@@ -249,21 +259,27 @@
$ cat > .hg/hgrc <<EOF
> [extensions]
> intercept_rename = $TESTTMP/intercept_after_rename.py
- > [hooks]
- > pretxnchangegroup = python:$TESTTMP/killme.py:killme
> EOF
#if chg
$ hg pull ../troffset-computation
pulling from ../troffset-computation
+ searching for changes
+ adding changesets
+ adding manifests
+ adding file changes
[255]
#else
$ hg pull ../troffset-computation
pulling from ../troffset-computation
+ searching for changes
+ adding changesets
+ adding manifests
+ adding file changes
Killed
[137]
#endif
-the revlog has been split on disk
+The inline revlog was over written on disk
$ f -s .hg/store/data/file*
.hg/store/data/file.d: size=132139
@@ -272,16 +288,14 @@
$ cat .hg/store/journal | tr -s '\000' ' ' | grep data/file
data/file.i 1174
data/file.d 0
- data/file.d 1046
+
+recover is rolling the split back, the fncache is still valid
$ hg recover
rolling back interrupted transaction
- abort: attempted to truncate data/file.i to 1174 bytes, but it was already 256 bytes
-
- [255]
+ (verify step skipped, run `hg verify` to check your repository content)
$ f -s .hg/store/data/file*
- .hg/store/data/file.d: size=1046
- .hg/store/data/file.i: size=256
+ .hg/store/data/file.i: size=1174
$ hg tip
changeset: 1:cfa8d6e60429
tag: tip
@@ -290,23 +304,6 @@
summary: b
$ hg verify -q
- abandoned transaction found - run hg recover
- warning: revlog 'data/file.d' not in fncache!
- file@0: data length off by -131093 bytes
- file@2: unpacking fa1120531cc1: partial read of revlog data/file.d; expected 21 bytes from offset 1046, got 0
- file@3: unpacking a631378adaa3: partial read of revlog data/file.d; expected 131072 bytes from offset 1067, got -21
- file@?: rev 2 points to nonexistent changeset 2
- (expected )
- file@?: fa1120531cc1 not in manifests
- file@?: rev 3 points to nonexistent changeset 3
- (expected )
- file@?: a631378adaa3 not in manifests
- not checking dirstate because of previous errors
- 3 warnings encountered!
- hint: run "hg debugrebuildfncache" to recover from corrupt fncache
- 7 integrity errors encountered!
- (first damaged changeset appears to be 0)
- [1]
$ cd ..
Have the transaction rollback itself without any hard crash
@@ -332,11 +329,12 @@
abort: pretxnchangegroup hook exited with status 1
[40]
-File are still split on disk, with the expected size.
+The split was rollback
$ f -s .hg/store/data/file*
- .hg/store/data/file.d: size=1046
- .hg/store/data/file.i: size=128
+ .hg/store/data/file.d: size=0
+ .hg/store/data/file.i: size=1174
+
$ hg tip
changeset: 1:cfa8d6e60429
@@ -346,9 +344,6 @@
summary: b
$ hg verify -q
- warning: revlog 'data/file.d' not in fncache!
- 1 warnings encountered!
- hint: run "hg debugrebuildfncache" to recover from corrupt fncache
$ cd ..
Read race