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

[Dave Chinner]

On Fri, Jun 22, 2018 at 03:24:48PM +0200, Arnd Bergmann wrote:
> 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.

The interface documentation should be introduced with the new
interfaces, not left for later as that leaves people like me with no
fucking clue about what the changes actually mean or why they are
being done.  Perhaps you'd have done better to explain this API as
an internal implementation of clock_gettime(CLOCK_REALTIME_COARSE),
which is clearly documented in the man page as:

"Use when you need very fast, but not fine-grained timestamps."

Put that comment on ktime_get_coarse_real_ts64(), and almost all the
questions about "WTF does this whacky function do?" go away....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx