[v5,1/5] vfs: Add current_time() api

Commit Message

Deepa Dinamani Sept. 14, 2016, 2:48 p.m. UTC

current_fs_time() is used for inode timestamps.

Change the signature of the function to take inode pointer
instead of superblock as per Linus's suggestion.

Also, move the api under vfs as per the discussion on the
thread: https://lkml.org/lkml/2016/6/9/36 . As per Arnd's
suggestion on the thread, changing the function name.

current_fs_time() will be deleted after all the references
to it are replaced by current_time().

There was a bug reported by kbuild test bot with the change
as some of the calls to current_time() were made before the
super_block was initialized. Catch these accidental assignments
as timespec_trunc() does for wrong granularities. This allows
for the function to work right even in these circumstances.
But, adds a warning to make the user aware of the bug.

A coccinelle script was used to identify all the current
.alloc_inode super_block callbacks that updated inode timestamps.
proc filesystem was the only one that was modifying inode times
as part of this callback. The series includes a patch to fix that.

Note that timespec_trunc() will also be moved to fs/inode.c
in a separate patch when this will need to be revamped for
bounds checking purposes.

Signed-off-by: Deepa Dinamani <[e-mail]>
Reviewed-by: Arnd Bergmann <[e-mail]>
---
 fs/inode.c         | 23 +++++++++++++++++++++++
 include/linux/fs.h |  1 +
 2 files changed, 24 insertions(+)

Comments

Thomas Gleixner Sept. 14, 2016, 6:52 p.m. UTC | #1

On Wed, 14 Sep 2016, Deepa Dinamani wrote:
> +/**
> + * current_time - Return FS time
> + * @inode: inode.
> + *
> + * Return the current time truncated to the time granularity supported by
> + * the fs.
> + *
> + * Note that inode and inode->sb cannot be NULL.
> + * Otherwise, the function warns and returns time without truncation.
> + */
> +struct timespec current_time(struct inode *inode)

Just a nitpick. Can we please rename that to fs_current_time() or something
similar which tells right away that this is a FS specific
function. current_time() is pretty generic and used in many places as
variable/argument name.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deepa Dinamani Sept. 14, 2016, 8:29 p.m. UTC | #2

>> + * current_time - Return FS time
>> + * @inode: inode.
>> + *
>> + * Return the current time truncated to the time granularity supported by
>> + * the fs.
>> + *
>> + * Note that inode and inode->sb cannot be NULL.
>> + * Otherwise, the function warns and returns time without truncation.
>> + */
>> +struct timespec current_time(struct inode *inode)
>
> Just a nitpick. Can we please rename that to fs_current_time() or something
> similar which tells right away that this is a FS specific
> function. current_time() is pretty generic and used in many places as
> variable/argument name.

I proposed this as one of the names before:
https://lkml.org/lkml/2016/6/14/613 .
But, decided against it because of the inode argument.
If this is preferred, I will change the patches.

There are only about 30 (20, outside of staging) current_time
references outside of Documentation/

-Deepa
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thomas Gleixner Sept. 14, 2016, 8:33 p.m. UTC | #3

On Wed, 14 Sep 2016, Deepa Dinamani wrote:

> >> + * current_time - Return FS time
> >> + * @inode: inode.
> >> + *
> >> + * Return the current time truncated to the time granularity supported by
> >> + * the fs.
> >> + *
> >> + * Note that inode and inode->sb cannot be NULL.
> >> + * Otherwise, the function warns and returns time without truncation.
> >> + */
> >> +struct timespec current_time(struct inode *inode)
> >
> > Just a nitpick. Can we please rename that to fs_current_time() or something
> > similar which tells right away that this is a FS specific
> > function. current_time() is pretty generic and used in many places as
> > variable/argument name.
> 
> I proposed this as one of the names before:
> https://lkml.org/lkml/2016/6/14/613 .
> But, decided against it because of the inode argument.
> If this is preferred, I will change the patches.
> 
> There are only about 30 (20, outside of staging) current_time
> references outside of Documentation/

It just occured to me and I can live with current_time() as well. Up to Al.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/inode.c b/fs/inode.c
index fd604c0..a95e04e 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2075,3 +2075,26 @@  void inode_nohighmem(struct inode *inode)
 	mapping_set_gfp_mask(inode->i_mapping, GFP_USER);
 }
 EXPORT_SYMBOL(inode_nohighmem);
+
+/**
+ * current_time - Return FS time
+ * @inode: inode.
+ *
+ * Return the current time truncated to the time granularity supported by
+ * the fs.
+ *
+ * Note that inode and inode->sb cannot be NULL.
+ * Otherwise, the function warns and returns time without truncation.
+ */
+struct timespec current_time(struct inode *inode)
+{
+	struct timespec now = current_kernel_time();
+
+	if (unlikely(!inode->i_sb)) {
+		WARN(1, "current_time() called with uninitialized super_block in the inode");
+		return now;
+	}
+
+	return timespec_trunc(now, inode->i_sb->s_time_gran);
+}
+EXPORT_SYMBOL(current_time);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fde36d2..ab249e1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1471,6 +1471,7 @@  static inline void i_gid_write(struct inode *inode, gid_t gid)
 }
 
 extern struct timespec current_fs_time(struct super_block *sb);
+extern struct timespec current_time(struct inode *inode);
 
 /*
  * Snapshotting support.