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

[Deepa Dinamani]

On Mon, Jan 18, 2016 at 2:56 AM, Arnd Bergmann  wrote:
> On Sunday 17 January 2016 22:09:26 Deepa Dinamani wrote:
>> Based on the discussion, here is how I propose to proceed:
>>
>> 1. Series for timestamp range check and clamping
>> 2. Bug fixing patches like change all CURRENT_TIME use cases to
>> current_fs_time()
>> 3. Patches for vfs to use timespec64 internally (maybe a series, if
>> required)
>> 4. Patches that change all fs that use vfs APIs using timestamp arguments
>> (not a series)
>> 5. Change individual fs to use timespec64 (not a series)
>> 6. Change back whatever time conversion APIs left in vfs or individual fs
>> (maybe a series, if required)
>>
>> So, I don't see a need for submitting another series as all the changes now
>> are handled on a case by case basis and no longer have a generic theme.
>>
>> If everyone's in sync then I can proceed with the above plan.
>
> 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.

-Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

[Arnd Bergmann]

On Monday 18 January 2016 09:40:12 Deepa Dinamani wrote:
> On Mon, Jan 18, 2016 at 2:56 AM, Arnd Bergmann  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, 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?

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
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;
}

4. change the internal representation in one patch that changes those
   helpers along with the struct members.
5. change the file systems to use timespec64 internally instead of
   timespec.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

[Dave Chinner]

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

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

OK, just wanted to make clear, because to me "internal" tends to
mean "within a specific filesystem" whilst "generic" is used to
refer to things at the VFS layer...

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [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  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
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

[Dave Chinner]

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