Messages in this thread | | | From | Linus Torvalds <> | Date | Mon, 18 Sep 2023 13:18:07 -0700 | Subject | Re: [GIT PULL] timestamp fixes |
| |
On Mon, 18 Sept 2023 at 12:39, Jeff Layton <jlayton@kernel.org> wrote: > > In general, we always update the atime with a coarse-grained timestamp, > since atime and ctime updates are never done together during normal read > and write operations. As you note, things are a little more murky with > utimes() updates but I think we should be safe to overwrite the atime > with a coarse-grained timestamp unconditionally.
I do think utimes() ends up always overwriting, but that's a different code-path entirely (ie it goes through the ->setattr() logic, not this inode_update_timestamps() code).
So I *think* that even with your patch, doing a "touch" would end up doing the right thing - it would update atime even if it was in the future before.
But doing a plain "read()" would break, and not update atime.
That said, I didn't actually ever *test* any of this, so this is purely from reading the patch, and I can easily have missed something.
Anyway, I do think that the timespec64_equal() tests are a bit iffy in fs/inode.c now, since the timespecs that are being tested might be of different precision.
So I do think there's a *problem* here, I just do not believe that doing that timespec64_equal() -> timespec64_compare() is at all the right thing to do.
My *gut* feel is that in both cases, we have this
if (timespec64_equal(&inode->i_atime, &now))
and the problem is that *sometimes* 'now' is the coarse time, but sometimes it's the fine-grained one, and so checking for equality is simply nonsensical.
I get the feeling that that timespec64_equal() logic for those atime updates should be something like
- if 'now' is in the future, we always considering it different, and update the time
- if 'now' is further in the past than the coarse granularity, we also update the time ("clearly not equal")
- but if 'now' is in the past, but within the coarse time granularity, we consider it equal and do not update anything
but it's not like I have really given this a huge amount of thought. It's just that "don't update if in the past" that I am pretty sure can *not* be right.
Linus
| |