Cygwin Filesystem Performance degradation 1.7.5 vs 1.7.7, and methods for improving performance

[Yoni Londner]

Hi,

 >> Abstract: I prepared a patch that improves Cygwin Filesystem
 >> performance by x4 on Cygwin 1.7.5 (1.7.5 vanilla 530ms -->  1.7.5
 >> patched 120ms). I ported the patch to 1.7.7, did tests, and found
 >> out that 1.7.7 had a very serious 9x (!) performance degradation
 >> from 1.7.5 (1.7.5 vanilla 530ms -->  1.7.7 vanilla 3900ms -->  1.7.7
 >> patched 3500ms), which does makes this patch useless until the
 >> performance degradation is fixed.
 >
 > The problem is, I can't reproduce such a degradation.  If I run sometimg
 > like `time ls -l /bin>  /dev/null', the times are very slightly better
 > with 1.7.7 than with 1.7.5 (without caching effect 1200ms vs. 1500ms,
 > with caching effect 500ms vs. 620ms on average).  Starting with 1.7.6,
 > Cygwin reused handles from symlink_info::check in stat() and other
 > syscalls.  If there is such degradation under some circumstances, I need
 > a reproducible scenario, or at least some strace output which shows at
 > which point this happens.  Apart from actual patches this should be
 > discussed on the cygwin-developer list.
 >

First of all, even your results of 1200-1500ms (1st time) and 500-600ms 
(2nd time) is still way way way too long. On linux with an NTFS mount of 
C:/cygwin, this took <2ms!

And even on Win32 CMD.EXE this same operation will take you less than 
100ms. which is 5x to 10x faster.

The main reason for the difference: the Windows CMD.EXE does not open 
file handles, which make the NTFS file system to actually go and read 
each file's first 16KB of contents (even though you did not ask for it!).

My results above were on XP SP3, with anti-virus installed (which is a 
VERY common scenario in Enterprises).

On Win7 without an anti-virus (except the built-in MS anti-virus), my 
results are:
Without the patch 1.7.7: 800ms
With the patch 1.7.7: 320ms

strace is no good at profiling, since it itself modifies the time-flow 
of the execution.

The correct way to measure performance and pin-point it to specific APIs 
stack-traces is by running the long repeated operations (for example: 
'gdb --args ls -l /bin /usr/bin /etc...' so that it will take a LONG 
time to complete, and then pressing CTRL-C in the middle and seeing 
where it is 'stuck'.

After repeating this 4-5 times you easily see what are the Win32 
syscalls that make it slow, since their stack-trace will reappear again 
and again.

The points in the code that I worked on performance improvements in the 
submitted patch were the places which appeared repeatedly in the 
profiling check I did.

 >> My patch:
 >> ------------
 >
 > First of all, your patch is very certainly significant in terms of code
 > size.  If you want to contribute code from it, we need a signed copyright
 > assignment from you.  See http://cygwin.com/contrib.html, the "Before you
 > get started" section.
 >
 > The patch is also missing a ChangeLog entry.  I only took a quick glance
 > over the patch itself.  The code doesn't look correctly formatted in GNU
 > style.  Also, using the diff -up flags would be helpful.
 >

I sent the copyright assignment by snail.
I the patch again, this time using the '-up' options.
I also 'ifdef'ed my changes, each under a different ifdef, so it can be 
easily read.

 >> So I did some performace test on Cygwin 1.7.5, and found out the
 >> biggest bottleneck were:
 >>
 >> - CreateFile()/CloseHandle() on syscalls that only need file node
 >> info retrival, not file contents retrival (such as stat()). This can
 >> be solved by calling Win32 that do not open the File handle itself,
 >> rather query by name, or opening the directory handle (which is MUCH
 >> faster).
 >
 > That's what I tried to speed up in 1.7.6 by keeping the handle from
 > symlink_info::check for reuse in stat.
 >

Its correct that in 1.7.6 you improved re-using of file handles, yet 
there is still some unnecessary openings of file handles, so my patch 
further improves the re-use in additional code-flows.

The 'PC_KEEP_HANDLE' flag you added in 1.7.6 is nice, but a bit problematic.
In my code I managed in many cases not to open a handle at all, and when 
the caller calls with PC_KEEP_HANDLE, then I must open the handle even 
though it was not needed.

The correct way is to open handles on-demand, exactly where they are 
REALLY needed.
So the check_symlink should always return a handle, IF it opened it, and 
the caller should never assume the handle is already open, and the 
caller should open the handle on-demand only when it ACTUALLY needs the 
handle (in which case it will first check if it already has it open, and 
call CreateFile() if not).

Its important to bring down to zero all the unnecessary file handle 
openings.

 >> - ACLs: Windows is not a secure system. And its ACL model is strange
 >> to say the least... Most cygwin users do not use the unix
 >> permissions system on cygwin to achieve security. All they want is
 >> that the unix tools will run on Windows.
 >
 > I don't agree.  Windows is a secure system on the file level if users
 > use the ACLs correctly.  It's not exactly a strange model, it's just
 > unnecessarily complicated for home users.  However, in corporate
 > environments it's utilized a lot.
 >
 > Besides, I made some performance tests myself before 1.7.6, and it
 > turned out that the ACL checks for testing the POSIX permission bits
 > are really not the problem.  The problem was that ls(1) had a test which
 > resulted in calling getacl() twice, just to print the '+' character
 > attached to the POSIX permission bits in `ls -l' output.  That should
 > have gotten slightly better with the latest coreutils.
 >
 > Other than that, just use the "noacl" mount option if you don't like to
 > be bothered with Windows ACLs or, FWIW, POSIX-like permissions.
 >

