[PATCH 2 of 2 RFC] localrepo: make it possible to use flock in a repo

[Gregory Szorc]

On Fri, May 19, 2017 at 8:26 PM, Jun Wu <quark at fb.com> wrote:

> Thanks for the detailed comment! Replied inline.
>
> Excerpts from Gregory Szorc's message of 2017-05-19 18:03:58 -0700:
> > From a high level, this approach seems reasonable. There are some obvious
> > missing components to the implementation:
> >
> > * Docs for the new requirement in internals/requirements.txt
> > * Ability to create new repos with the requirement
> >
> > I initially thought it surprising that we didn't check for flock support
> at
> > repo open time as part of validating requirements. But your commit
> message
> > explains that we'll error at lock time upon missing flock support. Since
> we
> > don't have reader locks and I think it is user hostile to lock readers
> out
> > of repos no longer supporting flock, I think this is fine. But there
> should
> > be a test demonstrating the behavior of a flock requirement without flock
> > support.
>
> Lacking of tests, docs, debug commands (to turn this on and off) will be
> addressed once this graduates from RFC.
>
> > Also, I'm going to bikeshed the requirement name. Requirements are global
> > and will persist for all of time. With that context in mind, "flock" is
> > arguably too generic. What this feature/requirement actually resembles is
> > "use flock() for the locking strategy used by the 'store'
>
> More precisely, the requirement is "svfs.base" being a different directory
> from "vfs.base".
>
> > layout/requirement which implies use of flock() on the lock and wlock
> > files." The meaning of the "flock" requirement can thus only be
> interpreted
> > in the presence of another requirement, such as "store." After all, if
> > there is a "store2" requirement, then "flock" likely takes on new meaning
>
> If "store2" provides a directory, like repo.svfs = vfs('.hg/store2').
> The existing code will just work without modification.
>

OK. I failed to understand that it is locking directories and not files.

I don't want to go down this rat hole, but since it may help my point...

The next store format will likely use some form of "generational store."
Files protected by transactions that aren't append only (like bookmarks and
phases) will exist in a directory corresponding to their
transaction/generation number. e.g. .hg/store/txn0/bookmarks,
.hg/store/txn1500/phaseroots. This allows readers to have consistent
snapshots since we never rewrite existing bytes: we just copy or create
files and have a pointer to the "current" generation. In this model you
have reader locks so you know when there are no more active consumers for a
generation and can GC its files.

Anyway, the locking model may be radically different. You can't rely on it
being based on whole directories. So the semantics of a "flock" requirement
could vary drastically depending on the store and other repo features in
use. And I think having the semantics of a requirement vary depending on
the presence of other requirements is not ideal. You want a requirement to
mean one thing and one thing only and not be subject to multiple
interpretations.


>
> So I don't think it's necessary to distinguish "store-flock" and
> "store2-flock".
>
> > (since we'd likely use a different locking strategy). I believe
> > requirements should be narrowly tailored to the exact feature they
> support.
> > Otherwise if their meaning changes over time, that opens us up to bugs
> and
> > possibly repo corruption. So I think a more appropriate requirement name
> is
> > something like "store-flock" so the limits of its application (to the
> lock
> > and wlock files for the "store" requirement) are explicit and not subject
> > to reinterpretation. Semantically, this is probably ideally expressed as
> a
> > sub-requirement. e.g. "store+flock." But I'd rather punt on that
> discussion
> > until we have a few more requirements and the relationship between them
> is
> > less clear.
>
> With a second thought, I think it may be possible to just unify the locks
> if
> svfs.base == vfs.base. That could be done by letting repo.lock returns
> repo.wlock() in this special case (no store + flock).
>

The "store" requirement was added in Mercurial 0.9.2. The chances of this
special case of
no store + flock occurring in 2017 are approximately 0%. I dare say we
should add code to
prevent combinations of requirements like this because they make no sense
and are a footgun waiting to fire. To prove my point, I just created a repo
with format.usestore=false and experimental.treemanifest=true. It did seem
to work, surprisingly. But it would be foolish to run such a config given
the pre-store format has been out of style for over 10 years.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170519/93a4e99a/attachment.html>