Patchwork [7,of,7,V3] util: enable hardlink for copyfile

login
register
mail settings
Submitter Jun Wu
Date March 12, 2017, 9:23 a.m.
Message ID <9da40a9e54c419490a2f.1489310636@localhost.localdomain>
Download mbox | patch
Permalink /patch/19139/
State Changes Requested
Headers show

Comments

Jun Wu - March 12, 2017, 9:23 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1489309403 28800
#      Sun Mar 12 01:03:23 2017 -0800
# Node ID 9da40a9e54c419490a2ff23b9dda7acc109f81cd
# Parent  de28b62236a7d47a896bc4aba2bd95dcd8defc87
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9da40a9e54c4
util: enable hardlink for copyfile

Because why not, if we are sure that the filesystem has good hardlink
support (and we are - see "posix: implement a method to get filesystem type
for Linux").

This patch removes the global variable "allowhardlinks" added by "util: add
allowhardlinks module variable". Third party extensions wanting to enable
hardlink support unconditionally can replace "_hardlinkfswhitelist".
Mads Kiilerich - March 12, 2017, 3:02 p.m.
On 03/12/2017 01:23 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1489309403 28800
> #      Sun Mar 12 01:03:23 2017 -0800
> # Node ID 9da40a9e54c419490a2ff23b9dda7acc109f81cd
> # Parent  de28b62236a7d47a896bc4aba2bd95dcd8defc87
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9da40a9e54c4
> util: enable hardlink for copyfile

I guess this should reference 
https://bz.mercurial-scm.org/show_bug.cgi?id=4580 .

Also, I wonder if hghave has_hardlink should be kept in sync with this.

/Mads
Mads Kiilerich - March 12, 2017, 6 p.m.
On 03/12/2017 01:23 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1489309403 28800
> #      Sun Mar 12 01:03:23 2017 -0800
> # Node ID 9da40a9e54c419490a2ff23b9dda7acc109f81cd
> # Parent  de28b62236a7d47a896bc4aba2bd95dcd8defc87
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9da40a9e54c4
> util: enable hardlink for copyfile
>
> Because why not, if we are sure that the filesystem has good hardlink
> support (and we are - see "posix: implement a method to get filesystem type
> for Linux").
>
> This patch removes the global variable "allowhardlinks" added by "util: add
> allowhardlinks module variable". Third party extensions wanting to enable
> hardlink support unconditionally can replace "_hardlinkfswhitelist".

How about using the approximation that all case sensitive filesystems 
are safe to try to use hardlinks? Or to be kind to macOS: All 
filesystems that support symlinks?

e5ce49a30146 was a last minute change and *very* safe, disabling it on 
all platforms.
It seems like the problem only is seen when running Windows on the 
client side - perhaps only disable these hardlinks on Windows? It seems 
like there really is no need for detecting any file systems on Linux?

The infrastructure for detecting file system seems cool, but also quite 
low level and high-maintenance for each platform. If we need to detect 
CIFS, could we perhaps just detect CIFS through generic file system 
operations instead of whitelisting everything that isn't CIFS?

/Mads
Augie Fackler - March 12, 2017, 6:03 p.m.
On Sun, Mar 12, 2017 at 11:00:05AM -0700, Mads Kiilerich wrote:
> On 03/12/2017 01:23 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1489309403 28800
> > #      Sun Mar 12 01:03:23 2017 -0800
> > # Node ID 9da40a9e54c419490a2ff23b9dda7acc109f81cd
> > # Parent  de28b62236a7d47a896bc4aba2bd95dcd8defc87
> > # Available At https://bitbucket.org/quark-zju/hg-draft
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9da40a9e54c4
> > util: enable hardlink for copyfile
> >
> > Because why not, if we are sure that the filesystem has good hardlink
> > support (and we are - see "posix: implement a method to get filesystem type
> > for Linux").
> >
> > This patch removes the global variable "allowhardlinks" added by "util: add
> > allowhardlinks module variable". Third party extensions wanting to enable
> > hardlink support unconditionally can replace "_hardlinkfswhitelist".
>
> How about using the approximation that all case sensitive filesystems are
> safe to try to use hardlinks? Or to be kind to macOS: All filesystems that
> support symlinks?
>
> e5ce49a30146 was a last minute change and *very* safe, disabling it on all
> platforms.
>
> It seems like the problem only is seen when running Windows on the client
> side - perhaps only disable these hardlinks on Windows? It seems like there
> really is no need for detecting any file systems on Linux?

It's actually believed the problem is in the Windows CIFS *server*, if
you read the bug attached to the previous change.

I'm hesitant to turn on hardlinks at all because of the way problems
manifest (unrecoverable data corruption for users, at some random
point in the future), so if we're going to do it we need to be very
conservative to avoid the bug again.

> The infrastructure for detecting file system seems cool, but also quite low
> level and high-maintenance for each platform. If we need to detect CIFS,
> could we perhaps just detect CIFS through generic file system operations
> instead of whitelisting everything that isn't CIFS?
>
> /Mads
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - March 12, 2017, 6:27 p.m.
Excerpts from Augie Fackler's message of 2017-03-12 14:03:44 -0400:
> It's actually believed the problem is in the Windows CIFS *server*, if
> you read the bug attached to the previous change.
> 
> I'm hesitant to turn on hardlinks at all because of the way problems
> manifest (unrecoverable data corruption for users, at some random
> point in the future), so if we're going to do it we need to be very
> conservative to avoid the bug again.

The code only affects transaction backing up for atomatictemp files. I'm not
sure how is it related to manifests.

It is very conservative. It makes sure the information is correct, unless a
power user wants to break us intentionally. None of the network filesystems
(fuse-sshfs, cifs, nfs) is considered safe. And non-Linux systems remain
unsafe. The comment about CIFS may be inaccurate, I can update it. But Patch
1 should be very solid.

Modern filesystems will have new features outside POSIX. For example, if
btrfs becomes more popular, we may have fancy transaction support.
Augie Fackler - March 12, 2017, 6:28 p.m.
> On Mar 12, 2017, at 11:27, Jun Wu <quark@fb.com> wrote:
> 
> Modern filesystems will have new features outside POSIX. For example, if
> btrfs becomes more popular, we may have fancy transaction support.

APFS (Apple's new shiny) has interesting cheap COW directory copy support that we might want to use as well. I think detecting filesystems in general is a worthy idea.
Mads Kiilerich - March 12, 2017, 6:48 p.m.
On 03/12/2017 11:03 AM, Augie Fackler wrote:
> On Sun, Mar 12, 2017 at 11:00:05AM -0700, Mads Kiilerich wrote:
>> On 03/12/2017 01:23 AM, Jun Wu wrote:
>>> # HG changeset patch
>>> # User Jun Wu <quark@fb.com>
>>> # Date 1489309403 28800
>>> #      Sun Mar 12 01:03:23 2017 -0800
>>> # Node ID 9da40a9e54c419490a2ff23b9dda7acc109f81cd
>>> # Parent  de28b62236a7d47a896bc4aba2bd95dcd8defc87
>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9da40a9e54c4
>>> util: enable hardlink for copyfile
>>>
>>> Because why not, if we are sure that the filesystem has good hardlink
>>> support (and we are - see "posix: implement a method to get filesystem type
>>> for Linux").
>>>
>>> This patch removes the global variable "allowhardlinks" added by "util: add
>>> allowhardlinks module variable". Third party extensions wanting to enable
>>> hardlink support unconditionally can replace "_hardlinkfswhitelist".
>> How about using the approximation that all case sensitive filesystems are
>> safe to try to use hardlinks? Or to be kind to macOS: All filesystems that
>> support symlinks?
>>
>> e5ce49a30146 was a last minute change and *very* safe, disabling it on all
>> platforms.
>>
>> It seems like the problem only is seen when running Windows on the client
>> side - perhaps only disable these hardlinks on Windows? It seems like there
>> really is no need for detecting any file systems on Linux?
> It's actually believed the problem is in the Windows CIFS *server*, if
> you read the bug attached to the previous change.

I only see mentioning of problems with Windows on the client side. 
Matt's theory of the source of the cache coherency issue suggested that 
it was interaction between client and server side caches. Non-windows 
client side implementations may or may not have the same problem, but I 
see nothing suggesting they have.

That might of course be because most users of repos on CIFS are Windows 
users. The problem is serious when it happens, but considering the 
non-Windows uncertainty, the small amount of non-Windows users using 
CIFS repos, and the negative impact on all non-Windows users, it might 
be justified to be less conservative for non-Windows.

/Mads


> I'm hesitant to turn on hardlinks at all because of the way problems
> manifest (unrecoverable data corruption for users, at some random
> point in the future), so if we're going to do it we need to be very
> conservative to avoid the bug again.
>
>> The infrastructure for detecting file system seems cool, but also quite low
>> level and high-maintenance for each platform. If we need to detect CIFS,
>> could we perhaps just detect CIFS through generic file system operations
>> instead of whitelisting everything that isn't CIFS?
>>
>> /Mads
Augie Fackler - March 12, 2017, 6:53 p.m.
> On Mar 12, 2017, at 11:48, Mads Kiilerich <mads@kiilerich.com> wrote:
> 
> That might of course be because most users of repos on CIFS are Windows users. The problem is serious when it happens, but considering the non-Windows uncertainty, the small amount of non-Windows users using CIFS repos, and the negative impact on all non-Windows users, it might be justified to be less conservative for non-Windows.

I'm much more comfortable with a plan that just avoids hard links on all filesystem types we don't *know* will work, especially when it comes to network filesystems or FUSE filesystems.
Jun Wu - March 12, 2017, 7:18 p.m.
Excerpts from Mads Kiilerich's message of 2017-03-12 11:48:35 -0700:
> I only see mentioning of problems with Windows on the client side. 
> Matt's theory of the source of the cache coherency issue suggested that 
> it was interaction between client and server side caches. Non-windows 
> client side implementations may or may not have the same problem, but I 
> see nothing suggesting they have.
> 
> That might of course be because most users of repos on CIFS are Windows 
> users. The problem is serious when it happens, but considering the 
> non-Windows uncertainty, the small amount of non-Windows users using 
> CIFS repos, and the negative impact on all non-Windows users, it might 
> be justified to be less conservative for non-Windows.

Per discussion with Augie yesterday, I prefer the very conservative
approach. That's why I added the "_isprocgenuine" check which looks
expensive but will greatly increase our confidence, and spent 2 hours
checking kernel code, doing experiments, and writing the commit message.

> 
> /Mads
David Soria Parra - March 14, 2017, 5:12 p.m.
On Sun, Mar 12, 2017 at 01:23:56AM -0800, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1489309403 28800
> #      Sun Mar 12 01:03:23 2017 -0800
> # Node ID 9da40a9e54c419490a2ff23b9dda7acc109f81cd
> # Parent  de28b62236a7d47a896bc4aba2bd95dcd8defc87
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9da40a9e54c4
> util: enable hardlink for copyfile

I've looked through this in the last hours and I think the patch series is fine.
It turns out it is much more conservative then gnulib on which df relies, which
just read /proc/self/mountinfo and /proc/self/mounts and can potentially be
fooled.

Note that we probably want to per-process cache the output of _getfstypetable().

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1058,9 +1058,4 @@  def checksignature(func):
     return check
 
-# Hardlinks are problematic on CIFS, do not allow hardlinks
-# until we find a way to work around it cleanly (issue4546).
-# This is a variable so extensions can opt-in to using them.
-allowhardlinks = False
-
 # a whilelist of known filesystems where hardlink works reliably
 _hardlinkfswhitelist = set([
@@ -1094,5 +1089,5 @@  def copyfile(src, dest, hardlink=False, 
         if fstype not in _hardlinkfswhitelist:
             hardlink = False
-    if allowhardlinks and hardlink:
+    if hardlink:
         try:
             oslink(src, dest)
diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks-whitelisted.t
copy from tests/test-hardlinks.t
copy to tests/test-hardlinks-whitelisted.t
--- a/tests/test-hardlinks.t
+++ b/tests/test-hardlinks-whitelisted.t
@@ -1,3 +1,9 @@ 
 #require hardlink
+#require hardlink-whitelisted
+
+This test is similar to test-hardlinks.t, but will only run on some filesystems
+that we are sure to have known good hardlink supports (see issue4546 for an
+example where the filesystem claims hardlink support but is actually
+problematic).
 
   $ cat > nlinks.py <<EOF
@@ -167,5 +173,5 @@  Push to repo r1 should break up most har
   1 r2/.hg/store/data/d1/f2.i
   2 r2/.hg/store/data/f1.i
-  1 r2/.hg/store/fncache
+  2 r2/.hg/store/fncache
 
   $ hg -R r2 verify
@@ -192,5 +198,5 @@  Committing a change to f1 in r1 must bre
   1 r2/.hg/store/data/d1/f2.i
   1 r2/.hg/store/data/f1.i
-  1 r2/.hg/store/fncache
+  2 r2/.hg/store/fncache
 
 
@@ -234,9 +240,9 @@  r4 has hardlinks in the working dir (not
   2 r4/.hg/store/undo.backupfiles
   2 r4/.hg/store/undo.phaseroots
-  2 r4/.hg/undo.backup.dirstate
+  4 r4/.hg/undo.backup.dirstate
   2 r4/.hg/undo.bookmarks
   2 r4/.hg/undo.branch
   2 r4/.hg/undo.desc
-  2 r4/.hg/undo.dirstate
+  4 r4/.hg/undo.dirstate
   2 r4/d1/data1
   2 r4/d1/f2
@@ -273,9 +279,9 @@  Update back to revision 11 in r4 should 
   2 r4/.hg/store/undo.backupfiles
   2 r4/.hg/store/undo.phaseroots
-  2 r4/.hg/undo.backup.dirstate
+  4 r4/.hg/undo.backup.dirstate
   2 r4/.hg/undo.bookmarks
   2 r4/.hg/undo.branch
   2 r4/.hg/undo.desc
-  2 r4/.hg/undo.dirstate
+  4 r4/.hg/undo.dirstate
   2 r4/d1/data1
   2 r4/d1/f2
diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
--- a/tests/test-hardlinks.t
+++ b/tests/test-hardlinks.t
@@ -167,5 +167,5 @@  Push to repo r1 should break up most har
   1 r2/.hg/store/data/d1/f2.i
   2 r2/.hg/store/data/f1.i
-  1 r2/.hg/store/fncache
+  [12] r2/\.hg/store/fncache (re)
 
   $ hg -R r2 verify
@@ -192,5 +192,5 @@  Committing a change to f1 in r1 must bre
   1 r2/.hg/store/data/d1/f2.i
   1 r2/.hg/store/data/f1.i
-  1 r2/.hg/store/fncache
+  [12] r2/\.hg/store/fncache (re)
 
 
@@ -234,9 +234,9 @@  r4 has hardlinks in the working dir (not
   2 r4/.hg/store/undo.backupfiles
   2 r4/.hg/store/undo.phaseroots
-  2 r4/.hg/undo.backup.dirstate
+  [24] r4/\.hg/undo\.backup\.dirstate (re)
   2 r4/.hg/undo.bookmarks
   2 r4/.hg/undo.branch
   2 r4/.hg/undo.desc
-  2 r4/.hg/undo.dirstate
+  [24] r4/\.hg/undo\.dirstate (re)
   2 r4/d1/data1
   2 r4/d1/f2
@@ -273,9 +273,9 @@  Update back to revision 11 in r4 should 
   2 r4/.hg/store/undo.backupfiles
   2 r4/.hg/store/undo.phaseroots
-  2 r4/.hg/undo.backup.dirstate
+  [24] r4/\.hg/undo\.backup\.dirstate (re)
   2 r4/.hg/undo.bookmarks
   2 r4/.hg/undo.branch
   2 r4/.hg/undo.desc
-  2 r4/.hg/undo.dirstate
+  [24] r4/\.hg/undo\.dirstate (re)
   2 r4/d1/data1
   2 r4/d1/f2