ACLs are a huge performance degradation, and the mount opetions are a 
very crud implementation for enable/disable.

Changing a mount option is a little like a 'reboot' to the cygwin 
'operating system'. To change it on C:/ you need to close all cygwin 
apps and reopen them.
Also, it is global. It does not allow some applications to work with 
ACLs, and some without.

For example, if compilation or rsync works 4x faster with ACLs off, but 
you still want ACLs enabled for everything else, you can just do in bash:
alias rsync_fast=CYGWIN=noacl /bin/rsync
alias make_fast=CYGWIN=noacl /bin/make

mount options are global and 'per-boot'.
Since it is such a big performance issue, it shou

Microsoft's moto is to force decisions on the end-user. The unix world 
is all about giving the end-user choice. So why Cygwin not give the end 
user the option to dynamically per-process disable ACLs if he wants to?

My run-time per-process ACL disabling options are in ADDITION to the 
mount options, not instead. Let the end-user will decide what to use.

 >> - stat inode number and inode link count: getting the inode in
 >> Windows requires a File handle or Directory handle (not possible to
 >> get inode by file name). Very few applications actually need a REAL
 >> inode number. So using the 'get file info by name' apis are used
 >> (and of course there is an envirnoment if you really want real inode
 >> numbers).
 >
 > Just use the "ihash" mount option if you don't like to use real inode
 > numbers.  However, you don't see hardlinks anymore.
 >
 > I don't like the idea to extend the CYGWIN environment variable with
 > this functionality at all.  I also don't like the idea that these
 > settings are on by default.  So by default you're trading correctness
 > (in a POSIX sense) for speed.  Speed is all well and nice, but it
 > shouldn't be handled more important than correctness.
 >
 > There are certainly other ways to improve Cygwin's performance, which
 > don't trade in correctness in the first place.  We should more
 > concentrate on intelligent caching of file info and handles.
 >

Again this is a matter of global-static options (via mount), or dynamic 
per process/session options via envirnoment.

Again my patch gives the user an OPTION. He can select via mount options 
AND/OR he can select via envirnoment.

For example, he may select to have everything with ihash (for speed), 
yet he might want a specific rsync operation to preserve hardlinks:
alias rsync_h=rsync --hard-links

Give the end-user choice!

 >> - GetVolumeInfo: The C:\ drive does not tend to be changed every
 >> millisecond! Therefore no reason for every filesystem syscall to
 >> call it. Caching this further increased the performance.
 >
 > Does your FS caching take volume mount points into account?
 >

