Re: [PATCH] vfs: replace current_kernel_time64 with ktime equivalent

[Arnd Bergmann]

On Thu, Jun 21, 2018 at 10:23 PM, Dave Chinner <david@xxxxxxxxxxxxx> 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