Re: [GIT PULL] timestamp fixes

[Jeff Layton]

On Mon, 2023-09-18 at 13:18 -0700, Linus Torvalds wrote:
> On Mon, 18 Sept 2023 at 12:39, Jeff Layton <jlayton@xxxxxxxxxx> 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.
> 

No, you're quite right. That's exactly what would have happened.

> 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.
> 
> 

I think the atime problem is solved by just dropping the patch I
mentioned before. The atime is updated for read operations and the
m/ctime for write. You only get a fine-grained timestamp when updating
the ctime, so for an atime update we always use a coarse timestamp. It
should never roll backward. [1]

We may have a problem with the ctime update though, since you pointed it
out. We have this in inode_set_ctime_current(), in the codepath where
the QUERIED bit isn't set:

                /*
                 * If we've recently updated with a fine-grained timestamp,
                 * then the coarse-grained one may still be earlier than the
                 * existing ctime. Just keep the existing value if so.
                 */
                ctime.tv_sec = inode->__i_ctime.tv_sec;
                if (timespec64_compare(&ctime, &now) > 0)
                        return ctime;

The ctime can't be set via utimes(), so that's not an issue here, but we
could get a realtime clock jump backward that causes this to not be
updated like it should be.

I think (like you suggest above) that this needs some bounds-checking
where we make sure that the current coarse grained time isn't more than
around 1-2 jiffies earlier than the existing ctime. If it is, then we'll
go ahead and just update it anyway.

Thoughts?
-- 
Jeff Layton <jlayton@xxxxxxxxxx>


[1]: You _could_ do a write and then a read against a file, and end up
with an atime that looks to be earlier than the ctime, even though you
know that they were done in the other order. I'm operating under the
assumption that this isn't a problem we need to worry about.