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

2016-01-20 Thread Deepa Dinamani 
On Tue, Jan 19, 2016 at 9:12 PM, Deepa Dinamani  wrote:
>
> On Tue, Jan 19, 2016 at 2:25 PM, Arnd Bergmann  wrote:
>> On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote:
>>> On Mon, Jan 18, 2016 at 09:27:13PM -0800, Deepa Dinamani wrote:
>>> > On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner  wrote:
>>> > > 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:
>>> >
>>> > Let's back out a bit and consider a few changes with the suggested 
>>> > "abstraction":
>>> >
>>> > original code:
>>> >
>>> > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec 
>>> > *ts,
>>> > __le16 __time, __le16 __date, u8 time_cs);
>>> >
>>> > fat_time_fat2unix(sbi, >i_mtime, de->time, de->date, 0);
>>> >
>>> > becomes ugly
>>> >
>>> > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct 
>>> > timespec64 *ts,
>>> > __le16 __time, __le16 __date, u8 time_cs);
>>> >
>>> > struct timespec64 mtime = vfs_time_to_timespec64(i_mtime, inode);
>>> > fat_time_fat2unix(sbi, , de->time, de->date, 0);
>>>
>>> You're doing it wrong. fat_time_fat2unix() still gets passed
>>> >i_mtime, and the function prototype is changed to a
>>> timespec64.  *Nothing else needs to change*, because
>>> fat_time_fat2unix() does it own calculations and then stores the
>>> time directly into the timespec structure members
>>
>> That puts us back at the 'one big patch' problem: We can't change
>> fat_time_fat2unix() to pass a timespec64 until we also change
>> struct inode. The change may be small, but I see roughly 30 file
>> systems that assign i_?time into or from a local variable or pass it
>> into by reference into a function that is not from VFS.
>>
>> see http://pastebin.com/BSnwJa1N for a list (certainly some false
>> positives and some false negatives in there)
>>
>> Roughly two thirds of the instances can be handled easily using
>> vfs_time_to_timespec(), the others could be done much nicer
>> with additional helpers such as inode_timespec_compare()
>>
>>> I think you're making a mountain out of a molehill. Most filesystems
>>> will be unchanged except for s/timespec/timespec64/ as they store
>>> values directly into timespec members when encoding/decoding. There
>>> is no need for timestamp conversion in places like this - you're
>>> simply not looking deep enough and applying the conversion at the
>>> wrong layer.
>
> No, this is not a superficial argument and this is not at the wrong layer.
> Arnd and I both have tried converting these many ways and I'm proposing the
> idea I think is the best.
> And, I'm giving my reasons as to why it still is the best.
>
> Everything is still handled on a case by case basis in your proposal.
> Besides, my real concern is that once the series is done, no one would
> really like it.
> If someone else has a better proposal for handling all the cases in Arnd's
> pastebin link above, please comment.
>
>> Any idea how to improve this somewhat lacking patch?
>>
>> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
>> index b97f1df910ab..7fbb07dcad36 100644
>> --- a/fs/xfs/xfs_trans_inode.c
>> +++ b/fs/xfs/xfs_trans_inode.c
>> @@ -68,22 +68,24 @@ xfs_trans_ichgtime(
>> int flags)
>>  {
>> struct inode*inode = VFS_I(ip);
>> -   struct timespec tv;
>> +   struct timespec tv, mtime, ctime;
>>
>> ASSERT(tp);
>> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>>
>> -   tv = current_fs_time(inode->i_sb);
>> +   tv = vfs_time_to_timespec(current_fs_time(inode->i_sb));
>> +   mtime = vfs_time_to_timespec(inode->i_mtime);
>> +   ctime = vfs_time_to_timespec(inode->i_ctime);
>>
>> if ((flags & XFS_ICHGTIME_MOD) &&
>> -   !timespec_equal(>i_mtime, )) {
>> -   inode->i_mtime = tv;
>> +   !timespec_equal(, )) {
>> +   inode->i_mtime = timespec_to_vfs_time(tv);
>> ip->i_d.di_mtime.t_sec = tv.tv_sec;
>> ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>> }
>> if ((flags & XFS_ICHGTIME_CHG) &&
>> -   !timespec_equal(>i_ctime, )) {
>> -   inode->i_ctime = tv;
>> +   !timespec_equal(, )) {
>> +   inode->i_ctime = timespec_to_vfs_time(tv);
>> ip->i_d.di_ctime.t_sec = tv.tv_sec;
>> ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
>> }
>>
>>
>> The way that Deepa suggests I think would turn out as:
>
> My original proposal is
> 1. change everywhere within individual fs to use vfs_time,
>also make individual fs handle 64 bit arithmetic.
> 2. change vfs and vfs_time defines to use timespec64.
> 3. Get rid of all vfs_time.
>
> This makes sure every fs is touched twice and is doing the right thing
> at every step of the way.

Just to

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

2016-01-20 Thread Dave Chinner 
On Tue, Jan 19, 2016 at 11:25:02PM +0100, Arnd Bergmann wrote:
> On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote:
> > You're doing it wrong. fat_time_fat2unix() still gets passed
> > >i_mtime, and the function prototype is changed to a
> > timespec64.  *Nothing else needs to change*, because
> > fat_time_fat2unix() does it own calculations and then stores the
> > time directly into the timespec structure members
> 
> Any idea how to improve this somewhat lacking patch?
> 
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index b97f1df910ab..7fbb07dcad36 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -68,22 +68,24 @@ xfs_trans_ichgtime(
>   int flags)
>  {
>   struct inode*inode = VFS_I(ip);
> - struct timespec tv;
> + struct timespec tv, mtime, ctime;
>  
>   ASSERT(tp);
>   ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
> - tv = current_fs_time(inode->i_sb);
> + tv = vfs_time_to_timespec(current_fs_time(inode->i_sb));
> + mtime = vfs_time_to_timespec(inode->i_mtime);
> + ctime = vfs_time_to_timespec(inode->i_ctime);
>  
>   if ((flags & XFS_ICHGTIME_MOD) &&
> - !timespec_equal(>i_mtime, )) {
> - inode->i_mtime = tv;
> + !timespec_equal(, )) {
> + inode->i_mtime = timespec_to_vfs_time(tv);
>   ip->i_d.di_mtime.t_sec = tv.tv_sec;
>   ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>   }
>   if ((flags & XFS_ICHGTIME_CHG) &&
> - !timespec_equal(>i_ctime, )) {
> - inode->i_ctime = tv;
> + !timespec_equal(, )) {
> + inode->i_ctime = timespec_to_vfs_time(tv);
>   ip->i_d.di_ctime.t_sec = tv.tv_sec;
>   ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
>   }

WTF? That's insane and completely unnecessary. It's even worse than
the FAT example I've already pointed out was just fucking wrong.

The only change this function requires is:

> The way that Deepa suggests I think would turn out as:
> 
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index b97f1df910ab..54fc3c41047a 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -68,7 +68,7 @@ xfs_trans_ichgtime(
>   int flags)
>  {
>   struct inode*inode = VFS_I(ip);
> - struct timespec tv;
> + struct vfs_time tv;

+   struct timespec64   tv;

>  
>   ASSERT(tp);
>   ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> @@ -76,13 +76,13 @@ xfs_trans_ichgtime(
>   tv = current_fs_time(inode->i_sb);
>  
>   if ((flags & XFS_ICHGTIME_MOD) &&
> - !timespec_equal(>i_mtime, )) {
> + !vfs_time_equal(>i_mtime, )) {

+   !timespec64_equal(>i_mtime, )) {

>   inode->i_mtime = tv;
>   ip->i_d.di_mtime.t_sec = tv.tv_sec;
>   ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>   }
>   if ((flags & XFS_ICHGTIME_CHG) &&
> - !timespec_equal(>i_ctime, )) {
> + !vfs_time_equal(>i_ctime, )) {

+   !timespec64_equal(>i_ctime, )) {

>   inode->i_ctime = tv;
>   ip->i_d.di_ctime.t_sec = tv.tv_sec;
>   ip->i_d.di_ctime.t_nsec = tv.tv_nsec;

> which I would much prefer here.

IOWs, you're now finally suggesting doing the *simple conversion*
I've been suggesting needs to be made *since the start* of this
long, frustrating thread, except you *still want to abstract the
timestamp unnecessarily*.

For the last time: use timespec64 directly, do not abstract it
in any way.

-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

2016-01-20 Thread Arnd Bergmann 
On Thursday 21 January 2016 10:06:32 Dave Chinner wrote:
> 
> IOWs, you're now finally suggesting doing the *simple conversion*
> I've been suggesting needs to be made *since the start* of this
> long, frustrating thread, except you *still want to abstract the
> timestamp unnecessarily*.
> 
> For the last time: use timespec64 directly, do not abstract it
> in any way.

No, in an earlier mail you said

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

Changing 45 files in 20 file systems along with the VFS core in
one patch is what I consider a huge patch, it's the opposite
of small steps.

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

2016-01-19 Thread Dave Chinner 
On Mon, Jan 18, 2016 at 09:27:13PM -0800, Deepa Dinamani wrote:
> 
> 
> On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner  wrote:
> > 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
> 
> 
> Let's back out a bit and consider a few changes with the suggested 
> "abstraction":
> 
> original code:
> 
> extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
> __le16 __time, __le16 __date, u8 time_cs);
> 
> fat_time_fat2unix(sbi, >i_mtime, de->time, de->date, 0);
> 
> becomes ugly
> 
> extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 
> *ts,
> __le16 __time, __le16 __date, u8 time_cs);
> 
> struct timespec64 mtime = vfs_time_to_timespec64(i_mtime, inode);
> fat_time_fat2unix(sbi, , de->time, de->date, 0);

You're doing it wrong. fat_time_fat2unix() still gets passed
>i_mtime, and the function prototype is changed to a
timespec64.  *Nothing else needs to change*, because
fat_time_fat2unix() does it own calculations and then stores the
time directly into the timespec structure members

I think you're making a mountain out of a molehill. Most filesystems
will be unchanged except for s/timespec/timespec64/ as they store
values directly into timespec members when encoding/decoding. There
is no need for timestamp conversion in places like this - you're
simply not looking deep enough and applying the conversion at the
wrong 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

2016-01-18 Thread 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

2016-01-18 Thread 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

2016-01-18 Thread 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

2016-01-18 Thread 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,

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

2016-01-17 Thread Deepa Dinamani 
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.

-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

2016-01-16 Thread Arnd Bergmann 
On Saturday 16 January 2016 12:14:22 Andreas Dilger wrote:
> >> 
> >> Sure, and nfs is a pain because of all it's internal use of
> >> timespecs, too.
> > 
> > lustre is probably the worst.
> 
> Lustre currently only has one-second granularity in a 64-bit field,
> so it doesn't really care about the difference between timespec or
> timespec64 at all.
> 
> The only other uses are for measuring relative times, so the 64-bitness
> shouldn't really matter.
> 
> Could you please point out what issues exist so they can be fixed.

It's not really a bug that needs to be fixed, but more the general
issue of referencing inode->i_?time and attr->ia_?time and passing
them around. When we change the types in the inode and iattr from
timespec to timespec64, all assigments need to be modified, and lustre
has more of those assignments than any other file system I'm aware of.

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

2016-01-15 Thread Arnd Bergmann 
On Friday 15 January 2016 13:49:37 Dave Chinner wrote:
> On Thu, Jan 14, 2016 at 11:46:16PM +0100, Arnd Bergmann wrote:
> > On Friday 15 January 2016 08:00:01 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:
> > 
> > Yes, that is the obvious case, and I guess works for at least half the
> > file systems when they always assign righthand side and lefthand
> > side of the time stamps using the external types or helpers like
> > CURRENT_TIME and current_fs_time().
> > 
> > However, there are a couple of file systems that need a bit more refactoring
> > before we can do this, e.g. in ntfs_truncate:
> 
> Sure, and nfs is a pain because of all it's internal use of
> timespecs, too.

lustre is probably the worst.

> But we have these timespec_to_timespec64 helper
> functions, and that's what we should use in these cases where the
> filesystem cannot support full 64 bit timestamps internally. In
> those cases, they'll be telling the superblock this at mount time
> things like current_fs_time() won't be returning then a timestamp
> that is out of range for a 32 bit timestamp

I'm not worried about the runtime problems here, just how to
get a series of patches that are each doing one reasonable thing
at a time.

> > if (!IS_NOCMTIME(VFS_I(base_ni)) && !IS_RDONLY(VFS_I(base_ni))) {
> > struct timespec now = current_fs_time(VFS_I(base_ni)->i_sb);
> > int sync_it = 0;
> > 
> > if (!timespec_equal(_I(base_ni)->i_mtime, ) ||
> > !timespec_equal(_I(base_ni)->i_ctime, ))
> > sync_it = 1;
> > VFS_I(base_ni)->i_mtime = now;
> > VFS_I(base_ni)->i_ctime = now;
> > }
> > 
> > The type of the local variable must match the return code of
> > current_fs_time(), so if we change over i_mtime and current_fs_time
> > globally, this either has to be rewritten first to avoid the use of
> > local variables, or it needs temporary conversion helpers, or
> > it has to be changed in the same patch. None of those is particularly
> > appealing. There are a few dozen such things in various file systems.
> 
> it gets rewritten to:
> 
>   struct timespec now;
> 
>   now = 
> timespec64_to_timespec(current_fs_time(VFS_I(base_ni)->i_sb));
> 
> 
> and the valid timestamp range for ntfs is set to 32bit timestamps.
> This then leaves it up to the filesystem developers to make
> the ntfs filesystem code 64 bit timestamp clean if the on disk
> format is ever changed to support 64 bit times.
> 
> Same goes for NFS, and any of the other filesystems that use struct
> timespec internally for time representation.

That is what I meant in my previous mail about approach c) being ugly
because it requires sprinkling lots of timespec64_to_timespec() and
timespec_to_timespec64() in the initial patch in order to atomically
change the types in inode/iattr/kstat/... without introducing build
regressions.

It's a rather horrible patch, and quite likely to cause conflicts with
other patches that introduce another use of those structures in the
merge window.

> > Having a global sysctl knob or
> > a compile-time option is better than having each file system
> > implementor take a guess at what users might prefer, if we can't
> > come up with a behavior (e.g. clamp all the time, or error out
> > all the time) that everybody agrees is always correct.
> 
> filesystem implementors will use the helper funtions that are
> provided for this. If they don't (like all the current use of
> CURRENT_TIME), then that's a bug that needs fixing.

Ok, then we are in total agreement here: the policy remains
to be decided by common code, but the implementation can differ
per file system.

> i.e. we need
> a timespec_clamp() function, similar to timespec_trunc(), and y2038k
> compliant filesystems and syscalls need to use them

I was thinking we end up with a single function that does both
clamp() and trunk(), but that's an implementation detail.
> > > > Let me clarify what my idea is here: I want a global kernel option
> > > > that disables all code that has known y2038 issues. If anyone tries
> > > > to build an embedded system with support beyond 2038, that should
> > > > disable all of those things, including file systems, drivers and
> > > > system calls, so we can reasonably assume that everything that works
> > > > today with that kernel build will keep working in the future and
> > > > not break in random ways.
> > > 
> > > It's not that black and white when it comes to filesystems. y2038k
> > > support is determined by the on-disk structure of the filesystem
> > > being mounted, and that is determined at mount time.  When the
> > > filesystem mounts and sets it's valid timestamp ranges the VFS
> > > will need to decide as to whether the filesystem is allowed to
> > > continue mounting or not.
> > 
> > Some

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

2016-01-15 Thread Dave Chinner 
On Fri, Jan 15, 2016 at 06:01:36PM +0100, Arnd Bergmann wrote:
> On Friday 15 January 2016 13:27:34 Dave Chinner wrote:
> > The point I'm making is that we'll have to modify all the existing
> > filesystem code to supply a valid timestamp range to the VFS at
> > mount time for the range checking/clamping, similar to how we do the
> > granularity specification right now. That means we can do rejection
> > of non-y2038k compliant filesystems at runtime based on what the
> > filesystem tells the VFS it supports..  Set up the default to be
> > reject if rw, allow if ro, and provide a mount option to override ad
> > allow mounting rw.
> 
> We can't really default to "reject if rw", because that would break
> all systems using ext3 or xfs, unless users modify their fstab
> or set the flag that makes the partition y2038 compliant.

Right, I was refering to the behaviour of a y2038k compliant kernel,
A current non-compliant kernel will have the default behaviour you
are suggesting.

> The compile-time option that I'm thinking of would change the default
> beween "always allow" and "reject if rw", based on whether the
> system cares about this issue or not. Almost everyone today won't
> care about it at all and would be rather annoyed by being unable
> to mount their rootfs, but some people care about the behavior
> a lot.

Yup, that's exactly what I was implying.

> Having a global sysctl or mount option as an override would be good,
> maybe both if that isn't over-engineering the problem when we already
> have a compile-time option. 

Distros should not be forces to ship multiple kernels just to
provide all the different runtime compliance behaviours their users
require. Make the policy runtime enforcable, but select the default
behaviour and supported policies via compile time options.

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

2016-01-14 Thread Arnd Bergmann 
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  wrote:
> > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> >
> > 2. How to achieve a seamless transition?
> > Is inode_timespec solution agreed upon to achieve 1a?
> 
> No. Just convert direct to timespec64.

The hard part here is how to split that change into logical patches
per file system. We have already discussed all sorts of ways to
do that, but there is no ideal solution, as you usually end up
either having some really large patches, or you have to modify
the same lines multiple times.

The most promising approaches are:

a) In Deepa's current patch set, some infrastructure is first
   introduced by changing the type from timespec to an identical
   inode_timespec, which lets us convert one file system at a time
   to inode_timespec and then change the type once they are all
   done. The downside is then that all file systems have to get
   touched twice so we end up with timespec64 everywhere.

