Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
On 03/12/2017 01:23 AM, Jun Wu wrote: # HG changeset patch # User Jun Wu # 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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
On 03/12/2017 01:23 AM, Jun Wu wrote: # HG changeset patch # User Jun Wu # 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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
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 > > # 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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
> On Mar 12, 2017, at 11:27, Jun Wu 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
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 # 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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
> On Mar 12, 2017, at 11:48, Mads Kiilerich 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.___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7 V3] util: enable hardlink for copyfile
On Sun, Mar 12, 2017 at 01:23:56AM -0800, Jun Wu wrote: > # HG changeset patch > # User Jun Wu > # 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(). ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
