linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
To: Frank Sorenson <sorenson@redhat.com>
Cc: linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH V3 0/4] fat: timestamp updates
Date: Sun, 23 Sep 2018 19:31:39 +0900	[thread overview]
Message-ID: <877ejcijxw.fsf@mail.parknet.co.jp> (raw)
In-Reply-To: <20180922201959.10477-1-sorenson@redhat.com> (Frank Sorenson's message of "Sat, 22 Sep 2018 15:19:55 -0500")

Frank Sorenson <sorenson@redhat.com> writes:

> vfat/msdos timestamps are stored on-disk with several different
> granularities, some of them lower resolution than timespec_trunc()
> can provide.  In addition, they are only truncated as they are
> written to disk, so the timestamps in-memory for new or modified
> files/directories may be different from the same timestamps after
> a remount, as the now-truncated times are re-read from the on-disk
> format.
>
> These patches allow finer granularity for the timestamps where
> possible and add fat-specific ->update_time inode operation and
> fat_truncate_time functions to truncate each timestamp correctly,
> giving consistent times across remounts.

Looks like basically very good, however there are some issues.  Those
small stuff would be easier to tell as patch, instead of commenting
inline.

Please check the following patch and comment of it. Only noticeable one
is atime bug (6 in comment), looks like it is only missed to test well,
and need to re-test (in my quick test, looks works with this patch).

Other stuff are tweaks and optimizations basically.

Thanks.


[PATCH] fat: Fix/cleanup FAT timestamp works


1) Move SECS_PER_* family to misc.c, only used in misc.c

2) Don't use fat_update_time() to replace mark_inode_dirty(),
mark_inode_dirty_sync() and mark_inode_dirty() are different.

3) Set sb->s_time_gran, we use this only for ctime in vfat mode. So
nothing benefit to add additional overhead to get current time.

Instead, just truncate ctime manually.

4) Add i_version support to update_time(). FAT doesn't support
i_version, but user still can set it.

5) Make sure to use 64bits wide to mask 2secs (i.e. ~1ULL, instead ~1)

6) Fix atime bug

    - on 32bit arch, have to use div64 stuff
    - missing to back to unix time after converting localtime

7) Remove needless noinline. gcc should be able to choice to inline it
or not, without any issue.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/fat.h         |   12 ------------
 fs/fat/file.c        |    3 ++-
 fs/fat/inode.c       |   11 ++++++++---
 fs/fat/misc.c        |   45 ++++++++++++++++++++++++++++++++++-----------
 fs/fat/namei_msdos.c |    4 ++--
 fs/fat/namei_vfat.c  |    4 ++--
 6 files changed, 48 insertions(+), 31 deletions(-)

diff -puN fs/fat/fat.h~fat-timestamp-fix-tweaks fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-timestamp-fix-tweaks	2018-09-23 19:00:41.573501694 +0900
+++ linux-hirofumi/fs/fat/fat.h	2018-09-23 19:16:10.576363930 +0900
@@ -412,18 +412,6 @@ void fat_msg(struct super_block *sb, con
 	 } while (0)
 extern int fat_clusters_flush(struct super_block *sb);
 extern int fat_chain_add(struct inode *inode, int new_dclus, int nr_cluster);
-
-#define SECS_PER_MIN    60
-#define SECS_PER_HOUR   (60 * 60)
-#define SECS_PER_DAY    (SECS_PER_HOUR * 24)
-
-static inline int fat_tz_offset(struct msdos_sb_info *sbi)
-{
-	return  (sbi->options.tz_set ?
-		- sbi->options.time_offset :
-		sys_tz.tz_minuteswest) * SECS_PER_MIN;
-}
-
 extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
 			      __le16 __time, __le16 __date, u8 time_cs);
 extern void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec64 *ts,
diff -puN fs/fat/file.c~fat-timestamp-fix-tweaks fs/fat/file.c
--- linux/fs/fat/file.c~fat-timestamp-fix-tweaks	2018-09-23 19:00:41.574501693 +0900
+++ linux-hirofumi/fs/fat/file.c	2018-09-23 19:16:10.576363930 +0900
@@ -227,7 +227,8 @@ static int fat_cont_expand(struct inode
 	if (err)
 		goto out;
 
-	fat_update_time(inode, NULL, S_CTIME|S_MTIME);
+	fat_truncate_time(inode, NULL, S_CTIME|S_MTIME);
+	mark_inode_dirty(inode);
 	if (IS_SYNC(inode)) {
 		int err2;
 
diff -puN fs/fat/inode.c~fat-timestamp-fix-tweaks fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-timestamp-fix-tweaks	2018-09-23 19:00:41.575501692 +0900
+++ linux-hirofumi/fs/fat/inode.c	2018-09-23 19:16:10.576363930 +0900
@@ -244,8 +244,9 @@ static int fat_write_end(struct file *fi
 	if (err < len)
 		fat_write_failed(mapping, pos + len);
 	if (!(err < 0) && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
+		fat_truncate_time(inode, NULL, S_CTIME|S_MTIME);
 		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
-		fat_update_time(inode, NULL, S_CTIME|S_MTIME);
+		mark_inode_dirty(inode);
 	}
 	return err;
 }
@@ -563,7 +564,7 @@ int fat_fill_inode(struct inode *inode,
 				  de->cdate, de->ctime_cs);
 		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
 	} else
-		fat_update_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
+		fat_truncate_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
 
 	return 0;
 }
