From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753061AbcFNU5t (ORCPT ); Tue, 14 Jun 2016 16:57:49 -0400 Received: from mout.kundenserver.de ([212.227.126.134]:58616 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752182AbcFNU5r (ORCPT ); Tue, 14 Jun 2016 16:57:47 -0400 From: Arnd Bergmann To: Deepa Dinamani , John Stultz Cc: y2038@lists.linaro.org, Linus Torvalds , "Theodore Ts'o" , Linux Kernel Mailing List , Andreas Dilger , Al Viro , linux-fsdevel , Thomas Gleixner , "linux-ext4@vger.kernel.org" Subject: Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps Date: Tue, 14 Jun 2016 22:59:09 +0200 Message-ID: <67050321.8oB9X0ocRn@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: References: <1465448705-25055-1-git-send-email-deepa.kernel@gmail.com> <3299877.khhcQSIs8Z@wuerfel> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:noL44YVrX9CQRuPRyvM3SFUe8hbG9i3tNlBk7QetUBPDlGIO548 XEa8rDjnu/D3A6vK5Vl0njGyKAtWksJddITRsb7hkc6yD/kVlwl5QDf+4YNpi9lqgxBIo4r 06kemFKOjVWqBwrjGJgd5v/kmKIbi1x1k21ixBJdXFweec3P05SX0pwfMwWqevuHAaxBmjb cRe4AwzYI6izRRnG985cA== X-UI-Out-Filterresults: notjunk:1;V01:K0:G16xsaIeTqY=:XvouX0refJJrbNCGJ4dCr3 YnfsAl3bLrfi5kaW2R9RRrlfoByPMU+/nr5+q9XXDvN8zHrAxppu1q7xvBraeEX21Cjo0Yo8Y 59i59aiueyVEnq6g0rZ/3P+ixFNw8o/BkvomGbInE1odMQcWW3cBEAqmq84x84kYh8JxoLSIF GvrW80VHMAwmIs78U7kFJrDh+4vdBcGEUMRklwIdyH2nF0fGjd2v1W+gf1WVVBIB3n4Eb0vXs M92C2GcMkoemXxqGmjsiMYNnpD5YCorUqLzKZBCBybBdbW603vxECxmXmCljbD/QLC2LTyU8h NeadVuo4kM+hnJ8yoAFuQIqvly/rglESKYyFfp5ILi5H4qTsswGimLtBEA6nPvdt40MvksnlD pcPMxbOSs1IpSy7RC3xJgcNOr/ZaWVgqrMvZjk7TiSnTNsRghGoCF0ahhFvnIK72qyVLBm049 h8jRezI/dolhFbTWqLjKRbPkraWLNNs/8s3xaF8xo2THxv6NNYnXqeVI8u5msGmV67B0RGxTO 06mQ+k42ZG/bka6T0sahMR5TJAeBkclbc+nfsjdtZFUBGOZnubvlpx9dSzt4+3FhYHX7O/cp3 nO/L0LQr9AwPHfx6IGxPZ+sgkPwb2/OBlLUwVGJgTjCaAUAb7KL5EeNkVrA1CMFeLovjxXeW4 yQfUPs1lxrfnI2vJApN+AFzaZYpe/w3000fafqRIc4WsJ+FZqnFlnHG2Bz6zOhjL2+hR3v6AJ R8huWDxT55rrmAVN Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday, June 14, 2016 10:55:39 AM CEST Deepa Dinamani wrote: > On Fri, Jun 10, 2016 at 3:19 PM, Arnd Bergmann wrote: > > On Thursday, June 9, 2016 11:45:01 AM CEST Linus Torvalds wrote: > >> On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani wrote: > >> > CURRENT_TIME_SEC and CURRENT_TIME are not y2038 safe. > >> > current_fs_time() will be transitioned to be y2038 safe > >> > along with vfs. > >> > > >> > current_fs_time() returns timestamps according to the > >> > granularities set in the super_block. > >> > >> All existing users and all the ones in this patch (and the others too, > >> although I didn't go through them very carefully) really would prefer > >> just passing in the inode directly, rather than the superblock. > >> > >> So I don't want to add more users of this broken interface. It was a > >> mistake to use the superblock. The fact that the time granularity > >> exists there is pretty much irrelevant. If every single user wants to > >> use an inode pointer, then that is what the function should get. > > > > I guess it would help to give the function a new name in the process, > > if only to avoid possible conflicts. That new name of course needs to > > be at least as intuitive as the old one. How about > > > > struct timespec fs_timestamp(struct inode *); > > Would moving the function to fs/ directory (filesystems.c/ super.c / > inode.c) and calling it current_time() or fs_current_time() make > sense? > The declaration is already part of fs.h. > > This is actually a vfs function. > And, the time functions it uses are already exported. > Leaving it in the time.c by renaming to current_time() would be > confusing in spite of > the struct inode* argument. I've looked up the original patch that introduced current_fs_time at http://marc.info/?l=linux-kernel&m=110134111125012&w=3 >>From the patch, it's clear that current_fs_time was intentionally added to the same file as current_kernel_time() so it could be inlined there, but both functions have since been moved to different files. I agree moving both timespec_trunc and current_fs_time into fs/inode.c or fs/attr.c seems appropriate then, or we could move current_fs_time() into kernel/time/timekeeping.c and mark current_kernel_time64() inline again. When John Stultz moved this function in 2c6b47de17c7 ("Cleanup non-arch xtime uses, use get_seconds() or current_kernel_time()."), he evidently did not consider the "inline" behavior important there, no idea if this is even measurable. Arnd