Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310272 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-15 18:00 +0100
    Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Andreas Dilger <adilger@dilger.ca> - 2016-01-16 20:20 +0100
      Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-17 00:40 +0100
        Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Andreas Dilger <adilger@dilger.ca> - 2016-01-17 03:40 +0100
          Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit  time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-18 07:20 +0100
            Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-18 12:00 +0100
              [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-18 18:50 +0100
                Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-18 21:00 +0100
                  Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-18 22:20 +0100
                    Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-18 22:50 +0100
                      Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-19 02:50 +0100
                        Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-19 06:30 +0100
                          Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-19 22:00 +0100
                            Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-19 23:30 +0100
                              Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-20 06:20 +0100
                                Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-20 16:10 +0100
                              Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-21 00:10 +0100
                                Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-21 00:20 +0100
                                  Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit  time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-27 07:30 +0100

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

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(&VFS_I(base_ni)->i_mtime, &now) ||
> >                     !timespec_equal(&VFS_I(base_ni)->i_ctime, &now))
> >                         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 file systems are always broken around 2038 (e.g. HFS in 2040),
> > so if we can't fix them, I want to be able to turn them off
> > in Kconfig along with the 32-bit time_t syscalls.
> 
> That can be done with kconfig depends rules - it has nothing to do
> with this patch set.

kconfig dependencies is what I meant for the simple cases where a
file system is known to always be broken, we just need a small
modification for the cases you mentioned below.

> > ext2/3/4, xfs and ocfs2 (maybe one or two more, I'd have to check)
> > currently behave in a consistent manner across 32-bit and 64-bit
> > architectures by allowing a range between 1902 and 2037, and we
> > obviously don't have a choice there but to keep that current
> > behavior, and extend the time format in one way or another to
> > store additional bits for the epoch.
> 
> That's a filesystem implementation problem, not a generic inode
> timestamp problem. i.e. this is handled when the filesystem converts
> the inode timestamp from a timespec64 in the struct inode to
> whatever format it stores the timestamp on disk. That conversion
> does not change just because the VFS inode moves from a timespec to
> a timespec64. Again, those on-disk format changes to support beyond
> the current epoch are outside the scope of this patchset, because
> they are not affected by the timestamp format the VFS choses to use.

Fine with me, we can have another series to add the Kconfig dependencies
and modify the file systems that need this.

	Arnd

[toc] | [next] | [standalone]

#1311012

[Multipart message — attachments visible in raw view] — view raw

> On Jan 15, 2016, at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> 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.

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.

Cheers, Andreas

>> 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(&VFS_I(base_ni)->i_mtime, &now) ||
>>>                    !timespec_equal(&VFS_I(base_ni)->i_ctime, &now))
>>>                        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 file systems are always broken around 2038 (e.g. HFS in 2040),
>>> so if we can't fix them, I want to be able to turn them off
>>> in Kconfig along with the 32-bit time_t syscalls.
>> 
>> That can be done with kconfig depends rules - it has nothing to do
>> with this patch set.
> 
> kconfig dependencies is what I meant for the simple cases where a
> file system is known to always be broken, we just need a small
> modification for the cases you mentioned below.
> 
>>> ext2/3/4, xfs and ocfs2 (maybe one or two more, I'd have to check)
>>> currently behave in a consistent manner across 32-bit and 64-bit
>>> architectures by allowing a range between 1902 and 2037, and we
>>> obviously don't have a choice there but to keep that current
>>> behavior, and extend the time format in one way or another to
>>> store additional bits for the epoch.
>> 
>> That's a filesystem implementation problem, not a generic inode
>> timestamp problem. i.e. this is handled when the filesystem converts
>> the inode timestamp from a timespec64 in the struct inode to
>> whatever format it stores the timestamp on disk. That conversion
>> does not change just because the VFS inode moves from a timespec to
>> a timespec64. Again, those on-disk format changes to support beyond
>> the current epoch are outside the scope of this patchset, because
>> they are not affected by the timestamp format the VFS choses to use.
> 
> Fine with me, we can have another series to add the Kconfig dependencies
> and modify the file systems that need this.
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1311087

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

[toc] | [prev] | [next] | [standalone]

#1311103

[Multipart message — attachments visible in raw view] — view raw

On Jan 16, 2016, at 4:36 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> 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.

All of those accesses are with the LTIME_S() macro to get/set only the
seconds field of the inode time, so it should only be a one-line patch?

Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1311477

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.

	Arnd

[toc] | [prev] | [next] | [standalone]

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

On Mon, Jan 18, 2016 at 2:56 AM, Arnd Bergmann <arnd@arndb.de> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1311808 — 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@arndb.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@fromorbit.com

[toc] | [prev] | [next] | [standalone]

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

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@arndb.de> 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

[toc] | [prev] | [next] | [standalone]

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

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
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

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

On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner <david@fromorbit.com> 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, &inode->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, &mtime, de->time, de->date, 0);

with inode_timespec it becomes

extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct inode_timespec *ts,
__le16 __time, __le16 __date, u8 time_cs);

fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0);

Time-conversion function abstraction:

Pros:
1. do not have to change vfs core code.

Cons:
1. makes all the filesystems that have to use this ugly.
2. How do you make people use these all the time and not go back to use
inode_timestamps directly as is the case right now?
3. Even during this switch, how do you stop people from adding new code which does
not use these functions?
4. There are some scenarios like direct assignments when these conversions are not
required and some other times they are. Imposing something that needs to be only used
sometimes and not even having a clear guidelines on this is done is very very wrong.
5. And, if you do not plan on removing these functions once done switching to timespec64,
I doubt it will ever get used again and you are leaving dead code in.
6. And, we cannot proving everything is in sync is again a problem.