@@ -1625,7 +1626,11 @@ int fat_fill_super(struct super_block *s
 	sb->s_magic = MSDOS_SUPER_MAGIC;
 	sb->s_op = &fat_sops;
 	sb->s_export_op = &fat_export_ops;
-	sb->s_time_gran = isvfat ? 10000000 : 1000000000;
+	/*
+	 * timestamp is complex and truncated by fat itself, so we set
+	 * 1 here to be fast.
+	 */
+	sb->s_time_gran = 1;
 	mutex_init(&sbi->nfs_build_inode_lock);
 	ratelimit_state_init(&sbi->ratelimit, DEFAULT_RATELIMIT_INTERVAL,
 			     DEFAULT_RATELIMIT_BURST);
diff -puN fs/fat/misc.c~fat-timestamp-fix-tweaks fs/fat/misc.c
--- linux/fs/fat/misc.c~fat-timestamp-fix-tweaks	2018-09-23 19:00:41.576501690 +0900
+++ linux-hirofumi/fs/fat/misc.c	2018-09-23 19:16:10.542363973 +0900
@@ -7,6 +7,7 @@
  */
 
 #include "fat.h"
+#include <linux/iversion.h>
 
 /*
  * fat_fs_error reports a file system problem that might indicate fa data
@@ -170,6 +171,9 @@ int fat_chain_add(struct inode *inode, i
  * time:  5 - 10: min	(0 -  59)
  * time: 11 - 15: hour	(0 -  23)
  */
+#define SECS_PER_MIN	60
+#define SECS_PER_HOUR	(60 * 60)
+#define SECS_PER_DAY	(SECS_PER_HOUR * 24)
 /* days between 1.1.70 and 1.1.80 (2 leap days) */
 #define DAYS_DELTA	(365 * 10 + 2)
 /* 120 (2100 - 1980) isn't leap year */
@@ -182,6 +186,13 @@ static long days_in_year[] = {
 	0,   0,  31,  59,  90, 120, 151, 181, 212, 243, 273, 304, 334, 0, 0, 0,
 };
 
+static inline int fat_tz_offset(struct msdos_sb_info *sbi)
+{
+	return (sbi->options.tz_set ?
+		-sbi->options.time_offset :
+		sys_tz.tz_minuteswest) * SECS_PER_MIN;
+}
+
 /* Convert a FAT time/date pair to a UNIX date (seconds since 1 1 70). */
 void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
 		       __le16 __time, __le16 __date, u8 time_cs)
@@ -255,6 +266,11 @@ void fat_time_unix2fat(struct msdos_sb_i
 }
 EXPORT_SYMBOL_GPL(fat_time_unix2fat);
 
+static inline struct timespec64 fat_timespec64_trunc_2secs(struct timespec64 ts)
+{
+	return (struct timespec64){ ts.tv_sec & ~1ULL, 0 };
+}
+
 /*
  * truncate the various times with appropriate granularity:
  *   root inode:
@@ -266,7 +282,7 @@ EXPORT_SYMBOL_GPL(fat_time_unix2fat);
  *       vfat  - 10 milliseconds
  *     atime - 24 hours (00:00:00 in local timezone)
  */
-noinline int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
+int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	struct timespec64 ts;
@@ -280,22 +296,24 @@ noinline int fat_truncate_time(struct in
 	}
 
 	if (flags & S_ATIME) {
-		int offset = fat_tz_offset(sbi);
-		long seconds;
-
-		seconds = now->tv_sec;
-		seconds -= (now->tv_sec - offset) % 86400;
+		/* To localtime */
+		time64_t seconds = now->tv_sec - fat_tz_offset(sbi);
+		s32 remainder;
+		div_s64_rem(seconds, SECS_PER_DAY, &remainder);
+		/* To day boundary and back to unix time */
+		seconds = seconds + fat_tz_offset(sbi) - remainder;
 
 		inode->i_atime = (struct timespec64){ seconds, 0 };
 	}
 	if (flags & S_CTIME) {
 		if (sbi->options.isvfat)
-			inode->i_ctime = *now;
+			inode->i_ctime = timespec64_trunc(*now, 10000000);
 		else
-			inode->i_ctime = (struct timespec64){ now->tv_sec & ~1, 0 };
+			inode->i_ctime = fat_timespec64_trunc_2secs(*now);
 	}
 	if (flags & S_MTIME)
