[PATCH v3 1/2] ntdll/directory: Add support for EXT4 case folding per directory

[Huw Davies]

On Fri, May 24, 2019 at 03:09:34PM +0300, Gabriel Ivăncescu wrote:
> This adds support for checking the case-insensitive attribute on ext4 with
> newer kernels so that Wine can rely on it for performance.
> 
> It has some conditional processing for perf reasons. Checking for the
> EXT4_CASEFOLD_FL attribute involves an ioctl, which operates on file
> descriptors, while all the former checks operated directly on the dir pathname
> (e.g. statfs).
> 
> Obviously, it's best to avoid looking up the directory multiple times (also
> for correctness, so it refers to the same dir). So in the case that we *do*
> have a file descriptor, then use it everywhere, with e.g. fstatfs.
> 
> However, to avoid a perf regression or downgrade on systems that don't
> support the EXT2_IOC_GETFLAGS ioctl (e.g. MacOS, FreeBSD), we continue
> using statfs and the like directly, this shaves off two syscalls (open/close).
> 
> But in the case the EXT4_CASEFOLD_FL is not supported on Linux (i.e. on
> current kernels) or the directory doesn't have it, this will unfortunately
> involve a bit more syscalls now, because it has to open() and close() the fd,
> but it shouldn't be too much of a problem. (the fstatfs and fstatat make it
> less impactful somewhat, so they won't have to lookup the directory again,
> hopefully mitigating some of it)
> 
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> ---
> 
> v3: Clean up the code and don't have unused parameters in any case.
> 
> GET_DIR_CASE_SENSITIVITY_USE_FD is used so it can be as generic as possible
> for fast tweaking and future expansion on other platforms, if they get
> something similar.
> 
> As it is now, on Linux, we always set the EXT4_CASEFOLD_FL flag
> ourselves. Thus, I can of course remove the .ciopfs old code and keep just
> the fstatat() call by assuming we always use fd on Linux, if you prefer it
> that way, to keep the code smaller. I didn't remove it because I didn't
> want to "hardcode" this assumption, so please let me know if that should
> be done or not.
> 
>  dlls/ntdll/directory.c | 73 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 65 insertions(+), 8 deletions(-)
> 
> diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
> index bbdbbe9..354c00f 100644
> --- a/dlls/ntdll/directory.c
> +++ b/dlls/ntdll/directory.c
> @@ -115,6 +115,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(file);
>  
>  /* just in case... */
>  #undef VFAT_IOCTL_READDIR_BOTH
> +#undef EXT2_IOC_GETFLAGS
> +#undef EXT4_CASEFOLD_FL
>  
>  #ifdef linux
>  
> @@ -130,12 +132,25 @@ typedef struct
>  /* Define the VFAT ioctl to get both short and long file names */
>  #define VFAT_IOCTL_READDIR_BOTH  _IOR('r', 1, KERNEL_DIRENT [2] )
>  
> +/* Define the ext2 ioctl for handling extra attributes */
> +#define EXT2_IOC_GETFLAGS _IOR('f', 1, long)
> +
> +/* Case-insensitivity attribute */
> +#define EXT4_CASEFOLD_FL 0x40000000
> +

I'm not sure we need to define these ourselves, we can
just pull them from the headers.  I realise that you're
copying what was done with VFAT_IOCTL_READDIR_BOTH and
I don't know why that was done that way, but unless
there's a reason to do this, let's not add more.


> +/* Use a file descriptor for the case-sensitivity check if we have to */
> +#if defined(EXT2_IOC_GETFLAGS) && defined(EXT4_CASEFOLD_FL)
> +#define GET_DIR_CASE_SENSITIVITY_USE_FD 1
> +#else
> +#define GET_DIR_CASE_SENSITIVITY_USE_FD 0
> +#endif

This is now just used in one place, so get rid of it use the
preprocessor condition at the relevant place below.

>  static BOOLEAN get_dir_case_sensitivity( const char *dir )
>  {
> +    int case_sensitive, fd = -1;
> +
>  #if defined(HAVE_GETATTRLIST) && defined(ATTR_VOL_CAPABILITIES) && \
>      defined(VOL_CAPABILITIES_FORMAT) && defined(VOL_CAP_FMT_CASE_SENSITIVE)
> -    int case_sensitive = get_dir_case_sensitivity_attr( dir );
> +    case_sensitive = get_dir_case_sensitivity_attr( dir );
>      if (case_sensitive != -1) return case_sensitive;
>  #endif
> -    return get_dir_case_sensitivity_stat( dir );
> +
> +    if (GET_DIR_CASE_SENSITIVITY_USE_FD)

#if defined(EXT2_IOC_GETFLAGS) && defined(EXT4_CASEFOLD_FL)

No need for the if block.

> +    {
> +        if ((fd = open(dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE)) == -1)
> +            return TRUE;
> +        if ((case_sensitive = get_dir_case_sensitivity_ioctl(fd)) != -1)
> +            goto end;
> +    }
> +    case_sensitive = get_dir_case_sensitivity_stat(dir, fd);
> +
> +end:
> +    if (fd != -1) close(fd);
> +    return case_sensitive;
>  }

FWIW I'm not too convinced about the name/fd thing, but it now doesn't
look too ugly, so I guess it's ok.

Huw.