| Message ID | 20230518113216.126233-1-jlayton@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | syscalls/statx06: use a fine-grained timestamp for the second time fetch | expand |
on 2023/05/18 19:32, Jeff Layton wrote: > I have a patchset in progress to change the kernel to sometimes use > fine-grained timestamps for the mtime/ctime. With this, the statx06 test > sometimes fails. > > Change the test to grab a fine-grained timestamp for the "after" value, > which fixes the issue. Now, it is not a right time to review this patch, I prefer to review this when your kernel patchset is merged into linux master or linux-next. Then we can add a comment or a kernel commit id to explain this.. Best Regard Yang Xu > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > testcases/kernel/syscalls/statx/statx06.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c > index ce82b905bebd..fa75982b0901 100644 > --- a/testcases/kernel/syscalls/statx/statx06.c > +++ b/testcases/kernel/syscalls/statx/statx06.c > @@ -109,7 +109,7 @@ static void test_statx(unsigned int test_nr) > clock_wait_tick(); > tc->operation(); > clock_wait_tick(); > - SAFE_CLOCK_GETTIME(CLOCK_REALTIME_COARSE, &after_time); > + SAFE_CLOCK_GETTIME(CLOCK_REALTIME, &after_time); > > TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_ALL, &buff)); > if (TST_RET != 0) {
On Fri, 2023-05-19 at 09:16 +0000, Yang Xu (Fujitsu) wrote: > > on 2023/05/18 19:32, Jeff Layton wrote: > > I have a patchset in progress to change the kernel to sometimes use > > fine-grained timestamps for the mtime/ctime. With this, the statx06 test > > sometimes fails. > > > > Change the test to grab a fine-grained timestamp for the "after" value, > > which fixes the issue. > > Now, it is not a right time to review this patch, I prefer to review > this when your kernel patchset is merged into linux master or > linux-next. Then we can add a comment or a kernel commit id to explain > this.. > > Fair enough. I don't think there is any issue with taking this patch in earlier however. The problem with this test is that it makes the assumption that coarse- grained timestamps are sufficient to bracket a filesystem operation. While that has largely been true in the past, it's certainly not specified by any standard. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > testcases/kernel/syscalls/statx/statx06.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c > > index ce82b905bebd..fa75982b0901 100644 > > --- a/testcases/kernel/syscalls/statx/statx06.c > > +++ b/testcases/kernel/syscalls/statx/statx06.c > > @@ -109,7 +109,7 @@ static void test_statx(unsigned int test_nr) > > clock_wait_tick(); > > tc->operation(); > > clock_wait_tick(); > > - SAFE_CLOCK_GETTIME(CLOCK_REALTIME_COARSE, &after_time); > > + SAFE_CLOCK_GETTIME(CLOCK_REALTIME, &after_time); > > > > TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_ALL, &buff)); > > if (TST_RET != 0) {
Hi! > > Now, it is not a right time to review this patch, I prefer to review > > this when your kernel patchset is merged into linux master or > > linux-next. Then we can add a comment or a kernel commit id to explain > > this.. > > > > > > Fair enough. I don't think there is any issue with taking this patch in > earlier however. > > The problem with this test is that it makes the assumption that coarse- > grained timestamps are sufficient to bracket a filesystem operation. > While that has largely been true in the past, it's certainly not > specified by any standard. Do we have any API to ask the kernel what the granularity of filesystem the timestamps is? Would that make sense to be added if it's not present? Apart from that I think that this patch is fine, since the CLOCK_REALTIME value would be between CLOCK_REALTIME_COARSE and CLOCK_REALTIME_COARSE + one tick. So the change is backward compatible and we will not loose any precision in the assertions either. Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
On Wed, 2023-05-24 at 10:46 +0200, Cyril Hrubis wrote: > Hi! > > > Now, it is not a right time to review this patch, I prefer to review > > > this when your kernel patchset is merged into linux master or > > > linux-next. Then we can add a comment or a kernel commit id to explain > > > this.. > > > > > > > > > > Fair enough. I don't think there is any issue with taking this patch in > > earlier however. > > > > The problem with this test is that it makes the assumption that coarse- > > grained timestamps are sufficient to bracket a filesystem operation. > > While that has largely been true in the past, it's certainly not > > specified by any standard. > > Do we have any API to ask the kernel what the granularity of filesystem > the timestamps is? Would that make sense to be added if it's not > present? > No, we don't have such a thing now, and we don't really track that information in the kernel. We do have a sb->s_time_gran field which is in units of nanoseconds, but that really is more about the granularity that the filesystem itself can store on disk. It doesn't tell you whether the kernel decided to put a coarse or fine grained timestamp in there. > Apart from that I think that this patch is fine, since the > CLOCK_REALTIME value would be between CLOCK_REALTIME_COARSE and > CLOCK_REALTIME_COARSE + one tick. So the change is backward compatible > and we will not loose any precision in the assertions either. > > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> > Thanks, and yes. This should be safe to apply now even if the kernel patch series never goes in.
Hi all, Reviewed-by: Petr Vorel <pvorel@suse.cz> > Thanks, and yes. This should be safe to apply now even if the kernel > patch series never goes in. I'd be ok to merge this now. Xu, please let me know if you're against. Kind regards, Petr
Hi Petr > Hi all, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > >> Thanks, and yes. This should be safe to apply now even if the kernel >> patch series never goes in. > > I'd be ok to merge this now. Xu, please let me know if you're against. This patch is right, so please merged. Reviewed-by: Yang Xu <xuyang2018.jy@fujitsu.com> Best Regards Yang Xu > > Kind regards, > Petr
Hi all, I merged the patchset, linking the original RFC patchset in kernel and explaining that we merged it because the change is backward compatible anyway. Thanks to all! Kind regards, Petr [1] https://lore.kernel.org/lkml/20230411143702.64495-1-jlayton@kernel.org/
diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c index ce82b905bebd..fa75982b0901 100644 --- a/testcases/kernel/syscalls/statx/statx06.c +++ b/testcases/kernel/syscalls/statx/statx06.c @@ -109,7 +109,7 @@ static void test_statx(unsigned int test_nr) clock_wait_tick(); tc->operation(); clock_wait_tick(); - SAFE_CLOCK_GETTIME(CLOCK_REALTIME_COARSE, &after_time); + SAFE_CLOCK_GETTIME(CLOCK_REALTIME, &after_time); TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_ALL, &buff)); if (TST_RET != 0) {
I have a patchset in progress to change the kernel to sometimes use fine-grained timestamps for the mtime/ctime. With this, the statx06 test sometimes fails. Change the test to grab a fine-grained timestamp for the "after" value, which fixes the issue. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- testcases/kernel/syscalls/statx/statx06.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)