From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757654AbcASUt6 (ORCPT ); Tue, 19 Jan 2016 15:49:58 -0500 Received: from ipmail06.adl2.internode.on.net ([150.101.137.129]:12915 "EHLO ipmail06.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755652AbcASUtt (ORCPT ); Tue, 19 Jan 2016 15:49:49 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: A2DiDwDhn55WPGwtLHleKAECgw+BP4Jkg3qBeJ8xAQEBAQEBBotdiUaGCQICAQECgURNAQEBAQEBBwEBAQFBP0EOAYNlAQEEJxMcIxAIAxgJJQ8FJQMHGhOIGsAFAQEBAQEFAQEBARwYhVqFPIgWgRsFjUKJWI1WjwqOXIJ1G4FxKjSCEIReAQEB Date: Wed, 20 Jan 2016 07:49:46 +1100 From: Dave Chinner To: Deepa Dinamani Cc: Arnd Bergmann , Andreas Dilger , "y2038@lists.linaro.org" , "linux-fsdevel@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Message-ID: <20160119204946.GA20456@dastard> References: <1452144972-15802-1-git-send-email-deepa.kernel@gmail.com> <16314879.ZTNSN2flN7@wuerfel> <20160118211459.GA16611@dastard> <4006633.5eZd1aWvky@wuerfel> <20160119013806.GC16611@dastard> <20160119052713.GA20081@myubuntu.deepaubuntu.g7.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160119052713.GA20081@myubuntu.deepaubuntu.g7.internal.cloudapp.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 18, 2016 at 09:27:13PM -0800, Deepa Dinamani wrote: > > > On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner wrote: > > On Mon, Jan 18, 2016 at 10:46:07PM +0100, Arnd Bergmann wrote: > >> On Tuesday 19 January 2016 08:14:59 Dave Chinner wrote: > >> > On Mon, Jan 18, 2016 at 08:53:22PM +0100, Arnd Bergmann wrote: > >> > > 3. for each file system that uses struct timespec internally to pass > >> > > around inode timestamps, do one patch that adds a > >> > > timespec_to_inode_time() and vice versa, which gets defined like > >> > > > >> > > static inline struct timespec timespec_to_inode(struct timespec t) > >> > > { > >> > > return t; > >> > > } > >> > > >> > This works, and is much cleaner than propagating the macro nastiness > >> > everywhere. IMO vfs_time_to_timespec()/timespec_to_vfs_time would be > >> > better named as it describes the conversion exactly. I don't think > >> > this is a huge patch, though - it's mainly the setattr/kstat > >> > operations that need changing here. > >> > >> Good idea for the name. > >> > >> If you are ok with adding those helpers, then it can be done in small > >> steps indeed. I was under the assumption that you didn't like any > >> kind of abstraction of the type in struct inode at all. > > > > You're right, I don't like unnecessary abstractions. I guess I've > > not communicated the "convert timestamps at the edges, use native > > timestamp types everywhere inside" structure very well, because type > > conversion functions such as the above are an absolutely necessary > > part of ensuring we don't need abstractions in the core code... :P > > > Let's back out a bit and consider a few changes with the suggested "abstraction": > > original code: > > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts, > __le16 __time, __le16 __date, u8 time_cs); > > fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0); > > becomes ugly > > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts, > __le16 __time, __le16 __date, u8 time_cs); > > struct timespec64 mtime = vfs_time_to_timespec64(i_mtime, inode); > fat_time_fat2unix(sbi, &mtime, de->time, de->date, 0); You're doing it wrong. fat_time_fat2unix() still gets passed &inode->i_mtime, and the function prototype is changed to a timespec64. *Nothing else needs to change*, because fat_time_fat2unix() does it own calculations and then stores the time directly into the timespec structure members.... I think you're making a mountain out of a molehill. Most filesystems will be unchanged except for s/timespec/timespec64/ as they store values directly into timespec members when encoding/decoding. There is no need for timestamp conversion in places like this - you're simply not looking deep enough and applying the conversion at the wrong layer. Cheers, Dave. -- Dave Chinner david@fromorbit.com