Message-ID: <20160112054235.GA63984@myubuntu.deepaubuntu.g7.internal.cloudapp.net>
Date:	Mon, 11 Jan 2016 21:42:36 -0800
From:	Deepa Dinamani <deepa.kernel@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	y2038@...ts.linaro.org
Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

> On Jan 11, 2016, at 04:33, Dave Chinner <david@...morbit.com> 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

Powered by blists - more mailing lists