Patchwork [1,of,1,STABLE,V2] merge: increase safety of parallel updating/removing on icasefs

login
register
mail settings
Submitter Katsunori FUJIWARA
Date April 29, 2013, 7:07 a.m.
Message ID <b1dccec8c4a05fd7e853.1367219251@feefifofum>
Download mbox | patch
Permalink /patch/1494/
State Superseded
Commit 5cc71484ee9c9d370cc2a4cce205a8a59867096e
Headers show

Comments

Katsunori FUJIWARA - April 29, 2013, 7:07 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1367218695 -32400
#      Mon Apr 29 15:58:15 2013 +0900
# Branch stable
# Node ID b1dccec8c4a05fd7e8531fc8f4dcb954ab2aca78
# Parent  f01a351db79106ba96ac6d527cf69944fd98e665
merge: increase safety of parallel updating/removing on icasefs

"merge.applyupdates()" sorts "actions" in removal first order, and
"workeractions" derived from it should be also sorted.

If each actions in "workeractions" are executed in serial, this
sorting ensures that merging/updating process is collision free,
because updating the file in target context is always executed after
removing the existing file which causes case-folding collision against
the former.

In the other hand, if each actions are executed in parallel, updating
on a worker process may be executed before removing on another worker
process, because "worker.partition()" partitions list of actions
regardless of type of each actions.

This patch divides "workeractions" into removing and updating, and
executes the former first.

This patch still scans "actions"/"workeractions" some times for ease
of patch review, even though large list may cost much in this way.
(total cost should be as same as before)

This also changes some tests, because dividing "workeractions" affects
progress indication.
Mads Kiilerich - April 29, 2013, 10:32 a.m.
On 04/29/2013 09:07 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1367218695 -32400
> #      Mon Apr 29 15:58:15 2013 +0900
> # Branch stable
> # Node ID b1dccec8c4a05fd7e8531fc8f4dcb954ab2aca78
> # Parent  f01a351db79106ba96ac6d527cf69944fd98e665
> merge: increase safety of parallel updating/removing on icasefs

I guess the same problem could happen if a file is replaced by a 
directory. That could increase the severity from 'just' wrong casing on 
iOS to random failures on Linux.

We have tests for that case, but it will of course never use workers 
with the unfortunate partitioning of actions.

It would be nice if we could have a test for this case.

It seems like we don't have any test coverage of the new parallel update 
in the test suite - is that really true?

/Mads


>
> "merge.applyupdates()" sorts "actions" in removal first order, and
> "workeractions" derived from it should be also sorted.
>
> If each actions in "workeractions" are executed in serial, this
> sorting ensures that merging/updating process is collision free,
> because updating the file in target context is always executed after
> removing the existing file which causes case-folding collision against
> the former.
>
> In the other hand, if each actions are executed in parallel, updating
> on a worker process may be executed before removing on another worker
> process, because "worker.partition()" partitions list of actions
> regardless of type of each actions.
>
> This patch divides "workeractions" into removing and updating, and
> executes the former first.
>
> This patch still scans "actions"/"workeractions" some times for ease
> of patch review, even though large list may cost much in this way.
> (total cost should be as same as before)
>
> This also changes some tests, because dividing "workeractions" affects
> progress indication.
>
Mads Kiilerich - April 30, 2013, 9:43 a.m.
On 04/30/2013 08:10 AM, Noel Grandin wrote:
> Maybe create a debug flag to force it on? Then the same set of tests 
> could be rerun with the flag set.

The tricky part is to do it in a way where we are sure to exercise all 
the worst possible partitioning and scheduling ... and in a reliable way 
and without doubling the time it takes to run the test suite.

/Mads
Matt Mackall - April 30, 2013, 4:06 p.m.
On Mon, 2013-04-29 at 16:07 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1367218695 -32400
> #      Mon Apr 29 15:58:15 2013 +0900
> # Branch stable
> # Node ID b1dccec8c4a05fd7e8531fc8f4dcb954ab2aca78
> # Parent  f01a351db79106ba96ac6d527cf69944fd98e665
> merge: increase safety of parallel updating/removing on icasefs

Queued for stable, thanks.
Bryan O'Sullivan - April 30, 2013, 7:05 p.m.
On Tue, Apr 30, 2013 at 2:43 AM, Mads Kiilerich <mads@kiilerich.com> wrote:

