Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422151 > unrolled thread

Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps Deepa Dinamani <deepa.kernel@gmail.com> - 2016-06-14 20:00 +0200
    Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps Arnd Bergmann <arnd@arndb.de> - 2016-06-14 23:00 +0200

#1422151 — Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps

On Fri, Jun 10, 2016 at 3:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday, June 9, 2016 11:45:01 AM CEST Linus Torvalds wrote:
>> On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>> > CURRENT_TIME_SEC and CURRENT_TIME are not y2038 safe.
>> > current_fs_time() will be transitioned to be y2038 safe
>> > along with vfs.
>> >
>> > current_fs_time() returns timestamps according to the
>> > granularities set in the super_block.
>>
>> All existing users and all the ones in this patch (and the others too,
>> although I didn't go through them very carefully) really would prefer
>> just passing in the inode directly, rather than the superblock.
>>
>> So I don't want to add more users of this broken interface.  It was a
>> mistake to use the superblock. The fact that the time granularity
>> exists there is pretty much irrelevant. If every single user wants to
>> use an inode pointer, then that is what the function should get.
>
> I guess it would help to give the function a new name in the process,
> if only to avoid possible conflicts. That new name of course needs to
> be at least as intuitive as the old one. How about
>
> struct timespec fs_timestamp(struct inode *);

Would moving the function to fs/ directory (filesystems.c/ super.c /
inode.c) and calling it current_time() or fs_current_time() make
sense?
The declaration is already part of fs.h.

This is actually a vfs function.
And, the time functions it uses are already exported.
Leaving it in the time.c by renaming to current_time() would be
confusing in spite of
the struct inode* argument.

-Deepa

[toc] | [next] | [standalone]

#1422330

On Tuesday, June 14, 2016 10:55:39 AM CEST Deepa Dinamani wrote:
> On Fri, Jun 10, 2016 at 3:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday, June 9, 2016 11:45:01 AM CEST Linus Torvalds wrote:
> >> On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> >> > CURRENT_TIME_SEC and CURRENT_TIME are not y2038 safe.
> >> > current_fs_time() will be transitioned to be y2038 safe
> >> > along with vfs.
> >> >
> >> > current_fs_time() returns timestamps according to the
> >> > granularities set in the super_block.
> >>
> >> All existing users and all the ones in this patch (and the others too,
> >> although I didn't go through them very carefully) really would prefer
> >> just passing in the inode directly, rather than the superblock.
> >>
> >> So I don't want to add more users of this broken interface.  It was a
> >> mistake to use the superblock. The fact that the time granularity
> >> exists there is pretty much irrelevant. If every single user wants to
> >> use an inode pointer, then that is what the function should get.
> >
> > I guess it would help to give the function a new name in the process,
> > if only to avoid possible conflicts. That new name of course needs to
> > be at least as intuitive as the old one. How about
> >
> > struct timespec fs_timestamp(struct inode *);
> 
> Would moving the function to fs/ directory (filesystems.c/ super.c /
> inode.c) and calling it current_time() or fs_current_time() make
> sense?
> The declaration is already part of fs.h.
>
> This is actually a vfs function.
> And, the time functions it uses are already exported.
> Leaving it in the time.c by renaming to current_time() would be
> confusing in spite of
> the struct inode* argument.

I've looked up the original patch that introduced current_fs_time
at http://marc.info/?l=linux-kernel&m=110134111125012&w=3

From the patch, it's clear that current_fs_time was intentionally
added to the same file as current_kernel_time() so it could be
inlined there, but both functions have since been moved to different
files.

I agree moving both timespec_trunc and current_fs_time into
fs/inode.c or fs/attr.c seems appropriate then, or we could move
current_fs_time() into kernel/time/timekeeping.c and mark
current_kernel_time64() inline again.

When John Stultz moved this function in 2c6b47de17c7 ("Cleanup
non-arch xtime uses, use get_seconds() or current_kernel_time()."),
he evidently did not consider the "inline" behavior important
there, no idea if this is even measurable.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web