[PATCH 3 of 5] branchmap: acquires lock before writting the rev branch cache

Pierre-Yves David pierre-yves.david at ens-lyon.org
Mon Aug 8 10:53:41 EDT 2016



On 08/08/2016 04:50 PM, Mads Kiilerich wrote:
> On Sun, Aug 7, 2016 at 1:21 PM, Yuya Nishihara <yuya at tcha.org
> <mailto:yuya at tcha.org>> wrote:
>
>     On Sat, 06 Aug 2016 01:02:55 +0200, Pierre-Yves David wrote:
>     > # HG changeset patch
>     > # User Pierre-Yves David <pierre-yves.david at ens-lyon.org
>     <mailto:pierre-yves.david at ens-lyon.org>>
>     > # Date 1470401836 -7200
>     > #      Fri Aug 05 14:57:16 2016 +0200
>     > # Node ID 84885aeec2e442d18dcb70fcce2d65dd9fafbf91
>     > # Parent  fa3f05a4219c4ea6b9bc9682f258e4418b36c265
>     > # EXP-Topic vfsward
>     > branchmap: acquires lock before writting the rev branch cache
>     >
>     > We now attempt to acquire a lock and write the branch cache within
>     that lock.
>     > This would prevent cache corruption when multiple processes try to
>     write the cache
>     > at the same time.
>
>     (+CC Durham, Mads)
>
>     I wonder if revbranchcache is designed to not take a lock.
>     9347c15d8136 says
>     corruption can be recovered.
>
>
> Yes, I think it was given as an initial design constraint for acceptance
> upstream that the cache had to be updated on the fly by read only
> operations without locking. Hindsight perspective might have changed
> mine and others memory and opinion ;-)
>
> In my opinion, the revision branch cache should be treated exactly like
> the branch head cache and updated the same way. It might be expensive to
> update initially, but not doing it will end up being more expensive.
>
> Proper transaction support would however also require that the design
> constraint of doing in-place (non-atomic non-append) updates got lifted.
>
> The existing algorithm is not perfect lockfree, but I doubt it was
> possible to come up with a test case where simultaneous writes would
> give a corruption that not would be detected and recovered automatically.
>
> This change looks like a fine small improvement that will reduce the
> risk. But still, like most other write locks in Mercurial, it has the
> problem that data is read and checked before taking the lock and then
> "leaks" into the locked zone.

Yeah, it is not perfect.

> I wonder but haven't investigated: How will this handle the case where
> the repo can't be locked because it is on a read only filesystem? Will
> it get LockHeld (which would be a misleading exception name) and skip
> the writing?

It will raise LockUnavailable, yuya fixed it in flight

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list