On Thu, Jun 9, 2016 at 12:08 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani <deepa.ker...@gmail.com> > 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.
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 ------------------------------------------------------------------------------ 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 _______________________________________________ Jfs-discussion mailing list Jfs-discussion@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/jfs-discussion