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
On Sunday 17 January 2016 22:09:26 Deepa Dinamani wrote:
Based on the discussion, here is how I propose to proceed:
- Series for timestamp range check and clamping
- 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
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:
- Series for timestamp range check and clamping
- 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
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:
- cr_time
- 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:
- fat_time_fat2unix, fat_time_unix2fat
Both the above 2 functions also will have to be modified along with vfs.
CIFS:
- struct cifs_fscache_inode_auxdata - last_write_time, last_change_time
- cifs_fattr
- 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
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:
- cr_time
- 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:
- fat_time_fat2unix, fat_time_unix2fat
Both the above 2 functions also will have to be modified along with vfs.
CIFS:
- struct cifs_fscache_inode_auxdata - last_write_time, last_change_time
- cifs_fattr
- 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):
- add timestamp range checking and clamping
- 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.
- 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.
- 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.
- 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.
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):
- add timestamp range checking and clamping
- 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.
- 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.
- 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.
- 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
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:
- 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
- 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.
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:
- 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
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:
- 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.
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
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
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
- change everywhere within individual fs to use vfs_time, also make individual fs handle 64 bit arithmetic.
- change vfs and vfs_time defines to use timespec64.
- 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
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);
!timespec_equal(&inode->i_ctime, &tv)) {inode->i_ctime = tv;
!timespec_equal(&ctime, &tv)) {inode->i_ctime = timespec_to_vfs_time(tv);
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.
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
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
-
Arnd Bergmann -
Dave Chinner -
Deepa Dinamani