| Message ID | 14988333.dbUrT9DQnz@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 19, 2016 at 2:25 PM, Arnd Bergmann <arnd@arndb.de> 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@fromorbit.com> 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 19, 2016 at 9:12 PM, Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > On Tue, Jan 19, 2016 at 2:25 PM, Arnd Bergmann <arnd@arndb.de> 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@fromorbit.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 19, 2016 at 11:25:02PM +0100, Arnd Bergmann wrote: > On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote: > > 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.... > > 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; > } WTF? That's insane and completely unnecessary. It's even worse than the FAT example I've already pointed out was just fucking wrong. The only change this function requires is: > The way that Deepa suggests I think would turn out as: > > 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; + struct timespec64 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)) { + !timespec64_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)) { + !timespec64_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. IOWs, you're now finally suggesting doing the *simple conversion* I've been suggesting needs to be made *since the start* of this long, frustrating thread, except you *still want to abstract the timestamp unnecessarily*. For the last time: use timespec64 directly, do not abstract it in any way. -Dave.
On Thursday 21 January 2016 10:06:32 Dave Chinner wrote: > > IOWs, you're now finally suggesting doing the *simple conversion* > I've been suggesting needs to be made *since the start* of this > long, frustrating thread, except you *still want to abstract the > timestamp unnecessarily*. > > For the last time: use timespec64 directly, do not abstract it > in any way. No, in an earlier mail you said "Nobody is suggesting one huge patch here. This can all be done with small steps." Changing 45 files in 20 file systems along with the VFS core in one patch is what I consider a huge patch, it's the opposite of small steps. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd and I had a discussion about how to proceed here. We don't think anybody really is wanting a big patch touching 50 fs here. We are evaluating a couple of approaches that don't do this. I will post an update next week On Wed, Jan 20, 2016 at 3:17 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 21 January 2016 10:06:32 Dave Chinner wrote: >> >> IOWs, you're now finally suggesting doing the *simple conversion* >> I've been suggesting needs to be made *since the start* of this >> long, frustrating thread, except you *still want to abstract the >> timestamp unnecessarily*. >> >> For the last time: use timespec64 directly, do not abstract it >> in any way. > > No, in an earlier mail you said > > "Nobody is suggesting one huge patch here. This can all be done with > small steps." > > Changing 45 files in 20 file systems along with the VFS core in > one patch is what I consider a huge patch, it's the opposite > of small steps. > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: 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;