-		inode->i_mtime = (struct timespec64){ now->tv_sec & ~1, 0 };
+		inode->i_mtime = fat_timespec64_trunc_2secs(*now);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fat_truncate_time);
@@ -303,15 +321,20 @@ EXPORT_SYMBOL_GPL(fat_truncate_time);
 int fat_update_time(struct inode *inode, struct timespec64 *now, int flags)
 {
 	int iflags = I_DIRTY_TIME;
+	bool dirty = false;
 
 	if (inode->i_ino == MSDOS_ROOT_INO)
 		return 0;
-	fat_truncate_time(inode, now, flags);
 
+	fat_truncate_time(inode, now, flags);
+	if (flags & S_VERSION)
+		dirty = inode_maybe_inc_iversion(inode, false);
 	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
 	    !(inode->i_sb->s_flags & SB_LAZYTIME))
-		iflags |= I_DIRTY_SYNC;
+		dirty = true;
 
+	if (dirty)
+		iflags |= I_DIRTY_SYNC;
 	__mark_inode_dirty(inode, iflags);
 	return 0;
 }
diff -puN fs/fat/namei_msdos.c~fat-timestamp-fix-tweaks fs/fat/namei_msdos.c
--- linux/fs/fat/namei_msdos.c~fat-timestamp-fix-tweaks	2018-09-23 19:00:41.576501690 +0900
+++ linux-hirofumi/fs/fat/namei_msdos.c	2018-09-23 19:16:10.492364035 +0900
@@ -327,7 +327,7 @@ static int msdos_rmdir(struct inode *dir
 	drop_nlink(dir);
 
 	clear_nlink(inode);
-	fat_update_time(inode, NULL, S_CTIME);
+	fat_truncate_time(inode, NULL, S_CTIME);
 	fat_detach(inode);
 out:
 	mutex_unlock(&MSDOS_SB(sb)->s_lock);
@@ -413,7 +413,7 @@ static int msdos_unlink(struct inode *di
 	if (err)
 		goto out;
 	clear_nlink(inode);
-	fat_update_time(inode, NULL, S_CTIME);
+	fat_truncate_time(inode, NULL, S_CTIME);
 	fat_detach(inode);
 out:
 	mutex_unlock(&MSDOS_SB(sb)->s_lock);
diff -puN fs/fat/namei_vfat.c~fat-timestamp-fix-tweaks fs/fat/namei_vfat.c
--- linux/fs/fat/namei_vfat.c~fat-timestamp-fix-tweaks	2018-09-23 19:00:41.577501689 +0900
+++ linux-hirofumi/fs/fat/namei_vfat.c	2018-09-23 19:16:10.492364035 +0900
@@ -810,7 +810,7 @@ static int vfat_rmdir(struct inode *dir,
 	drop_nlink(dir);
 
 	clear_nlink(inode);
-	fat_update_time(inode, NULL, S_ATIME|S_MTIME);
+	fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
 	fat_detach(inode);
 	vfat_d_version_set(dentry, inode_query_iversion(dir));
 out:
@@ -836,7 +836,7 @@ static int vfat_unlink(struct inode *dir
 	if (err)
 		goto out;
 	clear_nlink(inode);
-	fat_update_time(inode, NULL, S_ATIME|S_MTIME);
+	fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
 	fat_detach(inode);
 	vfat_d_version_set(dentry, inode_query_iversion(dir));
 out:
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

      parent reply	other threads:[~2018-09-23 16:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-22 20:19 [PATCH V3 0/4] fat: timestamp updates Frank Sorenson
2018-09-22 20:19 ` [PATCH 1/4] fat: set the s_time_gran for msdos or vfat mounts Frank Sorenson
2018-09-22 20:19 ` [PATCH 2/4] fat: create function to calculate timezone offset Frank Sorenson
2018-09-22 20:19 ` [PATCH 3/4] fat: add functions to update and truncate the timestamps appropriately Frank Sorenson
2018-09-22 20:19 ` [PATCH 4/4] fat: change timestamp updates to fat_update_time or fat_truncate_time Frank Sorenson
2018-09-23 10:31 ` OGAWA Hirofumi [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877ejcijxw.fsf@mail.parknet.co.jp \
    --to=hirofumi@mail.parknet.co.jp \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=sorenson@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).