Date:	Thu, 14 Jan 2016 21:03:01 -0800
From:	Deepa Dinamani <deepa.kernel@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	Arnd Bergmann <arnd@...db.de>, y2038@...ts.linaro.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit
 time

On Fri, Jan 15, 2016 at 08:00:01AM +1100, Dave Chinner wrote:
> On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote:
> > On Thursday 14 January 2016 08:04:36 Dave Chinner wrote:
> > > On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote:
> > > > On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote:
> > > > > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
> > > > > > > On Jan 11, 2016, at 04:33, Dave Chinner <david@...morbit.com> wrote:
> > > > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> > > >
> > c) The opposite direction from b) is to first change the common
> >    code, but then any direct assignment between a timespec in
> >    a file system and the timespec64 in the inode/iattr/kstat/etc
> >    first needs a conversion helper so we can build cleanly,
> >    and then we do one file system at a time to remove them all
> >    again while changing the internal structures in the
> >    file system from timespec to timespec64.   
> 
> No new helpers are necessary - we've already got the helper
> functions we need. This:
> 
> int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
>  {
>         struct inode *inode = d_inode(old_dentry);
> +       struct inode_timespec now = current_fs_time(inode->i_sb);
> 
> -       inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> +       VFS_INODE_SET_XTIME(i_ctime, inode, now);
> +       VFS_INODE_SET_XTIME(i_mtime, dir, now);
> +       VFS_INODE_SET_XTIME(i_ctime, dir, now);
>         inc_nlink(inode);
> .....
> 
> is just wrong. All the type conversion and clamping and checking
> done in that VFS_INODE_SET_XTIME() should be done in
> current_fs_time() and have it return a timespec64 directly. Indeed,
> it already does truncation, and can easily be made to do range
> clamping, too.  i.e.  the change should simply be:
> 
> -       inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> +	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_fs_time(inode->i_sb);

The current patch series already does this and macros don't do any clamping.
The accesors are only illustrating a way of using a callback when clamping is detected.
And, if we don't need accessors, I will find a different way of doing that if we agree
it is necessary.

+struct timespec64 current_fs_time(struct super_block *sb)
+{
+       struct timespec64 now = current_kernel_time64();
+
+       return fs_time_trunc(now, sb);
+}
+EXPORT_SYMBOL(current_fs_time);
+
+struct timespec64 current_fs_time_sec(struct super_block *sb)
+{
+       struct timespec64 ts = {ktime_get_real_seconds(), 0};
+
+       /* range check for time. */
+       fs_time_range_check(sb, &ts);
+
+       return ts;
+}

+struct inode_timespec
+fs_time_trunc(struct inode_timespec t, struct super_block *sb)
+{
+       u32 gran = sb->s_time_gran;
+
+       /* range check for time. */
+       fs_time_range_check(sb, &t);
+       if (unlikely(is_fs_timestamp_bad(t)))
+               return t;
+
+       /* Avoid division in the common cases 1 ns and 1 s. */
+       if (gran == 1)
+               ;/* nothing */
+       else if (gran == NSEC_PER_SEC)
+               t.tv_nsec = 0;
+       else if (gran > 1 && gran < NSEC_PER_SEC)
+               t.tv_nsec -= t.tv_nsec % gran;
+       else
+               WARN(1, "illegal file time granularity: %u", gran);
+
+       return t;
+}

But really, you are still misunderstanding what inode_timespec does.
It only introduces aliases and not the accessors.
This is only a way to avoid code duplication since we cannot change all fs
at one time.

And, this is what you would do if you were coding in any object oriented
language. This is object polymorphism.

So, I will paste here again. Whatever is below is all the extra code
inode_timespec introduces:

+#ifdef CONFIG_FS_USES_64BIT_TIME
+
+/* Place holder defines until CONFIG_FS_USES_64BIT_TIME
+ * is enabled.
+ * timespec64 data type and functions will be used at that
+ * time directly and these defines will be deleted.
+ */
+#define inode_timespec timespec64
+
+#define inode_timespec_compare timespec64_compare
+#define inode_timespec_equal   timespec64_equal
+
+#else
+
+#define inode_timespec timespec
+
+#define inode_timespec_compare timespec_compare
+#define inode_timespec_equal   timespec_equal
+
+#endif
+

-Deepa

Powered by blists - more mailing lists