inode_timespec:

Pros:
1. does not have to change vfs code.
2. individual filesystem changes are also less ugly.
3. Easy to manage: 1 simple rule: always use inode_timespec from now on until the
conversion and then use timespec64.
4. Goes away in the end.
5. Every step is simple and can be proved theoretically right.

Cons:
1. Needs 2 step process as with the other approach.


I think inode_timespec is a much better abstraction.
And, if we are going to use one, then it better be the right one.

-Deepa

[toc] | [prev] | [next] | [standalone]

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

On Mon, Jan 18, 2016 at 09:27:13PM -0800, Deepa Dinamani wrote:
> 
> 
> On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner <david@fromorbit.com> 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, &inode->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, &mtime, de->time, de->date, 0);

You're doing it wrong. fat_time_fat2unix() still gets passed
&inode->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
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

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

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 <david@fromorbit.com> 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, &inode->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, &mtime, de->time, de->date, 0);
> 
> You're doing it wrong. fat_time_fat2unix() still gets passed
> &inode->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.

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(&inode->i_mtime, &tv)) {
-		inode->i_mtime = tv;
+	    !timespec_equal(&mtime, &tv)) {
+		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(&inode->i_ctime, &tv)) {
-		inode->i_ctime = tv;
+	    !timespec_equal(&ctime, &tv)) {
+		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:

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;
 
 	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(&inode->i_mtime, &tv)) {
+	    !vfs_time_equal(&inode->i_mtime, &tv)) {
 		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(&inode->i_ctime, &tv)) {
+	    !vfs_time_equal(&inode->i_ctime, &tv)) {
 		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.

	Arnd

[toc] | [prev] | [next] | [standalone]

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

On Tue, Jan 19, 2016 at 2:25 PM, Arnd Bergmann <arnd@arndb.de> 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 <david@fromorbit.com> 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, &inode->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, &mtime, de->time, de->date, 0);
>>
>> You're doing it wrong. fat_time_fat2unix() still gets passed
>> &inode->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(&inode->i_mtime, &tv)) {
> -               inode->i_mtime = tv;
> +           !timespec_equal(&mtime, &tv)) {
> +               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(&inode->i_ctime, &tv)) {
> -               inode->i_ctime = tv;
> +           !timespec_equal(&ctime, &tv)) {
> +               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.

The middle ground approach below is what came up in my discussion with 
Arnd yesterday.
This would require more manual verification.
Apart from that, the below method works fine.

I will pick a fs and convert it both ways and post series 2a and 2b
(middle ground approach) so that everyone can take a pick.
I will also post statistics on how many such individual fs patches will be 
required.

> 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;
>
>         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(&inode->i_mtime, &tv)) {
> +           !vfs_time_equal(&inode->i_mtime, &tv)) {
>                 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(&inode->i_ctime, &tv)) {
> +           !vfs_time_equal(&inode->i_ctime, &tv)) {
>                 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.
>
>         Arnd

-Deepa

[toc] | [prev] | [next] | [standalone]

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

On Tue, Jan 19, 2016 at 9:12 PM, Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> On Tue, Jan 19, 2016 at 2:25 PM, Arnd Bergmann <arnd@arndb.de> 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 <david@fromorbit.com> 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, &inode->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, &mtime, de->time, de->date, 0);
>>>
>>> You're doing it wrong. fat_time_fat2unix() still gets passed
>>> &inode->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(&inode->i_mtime, &tv)) {
>> -               inode->i_mtime = tv;
>> +           !timespec_equal(&mtime, &tv)) {
>> +               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(&inode->i_ctime, &tv)) {
>> -               inode->i_ctime = tv;
>> +           !timespec_equal(&ctime, &tv)) {
>> +               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 clarify that "original proposal" means in the idealistic case.
That has morphed because of discussions we've had like removing accessors.
It will be clear when I submit the two versions of the patch how vfs_time helps.

> The middle ground approach below is what came up in my discussion with
> Arnd yesterday.
> This would require more manual verification.
> Apart from that, the below method works fine.
>
> I will pick a fs and convert it both ways and post series 2a and 2b
> (middle ground approach) so that everyone can take a pick.
> I will also post statistics on how many such individual fs patches will be
> required.
>
>> 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;
>>
>>         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(&inode->i_mtime, &tv)) {
>> +           !vfs_time_equal(&inode->i_mtime, &tv)) {
>>                 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(&inode->i_ctime, &tv)) {
>> +           !vfs_time_equal(&inode->i_ctime, &tv)) {
>>                 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.
>>
>>         Arnd
>
> -Deepa

[toc] | [prev] | [next] | [standalone]

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

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
> > &inode->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(&inode->i_mtime, &tv)) {
> -		inode->i_mtime = tv;
> +	    !timespec_equal(&mtime, &tv)) {
> +		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(&inode->i_ctime, &tv)) {
> -		inode->i_ctime = tv;
> +	    !timespec_equal(&ctime, &tv)) {
> +		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(&inode->i_mtime, &tv)) {
> +	    !vfs_time_equal(&inode->i_mtime, &tv)) {

+	    !timespec64_equal(&inode->i_mtime, &tv)) {

>  		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(&inode->i_ctime, &tv)) {
> +	    !vfs_time_equal(&inode->i_ctime, &tv)) {

+	    !timespec64_equal(&inode->i_ctime, &tv)) {

>  		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
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1313660

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

[toc] | [prev] | [next] | [standalone]

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

Arnd and I had a discussion about how to proceed here.

We don't think anybody really is wanting a big patch touching 50 fs here.

We are evaluating a couple of approaches that don't do this.

I will post an update next week

On Wed, Jan 20, 2016 at 3:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web