Discussion:
[PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps
(too old to reply)
Steven Whitehouse
2016-06-09 10:28:36 UTC
Permalink
Raw Message
Hi,

GFS2 bits:
Acked-by: Steven Whitehouse <***@redhat.com>

Steve.
Ryusuke Konishi
2016-06-09 13:05:47 UTC
Permalink
Raw Message
for nilfs2 bits:

Acked-by: Ryusuke Konishi <***@lab.ntt.co.jp>

Thanks,
Ryusuke Konishi
Felipe Balbi
2016-06-09 07:51:31 UTC
Permalink
Raw Message
Hi,
drivers/usb/gadget/function/f_fs.c | 2 +-
drivers/usb/gadget/legacy/inode.c | 2 +-
for drivers/usb/gadget:

Acked-by: Felipe Balbi <***@kernel.org>
--
balbi
David Sterba
2016-06-09 07:55:39 UTC
Permalink
Raw Message
CURRENT_TIME macro is not appropriate for filesystems as it
doesn't use the right granularity for filesystem timestamps.
Use current_fs_time() instead.
CURRENT_TIME is also not y2038 safe.
This is also in preparation for the patch that transitions
vfs timestamps to use 64 bit time and hence make them
y2038 safe. As part of the effort current_fs_time() will be
extended to do range checks. Hence, it is necessary for all
file system timestamps to use current_fs_time(). Also,
current_fs_time() will be transitioned along with vfs to be
y2038 safe.
for the btrfs bits

Reviewed-by: David Sterba <***@suse.com>
Linus Torvalds
2016-06-09 19:08:04 UTC
Permalink
Raw Message
CURRENT_TIME macro is not appropriate for filesystems as it
doesn't use the right granularity for filesystem timestamps.
Use current_fs_time() instead.
Again - using the inode instead fo the syuperblock in tghis patch
would have made the patch much more obvious (it could have been 99%
generated with the sed-script I sent out a week or two ago), and it
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index e9f5043..85c12f0 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2359,6 +2359,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
{
struct usb_dev_state *ps = file->private_data;
struct inode *inode = file_inode(file);
+ struct super_block *sb = inode->i_sb;
struct usb_device *dev = ps->dev;
int ret = -ENOTTY;
where we add a new variable just because the calling convention was wrong.

It's not even 100% obvious that a filesystem has to have one single
time representation, so making the time function about the entity
whose time is set is also conceptually a much better model, never mind
that it is just what every single user seems to want anyway.

So I'd *much* rather see

+ inode->i_atime = inode->i_mtime = inode->i_ctime =
current_fs_time(inode);

over seeing either of these two variants::

+ inode->i_atime = inode->i_mtime = inode->i_ctime =
current_fs_time(inode->i_sb);
+ ret->i_atime = ret->i_mtime = ret->i_ctime = current_fs_time(sb);

because the first of those variants (grep for current_fs_time() in the
current git tree, and notice that it's the common one) we have the
pointless "let's chase a pointer in every caller"

And while it's true that the second variant is natural for *some*
situations, I've yet to find one where it wasn't equally sane to just
pass in the inode instead.

Linus
Deepa Dinamani
2016-06-09 20:38:21 UTC
Permalink
Raw Message
On Thu, Jun 9, 2016 at 12:08 PM, Linus Torvalds
Post by Linus Torvalds
CURRENT_TIME macro is not appropriate for filesystems as it
doesn't use the right granularity for filesystem timestamps.
Use current_fs_time() instead.
Again - using the inode instead fo the syuperblock in tghis patch
would have made the patch much more obvious (it could have been 99%
generated with the sed-script I sent out a week or two ago), and it
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index e9f5043..85c12f0 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2359,6 +2359,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
{
struct usb_dev_state *ps = file->private_data;
struct inode *inode = file_inode(file);
+ struct super_block *sb = inode->i_sb;
struct usb_device *dev = ps->dev;
int ret = -ENOTTY;
where we add a new variable just because the calling convention was wrong.
It's not even 100% obvious that a filesystem has to have one single
time representation, so making the time function about the entity
whose time is set is also conceptually a much better model, never mind
that it is just what every single user seems to want anyway.
So I'd *much* rather see
+ inode->i_atime = inode->i_mtime = inode->i_ctime =
current_fs_time(inode);
+ inode->i_atime = inode->i_mtime = inode->i_ctime =
current_fs_time(inode->i_sb);
+ ret->i_atime = ret->i_mtime = ret->i_ctime = current_fs_time(sb);
because the first of those variants (grep for current_fs_time() in the
current git tree, and notice that it's the common one) we have the
pointless "let's chase a pointer in every caller"
And while it's true that the second variant is natural for *some*
situations, I've yet to find one where it wasn't equally sane to just
pass in the inode instead.
I did try changing the patches to pass inode.
But, there are a few instances that made me think that keeping
super_block was beneficial.

1. There are a few link, rename functions which assign times like this:

- inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
+ inode->i_ctime = dir->i_ctime = dir->i_mtime =
current_fs_time(dir->i_sb);

Now, if we pass in inode, we end up making 2 calls to current_fs_time().
We could actually just use 1 call because for all parameters the
function uses, they are identical.
But, it seems odd to assume that the function wouldn't use the inode,
even though it is getting passed in to the function.

2. Also, this means that we will make it an absolute policy that any filesystem
timestamp that is not directly connected to an inode would have to use
ktime_get_* apis.
Some timestamps use the same on disk format and might be useful to
have same api to be reused.
Eg: [patch 6/21] of the current series

3. Even if the filesystem inode has extra timestamps and these are not
part of vfs inode, we still use
vfs inode to get the timestamps from current_fs_time(): Eg: ext4 create time

4. And, filesystem attributes must be assigned only after the inode is
created or use ktime apis.
And, only when these get assigned to inode, they will call timespec_trunc().

5. 2 and 3 might lead to more code rearrangement for few filesystems.
These will lead to more patches probably and they will not be mechanical.

If these are not a problem, I can update the series that accepts inode as an
argument instead of super_block.

-Deepa
Linus Torvalds
2016-06-09 21:02:03 UTC
Permalink
Raw Message
Post by Deepa Dinamani
- inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
+ inode->i_ctime = dir->i_ctime = dir->i_mtime =
current_fs_time(dir->i_sb);
So I think you should just pass one any of the two inodes and just add
a comment.

Then, if we hit a filesystem that actually wants to have different
granularity for different inodes, we'll split it up, but even then
we'd be better off than with the superblock, since then we *could*
easily split this one case up into "get directory time" and "get inode
time".
Post by Deepa Dinamani
2. Also, this means that we will make it an absolute policy that any filesystem
timestamp that is not directly connected to an inode would have to use
ktime_get_* apis.
The thing is, those kinds of things are all going to be inside the
filesystem itself.

At that point, the *filesystem* already knows what the timekeeping
rules for that filesystem is.

I think we should strive to design the "current_fs_time()" not for
internal filesystem use, but for actual generic use where we *don't*
know a priori what the rules are, and we have to go to this helper
function to figure it out.

Inside a filesystem, why *shouldn't* the low-level filesystem already
use the normal "get time" functions?

See what I'm saying? The primary value-add to "current_fs_time()" is
for layers like the VFS and security layer that don't know what the
filesystem itself does.

At the low-level filesystem layer, you may just know that "ok, I only
have 32-bit timestamps anyway, so I should just use a 32-bit time
function".
Post by Deepa Dinamani
3. Even if the filesystem inode has extra timestamps and these are not
part of vfs inode, we still use
vfs inode to get the timestamps from current_fs_time(): Eg: ext4 create time
But those already have an inode.

In fact, ext4 is a particularly bad example, since it uses the
ext4_current_time() function to get the time. And that one gets an
inode pointer.

So at least one filesystem that already does this, already uses a
inode-based model.

Everything I see just says "times are about inodes". Anything else
almost has to be filesystem-internal anyway, since the only thing that
is ever visible outside the filesystem (time-wise) is the inode.

And as mentioned, once it's internal to the low-level filesystem, it's
not obvious at all that you'd have to use "currenf_fs_time()" anyway.
The internal filesystem code might very well decide to use other
timekeeping functions.

Linus
Arnd Bergmann
2016-06-10 22:23:39 UTC
Permalink
Raw Message
...
Hi Deepa,


Just FYI, the vger.kernel.org list server and some others
intentionally reject mails with more than 1024 characters in the
Cc header, to stop people from cross-posting to too many folks.

I realize that you merged the patch after Linus' comment about
doing things in fewer steps for the simple conversion, which is
fine, but then the patch should be obvious enough that you
don't need to Cc every single maintainer and mailing list.

I've had some cases like this, and I usually remove the people
that are less likely to reply, leaving one per subsystem.
Leaving out the cleartext names is another trick you can use
if you think that you really need to Cc more people than
allowed ;-)

Arnd

Loading...