| Message ID | 20240828-statx-null-path-v4-1-33872399b15f@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] linux: Add linux statx(fd, NULL, AT_EMPTY_PATH) support | expand |
* Miao Wang via: > diff --git a/sysdeps/unix/sysv/linux/internal-stat.h b/sysdeps/unix/sysv/linux/internal-stat.h > index 9334059765..263e29637f 100644 > --- a/sysdeps/unix/sysv/linux/internal-stat.h > +++ b/sysdeps/unix/sysv/linux/internal-stat.h > @@ -29,3 +29,30 @@ > #else > # define FSTATAT_USE_STATX 0 > #endif > + > +#if FSTATAT_USE_STATX || XSTAT_IS_XSTAT64 > + > +/* buf MUST be a valid buffer, or the feature detection may not work properly */ > + “BUF must be a valid buffer”, I think. > +static inline int > +__statx_empty_path (int fd, int flag, struct statx *buf) > +{ > + flag |= AT_EMPTY_PATH; > +#ifdef __ASSUME_STATX_NULL_PATH > + return INTERNAL_SYSCALL_CALL (statx, fd, NULL, flag, STATX_BASIC_STATS, buf); > +#else > + static int statx_null_path_supported = 1; > + int r; > + int supported = atomic_load_relaxed (&statx_null_path_supported); > + if (__glibc_likely (supported)) This function shouldn't be inline because it defines a static variable. You should move its definition to an appropriate .c file (perhaps a new one) and add an attribute_hidden declaration to sysdeps/unix/sysv/linux/internal-stat.h. Thanks, Florian
Hi, > 2024年8月28日 17:12，Florian Weimer <fweimer@redhat.com> 写道： > > * Miao Wang via: > >> diff --git a/sysdeps/unix/sysv/linux/internal-stat.h b/sysdeps/unix/sysv/linux/internal-stat.h >> index 9334059765..263e29637f 100644 >> --- a/sysdeps/unix/sysv/linux/internal-stat.h >> +++ b/sysdeps/unix/sysv/linux/internal-stat.h >> @@ -29,3 +29,30 @@ >> #else >> # define FSTATAT_USE_STATX 0 >> #endif >> + >> +#if FSTATAT_USE_STATX || XSTAT_IS_XSTAT64 >> + >> +/* buf MUST be a valid buffer, or the feature detection may not work properly */ >> + > > “BUF must be a valid buffer”, I think. > >> +static inline int >> +__statx_empty_path (int fd, int flag, struct statx *buf) >> +{ >> + flag |= AT_EMPTY_PATH; >> +#ifdef __ASSUME_STATX_NULL_PATH >> + return INTERNAL_SYSCALL_CALL (statx, fd, NULL, flag, STATX_BASIC_STATS, buf); >> +#else >> + static int statx_null_path_supported = 1; >> + int r; >> + int supported = atomic_load_relaxed (&statx_null_path_supported); >> + if (__glibc_likely (supported)) > > This function shouldn't be inline because it defines a static variable. > You should move its definition to an appropriate .c file (perhaps a new > one) and add an attribute_hidden declaration to > sysdeps/unix/sysv/linux/internal-stat.h. I deliberately make this function inline because there will only be two users and it is expected that there will be no more users. So there will be limited copies of these code inside the compiled binary. Inlining this function help optimization of the two functions using it. The drawback will be the two functions will do the try-and-fall-back independently. Considering one of the functions, fxstat64, is provided for backward compatibility, I believe this drawback is acceptable. > > Thanks, > Florian >
On Wed, 2024-08-28 at 19:06 +0800, Miao Wang wrote: > Hi, > > > 2024年8月28日 17:12，Florian Weimer <fweimer@redhat.com> 写道： > > > > * Miao Wang via: > > > > > diff --git a/sysdeps/unix/sysv/linux/internal-stat.h b/sysdeps/unix/sysv/linux/internal-stat.h > > > index 9334059765..263e29637f 100644 > > > --- a/sysdeps/unix/sysv/linux/internal-stat.h > > > +++ b/sysdeps/unix/sysv/linux/internal-stat.h > > > @@ -29,3 +29,30 @@ > > > #else > > > # define FSTATAT_USE_STATX 0 > > > #endif > > > + > > > +#if FSTATAT_USE_STATX || XSTAT_IS_XSTAT64 > > > + > > > +/* buf MUST be a valid buffer, or the feature detection may not work properly */ > > > + > > > > “BUF must be a valid buffer”, I think. > > > > > +static inline int > > > +__statx_empty_path (int fd, int flag, struct statx *buf) > > > +{ > > > + flag |= AT_EMPTY_PATH; > > > +#ifdef __ASSUME_STATX_NULL_PATH > > > + return INTERNAL_SYSCALL_CALL (statx, fd, NULL, flag, STATX_BASIC_STATS, buf); > > > +#else > > > + static int statx_null_path_supported = 1; > > > + int r; > > > + int supported = atomic_load_relaxed (&statx_null_path_supported); > > > + if (__glibc_likely (supported)) > > > > This function shouldn't be inline because it defines a static variable. > > You should move its definition to an appropriate .c file (perhaps a new > > one) and add an attribute_hidden declaration to > > sysdeps/unix/sysv/linux/internal-stat.h. > > I deliberately make this function inline because there will only be two > users and it is expected that there will be no more users. So there will > be limited copies of these code inside the compiled binary. Inlining this > function help optimization of the two functions using it. The drawback > will be the two functions will do the try-and-fall-back independently. > Considering one of the functions, fxstat64, is provided for backward > compatibility, I believe this drawback is acceptable. Maybe separate the static variable to a global variable with hidden visibility? Then there won't be this drawback.
diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c index da496177c9..b67a5f4469 100644 --- a/sysdeps/unix/sysv/linux/fstatat64.c +++ b/sysdeps/unix/sysv/linux/fstatat64.c @@ -47,8 +47,12 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also 64-bit time_t support is done through statx syscall. */ struct statx tmp; - int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, - STATX_BASIC_STATS, &tmp); + flag |= AT_NO_AUTOMOUNT; + int r; + if ((flag & AT_EMPTY_PATH) && (file == NULL || *file == '\0')) + r = __statx_empty_path (fd, flag, &tmp); + else + r = INTERNAL_SYSCALL_CALL (statx, fd, file, flag, STATX_BASIC_STATS, &tmp); if (r != 0) return r; diff --git a/sysdeps/unix/sysv/linux/fxstat64.c b/sysdeps/unix/sysv/linux/fxstat64.c index 230374cb22..bbe52de36d 100644 --- a/sysdeps/unix/sysv/linux/fxstat64.c +++ b/sysdeps/unix/sysv/linux/fxstat64.c @@ -20,7 +20,7 @@ #include <sys/stat.h> #undef __fxstat #include <fcntl.h> -#include <kernel_stat.h> +#include <internal-stat.h> #include <sysdep.h> #include <xstatconv.h> #include <statx_cp.h> @@ -53,11 +53,11 @@ ___fxstat64 (int vers, int fd, struct stat64 *buf) # else /* New 32-bit kABIs with only 64-bit time_t support, e.g. arc, riscv32. */ struct statx tmp; - int r = INLINE_SYSCALL_CALL (statx, fd, "", AT_EMPTY_PATH, - STATX_BASIC_STATS, &tmp); - if (r == 0) - __cp_stat64_statx (buf, &tmp); - return r; + int r = __statx_empty_path (fd, 0, &tmp); + if (INTERNAL_SYSCALL_ERROR_P (r)) + return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); + __cp_stat64_statx (buf, &tmp); + return 0; # endif #else /* All kABIs with non-LFS support, e.g. arm, csky, i386, hppa, m68k, diff --git a/sysdeps/unix/sysv/linux/internal-stat.h b/sysdeps/unix/sysv/linux/internal-stat.h index 9334059765..263e29637f 100644 --- a/sysdeps/unix/sysv/linux/internal-stat.h +++ b/sysdeps/unix/sysv/linux/internal-stat.h @@ -29,3 +29,30 @@ #else # define FSTATAT_USE_STATX 0 #endif + +#if FSTATAT_USE_STATX || XSTAT_IS_XSTAT64 + +/* buf MUST be a valid buffer, or the feature detection may not work properly */ + +static inline int +__statx_empty_path (int fd, int flag, struct statx *buf) +{ + flag |= AT_EMPTY_PATH; +#ifdef __ASSUME_STATX_NULL_PATH + return INTERNAL_SYSCALL_CALL (statx, fd, NULL, flag, STATX_BASIC_STATS, buf); +#else + static int statx_null_path_supported = 1; + int r; + int supported = atomic_load_relaxed (&statx_null_path_supported); + if (__glibc_likely (supported)) + { + r = INTERNAL_SYSCALL_CALL (statx, fd, NULL, flag, STATX_BASIC_STATS, buf); + if (r != -EFAULT) + return r; + atomic_store_relaxed (&statx_null_path_supported, 0); + } + return INTERNAL_SYSCALL_CALL (statx, fd, "", flag, STATX_BASIC_STATS, buf); +#endif +} + +#endif diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h index a25cf07e9f..78aaf43a82 100644 --- a/sysdeps/unix/sysv/linux/kernel-features.h +++ b/sysdeps/unix/sysv/linux/kernel-features.h @@ -257,4 +257,9 @@ # define __ASSUME_FCHMODAT2 0 #endif +/* statx(fd, NULL, AT_EMPTY_PATH) was introduced in Linux 6.11. */ +#if __LINUX_KERNEL_VERSION >= 0x060b00 +# define __ASSUME_STATX_NULL_PATH 1 +#endif + #endif /* kernel-features.h */