I think it does, though not 100% sure. Your input on this would be welcome!

 >> - File security check: GetSecurityInfo() is implemented in Windows
 >> in usermode dll (Advapi32.dll). It calls at the end
 >> NtQuerySecurityObject(). So I implemented a faster version:
 >> zGetSecurityInfo() which does the same... just faster.
 >
 > Does your code preserve the inheritance entries which are available via
 > GetSecurityInfo?  Note that I don't care at all for the GetSecurityInfo
 > API from a usage POV.  I would prefer to use NtQuerySecurityObject
 > directly.  However, the sole reason for using GetSecurityInfo is the
 > fact that NtQuerySecurityObject only returns the plain ACL as it's
 > stored for the file.  It does not return the ACEs which are inherited
 > from the parent objects.  These are only available via GetSecurityInfo,
 > or by checking the parent ACLs manually.
 >

I followed the assembly of the Windows It performs exactly what Windows 
user-mode Advapi32.dll does.

So its 100% compatible.

 >> - symbolic link files: Opening a file and reading its contents is an
 >> expensive operation. All file cygwin operations must check whether
 >> the file is a symbolic link or not, which is done by opening the
 >> file and reading its contents and checking whether it has the
 >> symlink magic at the beginning of this. Since symbolic link must be
 >
 > That's not correct.  It only opens files which have cetain DOS flags
 > set.  Symlinks are either files with a SYSTEM DOS flag, or .lnk files
 > with the R/O DOS flag set, or reparse points.  So your extension for
 > the test might help a tiny little bit, but since the SYSTEM attribute
 > is only rarely set on file which are not Cygwin symlinks, it's a very
 > rare operation.

Still: why open a file that is by its size we already know for sure that 
it cannot possibly a symlink?!

Yoni

