Patchwork [1,of,7] fncache: remove the rewriting logic

login
register
mail settings
Submitter Durham Goode
Date March 25, 2014, 2:33 a.m.
Message ID <ab3be74e8022c31b7c95.1395714830@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/4052/
State Superseded
Commit cd03854a2e060179eae490a00560dca9c5c4eba9
Headers show

Comments

Durham Goode - March 25, 2014, 2:33 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1395700307 25200
#      Mon Mar 24 15:31:47 2014 -0700
# Node ID ab3be74e8022c31b7c95975fb09b3602ed7775d8
# Parent  3879ac3858ffd9bb46e19fcc3a2b31d7bb2b54c5
fncache: remove the rewriting logic

The fncache could rewrite itself during a read operation if it noticed any
entries that were no longer on disk. This was problematic because it caused
Mercurial to perform write operations outside the scope of a lock or
transaction, which could interefere with any other pending writes.

This will be replaced in a future patch by logic that cleans up the fncache
as files are deleted during strips.
Matt Mackall - March 26, 2014, 7:26 p.m.
On Mon, 2014-03-24 at 19:33 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1395700307 25200
> #      Mon Mar 24 15:31:47 2014 -0700
> # Node ID ab3be74e8022c31b7c95975fb09b3602ed7775d8
> # Parent  3879ac3858ffd9bb46e19fcc3a2b31d7bb2b54c5
> fncache: remove the rewriting logic
> 
> The fncache could rewrite itself during a read operation if it noticed any
> entries that were no longer on disk. This was problematic because it caused
> Mercurial to perform write operations outside the scope of a lock or
> transaction, which could interefere with any other pending writes.
> 
> This will be replaced in a future patch by logic that cleans up the fncache
> as files are deleted during strips.

From an ordering-of-patches standpoint, this looks problematic. I'd like
to take this off your plate, but since I have questions about the
subsequent patches, that would leave us in a state where there's nothing
that rewrites the fncache at all, right?
Durham Goode - March 31, 2014, 5:29 p.m.
On 3/26/14 12:26 PM, "Matt Mackall" <mpm@selenic.com> wrote:

>On Mon, 2014-03-24 at 19:33 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1395700307 25200
>> #      Mon Mar 24 15:31:47 2014 -0700
>> # Node ID ab3be74e8022c31b7c95975fb09b3602ed7775d8
>> # Parent  3879ac3858ffd9bb46e19fcc3a2b31d7bb2b54c5
>> fncache: remove the rewriting logic
>> 
>> The fncache could rewrite itself during a read operation if it noticed
>>any
>> entries that were no longer on disk. This was problematic because it
>>caused
>> Mercurial to perform write operations outside the scope of a lock or
>> transaction, which could interefere with any other pending writes.
>> 
>> This will be replaced in a future patch by logic that cleans up the
>>fncache
>> as files are deleted during strips.
>
>From an ordering-of-patches standpoint, this looks problematic. I'd like
>to take this off your plate, but since I have questions about the
>subsequent patches, that would leave us in a state where there's nothing
>that rewrites the fncache at all, right?

You are correct.  Getting rid of this first made future patches cleaner
and more correct (since this logic doesn't play nice when transactions are
introduced). Having junk data in the fncache doesn't seem to be an issue
though, since it's lazily cleaned up and only in certain scenarios.

Patch

diff --git a/mercurial/store.py b/mercurial/store.py
--- a/mercurial/store.py
+++ b/mercurial/store.py
@@ -409,10 +409,6 @@ 
         fp.close()
         self._dirty = False
 
-    def rewrite(self, files):
-        self._write(files, False)
-        self.entries = set(files)
-
     def write(self):
         if self._dirty:
             self._write(self.entries, True)
@@ -476,7 +472,6 @@ 
         return self.rawvfs.stat(path).st_size
 
     def datafiles(self):
-        rewrite = False
         existing = []
         for f in sorted(self.fncache):
             ef = self.encode(f)
@@ -486,12 +481,6 @@ 
             except OSError, err:
                 if err.errno != errno.ENOENT:
                     raise
-                # nonexistent entry
-                rewrite = True
-        if rewrite:
-            # rewrite fncache to remove nonexistent entries
-            # (may be caused by rollback / strip)
-            self.fncache.rewrite(existing)
 
     def copylist(self):
         d = ('data dh fncache phaseroots obsstore'