> The tricky part is to do it in a way where we are sure to exercise all the
> worst possible partitioning and scheduling ... and in a reliable way and
> without doubling the time it takes to run the test suite.
>

Probably the right way to do this is to set up some config that allows us
to force parallel updates to be used on platforms where it exists, and then
to always run the test suite with this flag set (unless specifically
directed not to).
Idan Kamara - May 3, 2013, 8:20 a.m.
On Tue, Apr 30, 2013 at 10:05 PM, Bryan O'Sullivan <bos@serpentine.com>wrote:

>
> On Tue, Apr 30, 2013 at 2:43 AM, Mads Kiilerich <mads@kiilerich.com>wrote:
>
>> The tricky part is to do it in a way where we are sure to exercise all
>> the worst possible partitioning and scheduling ... and in a reliable way
>> and without doubling the time it takes to run the test suite.
>>
>
> Probably the right way to do this is to set up some config that allows us
> to force parallel updates to be used on platforms where it exists, and then
> to always run the test suite with this flag set (unless specifically
> directed not to).
>

There's probably enough logic to justify unit tests as well.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -455,8 +455,10 @@ 
 
     numupdates = len(actions)
     workeractions = [a for a in actions if a[1] in 'gr']
-    updated = len([a for a in workeractions if a[1] == 'g'])
-    removed = len([a for a in workeractions if a[1] == 'r'])
+    updateactions = [a for a in workeractions if a[1] == 'g']
+    updated = len(updateactions)
+    removeactions = [a for a in workeractions if a[1] == 'r']
+    removed = len(removeactions)
     actions = [a for a in actions if a[1] not in 'gr']
 
     hgsub = [a[1] for a in workeractions if a[0] == '.hgsubstate']
@@ -465,7 +467,13 @@ 
 
     z = 0
     prog = worker.worker(repo.ui, 0.001, getremove, (repo, mctx, overwrite),
-                         workeractions)
+                         removeactions)
+    for i, item in prog:
+        z += i
+        repo.ui.progress(_('updating'), z, item=item, total=numupdates,
+                         unit=_('files'))
+    prog = worker.worker(repo.ui, 0.001, getremove, (repo, mctx, overwrite),
+                         updateactions)
     for i, item in prog:
         z += i
         repo.ui.progress(_('updating'), z, item=item, total=numupdates,
diff --git a/tests/test-issue672.t b/tests/test-issue672.t
--- a/tests/test-issue672.t
+++ b/tests/test-issue672.t
@@ -37,6 +37,7 @@ 
    1: other deleted -> r
    1a: remote created -> g
   removing 1
+  updating: 1 1/2 files (50.00%)
   getting 1a
   updating: 1a 2/2 files (100.00%)
   1 files updated, 0 files merged, 1 files removed, 0 files unresolved
diff --git a/tests/test-rename-dir-merge.t b/tests/test-rename-dir-merge.t
--- a/tests/test-rename-dir-merge.t
+++ b/tests/test-rename-dir-merge.t
@@ -46,6 +46,7 @@ 
    b/b: remote created -> g
   removing a/a
   removing a/b
+  updating: a/b 2/5 files (40.00%)
   getting b/a
   getting b/b
   updating: b/b 4/5 files (80.00%)
diff --git a/tests/test-rename-merge2.t b/tests/test-rename-merge2.t
--- a/tests/test-rename-merge2.t
+++ b/tests/test-rename-merge2.t
@@ -290,6 +290,7 @@ 
    rev: versions differ -> m
     preserving rev for resolve of rev
   removing a
+  updating: a 1/3 files (33.33%)
   getting b
   updating: b 2/3 files (66.67%)
   updating: rev 3/3 files (100.00%)
diff --git a/tests/test-update-reverse.t b/tests/test-update-reverse.t
--- a/tests/test-update-reverse.t
+++ b/tests/test-update-reverse.t
@@ -73,6 +73,7 @@ 
    main: remote created -> g
   removing side1
   removing side2
+  updating: side2 2/3 files (66.67%)
   getting main
   updating: main 3/3 files (100.00%)
   1 files updated, 0 files merged, 2 files removed, 0 files unresolved