[PATCH 4 of 4] mmapindex: set default to 1MB
Boris FELD
boris.feld at octobus.net
Fri Jan 18 09:04:25 EST 2019
On 18/01/2019 13:32, Yuya Nishihara wrote:
> On Wed, 16 Jan 2019 15:23:57 +0100, Boris FELD wrote:
>> On 11/01/2019 15:46, Yuya Nishihara wrote:
>>> On Fri, 11 Jan 2019 09:59:49 +0100, Boris FELD wrote:
>>>> On 11/01/2019 01:05, Gregory Szorc wrote:
>>>>> On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld at octobus.net
>>>>> <mailto:boris.feld at octobus.net>> wrote:
>>>>>
>>>>> On 08/01/2019 15:41, Yuya Nishihara wrote:
>>>>> > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote:
>>>>> >> On 03/01/2019 09:58, Yuya Nishihara wrote:
>>>>> >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote:
>>>>> >>>> On 04/12/2018 12:09, Yuya Nishihara wrote:
>>>>> >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote:
>>>>> >>>>>> # HG changeset patch
>>>>> >>>>>> # User Boris Feld <boris.feld at octobus.net
>>>>> <mailto:boris.feld at octobus.net>>
>>>>> >>>>>> # Date 1542949784 -3600
>>>>> >>>>>> # Fri Nov 23 06:09:44 2018 +0100
>>>>> >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f
>>>>> >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e
>>>>> >>>>>> # EXP-Topic mmap
>>>>> >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>>>>> >>>>>> # hg pull
>>>>> https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458
>>>>> >>>>>> mmapindex: set default to 1MB
>>>>> >>>>> Can you check if strip/rollback properly copy the revlog
>>>>> before truncating it?
>>>>> >>>>>
>>>>> >>>>> If a mmapped revlog is truncated by another process, the
>>>>> mapped memory could be
>>>>> >>>>> invalid. The worst case, the process would be killed by SIGBUS.
>>>>> >>>> Hum good catch, process reading a repository being stripped
>>>>> have always
>>>>> >>>> been up for troubles. However, mmap makes it worse by raising
>>>>> a signal
>>>>> >>>> instead of just having wonky seek. It also introduces new
>>>>> cases where
>>>>> >>>> this can happen.
>>>>> >>> mmap isn't worse because of SIGBUS, but because the index data
>>>>> can be updated
>>>>> >>> after the index length is determined. Before, a single
>>>>> in-memory revlog index
>>>>> >>> was at least consistent.
>>>>> >>>
>>>>> >>>> What shall we do here, I guess our best bet is to intercept
>>>>> these SIGBUS
>>>>> >>>> when reading revlog index?
>>>>> >> Yes, but it would be inconsistent with the data it was pointing to.
>>>>> >> Access to this data would result in error too.
>>>>> > Correct.
>>>>> >
>>>>> >> The new thing is that we
>>>>> >> can get SIGBUS while accessing the index data themselves, as
>>>>> you are
>>>>> >> pointing out.
>>>>> > Another new thing is that truncated revisions can be seen as
>>>>> valid changesets
>>>>> > of '000...' hash with 0-length text. If the whole (or maybe
>>>>> dozens of)
>>>>> > revisions were truncated, SIGBUS would be raised. In other
>>>>> cases, the truncated
>>>>> > part would probably be read as zeros.
>>>>> >
>>>>> >>> I don't think it'll be easy to handle SIGBUS properly. And
>>>>> SIGBUS won't always
>>>>> >>> be raised. Perhaps, the easiest solution is to copy the revlog
>>>>> index before
>>>>> >>> strip/rollback.
>>>>> >> I'm afraid at the performance impact, we are talking of potentially
>>>>> >> hundreds of MB of index data to be rolled back.
>>>>> >>
>>>>> >> Maybe we can keep the current truncation in normal transaction
>>>>> rollback
>>>>> >> and use the slower copies for the hg strip command itself (and
>>>>> rewrite)?
>>>>> >>
>>>>> >> However, I'm afraid we need to come up with a solution for mmap
>>>>> as it
>>>>> >> would be useful to use it more and more.
>>>>> >>
>>>>> >> Maybe we can come up with something catching the SIGBUS? Or
>>>>> maybe we
>>>>> >> need to never truncate files and keep an alternative way to
>>>>> track the
>>>>> >> maximum offset? Any other ideas?
>>>>> > I've no idea. My point is that catching SIGBUS wouldn't save us
>>>>> from many
>>>>> > possible failures.
>>>>> >
>>>>> >>> IIRC, the mmap implementation was highly experimental. I don't
>>>>> know if it's
>>>>> >>> widely tested other than in FB where strip is never used.
>>>>> >> We have been using it internally, and one of our clients
>>>>> deployed it
>>>>> >> too. It results in significant speed and memory improvement.
>>>>> > Yup. I'm just afraid of enabling it by default.
>>>>>
>>>>> Ok, what do you think we should do for 4.9?
>>>>>
>>>>>
>>>>> We're introducing a new repo requirement in 4.9 for sparse revlogs.
>>>> We are not introducing a new repo requirement in 4.9 for sparse-revlogs.
>>>> The repo-requirements and its guarantees have been introduced in 4.7.
>>>>> I believe this strip problem (along with a ton of other problems) goes
>>>>> away if we have reader locks on repos.
>>>>>
>>>>> Since we are already introducing a new repo requirement for new repos,
>>>>> we can introduce yet more repo requirements without any additional
>>>>> significant user inconvenience.
>>>> (except we are not introducing a new requirement)
>>>>> I know it is a stretch since we only have a few days left before the
>>>>> 4.9 RC, but is anybody will to implement reader locks (or some other
>>>>> repo mutual exclusion mechanism that could mitigate the strip
>>>>> problem)? Of course, reader locks break the ability to open a
>>>>> repository on a read-only filesystem. So, we shouldn't use them
>>>>> lightly! But we could do something like write a repo/transaction
>>>>> "generation number" into .hg/store and read it to detect repo
>>>>> mutations before certain operations. We could potentially also have
>>>>> "mmap mode" require writing a file, socket file, etc into .hg/store
>>>>> and have any writer refuse to strip if there is an active reader doing
>>>>> mmap.
>>>>>
>>>>> That's a lot of random ideas, I know. But I think that in order to
>>>>> enable mmap by default, we need to lock out mutations on mmap'd file
>>>>> segments in order to prevent crashes. This seemingly requires a reader
>>>>> lock or transaction changes of some form.
>>>> We are a week away from the freeze and reader locks are neither written
>>>> nor tested. In addition, reader locks solve the strip issue but come
>>>> with their own set of troubles, the first one being that they require
>>>> write access.
>>>>
>>>> I don't think it is realistic to use reader lock to solve the mmap
>>>> question right now. Having a new repository format to handle better
>>>> handle transaction is something we will need, but also a significant
>>>> amount of work and testing.
>>> Agreed. I don't think reader locks would be implemented correctly in 5 days.
>>> It'll require more careful inspection given our storage codes splattered
>>> around various modules. I also think mmap is in the same boat. It isn't
>>> a drop-in replacement for read()/write(). IMHO, we should carefully design
>>> the data structure and the operations so that mmap-ed data are reliably
>>> processed.
>>>
>>>> I would rather focus on having a "graceful" way to catch SIGBUS (and
>>>> potential other errors that Yuya pointed out). The mmap function has
>>>> been tested in multiple places without troubles, and it raises large
>>>> benefit. Just consolidating the potential crashing corner case seems
>>>> good enough to get it shipped to all. We can spend more time later to
>>>> narrow these corner case if they prove troublesome.
>>>>
>>>> In all case, we have the easy fallback of disabling it again in case of
>>>> need, it does not affect the on-disk format.
>>>>
>>>> Yuya, what do you think?
>>> My vote is to not enable the mmap by default. I'm okay to move the option
>>> out of the experimental so it gets more reachable by experienced users.
>>> FWIW, it appears not documented.
>> Okay, so let's put mmaped indexes back to disabled by default for this
>> release. The potential SIGBUS/zero issues pointed by Yuya seems serious
>> enough.
>>
>> I'm tempted to move the config back to experimental too until we decide
>> what final form the feature will take. Given the issue we need to solve
>> we will probably have to change some of the read/write/truncates
>> patterns and possibly the on-disk format. This will need a new
>> requirement. The simplest option is probably to have strip/rollback copy
>> the file over, but this will require careful performance testing.
>>
>> Moving mmap back to experimental means backing out 74a9f428227e and
>> 875d2af8cb4e.
> Can you send backout patches?
>
> Matt Harbison found Windows issue. Perhaps, an mmapped revlog file would
> be locked while it's open, and couldn't be updated by another process.
>
> https://groups.google.com/d/msg/thg-dev/X7RLiEzSPm0/qiTMjDRNEAAJ
Okay, series coming.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list