On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote:
On Tue, Jul 9, 2024, at 16:23, Jeff Layton wrote:
On Tue, 2024-07-09 at 16:16 +0200, Arnd Bergmann wrote:
On Tue, Jul 9, 2024, at 15:45, Geert Uytterhoeven wrote:
I think the simplest solution is to make the floor value I'm
tracking
be an atomic64_t. That looks like it should smooth over the
differences
between arches. I'm testing a patch to do that now.
Yes, atomic64_t should work, but be careful about using this
in a fast path since it can turn into a global spinlock
in lib/atomic64.c on architectures that don't support it
natively.
I'm still reading through the rest of your series, but
it appears that you pass the time value into
ktime_to_timespec64() directly afterwards, so I guess
that is already a fairly large overhead on 32-bit
architectures and an extra spinlock doesn't hurt too
much.
Thanks Arnd.
The context for this is generally a write or other change to an inode,
so I too am hoping the overhead won't be too bad. It does take great
pains to avoid changing the ctime_floor value whenever possible.
Two more things I noticed in your patch:
- smp_load_acquire() on a 64-bit variable seems problematic
as well, maybe this needs a spinlock on 32-bit
architectures?
That should go away with the conversion of ctime_floor to atomic64_t.
AFAICT, arches that don't have native a 64-bit cmpxchg op usually
emulate that with hashed spinlocks.
- for the coarse_ctime function, I think you should be
able to avoid the conversion to timespec by just calling
ktime_get_coarse_real_ts64() again instead of converting
monotonic to real and then to timespec.
Note that we might get different values for the coarse timestamps, but
if we do then the second fetch will just be a little later (which is
OK). I'll plan to make this change.
- inode_set_ctime_current() seems to now store a fine-grained
timespec in the inode even for the !is_mgtime case, skipping
the timestamp_truncate() step. This appears to potentially
leak a non-truncated value to userspace, which would be
inconsistent with the value read back from disk.
Oof, you're right. I'll fix that up for the next version.
Thanks for the review!
--
Jeff Layton <jlayton@xxxxxxxxxx>
