On Mon, Dec 7, 2015 at 1:30 AM, Arnd Bergmann arnd@arndb.de wrote:
On Sunday 06 December 2015 22:04:05 Deepa Dinamani wrote:
The current representation of inode times in struct inode, struct iattr, struct kstat: struct timespec are not y2038 safe.
Add provision to convert them to use 64 bit times in the future.
struct inode uses struct inode time to maintain same size for times across 32 bit and 64 bit architectures. structs iattr and kstat have no such requirement since these data types are only used to pass values in and out of vfs layer.
Ok, ignore my comment on patch 1, I should have read all the way until here ;-)
I think you can safely merge patch 1 into patch 3, it will actually be clearer that way.
will do
In addition, inode_time is defined as packed and aligned to a 4 byte boundary to make the structure use 12 bytes each instead of 16 bytes. This will help save RAM space as inode structure is cached in memory. The other structures are transient and such a change would not be beneficial.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Ah, this also explains the use of inode_time: it's much nicer if we don't need it in the file systems but only in struct inode.
This is the plan. I did not have to use this inside of vfs anywhere else.
@@ -616,9 +622,15 @@ struct inode { }; dev_t i_rdev; loff_t i_size; +#ifdef CONFIG_FS_USES_64BIT_TIME
struct inode_time i_atime;struct inode_time i_mtime;struct inode_time i_ctime;+#else struct timespec i_atime; struct timespec i_mtime; struct timespec i_ctime; +#endif spinlock_t i_lock; /* i_blocks, i_bytes, maybe i_size */ unsigned short i_bytes; unsigned int i_blkbits; @@ -679,10 +691,17 @@ struct inode { void *i_private; /* fs or device private pointer */ };
+#ifdef CONFIG_FS_USES_64BIT_TIME +#define FS_INODE_SET_XTIME(xtime, inode, ts64) \
((inode)->i_##xtime = timespec64_to_inode_time(ts64))+#define FS_INODE_GET_XTIME(xtime, inode) \
(inode_time_to_timespec64((inode)->i_##xtime))+#else #define FS_INODE_SET_XTIME(xtime, inode, ts) \ ((inode)->i_##xtime = (ts)) #define FS_INODE_GET_XTIME(xtime, inode) \ ((inode)->i_##xtime) +#endif
So this makes a lot of sense now. If it's the only use of timespec64_to_inode_time/inode_time_to_timespec64, you could also open-code that in the macro and avoid introducing the macro to start with.
This is how I had it to begin with. But, I think having this in separate function makes the code more modular. Are we trying to minimize number of functions?
We have had some discussion about limiting the range of the times in the inode to the range allowed by the file system at some point, so leaving them as inline functions will make it easier to extend them later that way.
Macros could be extended to call these functions or directly include range checking from superblock meta data?
I do not prefer using macros in general. But, in this instance I think it makes sense as we can avoid having 3 functions because of ## operator.
We could also probably pass in the actual inode_times to static inline functions and that could so away with macros. Is this what you meant?
-Deepa