[PATCH v4 1/2] 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. 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(a)gmail.com> --- v4: Simplify the #if checks and get rid of the GET_DIR_CASE_SENSITIVITY_USE_FD constant. Huw, I know you wanted to #include the header that defines EXT4_CASEFOLD_FL and others. I resent it without that so that Alexandre can see how the patch looks like, if he doesn't want to add another dependency. I know he's away today so his reply will have to wait probably after 4.10, I'll make sure to remind him if he doesn't, though ;-) That, plus the fact I'm really bad at autotools so I'll end up copy-pasting some similar check. Also, in case we do add a configure check for it, should we display a warning if it's not found? Like "ext2_fs.h not available, case-folding per directory will be unsupported." dlls/ntdll/directory.c | 69 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c index bbdbbe9..2d6563e 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 @@ -1109,18 +1117,36 @@ static int get_dir_case_sensitivity_attr( const char *dir ) } #endif +#ifdef EXT4_CASEFOLD_FL + +/*********************************************************************** + * get_dir_case_sensitivity_ioctl + * + * Checks if the specified directory is case sensitive or not. Uses ioctl(2). + */ +static int get_dir_case_sensitivity_ioctl(int fd) +{ + int flags; + if (ioctl(fd, EXT2_IOC_GETFLAGS, &flags) != -1 && (flags & EXT4_CASEFOLD_FL)) + return FALSE; + return -1; +} +#endif + /*********************************************************************** * 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), (f)statvfs(2), or fstatat(2). */ -static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) +static BOOLEAN get_dir_case_sensitivity_stat( const char *dir, int fd ) { #if defined(__APPLE__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__) struct statfs stfs; - if (statfs( dir, &stfs ) == -1) return FALSE; + if (fd != -1 && fstatfs( fd, &stfs ) == -1) return FALSE; + if (fd == -1 && statfs( dir, &stfs ) == -1) return FALSE; + /* Assume these file systems are always case insensitive on Mac OS. * For FreeBSD, only assume CIOPFS is case insensitive (AFAIK, Mac OS * is the only UNIX that supports case-insensitive lookup). @@ -1157,7 +1183,9 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) #elif defined(__NetBSD__) struct statvfs stfs; - if (statvfs( dir, &stfs ) == -1) return FALSE; + if (fd != -1 && fstatvfs( fd, &stfs ) == -1) return FALSE; + if (fd == -1 && statvfs( dir, &stfs ) == -1) return FALSE; + /* Only assume CIOPFS is case insensitive. */ if (strcmp( stfs.f_fstypename, "fusefs" ) || strncmp( stfs.f_mntfromname, "ciopfs", 5 )) @@ -1170,7 +1198,9 @@ static BOOLEAN get_dir_case_sensitivity_stat( const char *dir ) char *cifile; /* Only assume CIOPFS is case insensitive. */ - if (statfs( dir, &stfs ) == -1) return FALSE; + if (fd != -1 && fstatfs( fd, &stfs ) == -1) return FALSE; + if (fd == -1 && statfs( dir, &stfs ) == -1) return FALSE; + if (stfs.f_type != 0x65735546 /* FUSE_SUPER_MAGIC */) return TRUE; /* Normally, we'd have to parse the mtab to find out exactly what @@ -1180,6 +1210,13 @@ 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. */ + if (fd != -1) + { + if (fstatat( fd, ".ciopfs", &st, AT_NO_AUTOMOUNT ) == 0) + return FALSE; + return TRUE; + } + cifile = RtlAllocateHeap( GetProcessHeap(), 0, strlen( dir )+sizeof("/.ciopfs") ); if (!cifile) return TRUE; strcpy( cifile, dir ); @@ -1201,16 +1238,32 @@ 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 ) { + 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 ); + +#ifdef EXT4_CASEFOLD_FL + if ((fd = open(dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE)) == -1) + return TRUE; + if ((case_sensitive = get_dir_case_sensitivity_ioctl(fd)) != -1) + { + close(fd); + return case_sensitive; + } +#endif + + case_sensitive = get_dir_case_sensitivity_stat(dir, fd); + + if (fd != -1) close(fd); + return case_sensitive; } -- 2.21.0
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47099 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/ntdll/server.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index 094b530..7dff7c1 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -51,6 +51,12 @@ #ifdef HAVE_SYS_MMAN_H #include <sys/mman.h> #endif +#ifdef HAVE_SYS_IOCTL_H +#include <sys/ioctl.h> +#endif +#ifdef HAVE_LINUX_IOCTL_H +#include <linux/ioctl.h> +#endif #ifdef HAVE_SYS_PRCTL_H # include <sys/prctl.h> #endif @@ -84,6 +90,22 @@ WINE_DEFAULT_DEBUG_CHANNEL(server); +/* just in case... */ +#undef EXT2_IOC_GETFLAGS +#undef EXT2_IOC_SETFLAGS +#undef EXT4_CASEFOLD_FL + +#ifdef __linux__ + +/* Define the ext2 ioctls for handling extra attributes */ +#define EXT2_IOC_GETFLAGS _IOR('f', 1, long) +#define EXT2_IOC_SETFLAGS _IOW('f', 2, long) + +/* Case-insensitivity attribute */ +#define EXT4_CASEFOLD_FL 0x40000000 + +#endif + /* Some versions of glibc don't define this */ #ifndef SCM_RIGHTS #define SCM_RIGHTS 1 @@ -1119,6 +1141,28 @@ static void start_server(void) } +/*********************************************************************** + * set_case_insensitive + * + * Make the supplied directory case insensitive, if possible. + */ +static void set_case_insensitive(const char *dir) +{ +#ifdef EXT4_CASEFOLD_FL + int flags, fd; + + if ((fd = open(dir, O_RDONLY | O_NONBLOCK | O_LARGEFILE)) == -1) + return; + if (ioctl(fd, EXT2_IOC_GETFLAGS, &flags) != -1 && !(flags & EXT4_CASEFOLD_FL)) + { + flags |= EXT4_CASEFOLD_FL; + ioctl(fd, EXT2_IOC_SETFLAGS, &flags); + } + close(fd); +#endif +} + + /*********************************************************************** * setup_config_dir * @@ -1162,6 +1206,7 @@ static int setup_config_dir(void) /* create the drive symlinks */ mkdir( "drive_c", 0777 ); + set_case_insensitive( "drive_c" ); symlink( "../drive_c", "dosdevices/c:" ); symlink( "/", "dosdevices/z:" ); -- 2.21.0
1. I'm still sceptical on whether the name / fd thing is worth it, however it doesn't look too bad so it's probably ok. 2. I'll let Alexandre decide on whether we should use ext2fs/ext2_fs.h instead of defining EXT2_IOC_[GS]ETFLAGS and EXT4_CASEFOLD_FL ourselves. Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Huw Davies <huw(a)codeweavers.com> writes:
1. I'm still sceptical on whether the name / fd thing is worth it, however it doesn't look too bad so it's probably ok.
It doesn't look so good to me. There's no reason to change the function on all platforms for something that is Linux specific. Simply open the directory in the ifdef linux block. -- Alexandre Julliard julliard(a)winehq.org
On 6/11/19 8:06 PM, Alexandre Julliard wrote:
Huw Davies <huw(a)codeweavers.com> writes:
1. I'm still sceptical on whether the name / fd thing is worth it, however it doesn't look too bad so it's probably ok.
It doesn't look so good to me. There's no reason to change the function on all platforms for something that is Linux specific. Simply open the directory in the ifdef linux block.
But that would make the fd parameter unused in other platforms, unless you mean we open the dir in the *_stat function itself, but in that case the name might be misleading a bit, since it also does an ioctl, assuming it calls the *_ioctl function from there. Would that be a good way to approach this?
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On 6/11/19 8:06 PM, Alexandre Julliard wrote:
Huw Davies <huw(a)codeweavers.com> writes:
1. I'm still sceptical on whether the name / fd thing is worth it, however it doesn't look too bad so it's probably ok.
It doesn't look so good to me. There's no reason to change the function on all platforms for something that is Linux specific. Simply open the directory in the ifdef linux block.
But that would make the fd parameter unused in other platforms, unless you mean we open the dir in the *_stat function itself, but in that case the name might be misleading a bit, since it also does an ioctl, assuming it calls the *_ioctl function from there.
Would that be a good way to approach this?
Yes, just do everything in that function. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Gabriel Ivăncescu -
Huw Davies