[PATCH 4 of 5] manifest: use property instead of field for manifest revlog storage
FUJIWARA Katsunori
foozy at lares.dti.ne.jp
Thu Aug 18 07:53:02 EDT 2016
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 <durham at 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 confirmed that this issue can be avoided by some extra changes for
ExactCacheValidationPlan. I'll send patch series tomorrow or so.
----------------------------------------------------------------------
[FUJIWARA Katsunori] foozy at lares.dti.ne.jp
More information about the Mercurial-devel
mailing list