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