On 6/9/2010 4:24 PM, Corinna Vinschen wrote:
> Hi Yoni,
>
> On Sep  6 12:52, Yoni Londner wrote:
>> Hi,
>>
>> Abstract: I prepared a patch that improves Cygwin Filesystem
>> performance by x4 on Cygwin 1.7.5 (1.7.5 vanilla 530ms -->  1.7.5
>> patched 120ms). I ported the patch to 1.7.7, did tests, and found
>> out that 1.7.7 had a very serious 9x (!) performance degradation
>> from 1.7.5 (1.7.5 vanilla 530ms -->  1.7.7 vanilla 3900ms -->  1.7.7
>> patched 3500ms), which does makes this patch useless until the
>> performance degradation is fixed.
>
> The problem is, I can't reproduce such a degradation.  If I run sometimg
> like `time ls -l /bin>  /dev/null', the times are very slightly better
> with 1.7.7 than with 1.7.5 (without caching effect 1200ms vs. 1500ms,
> with caching effect 500ms vs. 620ms on average).  Starting with 1.7.6,
> Cygwin reused handles from symlink_info::check in stat() and other
> syscalls.  If there is such degradation under some circumstances, I need
> a reproducible scenario, or at least some strace output which shows at
> which point this happens.  Apart from actual patches this should be
> discussed on the cygwin-developer list.
>
>> =======================================================================
>> My patch:
>> ------------
>
> First of all, your patch is very certainly significant in terms of code
> size.  If you want to contribute code from it, we need a signed copyright
> assignment from you.  See http://cygwin.com/contrib.html, the "Before you
> get started" section.
>
> The patch is also missing a ChangeLog entry.  I only took a quick glance
> over the patch itself.  The code doesn't look correctly formatted in GNU
> style.  Also, using the diff -up flags would be helpful.
>
>> So I did some performace test on Cygwin 1.7.5, and found out the
>> biggest bottleneck were:
>>
>> - CreateFile()/CloseHandle() on syscalls that only need file node
>> info retrival, not file contents retrival (such as stat()). This can
>> be solved by calling Win32 that do not open the File handle itself,
>> rather query by name, or opening the directory handle (which is MUCH
>> faster).
>
> That's what I tried to speed up in 1.7.6 by keeping the handle from
> symlink_info::check for reuse in stat.
>
>> - repetitive Win32 Kernel calls on a single syscall (stat() would
>> call up to 5 CreateFile/CloseHandle pairs plus another additional 5
>> to 10 Win32 APIs).
>> on stat() system calls, managed to improve the performance of ls
>> approx. by 4 times. This can be solved by caching: first time in a
>> syscall the API is called the result is stored, so the second time
>> the info is needed, it is re-used.
>
> I agree.  We could use caching fileinfo more extensively, though carefully,
> at least for the time of a single syscall.
>
>> - ACLs: Windows is not a secure system. And its ACL model is strange
>> to say the least... Most cygwin users do not use the unix
>> permissions system on cygwin to achieve security. All they want is
>> that the unix tools will run on Windows.
>
> I don't agree.  Windows is a secure system on the file level if users
> use the ACLs correctly.  It's not exactly a strange model, it's just
> unnecessarily complicated for home users.  However, in corporate
> environments it's utilized a lot.
>
> Besides, I made some performance tests myself before 1.7.6, and it
> turned out that the ACL checks for testing the POSIX permission bits
> are really not the problem.  The problem was that ls(1) had a test which
> resulted in calling getacl() twice, just to print the '+' character
> attached to the POSIX permission bits in `ls -l' output.  That should
> have gotten slightly better with the latest coreutils.
>
> Other than that, just use the "noacl" mount option if you don't like to
> be bothered with Windows ACLs or, FWIW, POSIX-like permissions.
>
>> - stat inode number and inode link count: getting the inode in
>> Windows requires a File handle or Directory handle (not possible to
>> get inode by file name). Very few applications actually need a REAL
>> inode number. So using the 'get file info by name' apis are used
>> (and of course there is an envirnoment if you really want real inode
>> numbers).
>
> Just use the "ihash" mount option if you don't like to use real inode
> numbers.  However, you don't see hardlinks anymore.
>
> I don't like the idea to extend the CYGWIN environment variable with
> this functionality at all.  I also don't like the idea that these
> settings are on by default.  So by default you're trading correctness
> (in a POSIX sense) for speed.  Speed is all well and nice, but it
> shouldn't be handled more important than correctness.
>
> There are certainly other ways to improve Cygwin's performance, which
> don't trade in correctness in the first place.  We should more
> concentrate on intelligent caching of file info and handles.
>
>> - GetVolumeInfo: The C:\ drive does not tend to be changed every
>> millisecond! Therefore no reason for every filesystem syscall to
>> call it. Caching this further increased the performance.
>
> Does your FS caching take volume mount points into account?
>
>> - File security check: GetSecurityInfo() is implemented in Windows
>> in usermode dll (Advapi32.dll). It calls at the end
>> NtQuerySecurityObject(). So I implemented a faster version:
>> zGetSecurityInfo() which does the same... just faster.
>
> Does your code preserve the inheritance entries which are available via
> GetSecurityInfo?  Note that I don't care at all for the GetSecurityInfo
> API from a usage POV.  I would prefer to use NtQuerySecurityObject
> directly.  However, the sole reason for using GetSecurityInfo is the
> fact that NtQuerySecurityObject only returns the plain ACL as it's
> stored for the file.  It does not return the ACEs which are inherited
> from the parent objects.  These are only available via GetSecurityInfo,
> or by checking the parent ACLs manually.
>
>> - symbolic link files: Opening a file and reading its contents is an
>> expensive operation. All file cygwin operations must check whether
>> the file is a symbolic link or not, which is done by opening the
>> file and reading its contents and checking whether it has the
>> symlink magic at the beginning of this. Since symbolic link must be
>
> That's not correct.  It only opens files which have cetain DOS flags
> set.  Symlinks are either files with a SYSTEM DOS flag, or .lnk files
> with the R/O DOS flag set, or reparse points.  So your extension for
> the test might help a tiny little bit, but since the SYSTEM attribute
> is only rarely set on file which are not Cygwin symlinks, it's a very
> rare operation.
>
>
> Corinna
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cygwin.patch
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20100913/bbae2e43/attachment.ksh>