On Thu, Jun 21, 2018 at 10:23 PM, Dave Chinner david@fromorbit.com wrote:
On Wed, Jun 20, 2018 at 05:01:24PM +0200, Arnd Bergmann wrote:
diff --git a/fs/inode.c b/fs/inode.c index 2c300e981796..e27bd9334939 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2133,7 +2133,9 @@ EXPORT_SYMBOL(timespec64_trunc); */ struct timespec64 current_time(struct inode *inode) {
struct timespec64 now = current_kernel_time64();
struct timespec64 now;ktime_get_coarse_real_ts64(&now);Can I just say as a filesystem dev who has no idea at all about kernel timer implementations: this is an awful API change. There are hundreds of callers of current_time(), so I'm not going to be the only person looking at this function who has no clue about WTF "ktime_get_coarse_real" actually means or does. Further, this function is not documented, and jumps straight into internal time implementation stuff, so I'm lost straight away if somebody asks me "what does that function do"?. i.e. I have *no clue* what this function returns or why this code uses it.
You definitely have a point about the documentation. I meant to fix that as part of the recent rework of the timekeeping.h header but haven't finished it, partly because the header is still being changed as we get rid of the older interfaces.
i.e. the function goes from an obvious self documenting name that has been blessed as the current kernel timestamp to something that only people who work on the time subsystem understand and know when to use. It might make sense to you, but it sucks for everyone else....
Keep the wrapper, please. Change it to ktime_get_current(), if you really must change the function namespace...
The thing about current_kernel_time/current_kernel_time64/ ktime_get_coarse_real_ts64 is that it hasn't been a good choice as a default time accessor for a long time, pretty much everything outside of current_time() has a better option:
- For measuring time intervals, you want a monotonic time source, not a 'real' one, to prevent time from going backwards during leap seconds or settimeofday() adjustments. - For getting a timestamp as fast as possible, using a timespec64 based interface adds extra overhead compared to 'jiffies' or ktime_t (64-bit nanoseconds), especially when passing the struct as the return value. - As mentioned before, almost all machines these days have a fast clocksource device, so getting a 'coarse' timestamp is barely faster than an accurate one. - Having an interface with (at best) millisecond precsision but nanosecond resolution can be confusing, as it suggests a much more precision than we can give (the 'coarse' in the name is meant to clarify that).
While changing over all device drivers from the old-style interfaces with 32-bit time_t (current_kernel_time, get_seconds, do_getimeofday and getnstimeofday, getrawmonotonic, get_monotonic_coarse, get_monotonic_boottime), we tried to also address those problems, using ktime_get() as the preferred interface, or an interface with a longer name where we had specific reasons.
Outside of file system timestamps, there are only two other files left that ask for a timestamp that is both 'coarse' and 'realtime', and both only do that when user space specifically asks for that combination.
Arnd