At Thu, 18 Aug 2016 20:53:02 +0900, FUJIWARA Katsunori wrote: > > > At Wed, 17 Aug 2016 13:35:41 -0700, > Durham Goode wrote: > > > > On 8/10/16 1:01 AM, FUJIWARA Katsunori wrote: > > > At Mon, 8 Aug 2016 18:17:13 -0700, > > > Durham Goode wrote: > > >> # HG changeset patch > > >> # User Durham Goode <dur...@fb.com> > > >> # Date 1470697899 25200 > > >> # Mon Aug 08 16:11:39 2016 -0700 > > >> # Node ID 6a4c09571793d56c8dad1a6760e3fc1293b9a0b6 > > >> # Parent f73abdf84e8e2eb9e2029cb28e2246a55b2d2f49 > > >> manifest: use property instead of field for manifest revlog storage > > >> > > >> The file caches we're using to avoid reloading the manifest from disk > > >> everytime > > >> has an annoying bug that causes the in memory structure to not be > > >> reloaded if > > >> the mtime and the size haven't changed. This causes a breakage in the > > >> tests > > >> because the manifestlog is not being reloaded after a commit+strip > > >> operation in > > >> mq (the mtime is the same because it all happens in the same second, and > > >> the > > >> resulting size is the same because we add 1 and remove 1). The only > > >> reason this > > >> doesn't affect the manifest itself is because we touch it so often that > > >> we > > >> had already reloaded it after the commit, but before the strip. > > > All of main tasks of ExactCacheValidationPlan were included into main > > > repository before release of 3.9. > > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_ExactCacheValidationPlan&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=HQ7AyzrzLXk4BBn1y_0Ie6XNfoDjDZ9vlLuu7fzE-Nw&s=NEEztqsTZV1sABBrBz8cXUPhWSd1PHC-bmXCI4JPFM8&e= > > > > > > Therefore, change of file should be detected at validation of file > > > cache, even if writing changed file out (without extra handling in > > > this plan) causes same ctime/mtime/size/i-node and so on. > > From looking at filecachesubentry.changed(), it looks like it just does > > 'self.cachestat != newstat'. Where is the code that's supposed to > > handle this case? > > If file is: > > - created via util.atomictempfile with checkambig=True > - renamed via vfs.rename() with checkambig=True > - copied via util.copyfile() with checkambig=True > > mtime of newly created file is +1, and this ensures that > "oldstat.mtime != newstat.mtime". > > Therefore, just checking "self.cachestat != newstat" can detect change > of file (if ExactCacheValidationPlan works as expected :-)) > > > > Perhaps it was some issue with filecachesubentry.refresh(). For > > instance, it seems like all the filecache entries are 'refreshed' when a > > transaction closes. If the series of events was: > > > > 1. read repo.manifest (it's now in the file cache) > > 2. read repo.manifestlog (it's now in the file cache) > > 3. lock, open transaction > > 4. add commit (mq stuff; in memory manifest is refreshed) > > 5. strip different commit (mq stuff) > > 6. close transaction + lock (refresh all file caches - i.e. blindly > > updates their stat data) > > 7. read manifestlog <- this doesn't read anything from disk because the > > close transaction told it that all filecaches are in good shape, but > > this was never read inside the transaction, and therefore it is not in > > good shape. > > > > It's been several weeks since I investigated this bug, so my memory is > > fuzzy and this might not be the correct analysis. > > > > > Would you tell me the case to reproduce the issue around caching > > > manifest, if it still occurs on recent Mercurial ? It helps to find > > > out code paths overlooked by ExactCacheValidationPlan. > > > > > $ hg pull -r 7f14ccb https://bitbucket.org/DurhamG/hg > > $ hg up 7f14ccb > > > > Apply this change: > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > > --- a/mercurial/localrepo.py > > +++ b/mercurial/localrepo.py > > @@ -504,7 +504,7 @@ class localrepository(object): > > def manifest(self): > > return manifest.manifest(self.svfs) > > > > - @property > > + @storecache('00manifest.i') > > def manifestlog(self): > > return manifest.manifestlog(self.svfs, self) > > > > > > $ ./run-tests.py test-mq-merge.t > > Thank you for information! I can reproduce the issue (BTW, you would > mention "integrity check failed on 00manifest.i" at "hg qpush -m", > wouldn't you?)
I found the root cause of "integrity check failed on 00manifest.i" at "hg qpush -m" while additional working for ExactCacheValidationPlan. At first, preconditions: A. at closing transaction, _refreshfilecachestats() is invoked to refresh filestat of filecache-ed properties in localrepository. This expects that all cached properties should be consistent with committed transaction result. https://selenic.com/repo/hg/file/3.9/mercurial/localrepo.py#l1090 B. truncation while strip() is also causes _refreshfilecachestats(), because stripping logic is executed under the transaction Therefore, keeping filecache properties even after strip() is expected behavior. C. repo.invalidate() makes filecache properties a kind of "pending" status Cached objects are still kept in repo._filecache, but they are removed from repo.__dict__. This "pending" status is cleared, when a property is accessed (and filestat isn't changed on disk). repo.invalidate() is executed at the beginning of (s)lock scope. D. _refreshfilecachestats() omits updating filestat of filecache property, if that property is "pending" "k not in self.__dict__" case: https://selenic.com/repo/hg/file/3.9/mercurial/localrepo.py#l1271 Then, at 7f14ccbbadab in your repo: 1. updating filestat of repo.manifest property is omitted while some transactions before strip() in the issue case because repo.commitctx() uses repo.manifestlog instead of repo.manifest. 2. the object instantiated for repo.manifest is also cached by repo.manifestlog._revlog field Call the former MF1, and the latter MFL1 3. accessing to repo.manifest at the beginning of strip() causes re-instantiating manifest object because filestat of repo.manifest property differs from actual filestat of 00manifest.i file on disk at this point: see (1) above. Call newly created object MF2. 4. stripping applies strip() on MF2 This removes some nodes added before stripping from MF2. But MF1 referred by MFL1 still contains stripped nodes. In addition to it, _mancache of MFL1 also contains already stripped nodes. At this point, MFL1 breaks precondition (A): cached property should be consistent with committed transaction result. 5. subsequent committing might cause inconsistent result because of invalid MFL1 for example, in test-mq-merge.t case, both #6 and #7 entries in MF1 have linkrev=6 (I'm not sure about detail flow of inconsistent result). > I confirmed that this issue can be avoided by some extra changes for > ExactCacheValidationPlan. I'll send patch series tomorrow or so. FYI, mergeone() has 3 lock scopes and 4 transaction scopes detecting integrity error as below :-) mergeone(): self.apply(): with repo.lock(): # repo.invalidate(), here with repo.transaction(): do_committing # repo._refreshfilecachestats(), here strip.strip(): with repo.lock(): # repo.invalidate(), here repair.strip(): with repo.transaction(): do_stripping # repo._refreshfilecachestats(), here with repo.transaction(): do_bookmark_repairing # repo._refreshfilecachestats(), here # (*) newcommit(): # => repo.commit() with repo.lock(): # repo.invalidate(), here with repo.transaction(): do_committing # repo._refreshfilecachestats(), here self.printdiff(): cmdutil.diffordiffstat(): # integrity error is detected here "some extra changes for ExactCacheValidationPlan" makes repo.manifestlog property detect change of 00manifest.i on disk at (*) in above flow. repo.manifestlog is "pending" while stripping, because repair.strip() only changes repo.manifest. > ---------------------------------------------------------------------- > [FUJIWARA Katsunori] fo...@lares.dti.ne.jp > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel ---------------------------------------------------------------------- [FUJIWARA Katsunori] fo...@lares.dti.ne.jp _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel