[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