Message-ID: <20160113163313.GA22698@deepa-ubuntu>
Date:	Wed, 13 Jan 2016 08:33:16 -0800
From:	Deepa Dinamani <deepa.kernel@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	Deepa Dinamani <deepa.kernel@...il.com>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	y2038@...ts.linaro.org
Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

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:

Summarizing, here are the open questions that need to be sorted before another
series:
1. What should be part of the series?
	a. y2038 safe data types conversion.
	b. range check and clamping
2. How to achieve a seamless transition?
	Is inode_timespec solution agreed upon to achieve 1a?
	An alternate approach is included in the cover letter.
3. policy for handling out of range timestamps:
	There was no conclusion on this from the previous series as noted in the
	cover letter.
	I think this can be solved by figuring out the answer to question:
	who should have a say in deciding the course of action if the timestamps
	are out of range:
	
	a. sysadmin through sysctl (Arnd's suggestion)
	b. have default vfs handlers with an option for individual fs to override.
	c. clamp and ignore
	d. disable expired fs at compile time (Arnd's suggestion)


> The inode_timespec alias is part of the problem - AFAICT it exists
> because you need a representation that is independent of both the
> old and new in-memory structures.

Maybe you are misunderstanding the fact that only struct inode is changed
to have individual fields. Whereas, timespec64 is used everywhere else in
the series.
inode_timespec is an alias to transition timespec references to
timespec64 without breaking anything. This is needed because we
don't want to change all references to timespec in a single patch like
the cover letter says.
The same RFC Patch 2 has more details.

> The valid timestamp range stuff in the superblock is absolutely
> necessary, but that's something that can be done completely
> independently to the changes for supporting a differnet time storage
> format.

If I'm defining new functions to support new format, then I should at
least get the function signatures right before using them.
These can be in a different patch, but should be in the same patch series,
before they are used anywhere.
For instance,
struct timespec timespec_trunc(struct timespec t, unsigned gran);
should now take superblock as an argument instead of gran.

> And the fs changes cannot really be commented on until the VFs time
> representation is sorted out properly...

Each fs is changed twice in the current approach to transition everything to
timespec64. And, there are different ways of doing this.
For instance, Arnd had an idea different from mine as to how this can be done:
He was suggesting using something like these accessor macros and incorporating
timespec64 from the beginning in the individual filesystems rather than
inode_timespec.
Again, this is independent of how timestamps are stored in struct inode.
There are others that are independent of inode timestamp representation as well


> Besides, have you looked at the existing timestamp definitions? they
> use a struct timespec, which on a 64 bit system:
> 
>         struct timespec            i_atime;              /*    88    16 */
>         struct timespec            i_mtime;              /*   104    16 */
>         struct timespec            i_ctime;              /*   120    16 */
> 
> use 2 longs and are identical to a timespec64 in everything but
> name. These should just be changed to a timespec64, and we suck up
> the fact it increases the size of the 32 bit inode as we have to
> increase it's size to support time > y2038 anyway.
> 
> This is what I meant about premature optimisation - you've got a
> wonderfully complex solution to a problem that we don't need to
> solve to support timestamps >y2038. It's also why it goes down the
> wrong path at this point - most of the changes are not necessary if
> all we need to do is a simple timespec -> timespec64 type change and
> the addition timestamp range limiting in the existing truncation
> function...

The pahole output I pasted in the previous email(on the left) was for
timespec.

Yes, I do know that timespec64 is same as timespec on 64 bit systems:
#if __BITS_PER_LONG == 64
# define timespec64 timespec

I think it's been agreed upon now that inode timestamps will be changed
to use timespec64 as Arnd mentioned, if I do not hear any objections.
The whole purpose of this is to gather comments.

> > The problem really is that
> > there is more than one way of updating these attributes(timestamps in
> > this particular case). The side effect of this is that we don't always
> > call timespec_trunc() before assigning timestamps which can lead to
> > inconsistencies between on disk and in memory inode timestamps.
> 
> That's a problem that can be fixed independently of y2038 support.
> Indeed, we can be quite lazy about updating timestamps - by intent
> and design we usually have different timestamps in memory compared
> to on disk, which is one of the reasons why there are so many
> different ways to change and update timestamps....

This has nothing to do with lazy updates.
This is about writing wrong granularities and non clamped values to
in-memory inode.

-Deepa

Powered by blists - more mailing lists