Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

[Dave Chinner <david@fromorbit.com>]

On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> The current representation of inode times in struct inode, struct iattr,
> and struct kstat use struct timespec. timespec is not y2038 safe.
> 
> Use scalar data types (seconds and nanoseconds stored separately) to
> represent timestamps in struct inode in order to maintain same size for
> times across 32 bit and 64 bit architectures.
> In addition, lay them out such that they are packed on a naturally
> aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
> This will help save RAM space as inode structure is cached in memory.
> The other structures are transient and do not benefit from these
> changes.

IMO, this decisions sends the patch series immediately down the
wrong path. TO me, this is a severe case of premature optimisation
because everything gets way more complex just to save those 8 bytes,
especially as those holes can be filled simply by changing the
variable declaration order in the structure and adding a simple
comment.

And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at
all; you've got to touch lots of code(*), making it all shouty and
harder to read.  They seem only to exist because of the above
structural change requires an abstract timestamp accessor while
CONFIG_FS_USES_64BIT_TIME exists. Given that goes away at the end o
the series, so should the macro - if we use a struct timespec64 in
the first place, it isn't even necessary as a temporary construct.

(*) I note you haven't touched XFS, which means you've probably
broken lots of other filesystem code. e.g. in XFS, functions like
xfs_vn_getattr() and xfs_vn_update_time() access inode->i_[acm]time
directly and hence are not going to compile after this patch series.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com