b) A variation of that which I would do is to use have a smaller
   set of infrastructure first, so we can change one file system
   at a time to timespec64 while leaving the common structures to
   use timespec until all file systems are converted. The downside
   is the use of some conversion macros when accessing the times
   in the inode.
   When the common code is changed, those accessor macros get
   turned into trivial assignments that can be removed up later
   or changed in the same patch.

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.   

> > 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.
> > a. sysadmin through sysctl (Arnd's suggestion)
> > b. have default vfs handlers with an option for individual fs to 
> > override.
> > c. clamp and ignore
> 
> I think it's a mix - if the timestamps come in from userspace,
> fail with ERANGE. That could be controlled by sysctl via VFS
> part of the ->setattr operation, or in each of the individual FS
> implementations. If they come from the kernel (e.g. atime update) then
> the generic behvaiour is to warn and continue, filesystems can
> otherwise select their own policy for kernel updates via
> ->update_time.

I'd prefer not to have it done by the individual file system
implementation, so we get a consistent behavior. Normally you either
care about correct time stamps, or you care about interoperability
and you don't want to have errors returned here.

It could be done per mount, but that seems overly complicated
for rather little to be gained.

> > d. disable expired fs at compile time (Arnd's suggestion)
> 
> Not really an option, because it means we can't use filesystems that
> interop with other systems (e.g. cameras, etc) because they won't
> support y2038k timestamps for a long time, if ever (e.g. vfat).

Let me clarify what my idea is here: I want a global kernel option
that disables all code that has known y2038 issues. If anyone tries
to build an embedded system with support beyond 2038, that should
disable all of those things, including file systems, drivers and
system calls, so we can reasonably assume that everything that works
today with that kernel build will keep working in the future and
not break in random ways.

For a file system, this can be done in a number of ways:

* Most file systems today interpret the time as an unsigned 32-bit
  number (as opposed to signed as ext3, xfs and few others do),
  so as long as we use timespec64 in the syscalls, we are ok.

* Some legacy file systems (maybe hfs) can remain disabled, as
  nobody cares about them any more.

* If we still care about them (e.g. ext2), we can make them support
  only read-only mode. In ext4, this would mean forbidding write
  access to file systems that don't have the extended inode format
  enabled.

Normal users that don't care about not breaking in 2038 obviously
won't set the option, and have the same level of backwards compatibility
support as today.

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

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

2016-01-13 Thread Arnd Bergmann 
On Wednesday 13 January 2016 17:27:16 Dave Chinner wrote:
> > I think
> > it was more than that when I first looked, so it's between 0.2% and 0.3%
> > of savings in total memory, which is certainly worth discussing about,
> > given the renewed interest in conserving RAM in general.  If we want to
> > save this memory, then doing it at the same time as the timespec64 
> > conversion
> > is the right time so we don't need to touch every file twice.
> 
> You just uttered the key words: "If we want to save this memory"
> 
> So let's stop conflating two different lines of development because
> we only actually *need* y2038k support.
> 
> The fact we haven't made timestamp space optimisations means
> that nobody has thought it necessary or worthwhile. y2038k support
> doesn't change the landscape under which we might consider the
> optimisation, so we need to determine if the benefit outweighs the
> cost in terms of code complexity and maintainability.
> 
> So separate the two changes - make the y2038k change simple and
> obviously correct first by changing everything to timespec64. Then it
> won't get delayed by bikeshedding about an optimisation of that is
> of questionable benefit.

Fine with me. I think Deepa already started simplifying the series
already. I agree that for 64-bit machines, there is no need to optimize
that code now, since we are not regressing in terms of memory size.

For 32-bit machines, we are regressing anyway, the question is whether
it's by 12 or 24 bytes per inode. Let me try to estimate the worse-case
scenario here: let's assume that we have 1GB of RAM (anything more
on a 32-bit system gets you into trouble, and if you have less, there
will be less of a problem). Filling all of system ram with small tmpfs
files means a single 4K page plus 280 bytes for the minimum inode,
so we need an additional 6MB or 12MB to store the extra timespec
bits. Probably not too bad for a worst-case scenario, but there is
also the case of storing just the inodes but no pages, and that would
be worse.

I've added the linux-arm and linux-mips lists to cc, to see if anyone has
strong opinions on this matter. We don't have to worry about x86-32 here,
because sizeof(struct timespec64) is 12 bytes there anyway, and I don't
think there are any other 32-bit architectures that have large-scale
deployments or additional requirements we don't already have on ARM.

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

2016-01-13 Thread Dave Chinner 
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  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

Yes.

> 2. How to achieve a seamless transition?
>   Is inode_timespec solution agreed upon to achieve 1a?

No. Just convert direct to timespec64.

>   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.
>   a. sysadmin through sysctl (Arnd's suggestion)
>   b. have default vfs handlers with an option for individual fs to 
> override.
>   c. clamp and ignore

I think it's a mix - if the timestamps come in from userspace,
fail with ERANGE. That could be controlled by sysctl via VFS
part of the ->setattr operation, or in each of the individual FS
implementations. If they come from the kernel (e.g. atime update) then
the generic behvaiour is to warn and continue, filesystems can
otherwise select their own policy for kernel updates via
->update_time.

>   d. disable expired fs at compile time (Arnd's suggestion)

Not really an option, because it means we can't use filesystems that
interop with other systems (e.g. cameras, etc) because they won't
support y2038k timestamps for a long time, if ever (e.g. vfat).

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

Which really shouldn't happen because we should be clamping and/or
truncating timestamps at the creation/entry point into the
VFS/filesystem.

e.g.  current_fs_time(sb) is how filesystems grab the current kernel
time for timestamp updates. Add an equivalent current_fs_time64(sb)
to do return timespec64 and do clamping and limit warning, and now
you have a simple vehicle for converting the VFS and filesystems to
support y2038k clean date formats.

If there are places where filesystems are receiving or using
unchecked timestamps then those are bugs that need fixing. Those
need to be in separate patches to y2038k support...

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

2016-01-13 Thread Deepa Dinamani 
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  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 timespeci_atime;  /*8816 */
> struct timespeci_mtime;  /*   10416 */
> struct timespeci_ctime;  /*   12016 */
> 
> 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

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

2016-01-12 Thread Dave Chinner 
On Tue, Jan 12, 2016 at 10:27:07AM +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 19:29:57 Dave Chinner wrote:
> > 
> > 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...
> 
> I originally suggested doing the split representation because I
> was worried about the downsides of using timespec64 on 32-bit systems
> after looking at actual memory consumption on my test box.
> 
> At this moment, I have a total of 145712700 inodes in memory on a machine

Is that all? :P

> with 64GB ram, saving 12 bytes on each amounts to a total of 145MB.

I just posted a patchset that knocks 104 bytes off the XFS inode
(~12% reduction in size).  We need the changes in that patchset to
sanely support >y2038k support in XFS, and it means we now won't
need to grow the XFS inode to add that support, either.

> I think
> it was more than that when I first looked, so it's between 0.2% and 0.3%
> of savings in total memory, which is certainly worth discussing about,
> given the renewed interest in conserving RAM in general.  If we want to
> save this memory, then doing it at the same time as the timespec64 conversion
> is the right time so we don't need to touch every file twice.

You just uttered the key words: "If we want to save this memory"

So let's stop conflating two different lines of development because
we only actually *need* y2038k support.

The fact we haven't made timestamp space optimisations means
that nobody has thought it necessary or worthwhile. y2038k support
doesn't change the landscape under which we might consider the
optimisation, so we need to determine if the benefit outweighs the
cost in terms of code complexity and maintainability.

So separate the two changes - make the y2038k change simple and
obviously correct first by changing everything to timespec64. Then it
won't get delayed by bikeshedding about an optimisation of that is
of questionable benefit.

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

2016-01-12 Thread Dave Chinner 
On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
> > On Jan 11, 2016, at 04:33, Dave Chinner  wrote:
> >
> >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> >> The current representation of inode times in struct inode, struct iattr,
> >> and struct kstat use struct timespec. timespec is not y2038 safe.
> >>
> >> Use scalar data types (seconds and nanoseconds stored separately) to
> >> represent timestamps in struct inode in order to maintain same size for
> >> times across 32 bit and 64 bit architectures.
> >> In addition, lay them out such that they are packed on a naturally
> >> aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
> >> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
> >> This will help save RAM space as inode structure is cached in memory.
> >> The other structures are transient and do not benefit from these
> >> changes.
> >
> > IMO, this decisions sends the patch series immediately down the
> > wrong path.
> 
> There are other things the patch does that I would like to get comments
> on: inode_timespec aliases, range check, individual fs changes etc.
> These are independent of the inode timestamp representation changes.

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.

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.

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

> >TO me, this is a severe case of premature optimisation
> > because everything gets way more complex just to save those 8 bytes,
> > especially as those holes can be filled simply by changing the
> > variable declaration order in the structure and adding a simple
> > comment.
> 
> I had tried rearranging the structure and the pahole tool does not show
> any difference unless you pack and align the struct to 4 bytes on 64
> bit arch.  The change actually saves 16 bytes on x86_64 and adds 12
> bytes on i386.
> 
> Here is the breakdown for struct inode before and after the patch:
> 
> x86_64:
> /* size: 544, cachelines: 9, members: 44 */   |
> /* size: 528, cachelines: 9, members: 47 */
> /* sum members: 534, holes: 3, sum holes: 10 */   |
> /* sum members: 522, holes: 2, sum holes: 6 */
> 
> i386:
> /* size: 328, cachelines: 6, members: 45 */   |
> /* size: 340, cachelines: 6, members: 48 */
> /* sum members: 326, holes: 1, sum holes: 2 */|
> /* sum members: 338, holes: 1, sum holes: 2 */
> 
> According to /proc/slabinfo I estimated savings of 4MB on a lightly
> loaded system.
> 
> > And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at
> > all; you've got to touch lots of code(*), making it all shouty and
> > harder to read.  They seem only to exist because of the above
> > structural change requires an abstract timestamp accessor while
> > CONFIG_FS_USES_64BIT_TIME exists.
> > Given that goes away at the end o
> > the series, so should the macro - if we use a struct timespec64 in
> > the first place, it isn't even necessary as a temporary construct
> 
> timespec64 was the first option considered here.  The problem with using
> timespec64 is the long data type to represent nsec.  If it were possible
> to change timespec64 nsec to int data type then it might be okay  to use
> that if we are not worried about holes.  I do not see why time stamps
> should have different representations on a 32 bit vs a 64 bit arch.

What's it matter? iot's irrelevant to the problem at hand.

Besides, have you looked at the existing timestamp definitions? they
use a struct timespec, which on a 64 bit system:

struct timespeci_atime;  /*8816 */
struct timespeci_mtime;  /*   10416 */
struct timespeci_ctime;  /*   12016 */

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

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

2016-01-11 Thread Deepa Dinamani 
> On Jan 11, 2016, at 04:33, Dave Chinner  wrote:
>
>> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
>> The current representation of inode times in struct inode, struct iattr,
>> and struct kstat use struct timespec. timespec is not y2038 safe.
>>
>> Use scalar data types (seconds and nanoseconds stored separately) to
>> represent timestamps in struct inode in order to maintain same size for
>> times across 32 bit and 64 bit architectures.
>> In addition, lay them out such that they are packed on a naturally
>> aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
>> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
>> This will help save RAM space as inode structure is cached in memory.
>> The other structures are transient and do not benefit from these
>> changes.
>
> IMO, this decisions sends the patch series immediately down the
> wrong path.

There are other things the patch does that I would like to get comments
on: inode_timespec aliases, range check, individual fs changes etc.
These are independent of the inode timestamp representation changes.

>TO me, this is a severe case of premature optimisation
> because everything gets way more complex just to save those 8 bytes,
> especially as those holes can be filled simply by changing the
> variable declaration order in the structure and adding a simple
> comment.

I had tried rearranging the structure and the pahole tool does not show
any difference unless you pack and align the struct to 4 bytes on 64
bit arch.  The change actually saves 16 bytes on x86_64 and adds 12
bytes on i386.

Here is the breakdown for struct inode before and after the patch:

x86_64:
/* size: 544, cachelines: 9, members: 44 */   |
/* size: 528, cachelines: 9, members: 47 */
/* sum members: 534, holes: 3, sum holes: 10 */   |
/* sum members: 522, holes: 2, sum holes: 6 */

i386:
/* size: 328, cachelines: 6, members: 45 */   |
/* size: 340, cachelines: 6, members: 48 */
/* sum members: 326, holes: 1, sum holes: 2 */|
/* sum members: 338, holes: 1, sum holes: 2 */

According to /proc/slabinfo I estimated savings of 4MB on a lightly
loaded system.

> And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at
> all; you've got to touch lots of code(*), making it all shouty and
> harder to read.  They seem only to exist because of the above
> structural change requires an abstract timestamp accessor while
> CONFIG_FS_USES_64BIT_TIME exists.
> Given that goes away at the end o
> the series, so should the macro - if we use a struct timespec64 in
> the first place, it isn't even necessary as a temporary construct

timespec64 was the first option considered here.  The problem with using
timespec64 is the long data type to represent nsec.  If it were possible
to change timespec64 nsec to int data type then it might be okay  to use
that if we are not worried about holes.  I do not see why time stamps
should have different representations on a 32 bit vs a 64 bit arch.
This left us with the option define a new data type to represent
timestamps.  I agreed with the concerns on the earlier RFC series that
there are already very many data types to represent time in the kernel.
So this left me with the option of using scalar types to represent these.
The scalar types were not used for optimization. They just happened to
serve that purpose as well.  This could be in a follow on patch, but as
long as we are changing the representation everywhere, I don't see why
there should be an intermediate step to change it to timespec64 only to
change it to this representation later.

As far as accessors are concerned, there already are accessors in the
VFS: generic_fillattr() and setattr_copy(). 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. Also,
since these also touch other attributes, these become more restrictive.
The accessors were an idea to streamline all accesses to timestamps in
inode.  Right now the accessor macros also figure out if the timestamps
were clamped and then call the registered callback.  But, this can be
extended to include fs_time_trunc() and then all the end users can
just use these and not worry about the right granularity or range.
As the commit text says, these can be changed to inline functions to
avoid shouty case.

> (*) I note you haven't touched XFS, which means you've probably
> broken lots of other filesystem code. e.g. in XFS, functions like
> xfs_vn_getattr() and xfs_vn_update_time() access inode->i_[acm]time
> directly and hence are not going to compile after this patch series.

I think I should have explained this more in my cover letter, as this
has come up twice now.  Patches 1-7 are the only ones that are relevant

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

2016-01-10 Thread Dave Chinner 
On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> The current representation of inode times in struct inode, struct iattr,
> and struct kstat use struct timespec. timespec is not y2038 safe.
> 
> Use scalar data types (seconds and nanoseconds stored separately) to
> represent timestamps in struct inode in order to maintain same size for
> times across 32 bit and 64 bit architectures.
> In addition, lay them out such that they are packed on a naturally
> aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
> This will help save RAM space as inode structure is cached in memory.
> The other structures are transient and do not benefit from these
> changes.

IMO, this decisions sends the patch series immediately down the
wrong path. TO me, this is a severe case of premature optimisation
because everything gets way more complex just to save those 8 bytes,
especially as those holes can be filled simply by changing the
variable declaration order in the structure and adding a simple
comment.

And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at
all; you've got to touch lots of code(*), making it all shouty and
harder to read.  They seem only to exist because of the above
structural change requires an abstract timestamp accessor while
CONFIG_FS_USES_64BIT_TIME exists. Given that goes away at the end o
the series, so should the macro - if we use a struct timespec64 in
the first place, it isn't even necessary as a temporary construct.

(*) I note you haven't touched XFS, which means you've probably
broken lots of other filesystem code. e.g. in XFS, functions like
xfs_vn_getattr() and xfs_vn_update_time() access inode->i_[acm]time
directly and hence are not going to compile after this patch series.

Cheers,

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