Message-ID: <20160118211459.GA16611@dastard>
Date:	Tue, 19 Jan 2016 08:14:59 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Deepa Dinamani <deepa.kernel@...il.com>,
	Andreas Dilger <adilger@...ger.ca>,
	"y2038@...ts.linaro.org" <y2038@...ts.linaro.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

On Mon, Jan 18, 2016 at 08:53:22PM +0100, Arnd Bergmann wrote:
> On Monday 18 January 2016 09:40:12 Deepa Dinamani wrote:
> > On Mon, Jan 18, 2016 at 2:56 AM, Arnd Bergmann <arnd@...db.de> wrote:
> > > On Sunday 17 January 2016 22:09:26 Deepa Dinamani wrote:
> > >> Based on the discussion, here is how I propose to proceed:
> > > Sounds good to me. Step 3 of course is the hard one, and you may run into
> > > further problems with it, as we both have in our previous attempts to
> > > crack this nut, but with step 2 before it that may become manageable.
> > 
> > Right, I don't agree with this approach and it will get very ugly.
> > I was just proposing a way to move forward because it looked like we are at
> > a stalemate.
> > 
> > Maybe xfs doesn't have these problems but some of the other fs-es do.
> > And, these will need changing twice: before(to use 64 bit arithmetic
> > like cifs, use current_fs_time() like fat etc) and along with vfs.
> > 
> > It will unnecessarily bloat the vfs switching to timespec64 code.
> > Below are 3 example filesystem changes that illustrates this problem:
> > 
> > Ext4:
> > 1. cr_time
> > 2. Encode and Decode api's
> > 
> > Both these ext4 changes need to made along with vfs change to ext4.
> > Many such fs exists and will make the vfs switch over very ugly.
> > 
> > FAT:
> > 1. fat_time_fat2unix, fat_time_unix2fat
> > 
> > Both the above 2 functions also will have to be modified along with vfs.
> > 
> > CIFS:
> > 1.  struct cifs_fscache_inode_auxdata - last_write_time, last_change_time
> > 2.  cifs_fattr
> > 3.  cifs_NTtimeToUnix, cifs_UnixTimeToNT, cnvrtDosUnixTm
> > 
> > All the above cifs changes also need to be changed in the same patch as
> > vfs switch to timespec64.
> > 
> > I don't think there is any nicer way to do this without having an
> > encapsulation layer like inode_timespec or accessors you mentioned to
> > change the underlying data type in the vfs.
> > 
> > Also, this scheme is so outrageously ugly that you can easily miss
> > some change.  There is no way of verifying the approach theoretically.
> > Of course, I will be using kernel tests like in other cases.
> 
> I agree it's ugly and fragile to have one huge patch,

Nobody is suggesting one huge patch here. This can all be done with
small steps.

> but I think the
> best way to illustrate it is to make it as small as possible and
> then talk about whether that makes it acceptable or how we can
> work around the problems.
> 
> Do you have an estimate what portion of the file systems need any
> changes at all before we can flip over VFS to the new types?

All filesystems will, at least, need auditing. A large number of
them will need changes, no matter how we "abstract" the VFS type
change, even if it is just for 32->64 bit sign extension bugs.

Filesystems that have intermediate timestamp formats such as Lustre,
NFS, CIFS, etc will need conversion at the vfs/filesytem entry
points, and their internals will remain unchanged. Fixing the
internals is outside the scope fo the VFS change - the 64 bit VFS
inode support stops at the VFS inode/filesystem boundary.

> If it's less than half, we you can try yet another variation (nothing
> new really, we are always dealing with the same few tricks):
> 
> 1. add timestamp range checking and clamping
> 2. kill off CURRENT_TIME

Other way around. First make everything use the existing current
time functions, then ensure that incoming timestamps are truncated
correctly, then add range checking and clamping to the existing
time modification functions.

> 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.

> 4. change the internal representation in one patch that changes those
>    helpers along with the struct members.

If you are talking about converting internal filesystem
representations to (e.g. CIFS fattr, NFS fattr, etc) then this is
wrong. Those filesystems are isolated and able to use timespecs
internally by step 3, and without protocol/format changes can't
support y2038k compliant dates. Hence fixing such problems is a
problem for the filesystem developers and is not an issue for the
VFS timestamp conversion.

That said, stuff like the ext4 encode/decode routines (my eyes, they
bleed!) that pass the VFS inode timestamp by reference to other
functions will need fixing here.

> 5. change the file systems to use timespec64 internally instead of
>    timespec.

I think that will work and leave use with a relatively clean code
base, as well as be able to address y2038k support each individual
filesystem in our own time.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists