> On Jan 11, 2016, at 04:33, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > >> 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. There are other things the patch does that I would like to get comments on: inode_timespec aliases, range check, individual fs changes etc. These are independent of the inode timestamp representation changes. >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. I had tried rearranging the structure and the pahole tool does not show any difference unless you pack and align the struct to 4 bytes on 64 bit arch. The change actually saves 16 bytes on x86_64 and adds 12 bytes on i386. Here is the breakdown for struct inode before and after the patch: x86_64: /* size: 544, cachelines: 9, members: 44 */ | /* size: 528, cachelines: 9, members: 47 */ /* sum members: 534, holes: 3, sum holes: 10 */ | /* sum members: 522, holes: 2, sum holes: 6 */ i386: /* size: 328, cachelines: 6, members: 45 */ | /* size: 340, cachelines: 6, members: 48 */ /* sum members: 326, holes: 1, sum holes: 2 */ | /* sum members: 338, holes: 1, sum holes: 2 */ According to /proc/slabinfo I estimated savings of 4MB on a lightly loaded system. > 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 timespec64 was the first option considered here. The problem with using timespec64 is the long data type to represent nsec. If it were possible to change timespec64 nsec to int data type then it might be okay to use that if we are not worried about holes. I do not see why time stamps should have different representations on a 32 bit vs a 64 bit arch. This left us with the option define a new data type to represent timestamps. I agreed with the concerns on the earlier RFC series that there are already very many data types to represent time in the kernel. So this left me with the option of using scalar types to represent these. The scalar types were not used for optimization. They just happened to serve that purpose as well. This could be in a follow on patch, but as long as we are changing the representation everywhere, I don't see why there should be an intermediate step to change it to timespec64 only to change it to this representation later. As far as accessors are concerned, there already are accessors in the VFS: generic_fillattr() and setattr_copy(). The problem really is that there is more than one way of updating these attributes(timestamps in this particular case). The side effect of this is that we don't always call timespec_trunc() before assigning timestamps which can lead to inconsistencies between on disk and in memory inode timestamps. Also, since these also touch other attributes, these become more restrictive. The accessors were an idea to streamline all accesses to timestamps in inode. Right now the accessor macros also figure out if the timestamps were clamped and then call the registered callback. But, this can be extended to include fs_time_trunc() and then all the end users can just use these and not worry about the right granularity or range. As the commit text says, these can be changed to inline functions to avoid shouty case. > (*) 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. I think I should have explained this more in my cover letter, as this has come up twice now. Patches 1-7 are the only ones that are relevant and compiled and tested. These change three example filesystems as an illustration of the proposed solution. Of course, every filesystem will have to be changed similarly before patches 8-15 and a few more to change additional filesystems to use timespec64 can be merged. Patches 8-15 were included merely to provide a complete picture, as I thought patches explained the concept better than words only. These have not even been compiled, as these are for illustration purposes as noted in the cover letter. -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
