On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote:
On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
On Jan 11, 2016, at 04:33, Dave Chinner david@fromorbit.com wrote:
On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
Summarizing, here are the open questions that need to be sorted before another series: 1. What should be part of the series? a. y2038 safe data types conversion. b. range check and clamping 2. How to achieve a seamless transition? Is inode_timespec solution agreed upon to achieve 1a? An alternate approach is included in the cover letter. 3. policy for handling out of range timestamps: There was no conclusion on this from the previous series as noted in the cover letter. I think this can be solved by figuring out the answer to question: who should have a say in deciding the course of action if the timestamps are out of range: a. sysadmin through sysctl (Arnd's suggestion) b. have default vfs handlers with an option for individual fs to override. c. clamp and ignore d. disable expired fs at compile time (Arnd's suggestion)
The inode_timespec alias is part of the problem - AFAICT it exists because you need a representation that is independent of both the old and new in-memory structures.
Maybe you are misunderstanding the fact that only struct inode is changed to have individual fields. Whereas, timespec64 is used everywhere else in the series. inode_timespec is an alias to transition timespec references to timespec64 without breaking anything. This is needed because we don't want to change all references to timespec in a single patch like the cover letter says. The same RFC Patch 2 has more details.
The valid timestamp range stuff in the superblock is absolutely necessary, but that's something that can be done completely independently to the changes for supporting a differnet time storage format.
If I'm defining new functions to support new format, then I should at least get the function signatures right before using them. These can be in a different patch, but should be in the same patch series, before they are used anywhere. For instance, struct timespec timespec_trunc(struct timespec t, unsigned gran); should now take superblock as an argument instead of gran.
And the fs changes cannot really be commented on until the VFs time representation is sorted out properly...
Each fs is changed twice in the current approach to transition everything to timespec64. And, there are different ways of doing this. For instance, Arnd had an idea different from mine as to how this can be done: He was suggesting using something like these accessor macros and incorporating timespec64 from the beginning in the individual filesystems rather than inode_timespec. Again, this is independent of how timestamps are stored in struct inode. There are others that are independent of inode timestamp representation as well
Besides, have you looked at the existing timestamp definitions? they use a struct timespec, which on a 64 bit system:
struct timespec i_atime; /* 88 16 */ struct timespec i_mtime; /* 104 16 */ struct timespec i_ctime; /* 120 16 */use 2 longs and are identical to a timespec64 in everything but name. These should just be changed to a timespec64, and we suck up the fact it increases the size of the 32 bit inode as we have to increase it's size to support time > y2038 anyway.
This is what I meant about premature optimisation - you've got a wonderfully complex solution to a problem that we don't need to solve to support timestamps >y2038. It's also why it goes down the wrong path at this point - most of the changes are not necessary if all we need to do is a simple timespec -> timespec64 type change and the addition timestamp range limiting in the existing truncation function...
The pahole output I pasted in the previous email(on the left) was for timespec.
Yes, I do know that timespec64 is same as timespec on 64 bit systems: #if __BITS_PER_LONG == 64 # define timespec64 timespec
I think it's been agreed upon now that inode timestamps will be changed to use timespec64 as Arnd mentioned, if I do not hear any objections. The whole purpose of this is to gather comments.
The problem really is that there is more than one way of updating these attributes(timestamps in this particular case). The side effect of this is that we don't always call timespec_trunc() before assigning timestamps which can lead to inconsistencies between on disk and in memory inode timestamps.
That's a problem that can be fixed independently of y2038 support. Indeed, we can be quite lazy about updating timestamps - by intent and design we usually have different timestamps in memory compared to on disk, which is one of the reasons why there are so many different ways to change and update timestamps....
This has nothing to do with lazy updates. This is about writing wrong granularities and non clamped values to in-memory inode.
-Deepa