Re: [PATCH 01/10] fs: turn inode ctime fields into a single ktime_t

On Tue, 2024-07-02 at 08:12 -0700, Christoph Hellwig wrote:

On Tue, Jul 02, 2024 at 08:21:42AM -0400, Jeff Layton wrote:

quoted

Many of the existing callers of inode_ctime_to_ts are in void
return
functions. They're just copying data from an internal
representation to
struct inode and assume it always succeeds. For those we'll
probably
have to catch bad ctime values earlier.

So, I think I'll probably have to roll bespoke error handling in
all of
the relevant filesystems if we go this route. There are also
differences between filesystems -- does it make sense to refuse to
load
an inode with a bogus ctime on NFS or AFS? Probably not.

Hell, it may be simpler to just ditch this patch and reimplement
mgtimes using the nanosecond fields like the earlier versions did.

Thatdoes for sure sound simpler.  What is the big advantage of the
ktime_t?  Smaller size?

Yeah, mostly. We shrink struct inode by 8 bytes with that patch, and we
(probably) get a better cache footprint, since i_version ends up in the
same cacheline as the ctime. That's really a separate issue though, so
I'm not too worked up about dropping that patch.

As a bonus, leaving it split across separate fields means that we can
use unused bits in the nsec field for the flag, so we don't need to
sacrifice any timestamp granularity either.

I've got a draft rework that does this that I'm testing now. Assuming
it works OK, I'll resend in a few days.

Thanks for the feedback!
-- 
Jeff Layton [off-list ref]