From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail07.adl2.internode.on.net ([150.101.137.131]:35037 "EHLO ipmail07.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933424AbeFZAYU (ORCPT ); Mon, 25 Jun 2018 20:24:20 -0400 Date: Tue, 26 Jun 2018 10:24:17 +1000 From: Dave Chinner To: Arnd Bergmann Cc: Andrew Morton , Alexander Viro , y2038 Mailman List , Andi Kleen , "Darrick J. Wong" , Jeff Layton , Jan Kara , Brian Foster , Deepa Dinamani , Miklos Szeredi , Jens Axboe , Pavel Tatashin , Linux FS-devel Mailing List , Linux Kernel Mailing List Subject: Re: [PATCH] vfs: replace current_kernel_time64 with ktime equivalent Message-ID: <20180626002417.GH13748@dastard> References: <20180620150138.49380-1-arnd@arndb.de> <20180621202339.GC13748@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, Jun 22, 2018 at 03:24:48PM +0200, Arnd Bergmann wrote: > On Thu, Jun 21, 2018 at 10:23 PM, Dave Chinner 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@fromorbit.com