| Message ID | 20210317130352.1782897-1-stli@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | S390: Don't test nanoseconds in io/tst-stat.c | expand |
On Wed, 17 Mar 2021, Stefan Liebler via Libc-alpha wrote: > Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64) > are comparing the nanosecond fields with the statx result. Unfortunately > on s390(31bit) those fields are always zero if old KABI with non-LFS > support is used. With _FILE_OFFSET_BITS=64 stat is using statx internally. My understanding is that, if using a kernel that supports statx, this disabling is not needed. If so, we want to be sure to remove this disabling once we can assume a kernel supporting statx (on s390). The way we ensure that is having a conditional on __ASSUME_STATX in the code in question so that it's immediately visible when we remove __ASSUME_STATX that this disabling can be removed from the glibc source code as well.
On 17/03/2021 18:01, Joseph Myers wrote: > On Wed, 17 Mar 2021, Stefan Liebler via Libc-alpha wrote: > >> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64) >> are comparing the nanosecond fields with the statx result. Unfortunately >> on s390(31bit) those fields are always zero if old KABI with non-LFS >> support is used. With _FILE_OFFSET_BITS=64 stat is using statx internally. > > My understanding is that, if using a kernel that supports statx, this > disabling is not needed. The non-LFS stat does not call statx, so the nanoseconds call should be disable only for non-LFS mode. > > If so, we want to be sure to remove this disabling once we can assume a > kernel supporting statx (on s390). The way we ensure that is having a > conditional on __ASSUME_STATX in the code in question so that it's > immediately visible when we remove __ASSUME_STATX that this disabling can > be removed from the glibc source code as well. >
On 17/03/2021 10:03, Stefan Liebler wrote: > Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64) > are comparing the nanosecond fields with the statx result. Unfortunately > on s390(31bit) those fields are always zero if old KABI with non-LFS > support is used. With _FILE_OFFSET_BITS=64 stat is using statx internally. > > As suggested by Adhemerval this patch disables the nanosecond check for > s390(31bit). LGTM, the fstatat call does not call statx and even for LFS that call statx it might ended calling old stat syscall in the fallback part that does not About the __ASSUME_STATX note Joseph has raised, I think we should add it on Linux at least for fstatat64 implementation. However it does not really help on the fstatat one. I will try to spare some time to make fstatat.c use statx as well, so we can tie the test to __ASSUME_STATX. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > io/tst-stat.c | 7 +++++-- > support/Makefile | 1 + > support/support.h | 3 +++ > support/support_stat_nanoseconds.c | 31 ++++++++++++++++++++++++++++++ > 4 files changed, 40 insertions(+), 2 deletions(-) > create mode 100644 support/support_stat_nanoseconds.c > > diff --git a/io/tst-stat.c b/io/tst-stat.c > index 445ac4176c..397d480ecc 100644 > --- a/io/tst-stat.c > +++ b/io/tst-stat.c > @@ -91,9 +91,12 @@ do_test (void) > TEST_COMPARE (stx.stx_blocks, st.st_blocks); > > TEST_COMPARE (stx.stx_ctime.tv_sec, st.st_ctim.tv_sec); > - TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec); > TEST_COMPARE (stx.stx_mtime.tv_sec, st.st_mtim.tv_sec); > - TEST_COMPARE (stx.stx_mtime.tv_nsec, st.st_mtim.tv_nsec); > + if (support_stat_nanoseconds ()) > + { > + TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec); > + TEST_COMPARE (stx.stx_mtime.tv_nsec, st.st_mtim.tv_nsec); > + } > } > > return 0; > diff --git a/support/Makefile b/support/Makefile > index fc9f4936a8..900e17f94f 100644 > --- a/support/Makefile > +++ b/support/Makefile > @@ -71,6 +71,7 @@ libsupport-routines = \ > support_set_small_thread_stack_size \ > support_shared_allocate \ > support_small_stack_thread_attribute \ > + support_stat_nanoseconds \ > support_subprocess \ > support_test_compare_blob \ > support_test_compare_failure \ > diff --git a/support/support.h b/support/support.h > index 2e477c9e7c..90f3ff9d1a 100644 > --- a/support/support.h > +++ b/support/support.h > @@ -134,6 +134,9 @@ extern ssize_t support_copy_file_range (int, off64_t *, int, off64_t *, > operations (such as fstatat or utimensat). */ > extern bool support_path_support_time64 (const char *path); > > +/* Return true if stat supports nanoseconds resolution. */ > +extern bool support_stat_nanoseconds (void); > + > __END_DECLS > > #endif /* SUPPORT_H */ > diff --git a/support/support_stat_nanoseconds.c b/support/support_stat_nanoseconds.c > new file mode 100644 > index 0000000000..c0d5b2c3a9 > --- /dev/null > +++ b/support/support_stat_nanoseconds.c > @@ -0,0 +1,31 @@ > +/* Check if stat supports nanosecond resolution. > + Copyright (C) 2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <stdbool.h> > + > +bool > +support_stat_nanoseconds (void) > +{ > + /* s390 stat64 compat symbol does not support nanoseconds resolution > + and it used on non-LFS [f,l]stat[at] implementations. */ > +#if defined __linux__ && !defined __s390x__ && defined __s390__ > + return false; > +#else > + return true; > +#endif > +} >
On 18/03/2021 14:31, Adhemerval Zanella wrote: > > > On 17/03/2021 10:03, Stefan Liebler wrote: >> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64) >> are comparing the nanosecond fields with the statx result. Unfortunately >> on s390(31bit) those fields are always zero if old KABI with non-LFS >> support is used. With _FILE_OFFSET_BITS=64 stat is using statx internally. >> >> As suggested by Adhemerval this patch disables the nanosecond check for >> s390(31bit). > > LGTM, the fstatat call does not call statx and even for LFS that call statx > it might ended calling old stat syscall in the fallback part that does not > > About the __ASSUME_STATX note Joseph has raised, I think we should add it > on Linux at least for fstatat64 implementation. However it does not really > help on the fstatat one. I will try to spare some time to make fstatat.c > use statx as well, so we can tie the test to __ASSUME_STATX. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > Hi Adhemerval, Sorry for the delay, I was busy with another project. Thanks for your series "[PATCH 1/5] linux: Implement fstatat with __fstatat64_time64" https://sourceware.org/pipermail/libc-alpha/2021-March/124191.html As also mentioned there, with your series, at least on my s390 systems, stat is then using statx and the nanosecond fields are not zero anymore. Shall I commit my patch as is and as soon as you've commited your series, you can adjust support_stat_nanoseconds to return false if __ASSUME_STATX is not defined? Thanks, Stefan
On 23/03/2021 13:13, Stefan Liebler wrote: > On 18/03/2021 14:31, Adhemerval Zanella wrote: >> >> >> On 17/03/2021 10:03, Stefan Liebler wrote: >>> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64) >>> are comparing the nanosecond fields with the statx result. Unfortunately >>> on s390(31bit) those fields are always zero if old KABI with non-LFS >>> support is used. With _FILE_OFFSET_BITS=64 stat is using statx internally. >>> >>> As suggested by Adhemerval this patch disables the nanosecond check for >>> s390(31bit). >> >> LGTM, the fstatat call does not call statx and even for LFS that call statx >> it might ended calling old stat syscall in the fallback part that does not >> >> About the __ASSUME_STATX note Joseph has raised, I think we should add it >> on Linux at least for fstatat64 implementation. However it does not really >> help on the fstatat one. I will try to spare some time to make fstatat.c >> use statx as well, so we can tie the test to __ASSUME_STATX. >> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> >> > > Hi Adhemerval, > > Sorry for the delay, I was busy with another project. > Thanks for your series > "[PATCH 1/5] linux: Implement fstatat with __fstatat64_time64" > https://sourceware.org/pipermail/libc-alpha/2021-March/124191.html > > As also mentioned there, with your series, at least on my s390 systems, > stat is then using statx and the nanosecond fields are not zero anymore. > > Shall I commit my patch as is and as soon as you've commited your > series, you can adjust support_stat_nanoseconds to return false if > __ASSUME_STATX is not defined? Yes, I can rebase on top your patch. I think we still need to handle the nanosecond missing support on older kernels. Thanks for checking on s390, if you can review the patchset I would be grateful
On 24/03/2021 18:40, Adhemerval Zanella wrote: > > > On 23/03/2021 13:13, Stefan Liebler wrote: >> On 18/03/2021 14:31, Adhemerval Zanella wrote: >>> >>> >>> On 17/03/2021 10:03, Stefan Liebler wrote: >>>> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64) >>>> are comparing the nanosecond fields with the statx result. Unfortunately >>>> on s390(31bit) those fields are always zero if old KABI with non-LFS >>>> support is used. With _FILE_OFFSET_BITS=64 stat is using statx internally. >>>> >>>> As suggested by Adhemerval this patch disables the nanosecond check for >>>> s390(31bit). >>> >>> LGTM, the fstatat call does not call statx and even for LFS that call statx >>> it might ended calling old stat syscall in the fallback part that does not >>> >>> About the __ASSUME_STATX note Joseph has raised, I think we should add it >>> on Linux at least for fstatat64 implementation. However it does not really >>> help on the fstatat one. I will try to spare some time to make fstatat.c >>> use statx as well, so we can tie the test to __ASSUME_STATX. >>> >>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> >>> >> >> Hi Adhemerval, >> >> Sorry for the delay, I was busy with another project. >> Thanks for your series >> "[PATCH 1/5] linux: Implement fstatat with __fstatat64_time64" >> https://sourceware.org/pipermail/libc-alpha/2021-March/124191.html >> >> As also mentioned there, with your series, at least on my s390 systems, >> stat is then using statx and the nanosecond fields are not zero anymore. >> >> Shall I commit my patch as is and as soon as you've commited your >> series, you can adjust support_stat_nanoseconds to return false if >> __ASSUME_STATX is not defined? > > Yes, I can rebase on top your patch. I think we still need to handle > the nanosecond missing support on older kernels. I've just committed this patch. > > Thanks for checking on s390, if you can review the patchset I would be > grateful Sure. I've left some comments Thanks, Stefan
diff --git a/io/tst-stat.c b/io/tst-stat.c index 445ac4176c..397d480ecc 100644 --- a/io/tst-stat.c +++ b/io/tst-stat.c @@ -91,9 +91,12 @@ do_test (void) TEST_COMPARE (stx.stx_blocks, st.st_blocks); TEST_COMPARE (stx.stx_ctime.tv_sec, st.st_ctim.tv_sec); - TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec); TEST_COMPARE (stx.stx_mtime.tv_sec, st.st_mtim.tv_sec); - TEST_COMPARE (stx.stx_mtime.tv_nsec, st.st_mtim.tv_nsec); + if (support_stat_nanoseconds ()) + { + TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec); + TEST_COMPARE (stx.stx_mtime.tv_nsec, st.st_mtim.tv_nsec); + } } return 0; diff --git a/support/Makefile b/support/Makefile index fc9f4936a8..900e17f94f 100644 --- a/support/Makefile +++ b/support/Makefile @@ -71,6 +71,7 @@ libsupport-routines = \ support_set_small_thread_stack_size \ support_shared_allocate \ support_small_stack_thread_attribute \ + support_stat_nanoseconds \ support_subprocess \ support_test_compare_blob \ support_test_compare_failure \ diff --git a/support/support.h b/support/support.h index 2e477c9e7c..90f3ff9d1a 100644 --- a/support/support.h +++ b/support/support.h @@ -134,6 +134,9 @@ extern ssize_t support_copy_file_range (int, off64_t *, int, off64_t *, operations (such as fstatat or utimensat). */ extern bool support_path_support_time64 (const char *path); +/* Return true if stat supports nanoseconds resolution. */ +extern bool support_stat_nanoseconds (void); + __END_DECLS #endif /* SUPPORT_H */ diff --git a/support/support_stat_nanoseconds.c b/support/support_stat_nanoseconds.c new file mode 100644 index 0000000000..c0d5b2c3a9 --- /dev/null +++ b/support/support_stat_nanoseconds.c @@ -0,0 +1,31 @@ +/* Check if stat supports nanosecond resolution. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdbool.h> + +bool +support_stat_nanoseconds (void) +{ + /* s390 stat64 compat symbol does not support nanoseconds resolution + and it used on non-LFS [f,l]stat[at] implementations. */ +#if defined __linux__ && !defined __s390x__ && defined __s390__ + return false; +#else + return true; +#endif +}