[PATCH v5] ntdll/directory: Add support for EXT4 case folding per directory
This adds support for checking the case-insensitive attribute on ext4 with newer kernels so that Wine can rely on it for performance. 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. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v5: Only use the file descriptor on Linux. The second patch is not resent since it's in staging for users to test out first before going upstream. dlls/ntdll/directory.c | 44 +++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c index bbdbbe9..9b4e61b 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,6 +132,12 @@ 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 + #ifndef O_DIRECTORY # define O_DIRECTORY 0200000 /* must be directory */ #endif @@ -1113,7 +1121,7 @@ static int get_dir_case_sensitivity_attr( const char *dir ) * get_dir_case_sensitivity_stat * * Checks if the volume containing the specified directory is case - * sensitive or not. Uses statfs(2) or statvfs(2). + * sensitive or not. Uses (f)statfs(2), statvfs(2), fstatat(2), or ioctl(2). */ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) { @@ -1165,14 +1173,23 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) return FALSE; #elif defined(__linux__) + BOOLEAN sens = FALSE; struct statfs stfs; struct stat st; - char *cifile; + int fd, flags; + + if ((fd = open( dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE )) == -1) + return FALSE; + + if (ioctl( fd, EXT2_IOC_GETFLAGS, &flags ) != -1 && (flags & EXT4_CASEFOLD_FL)) + goto end; /* Only assume CIOPFS is case insensitive. */ - if (statfs( dir, &stfs ) == -1) return FALSE; + if (fstatfs( fd, &stfs ) == -1) goto end; + + sens = TRUE; if (stfs.f_type != 0x65735546 /* FUSE_SUPER_MAGIC */) - return TRUE; + goto end; /* Normally, we'd have to parse the mtab to find out exactly what * kind of FUSE FS this is. But, someone on wine-devel suggested * a shortcut. We'll stat a special file in the directory. If it's @@ -1180,17 +1197,12 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) * This will break if somebody puts a file named ".ciopfs" in a non- * CIOPFS directory. */ - cifile = RtlAllocateHeap( GetProcessHeap(), 0, strlen( dir )+sizeof("/.ciopfs") ); - if (!cifile) return TRUE; - strcpy( cifile, dir ); - strcat( cifile, "/.ciopfs" ); - if (stat( cifile, &st ) == 0) - { - RtlFreeHeap( GetProcessHeap(), 0, cifile ); - return FALSE; - } - RtlFreeHeap( GetProcessHeap(), 0, cifile ); - return TRUE; + if (fstatat( fd, ".ciopfs", &st, AT_NO_AUTOMOUNT ) == 0) + sens = FALSE; + +end: + close( fd ); + return sens; #else return TRUE; #endif @@ -1201,7 +1213,7 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) * get_dir_case_sensitivity * * Checks if the volume containing the specified directory is case - * sensitive or not. Uses statfs(2) or statvfs(2). + * sensitive or not. Uses multiple methods, depending on platform. */ static BOOLEAN get_dir_case_sensitivity( const char *dir ) { -- 2.21.0
ntdll/directory: -> ntdll: On Thu, Jun 13, 2019 at 02:44:58PM +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. 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.
This could be re-worded a bit. In particular mentioning the file descriptor change is unnecessary.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
v5: Only use the file descriptor on Linux. The second patch is not resent since it's in staging for users to test out first before going upstream.
dlls/ntdll/directory.c | 44 +++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c index bbdbbe9..9b4e61b 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,6 +132,12 @@ 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 + #ifndef O_DIRECTORY # define O_DIRECTORY 0200000 /* must be directory */ #endif @@ -1113,7 +1121,7 @@ static int get_dir_case_sensitivity_attr( const char *dir ) * get_dir_case_sensitivity_stat * * Checks if the volume containing the specified directory is case - * sensitive or not. Uses statfs(2) or statvfs(2). + * sensitive or not. Uses (f)statfs(2), statvfs(2), fstatat(2), or ioctl(2). */ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) { @@ -1165,14 +1173,23 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) return FALSE;
#elif defined(__linux__) + BOOLEAN sens = FALSE; struct statfs stfs; struct stat st; - char *cifile; + int fd, flags; + + if ((fd = open( dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE )) == -1) + return FALSE;
I think returning TRUE in an error case here is the correct thing to do. TRUE is after all the default.
+ + if (ioctl( fd, EXT2_IOC_GETFLAGS, &flags ) != -1 && (flags & EXT4_CASEFOLD_FL)) + goto end;
/* Only assume CIOPFS is case insensitive. */ - if (statfs( dir, &stfs ) == -1) return FALSE; + if (fstatfs( fd, &stfs ) == -1) goto end; + + sens = TRUE; if (stfs.f_type != 0x65735546 /* FUSE_SUPER_MAGIC */) - return TRUE; + goto end; /* Normally, we'd have to parse the mtab to find out exactly what * kind of FUSE FS this is. But, someone on wine-devel suggested * a shortcut. We'll stat a special file in the directory. If it's @@ -1180,17 +1197,12 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) * This will break if somebody puts a file named ".ciopfs" in a non- * CIOPFS directory. */ - cifile = RtlAllocateHeap( GetProcessHeap(), 0, strlen( dir )+sizeof("/.ciopfs") ); - if (!cifile) return TRUE; - strcpy( cifile, dir ); - strcat( cifile, "/.ciopfs" ); - if (stat( cifile, &st ) == 0) - { - RtlFreeHeap( GetProcessHeap(), 0, cifile ); - return FALSE; - } - RtlFreeHeap( GetProcessHeap(), 0, cifile ); - return TRUE; + if (fstatat( fd, ".ciopfs", &st, AT_NO_AUTOMOUNT ) == 0) + sens = FALSE; + +end: + close( fd ); + return sens;
This could all be re-written without the gotos which would make it much clearer. Something like: BOOLEAN sens = TRUE; if ((fd = open( dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE )) == -1) return TRUE; if (ioctl( fd, EXT2_IOC_GETFLAGS, &flags ) != -1 && (flags & EXT4_CASEFOLD_FL)) { sens = FALSE; } else { /* Long comment about some chat on wine-devel... */ if ((fstatfs( fd, &stfs ) != -1) && (stfs.f_type == 0x65735546 /* FUSE_SUPER_MAGIC */) && (fstatat( fd, ".ciopfs", &st, AT_NO_AUTOMOUNT ) == 0)) sens = FALSE; } close( fd ); return sens;
On 6/13/19 4:45 PM, Huw Davies wrote:
ntdll/directory: -> ntdll:
On Thu, Jun 13, 2019 at 02:44:58PM +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. 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.
This could be re-worded a bit. In particular mentioning the file descriptor change is unnecessary.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
v5: Only use the file descriptor on Linux. The second patch is not resent since it's in staging for users to test out first before going upstream.
dlls/ntdll/directory.c | 44 +++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c index bbdbbe9..9b4e61b 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,6 +132,12 @@ 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 + #ifndef O_DIRECTORY # define O_DIRECTORY 0200000 /* must be directory */ #endif @@ -1113,7 +1121,7 @@ static int get_dir_case_sensitivity_attr( const char *dir ) * get_dir_case_sensitivity_stat * * Checks if the volume containing the specified directory is case - * sensitive or not. Uses statfs(2) or statvfs(2). + * sensitive or not. Uses (f)statfs(2), statvfs(2), fstatat(2), or ioctl(2). */ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) { @@ -1165,14 +1173,23 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) return FALSE;
#elif defined(__linux__) + BOOLEAN sens = FALSE; struct statfs stfs; struct stat st; - char *cifile; + int fd, flags; + + if ((fd = open( dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE )) == -1) + return FALSE;
I think returning TRUE in an error case here is the correct thing to do. TRUE is after all the default.
Yeah it seemed fishy to me as well but the previous code returned FALSE on error so I assumed it had some reason to do it. I'll do all the changes, thanks.
participants (2)
-
Gabriel Ivăncescu -
Huw Davies