>> >> @@ -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?
>
> Generally the optimization should prefer readability and performance.
> I was thinking here that the code is easier to follow if there is
> only one intermediate step that we introduce rather than two.
>
> Another option would be to drop the structure entirely and put the
> members directly into the inode as i_atime_sec/i_atime_nsec/i_mtime_sec/
> etc. That would also avoid marking the 64-bit members as 'packed',
> which may be inefficient on some CPU architectures.
>
I considered this option. This would need weird way of adding the fields:
iatime_sec;
imtime_sec;
ictime_sec;
iatime_nsec;
iatime_nsec;
ictime_nsec;
And based on if anybody rearranges these fields while adding new ones,
we might have different kind of packing.
While the struct is guaranteed to have 12 bytes always.
>> > 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?
>
> Yes, that would be one way to handle it. As long as we have a macro
> or inline function abstraction, we can hide it in there.
>
> Not needing macros might actually be nice too, so we could have a set
> of inline functions instead, like
>
> static inline struct timespec64 inode_get_atime(struct inode *inode)
> {
> return (struct timespec64) {
> .tv_sec = inode->i_atime_sec,
> .tv_nsec = inode->i_atime_nsec,
> };
> }
>
> static inline void inode_set_atime(struct inode *inode, struct timespec64 *t)
> {
> inode->i_atime_sec = t->tv_sec;
> inode->i_atime_nsec = 0;
> }
>
> There are only three of them, so it's not much more code than the
> macros, and those are certainly more readable and easier to extend.
>
I can go ahead and change it to use only inline functions instead of macros.
I was thinking of this:
static inline struct timespec64 inode_get_inode_time(struct inode_time atime)
These can have definitions similar to that of inode_time and ts64
conversions I have now.
I will pair them with the inode struct as well and not put them in time64.h.
> There is also some code already in timespec_trunc that limits the
> precision to the resolution supported by the file system in the setattr
> functions. We can extend timespec_trunc to also limit the range, as
> long as it is used consistently.
>
Ok. Yes, this seems like a good place to do the range checking.
I will keep this in mind when I post an update.
Here is my plan:
1.Update the patches according to above comments.
2.Global vfs substitutions with inline functions.
3. Will extend ext4 and fat to support these inline functions as well.
4. Will use these file systems to do some testing.
5. Send out all the above patches for review together.
-Deepa
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
