Re: [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps

David Sterba
On Wed, Jun 08, 2016 at 10:04:48PM -0700, Deepa Dinamani wrote:

> 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.
>
> Signed-off-by: Deepa Dinamani <[hidden email]>
> Cc: David Sterba <[hidden email]>

for the btrfs bits

Reviewed-by: David Sterba <[hidden email]>

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps

Steven Whitehouse
Hi,

GFS2 bits:
Acked-by: Steven Whitehouse <[hidden email]>

Steve.


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps

Linus Torvalds-3
In reply to this post by David Sterba
On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani <[hidden email]> wrote:
> 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
would have made it unnecessary to add these kinds of things:

> 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

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps

Linus Torvalds-3
On Thu, Jun 9, 2016 at 1:38 PM, Deepa Dinamani <[hidden email]> wrote:
>
> 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);

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".


> 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".

> 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

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps

Arnd Bergmann
In reply to this post by David Sterba
On Wednesday, June 8, 2016 10:04:48 PM CEST Deepa Dinamani wrote:

>
> Signed-off-by: Deepa Dinamani <[hidden email]>
> Cc: Steve French <[hidden email]>
> Cc: [hidden email]
> Cc: [hidden email]
> Cc: Joern Engel <[hidden email]>
> Cc: Prasad Joshi <[hidden email]>
> Cc: [hidden email]
> Cc: Andrew Morton <[hidden email]>
> Cc: Julia Lawall <[hidden email]>
> Cc: David Howells <[hidden email]>
> Cc: Firo Yang <[hidden email]>
> Cc: Jaegeuk Kim <[hidden email]>
> Cc: Changman Lee <[hidden email]>
> ...


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

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel