On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner <david@xxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
