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@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.