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.