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

[Arnd Bergmann]

On Wednesday 13 January 2016 17:27:16 Dave Chinner wrote:
> > I think
> > it was more than that when I first looked, so it's between 0.2% and 0.3%
> > of savings in total memory, which is certainly worth discussing about,
> > given the renewed interest in conserving RAM in general.  If we want to
> > save this memory, then doing it at the same time as the timespec64 
> > conversion
> > is the right time so we don't need to touch every file twice.
> 
> You just uttered the key words: "If we want to save this memory"
> 
> So let's stop conflating two different lines of development because
> we only actually *need* y2038k support.
> 
> The fact we haven't made timestamp space optimisations means
> that nobody has thought it necessary or worthwhile. y2038k support
> doesn't change the landscape under which we might consider the
> optimisation, so we need to determine if the benefit outweighs the
> cost in terms of code complexity and maintainability.
> 
> So separate the two changes - make the y2038k change simple and
> obviously correct first by changing everything to timespec64. Then it
> won't get delayed by bikeshedding about an optimisation of that is
> of questionable benefit.

Fine with me. I think Deepa already started simplifying the series
already. I agree that for 64-bit machines, there is no need to optimize
that code now, since we are not regressing in terms of memory size.

For 32-bit machines, we are regressing anyway, the question is whether
it's by 12 or 24 bytes per inode. Let me try to estimate the worse-case
scenario here: let's assume that we have 1GB of RAM (anything more
on a 32-bit system gets you into trouble, and if you have less, there
will be less of a problem). Filling all of system ram with small tmpfs
files means a single 4K page plus 280 bytes for the minimum inode,
so we need an additional 6MB or 12MB to store the extra timespec
bits. Probably not too bad for a worst-case scenario, but there is
also the case of storing just the inodes but no pages, and that would
be worse.

I've added the linux-arm and linux-mips lists to cc, to see if anyone has
strong opinions on this matter. We don't have to worry about x86-32 here,
because sizeof(struct timespec64) is 12 bytes there anyway, and I don't
think there are any other 32-bit architectures that have large-scale
deployments or additional requirements we don't already have on ARM.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

[Dave Chinner]

On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote:
> On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote:
> > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
> > > > On Jan 11, 2016, at 04:33, Dave Chinner  wrote:
> > > >
> > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> 
> Summarizing, here are the open questions that need to be sorted before another
> series:
> 1. What should be part of the series?
>   a. y2038 safe data types conversion.
>   b. range check and clamping

Yes.

> 2. How to achieve a seamless transition?
>   Is inode_timespec solution agreed upon to achieve 1a?

No. Just convert direct to timespec64.

