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

[Deepa Dinamani]

On Tue, Jan 19, 2016 at 9:12 PM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
>
> On Tue, Jan 19, 2016 at 2:25 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote:
>>> On Mon, Jan 18, 2016 at 09:27:13PM -0800, Deepa Dinamani wrote:
>>> > On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>>> > > On Mon, Jan 18, 2016 at 10:46:07PM +0100, Arnd Bergmann wrote:
>>> > >> On Tuesday 19 January 2016 08:14:59 Dave Chinner wrote:
>>> > >> > On Mon, Jan 18, 2016 at 08:53:22PM +0100, Arnd Bergmann wrote:
>>> >
>>> > Let's back out a bit and consider a few changes with the suggested "abstraction":
>>> >
>>> > original code:
>>> >
>>> > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
>>> > __le16 __time, __le16 __date, u8 time_cs);
>>> >
>>> > fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0);
>>> >
>>> > becomes ugly
>>> >
>>> > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
>>> > __le16 __time, __le16 __date, u8 time_cs);
>>> >
>>> > struct timespec64 mtime = vfs_time_to_timespec64(i_mtime, inode);
>>> > fat_time_fat2unix(sbi, &mtime, de->time, de->date, 0);
>>>
>>> You're doing it wrong. fat_time_fat2unix() still gets passed
>>> &inode->i_mtime, and the function prototype is changed to a
>>> timespec64.  *Nothing else needs to change*, because
>>> fat_time_fat2unix() does it own calculations and then stores the
>>> time directly into the timespec structure members....
>>
>> That puts us back at the 'one big patch' problem: We can't change
>> fat_time_fat2unix() to pass a timespec64 until we also change
>> struct inode. The change may be small, but I see roughly 30 file
>> systems that assign i_?time into or from a local variable or pass it
>> into by reference into a function that is not from VFS.
>>
>> see http://pastebin.com/BSnwJa1N for a list (certainly some false
>> positives and some false negatives in there)
>>
>> Roughly two thirds of the instances can be handled easily using
>> vfs_time_to_timespec(), the others could be done much nicer
>> with additional helpers such as inode_timespec_compare()
>>
>>> I think you're making a mountain out of a molehill. Most filesystems
>>> will be unchanged except for s/timespec/timespec64/ as they store
>>> values directly into timespec members when encoding/decoding. There
>>> is no need for timestamp conversion in places like this - you're
>>> simply not looking deep enough and applying the conversion at the
>>> wrong layer.
>
> No, this is not a superficial argument and this is not at the wrong layer.
> Arnd and I both have tried converting these many ways and I'm proposing the
> idea I think is the best.
> And, I'm giving my reasons as to why it still is the best.
>
> Everything is still handled on a case by case basis in your proposal.
> Besides, my real concern is that once the series is done, no one would
> really like it.
> If someone else has a better proposal for handling all the cases in Arnd's
> pastebin link above, please comment.
>
>> Any idea how to improve this somewhat lacking patch?
>>
>> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
>> index b97f1df910ab..7fbb07dcad36 100644
>> --- a/fs/xfs/xfs_trans_inode.c
>> +++ b/fs/xfs/xfs_trans_inode.c
>> @@ -68,22 +68,24 @@ xfs_trans_ichgtime(
>>         int                     flags)
>>  {
>>         struct inode            *inode = VFS_I(ip);
>> -       struct timespec         tv;
>> +       struct timespec         tv, mtime, ctime;
>>
>>         ASSERT(tp);
>>         ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>>
>> -       tv = current_fs_time(inode->i_sb);
>> +       tv = vfs_time_to_timespec(current_fs_time(inode->i_sb));
>> +       mtime = vfs_time_to_timespec(inode->i_mtime);
>> +       ctime = vfs_time_to_timespec(inode->i_ctime);
>>
>>         if ((flags & XFS_ICHGTIME_MOD) &&
>> -           !timespec_equal(&inode->i_mtime, &tv)) {
>> -               inode->i_mtime = tv;
>> +           !timespec_equal(&mtime, &tv)) {
>> +               inode->i_mtime = timespec_to_vfs_time(tv);
>>                 ip->i_d.di_mtime.t_sec = tv.tv_sec;
>>                 ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>>         }
>>         if ((flags & XFS_ICHGTIME_CHG) &&
>> -           !timespec_equal(&inode->i_ctime, &tv)) {
>> -               inode->i_ctime = tv;
>> +           !timespec_equal(&ctime, &tv)) {
>> +               inode->i_ctime = timespec_to_vfs_time(tv);
>>                 ip->i_d.di_ctime.t_sec = tv.tv_sec;
>>                 ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
>>         }
>>
>>
>> The way that Deepa suggests I think would turn out as:
>
> My original proposal is
> 1. change everywhere within individual fs to use vfs_time,
>    also make individual fs handle 64 bit arithmetic.
> 2. change vfs and vfs_time defines to use timespec64.
> 3. Get rid of all vfs_time.
>
> This makes sure every fs is touched twice and is doing the right thing
> at every step of the way.

Just to clarify that "original proposal" means in the idealistic case.
That has morphed because of discussions we've had like removing accessors.
It will be clear when I submit the two versions of the patch how vfs_time helps.

> The middle ground approach below is what came up in my discussion with
> Arnd yesterday.
> This would require more manual verification.
> Apart from that, the below method works fine.
>
> I will pick a fs and convert it both ways and post series 2a and 2b
> (middle ground approach) so that everyone can take a pick.
> I will also post statistics on how many such individual fs patches will be
> required.
>
>> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
>> index b97f1df910ab..54fc3c41047a 100644
>> --- a/fs/xfs/xfs_trans_inode.c
>> +++ b/fs/xfs/xfs_trans_inode.c
>> @@ -68,7 +68,7 @@ xfs_trans_ichgtime(
>>         int                     flags)
>>  {
>>         struct inode            *inode = VFS_I(ip);
>> -       struct timespec         tv;
>> +       struct vfs_time         tv;
>>
>>         ASSERT(tp);
>>         ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>> @@ -76,13 +76,13 @@ xfs_trans_ichgtime(
>>         tv = current_fs_time(inode->i_sb);
>>
>>         if ((flags & XFS_ICHGTIME_MOD) &&
>> -           !timespec_equal(&inode->i_mtime, &tv)) {
>> +           !vfs_time_equal(&inode->i_mtime, &tv)) {
>>                 inode->i_mtime = tv;
>>                 ip->i_d.di_mtime.t_sec = tv.tv_sec;
>>                 ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>>         }
>>         if ((flags & XFS_ICHGTIME_CHG) &&
>> -           !timespec_equal(&inode->i_ctime, &tv)) {
>> +           !vfs_time_equal(&inode->i_ctime, &tv)) {
>>                 inode->i_ctime = tv;
>>                 ip->i_d.di_ctime.t_sec = tv.tv_sec;
>>                 ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
>>
>>
>> which I would much prefer here.
>>
>>         Arnd
>
> -Deepa