Message-ID: <2433031.kYHJ1ECYja@wuerfel>
Date:	Wed, 24 Feb 2016 14:56:16 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	y2038@...ts.linaro.org, Deepa Dinamani <deepa.kernel@...il.com>,
	linux-fsdevel@...r.kernel.org, david@...morbit.com,
	' Theodore Ts'''o <tytso@....edu>,
	linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk
Subject: Re: [Y2038] [RFC v2] vfs 64 bit time transition proposals

On Wednesday 24 February 2016 13:19:07 Thomas Gleixner wrote:
> On Fri, 12 Feb 2016, Arnd Bergmann wrote:
> > 2b has the main advantage of not changing behavior with the flip, so
> >    we can convert all file systems to use vfs_time relatively easily
> >    and then later make them actually use 64-bit timestamps with
> >    a patch that each file system developer can do for themselves.
> 
> And that's a very clear advantage of this approch. The change is purely
> mechanical and can be largely done with coccinelle.

Actually Deepa pointed out one common case where the behavior actually
changes:

	inode->i_mtime.tv_sec = le32_to_cpu(on_disk_mtime);

will change behavior when tv_sec is changed to a time64_t, but
it only changes in a way that is consistent with 64-bit architectures,
and it will not change for user space that uses the existing syscalls
to actually read the timestamps, so the risk of introducing a regression
here is still really low.

> >    One downside is that it leads to rather ugly code as discussed
> >    before, examples are in "[RFC v2b 5/5] fs: xfs: change inode
> >    times to use vfs_time data type" and "[RFC v2b 3/5] fs: btrfs:
> >    Use vfs_time accessors".
> 
> -       now = current_fs_time(inode->i_sb);
> -       if (!timespec_equal(&inode->i_mtime, &now))
> -               inode->i_mtime = now;
> +       now = vfs_time_to_timespec(current_fs_time(inode->i_sb));
> +       ts = vfs_time_to_timespec(inode->i_mtime);
> +       if (!timespec_equal(&ts, &now))
> +               inode->i_mtime = timespec_to_vfs_time(now);
> +
> +       ts = vfs_time_to_timespec(inode->i_mtime);
> +       if (!timespec_equal(&ts, &now))
> +               inode->i_ctime = timespec_to_vfs_time(now);
> 
> You can either provide a helper function which does that m/c_time update at
> the VFS level where you can hide the mess or just accept that this transition
> will introduce some odd constructs like the above.

I think we can live with this. There are around 10 files that do
timespec_compare() or timespec_equal(), comparing an inode timestamp
with a local variable or a filesystem specific timespec value,
but they are all slightly different so there is no obvious
helper function to address multiple file systems at once
without also introducing a 'struct vfs_time'.

> > 2c gets us the furthest along the way for the conversion, close
> >    to where we want to end up in the long run, so we could do that
> >    to file systems one by one. The behavior change is immediate,
> >    so there are fewer possible surprises than with 2a, but it
> >    also means the most upfront work.
> 
> I would'nt worry about the upfront work to much, if it is the cleanest way to
> do the conversion. Though, I'm not seing how that really makes the whole thing
> simpler.
> 
> So far we had good results with changing core code first and then fix up the
> users one by one and at the end remove any intermediary helpers which we
> introduced along the way. So I'd chose 2b as it's a very clear transition
> mechanism with pretty low risk.

Ok, thanks for your input, let's do that then.

I'd like to get the preparation patch ([RFC v2b 1/5] vfs: Add vfs_time accessors)
merged as soon as possible so we can do the rest of the series through
the file system maintainer trees where possible.

I guess even though the patch by itself is trivial, it's now too late to
do for 4.5, but we should be able to just put that into your tip tree, 
or Al's vfs tree for 4.6 and then try to get all file systems moved over
for 4.7, followed by the vfs patch to actually change the type.

	Arnd

Powered by blists - more mailing lists