>   An alternate approach is included in the cover letter.
> 3. policy for handling out of range timestamps:
>   There was no conclusion on this from the previous series as noted in the
>   cover letter.
>   a. sysadmin through sysctl (Arnd's suggestion)
>   b. have default vfs handlers with an option for individual fs to 
> override.
>   c. clamp and ignore

I think it's a mix - if the timestamps come in from userspace,
fail with ERANGE. That could be controlled by sysctl via VFS
part of the ->setattr operation, or in each of the individual FS
implementations. If they come from the kernel (e.g. atime update) then
the generic behvaiour is to warn and continue, filesystems can
otherwise select their own policy for kernel updates via
->update_time.

>   d. disable expired fs at compile time (Arnd's suggestion)

Not really an option, because it means we can't use filesystems that
interop with other systems (e.g. cameras, etc) because they won't
support y2038k timestamps for a long time, if ever (e.g. vfat).

> > > 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.
> > 
> > That's a problem that can be fixed independently of y2038 support.
> > Indeed, we can be quite lazy about updating timestamps - by intent
> > and design we usually have different timestamps in memory compared
> > to on disk, which is one of the reasons why there are so many
> > different ways to change and update timestamps
> 
> This has nothing to do with lazy updates.
> This is about writing wrong granularities and non clamped values to
> in-memory inode.

Which really shouldn't happen because we should be clamping and/or
truncating timestamps at the creation/entry point into the
VFS/filesystem.

e.g.  current_fs_time(sb) is how filesystems grab the current kernel
time for timestamp updates. Add an equivalent current_fs_time64(sb)
to do return timespec64 and do clamping and limit warning, and now
you have a simple vehicle for converting the VFS and filesystems to
support y2038k clean date formats.

If there are places where filesystems are receiving or using
unchecked timestamps then those are bugs that need fixing. Those
need to be in separate patches to y2038k support...

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

[Deepa Dinamani]

On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote:
> On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
> > > On Jan 11, 2016, at 04:33, Dave Chinner  wrote:
> > >
> > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:

Summarizing, here are the open questions that need to be sorted before another
series:
1. What should be part of the series?
a. y2038 safe data types conversion.
b. range check and clamping
2. How to achieve a seamless transition?
Is inode_timespec solution agreed upon to achieve 1a?
An alternate approach is included in the cover letter.
3. policy for handling out of range timestamps:
There was no conclusion on this from the previous series as noted in the
cover letter.
I think this can be solved by figuring out the answer to question:
who should have a say in deciding the course of action if the timestamps
are out of range:

a. sysadmin through sysctl (Arnd's suggestion)
b. have default vfs handlers with an option for individual fs to 
override.
c. clamp and ignore
d. disable expired fs at compile time (Arnd's suggestion)


> The inode_timespec alias is part of the problem - AFAICT it exists
> because you need a representation that is independent of both the
> old and new in-memory structures.

Maybe you are misunderstanding the fact that only struct inode is changed
to have individual fields. Whereas, timespec64 is used everywhere else in
the series.
inode_timespec is an alias to transition timespec references to
timespec64 without breaking anything. This is needed because we
don't want to change all references to timespec in a single patch like
the cover letter says.
The same RFC Patch 2 has more details.

> The valid timestamp range stuff in the superblock is absolutely
> necessary, but that's something that can be done completely
> independently to the changes for supporting a differnet time storage
> format.

If I'm defining new functions to support new format, then I should at
least get the function signatures right before using them.
These can be in a different patch, but should be in the same patch series,
before they are used anywhere.
For instance,
struct timespec timespec_trunc(struct timespec t, unsigned gran);
should now take superblock as an argument instead of gran.

> And the fs changes cannot really be commented on until the VFs time
> representation is sorted out properly...

Each fs is changed twice in the current approach to transition everything to
timespec64. And, there are different ways of doing this.
For instance, Arnd had an idea different from mine as to how this can be done:
He was suggesting using something like these accessor macros and incorporating
timespec64 from the beginning in the individual filesystems rather than
inode_timespec.
Again, this is independent of how timestamps are stored in struct inode.
There are others that are independent of inode timestamp representation as well


> Besides, have you looked at the existing timestamp definitions? they
> use a struct timespec, which on a 64 bit system:
> 
> struct timespeci_atime;  /*8816 */
> struct timespeci_mtime;  /*   10416 */
> struct timespeci_ctime;  /*   12016 */
> 
> use 2 longs and are identical to a timespec64 in everything but
> name. These should just be changed to a timespec64, and we suck up
> the fact it increases the size of the 32 bit inode as we have to
> increase it's size to support time > y2038 anyway.
> 
> This is what I meant about premature optimisation - you've got a
> wonderfully complex solution to a problem that we don't need to
> solve to support timestamps >y2038. It's also why it goes down the
> wrong path at this point - most of the changes are not necessary if
> all we need to do is a simple timespec -> timespec64 type change and
> the addition timestamp range limiting in the existing truncation
> function...

The pahole output I pasted in the previous email(on the left) was for
timespec.

Yes, I do know that timespec64 is same as timespec on 64 bit systems:
#if __BITS_PER_LONG == 64
# define timespec64 timespec

I think it's been agreed upon now that inode timestamps will be changed
to use timespec64 as Arnd mentioned, if I do not hear any objections.
The whole purpose of this is to gather comments.

> > 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.
> 
> That's a problem that can be fixed independently of y2038 support.
> Indeed, we can be quite lazy about updating timestamps - by intent
> and design we usually have