Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

[Arnd Bergmann]

On Tuesday 19 January 2016 08:14:59 Dave Chinner wrote:
> 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@xxxxxxxx> wrote:
> > 
> > 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.

What I meant with "one huge patch" is simply just the change that
is needed to modify the type, if we don't use conversion helper
functions.

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

Makes sense.

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

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

No, once we have the timespec_to_vfs_time helpers in all file
systems, that change is just for VFS, and should not touch
any file system specific code.

It is the equivalent of patch 8/15 in the current version
of the series, except that it changes one version of the
code to another rather than changing a CONFIG_* symbol
that alternates between the two versions coexisting in source.

When I first attempted the conversion, I ended up with a very
similar trick that Deepa has now, and it's very helpful to
find what the code locations are that need to be touched,
without doing them all at the same time, as you can simply
flip that option to try out another file system.

However, I agree that this is better not reflected in how the
patches get applied in the end, and there is no need to clutter
the git history with having both options in the code at the
same time, and we should try to avoid touching a lot of code
more than once wherever possible.

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

Ok, thanks.

	Arnd