[PATCH 0/2] exfat: timestamp fixes

[PATCH 0/2] exfat: timestamp fixes

[Eric Sandeen]

I've seen 2 issues w/ exfat timestamps - discovered by xfstests generic/003

1) time granularity is set wrong, so subsecond timestamps change when
   an inode cycles out of memory and is reread from disk

2) the disk format doesn't seem to support subsecond atime, so a similar
   problem exists there.e

The last xfstests generic/003 issue is re: change time but exfat doesn't
really support change times (?) so that'll be a test workaround.

[PATCH 1/2] exfat: properly set s_time_gran

[Eric Sandeen]

The s_time_gran superblock field indicates the on-disk nanosecond
granularity of timestamps, and for exfat that seems to be 10ms, so
set s_time_gran to 10000000ns. Without this, in-memory timestamps
change when they get re-read from disk.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 16ed202ef527..4f21bf8b550d 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -541,7 +542,7 @@ static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)
 	sb->s_magic = EXFAT_SUPER_MAGIC;
 	sb->s_op = &exfat_sops;
 
-	sb->s_time_gran = 1;
+	sb->s_time_gran = 10000000;
 	sb->s_time_min = EXFAT_MIN_TIMESTAMP_SECS;
 	sb->s_time_max = EXFAT_MAX_TIMESTAMP_SECS;
 

Re: [PATCH 1/2] exfat: properly set s_time_gran

[Matthew Wilcox]

On Wed, Apr 15, 2020 at 08:09:11PM -0500, Eric Sandeen wrote:
> -	sb->s_time_gran = 1;
> +	sb->s_time_gran = 10000000;

10 * NSEC_PER_MSEC?

RE: [PATCH 1/2] exfat: properly set s_time_gran

[Namjae Jeon]

> 
> On Wed, Apr 15, 2020 at 08:09:11PM -0500, Eric Sandeen wrote:
> > -	sb->s_time_gran = 1;
> > +	sb->s_time_gran = 10000000;
> 
> 10 * NSEC_PER_MSEC?
Looks cleaner.
Thanks!

[PATCH 2/2] exfat: zero out atime subsecond timestamp

[Eric Sandeen]

There doesn't seem to be a sub-second granularity for exfat atime,
so tv_nsec needs to be zeroed out every time atime gets set.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

I know doing it brute-force this way is terrible, so this may not be the
right approach but it highlights the issue at least.

diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 483f683757aa..3527c69b7038 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -273,6 +273,7 @@ int exfat_getattr(const struct path *path, struct kstat *stat,
 	struct exfat_inode_info *ei = EXFAT_I(inode);
 
 	generic_fillattr(inode, stat);
+	stat->atime.tv_nsec = 0;
 	stat->result_mask |= STATX_BTIME;
 	stat->btime.tv_sec = ei->i_crtime.tv_sec;
 	stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
@@ -339,6 +340,7 @@ int exfat_setattr(struct dentry *dentry, struct iattr *attr)
 	}
 
 	setattr_copy(inode, attr);
+	inode->i_atime.tv_nsec = 0;
 	mark_inode_dirty(inode);
 
 out:
diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
index 14a3300848f6..101e169a9a4b 100644
--- a/fs/exfat/misc.c
+++ b/fs/exfat/misc.c
@@ -88,7 +88,8 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 	if (time_ms) {
 		ts->tv_sec += time_ms / 100;
 		ts->tv_nsec = (time_ms % 100) * 10 * NSEC_PER_MSEC;
-	}
+	} else
+		ts->tv_nsec = 0;
 
 	if (tz & EXFAT_TZ_VALID)
 		/* Adjust timezone to UTC0. */
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index a8681d91f569..64d1b8ea4dfd 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -595,6 +595,7 @@ static int exfat_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	inode_inc_iversion(inode);
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		EXFAT_I(inode)->i_crtime = current_time(inode);
+	inode->i_atime.tv_nsec = 0;
 	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
 
 	d_instantiate(dentry, inode);
@@ -854,6 +855,7 @@ static int exfat_unlink(struct inode *dir, struct dentry *dentry)
 
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_atime = current_time(dir);
+	inode->i_atime.tv_nsec = 0;
 	if (IS_DIRSYNC(dir))
 		exfat_sync_inode(dir);
 	else
@@ -861,6 +863,7 @@ static int exfat_unlink(struct inode *dir, struct dentry *dentry)
 
 	clear_nlink(inode);
 	inode->i_mtime = inode->i_atime = current_time(inode);
+	inode->i_atime.tv_nsec = 0;
 	exfat_unhash_inode(inode);
 	exfat_d_version_set(dentry, inode_query_iversion(dir));
 unlock:
@@ -903,6 +906,7 @@ static int exfat_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	inode_inc_iversion(inode);
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		EXFAT_I(inode)->i_crtime = current_time(inode);
+	inode->i_atime.tv_nsec = 0;
 	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
 
 	d_instantiate(dentry, inode);
@@ -1019,6 +1023,7 @@ static int exfat_rmdir(struct inode *dir, struct dentry *dentry)
 
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_atime = current_time(dir);
+	inode->i_atime.tv_nsec = 0;
 	if (IS_DIRSYNC(dir))
 		exfat_sync_inode(dir);
 	else
@@ -1027,6 +1032,7 @@ static int exfat_rmdir(struct inode *dir, struct dentry *dentry)
 
 	clear_nlink(inode);
 	inode->i_mtime = inode->i_atime = current_time(inode);
+	inode->i_atime.tv_nsec = 0;
 	exfat_unhash_inode(inode);
 	exfat_d_version_set(dentry, inode_query_iversion(dir));
 unlock:
@@ -1387,6 +1393,7 @@ static int exfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	inode_inc_iversion(new_dir);
 	new_dir->i_ctime = new_dir->i_mtime = new_dir->i_atime =
 		EXFAT_I(new_dir)->i_crtime = current_time(new_dir);
+	new_dir->i_atime.tv_nsec = 0;
 	if (IS_DIRSYNC(new_dir))
 		exfat_sync_inode(new_dir);
 	else
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 16ed202ef527..4f21bf8b550d 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -351,6 +351,7 @@ static int exfat_read_root(struct inode *inode)
 	exfat_save_attr(inode, ATTR_SUBDIR);
 	inode->i_mtime = inode->i_atime = inode->i_ctime = ei->i_crtime =
 		current_time(inode);
+	inode->i_atime.tv_nsec = 0;
 	exfat_cache_init_inode(inode);
 	return 0;
 }

[PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Eric Sandeen]

exfat atimes are restricted to only 2s granularity so after
we set an atime, round it down to the nearest 2s and set the
sub-second component of the timestamp to 0.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

V2: don't just zero tv_nsec, must also round down tv_sec I think.

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 67d4e46fb810..d67fb8a6f770 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -507,6 +507,7 @@ void exfat_msg(struct super_block *sb, const char *lv, const char *fmt, ...)
 		__printf(3, 4) __cold;
 void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 		u8 tz, __le16 time, __le16 date, u8 time_ms);
+void exfat_truncate_atime(struct timespec64 *ts);
 void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 		u8 *tz, __le16 *time, __le16 *date, u8 *time_ms);
 unsigned short exfat_calc_chksum_2byte(void *data, int len,
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 483f683757aa..4f76764165cf 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -273,6 +273,7 @@ int exfat_getattr(const struct path *path, struct kstat *stat,
 	struct exfat_inode_info *ei = EXFAT_I(inode);
 
 	generic_fillattr(inode, stat);
+	exfat_truncate_atime(&stat->atime);
 	stat->result_mask |= STATX_BTIME;
 	stat->btime.tv_sec = ei->i_crtime.tv_sec;
 	stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
@@ -339,6 +340,7 @@ int exfat_setattr(struct dentry *dentry, struct iattr *attr)
 	}
 
 	setattr_copy(inode, attr);
+	exfat_truncate_atime(&inode->i_atime);
 	mark_inode_dirty(inode);
 
 out:
diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
index 14a3300848f6..c8b33278d474 100644
--- a/fs/exfat/misc.c
+++ b/fs/exfat/misc.c
@@ -88,7 +88,8 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 	if (time_ms) {
 		ts->tv_sec += time_ms / 100;
 		ts->tv_nsec = (time_ms % 100) * 10 * NSEC_PER_MSEC;
-	}
+	} else
+		exfat_truncate_atime(ts);
 
 	if (tz & EXFAT_TZ_VALID)
 		/* Adjust timezone to UTC0. */
@@ -124,6 +125,13 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 	*tz = EXFAT_TZ_VALID;
 }
 
+/* atime has only a 2-second resolution */
+void exfat_truncate_atime(struct timespec64 *ts)
+{
+	ts->tv_sec = round_down(ts->tv_sec, 2); 
+	ts->tv_nsec = 0;
+}
+
 unsigned short exfat_calc_chksum_2byte(void *data, int len,
 		unsigned short chksum, int type)
 {
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index a8681d91f569..b72d782568b8 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -595,6 +595,7 @@ static int exfat_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	inode_inc_iversion(inode);
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		EXFAT_I(inode)->i_crtime = current_time(inode);
+	exfat_truncate_atime(&inode->i_atime);
 	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
 
 	d_instantiate(dentry, inode);
@@ -854,6 +855,7 @@ static int exfat_unlink(struct inode *dir, struct dentry *dentry)
 
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_atime = current_time(dir);
+	exfat_truncate_atime(&dir->i_atime);
 	if (IS_DIRSYNC(dir))
 		exfat_sync_inode(dir);
 	else
@@ -861,6 +863,7 @@ static int exfat_unlink(struct inode *dir, struct dentry *dentry)
 
 	clear_nlink(inode);
 	inode->i_mtime = inode->i_atime = current_time(inode);
+	exfat_truncate_atime(&inode->i_atime);
 	exfat_unhash_inode(inode);
 	exfat_d_version_set(dentry, inode_query_iversion(dir));
 unlock:
@@ -903,6 +906,7 @@ static int exfat_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	inode_inc_iversion(inode);
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		EXFAT_I(inode)->i_crtime = current_time(inode);
+	exfat_truncate_atime(&inode->i_atime);
 	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
 
 	d_instantiate(dentry, inode);
@@ -1019,6 +1023,7 @@ static int exfat_rmdir(struct inode *dir, struct dentry *dentry)
 
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_atime = current_time(dir);
+	exfat_truncate_atime(&dir->i_atime);
 	if (IS_DIRSYNC(dir))
 		exfat_sync_inode(dir);
 	else
@@ -1027,6 +1032,7 @@ static int exfat_rmdir(struct inode *dir, struct dentry *dentry)
 
 	clear_nlink(inode);
 	inode->i_mtime = inode->i_atime = current_time(inode);
+	exfat_truncate_atime(&inode->i_atime);
 	exfat_unhash_inode(inode);
 	exfat_d_version_set(dentry, inode_query_iversion(dir));
 unlock:
@@ -1387,6 +1393,7 @@ static int exfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	inode_inc_iversion(new_dir);
 	new_dir->i_ctime = new_dir->i_mtime = new_dir->i_atime =
 		EXFAT_I(new_dir)->i_crtime = current_time(new_dir);
+	exfat_truncate_atime(&new_dir->i_atime);
 	if (IS_DIRSYNC(new_dir))
 		exfat_sync_inode(new_dir);
 	else
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 16ed202ef527..aed62cafb0da 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -351,6 +351,7 @@ static int exfat_read_root(struct inode *inode)
 	exfat_save_attr(inode, ATTR_SUBDIR);
 	inode->i_mtime = inode->i_atime = inode->i_ctime = ei->i_crtime =
 		current_time(inode);
+	exfat_truncate_atime(&inode->i_atime);
 	exfat_cache_init_inode(inode);
 	return 0;
 }

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Namjae Jeon]

2020-04-16 13:11 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
Hi Eric,

> exfat atimes are restricted to only 2s granularity so after
> we set an atime, round it down to the nearest 2s and set the
> sub-second component of the timestamp to 0.
Is there any reason why only atime is truncated with 2s granularity ?

Thanks.
>
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> ---
>
> V2: don't just zero tv_nsec, must also round down tv_sec I think.
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index 67d4e46fb810..d67fb8a6f770 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -507,6 +507,7 @@ void exfat_msg(struct super_block *sb, const char *lv,
> const char *fmt, ...)
>  		__printf(3, 4) __cold;
>  void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64
> *ts,
>  		u8 tz, __le16 time, __le16 date, u8 time_ms);
> +void exfat_truncate_atime(struct timespec64 *ts);
>  void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64
> *ts,
>  		u8 *tz, __le16 *time, __le16 *date, u8 *time_ms);
>  unsigned short exfat_calc_chksum_2byte(void *data, int len,
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 483f683757aa..4f76764165cf 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -273,6 +273,7 @@ int exfat_getattr(const struct path *path, struct kstat
> *stat,
>  	struct exfat_inode_info *ei = EXFAT_I(inode);
>
>  	generic_fillattr(inode, stat);
> +	exfat_truncate_atime(&stat->atime);
>  	stat->result_mask |= STATX_BTIME;
>  	stat->btime.tv_sec = ei->i_crtime.tv_sec;
>  	stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
> @@ -339,6 +340,7 @@ int exfat_setattr(struct dentry *dentry, struct iattr
> *attr)
>  	}
>
>  	setattr_copy(inode, attr);
> +	exfat_truncate_atime(&inode->i_atime);
>  	mark_inode_dirty(inode);
>
>  out:
> diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
> index 14a3300848f6..c8b33278d474 100644
> --- a/fs/exfat/misc.c
> +++ b/fs/exfat/misc.c
> @@ -88,7 +88,8 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
>  	if (time_ms) {
>  		ts->tv_sec += time_ms / 100;
>  		ts->tv_nsec = (time_ms % 100) * 10 * NSEC_PER_MSEC;
> -	}
> +	} else
> +		exfat_truncate_atime(ts);
>
>  	if (tz & EXFAT_TZ_VALID)
>  		/* Adjust timezone to UTC0. */
> @@ -124,6 +125,13 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
>  	*tz = EXFAT_TZ_VALID;
>  }
>
> +/* atime has only a 2-second resolution */
> +void exfat_truncate_atime(struct timespec64 *ts)
> +{
> +	ts->tv_sec = round_down(ts->tv_sec, 2);
> +	ts->tv_nsec = 0;
> +}
> +
>  unsigned short exfat_calc_chksum_2byte(void *data, int len,
>  		unsigned short chksum, int type)
>  {
> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> index a8681d91f569..b72d782568b8 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -595,6 +595,7 @@ static int exfat_create(struct inode *dir, struct dentry
> *dentry, umode_t mode,
>  	inode_inc_iversion(inode);
>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>  		EXFAT_I(inode)->i_crtime = current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
>  	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
>
>  	d_instantiate(dentry, inode);
> @@ -854,6 +855,7 @@ static int exfat_unlink(struct inode *dir, struct dentry
> *dentry)
>
>  	inode_inc_iversion(dir);
>  	dir->i_mtime = dir->i_atime = current_time(dir);
> +	exfat_truncate_atime(&dir->i_atime);
>  	if (IS_DIRSYNC(dir))
>  		exfat_sync_inode(dir);
>  	else
> @@ -861,6 +863,7 @@ static int exfat_unlink(struct inode *dir, struct dentry
> *dentry)
>
>  	clear_nlink(inode);
>  	inode->i_mtime = inode->i_atime = current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
>  	exfat_unhash_inode(inode);
>  	exfat_d_version_set(dentry, inode_query_iversion(dir));
>  unlock:
> @@ -903,6 +906,7 @@ static int exfat_mkdir(struct inode *dir, struct dentry
> *dentry, umode_t mode)
>  	inode_inc_iversion(inode);
>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>  		EXFAT_I(inode)->i_crtime = current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
>  	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
>
>  	d_instantiate(dentry, inode);
> @@ -1019,6 +1023,7 @@ static int exfat_rmdir(struct inode *dir, struct
> dentry *dentry)
>
>  	inode_inc_iversion(dir);
>  	dir->i_mtime = dir->i_atime = current_time(dir);
> +	exfat_truncate_atime(&dir->i_atime);
>  	if (IS_DIRSYNC(dir))
>  		exfat_sync_inode(dir);
>  	else
> @@ -1027,6 +1032,7 @@ static int exfat_rmdir(struct inode *dir, struct
> dentry *dentry)
>
>  	clear_nlink(inode);
>  	inode->i_mtime = inode->i_atime = current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
>  	exfat_unhash_inode(inode);
>  	exfat_d_version_set(dentry, inode_query_iversion(dir));
>  unlock:
> @@ -1387,6 +1393,7 @@ static int exfat_rename(struct inode *old_dir, struct
> dentry *old_dentry,
>  	inode_inc_iversion(new_dir);
>  	new_dir->i_ctime = new_dir->i_mtime = new_dir->i_atime =
>  		EXFAT_I(new_dir)->i_crtime = current_time(new_dir);
> +	exfat_truncate_atime(&new_dir->i_atime);
>  	if (IS_DIRSYNC(new_dir))
>  		exfat_sync_inode(new_dir);
>  	else
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 16ed202ef527..aed62cafb0da 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -351,6 +351,7 @@ static int exfat_read_root(struct inode *inode)
>  	exfat_save_attr(inode, ATTR_SUBDIR);
>  	inode->i_mtime = inode->i_atime = inode->i_ctime = ei->i_crtime =
>  		current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
>  	exfat_cache_init_inode(inode);
>  	return 0;
>  }
>
>

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Namjae Jeon]

2020-04-18 22:03 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
> 2020-04-16 13:11 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
> Hi Eric,
>
>> exfat atimes are restricted to only 2s granularity so after
>> we set an atime, round it down to the nearest 2s and set the
>> sub-second component of the timestamp to 0.
> Is there any reason why only atime is truncated with 2s granularity ?
I will directly update description more.
Sorry to bother you!
>
> Thanks.
>>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>
>> V2: don't just zero tv_nsec, must also round down tv_sec I think.
>>
>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> index 67d4e46fb810..d67fb8a6f770 100644
>> --- a/fs/exfat/exfat_fs.h
>> +++ b/fs/exfat/exfat_fs.h
>> @@ -507,6 +507,7 @@ void exfat_msg(struct super_block *sb, const char
>> *lv,
>> const char *fmt, ...)
>>  		__printf(3, 4) __cold;
>>  void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64
>> *ts,
>>  		u8 tz, __le16 time, __le16 date, u8 time_ms);
>> +void exfat_truncate_atime(struct timespec64 *ts);
>>  void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64
>> *ts,
>>  		u8 *tz, __le16 *time, __le16 *date, u8 *time_ms);
>>  unsigned short exfat_calc_chksum_2byte(void *data, int len,
>> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
>> index 483f683757aa..4f76764165cf 100644
>> --- a/fs/exfat/file.c
>> +++ b/fs/exfat/file.c
>> @@ -273,6 +273,7 @@ int exfat_getattr(const struct path *path, struct
>> kstat
>> *stat,
>>  	struct exfat_inode_info *ei = EXFAT_I(inode);
>>
>>  	generic_fillattr(inode, stat);
>> +	exfat_truncate_atime(&stat->atime);
>>  	stat->result_mask |= STATX_BTIME;
>>  	stat->btime.tv_sec = ei->i_crtime.tv_sec;
>>  	stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
>> @@ -339,6 +340,7 @@ int exfat_setattr(struct dentry *dentry, struct iattr
>> *attr)
>>  	}
>>
>>  	setattr_copy(inode, attr);
>> +	exfat_truncate_atime(&inode->i_atime);
>>  	mark_inode_dirty(inode);
>>
>>  out:
>> diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
>> index 14a3300848f6..c8b33278d474 100644
>> --- a/fs/exfat/misc.c
>> +++ b/fs/exfat/misc.c
>> @@ -88,7 +88,8 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi,
>> struct timespec64 *ts,
>>  	if (time_ms) {
>>  		ts->tv_sec += time_ms / 100;
>>  		ts->tv_nsec = (time_ms % 100) * 10 * NSEC_PER_MSEC;
>> -	}
>> +	} else
>> +		exfat_truncate_atime(ts);
>>
>>  	if (tz & EXFAT_TZ_VALID)
>>  		/* Adjust timezone to UTC0. */
>> @@ -124,6 +125,13 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi,
>> struct timespec64 *ts,
>>  	*tz = EXFAT_TZ_VALID;
>>  }
>>
>> +/* atime has only a 2-second resolution */
>> +void exfat_truncate_atime(struct timespec64 *ts)
>> +{
>> +	ts->tv_sec = round_down(ts->tv_sec, 2);
>> +	ts->tv_nsec = 0;
>> +}
>> +
>>  unsigned short exfat_calc_chksum_2byte(void *data, int len,
>>  		unsigned short chksum, int type)
>>  {
>> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
>> index a8681d91f569..b72d782568b8 100644
>> --- a/fs/exfat/namei.c
>> +++ b/fs/exfat/namei.c
>> @@ -595,6 +595,7 @@ static int exfat_create(struct inode *dir, struct
>> dentry
>> *dentry, umode_t mode,
>>  	inode_inc_iversion(inode);
>>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>>  		EXFAT_I(inode)->i_crtime = current_time(inode);
>> +	exfat_truncate_atime(&inode->i_atime);
>>  	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
>>
>>  	d_instantiate(dentry, inode);
>> @@ -854,6 +855,7 @@ static int exfat_unlink(struct inode *dir, struct
>> dentry
>> *dentry)
>>
>>  	inode_inc_iversion(dir);
>>  	dir->i_mtime = dir->i_atime = current_time(dir);
>> +	exfat_truncate_atime(&dir->i_atime);
>>  	if (IS_DIRSYNC(dir))
>>  		exfat_sync_inode(dir);
>>  	else
>> @@ -861,6 +863,7 @@ static int exfat_unlink(struct inode *dir, struct
>> dentry
>> *dentry)
>>
>>  	clear_nlink(inode);
>>  	inode->i_mtime = inode->i_atime = current_time(inode);
>> +	exfat_truncate_atime(&inode->i_atime);
>>  	exfat_unhash_inode(inode);
>>  	exfat_d_version_set(dentry, inode_query_iversion(dir));
>>  unlock:
>> @@ -903,6 +906,7 @@ static int exfat_mkdir(struct inode *dir, struct
>> dentry
>> *dentry, umode_t mode)
>>  	inode_inc_iversion(inode);
>>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>>  		EXFAT_I(inode)->i_crtime = current_time(inode);
>> +	exfat_truncate_atime(&inode->i_atime);
>>  	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
>>
>>  	d_instantiate(dentry, inode);
>> @@ -1019,6 +1023,7 @@ static int exfat_rmdir(struct inode *dir, struct
>> dentry *dentry)
>>
>>  	inode_inc_iversion(dir);
>>  	dir->i_mtime = dir->i_atime = current_time(dir);
>> +	exfat_truncate_atime(&dir->i_atime);
>>  	if (IS_DIRSYNC(dir))
>>  		exfat_sync_inode(dir);
>>  	else
>> @@ -1027,6 +1032,7 @@ static int exfat_rmdir(struct inode *dir, struct
>> dentry *dentry)
>>
>>  	clear_nlink(inode);
>>  	inode->i_mtime = inode->i_atime = current_time(inode);
>> +	exfat_truncate_atime(&inode->i_atime);
>>  	exfat_unhash_inode(inode);
>>  	exfat_d_version_set(dentry, inode_query_iversion(dir));
>>  unlock:
>> @@ -1387,6 +1393,7 @@ static int exfat_rename(struct inode *old_dir,
>> struct
>> dentry *old_dentry,
>>  	inode_inc_iversion(new_dir);
>>  	new_dir->i_ctime = new_dir->i_mtime = new_dir->i_atime =
>>  		EXFAT_I(new_dir)->i_crtime = current_time(new_dir);
>> +	exfat_truncate_atime(&new_dir->i_atime);
>>  	if (IS_DIRSYNC(new_dir))
>>  		exfat_sync_inode(new_dir);
>>  	else
>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>> index 16ed202ef527..aed62cafb0da 100644
>> --- a/fs/exfat/super.c
>> +++ b/fs/exfat/super.c
>> @@ -351,6 +351,7 @@ static int exfat_read_root(struct inode *inode)
>>  	exfat_save_attr(inode, ATTR_SUBDIR);
>>  	inode->i_mtime = inode->i_atime = inode->i_ctime = ei->i_crtime =
>>  		current_time(inode);
>> +	exfat_truncate_atime(&inode->i_atime);
>>  	exfat_cache_init_inode(inode);
>>  	return 0;
>>  }
>>
>>
>

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Eric Sandeen]

On 4/18/20 8:03 AM, Namjae Jeon wrote:
> 2020-04-16 13:11 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
> Hi Eric,
> 
>> exfat atimes are restricted to only 2s granularity so after
>> we set an atime, round it down to the nearest 2s and set the
>> sub-second component of the timestamp to 0.
> Is there any reason why only atime is truncated with 2s granularity ?

You're the expert, so I might be wrong! :)

My reading of the spec & code is that every timestamp has 2s granularity, in the
main time field, and some timestamps also have another field with 10ms granularity,
which can range from 0 to 199, i.e. 0 to 1990 ms, i.e. 0 to 1.99 seconds in the _ms
field.  i.e. the _ms fields can add more than 1 second to the timestamp, right?

                struct {
                        __u8 num_ext;
                        __le16 checksum;
                        __le16 attr;
                        __le16 reserved1;
                        __le16 create_time;
                        __le16 create_date;
                        __le16 modify_time;
                        __le16 modify_date;
                        __le16 access_time;
                        __le16 access_date;
                        __u8 create_time_ms;
                        __u8 modify_time_ms;
                        __u8 create_tz;
                        __u8 modify_tz;
                        __u8 access_tz;
                        __u8 reserved2[7];
                } __packed file; /* file directory entry */

and per the publiished spec,

7.4.8.1 DoubleSeconds Field

The DoubleSeconds field shall describe the seconds portion of the Timestamp field, in two-second multiples.

The valid range of values for this field shall be:

    0, which represents 0 seconds

    29, which represents 58 seconds

7.4.9 10msIncrement Fields

10msIncrement fields shall provide additional time resolution to their corresponding Timestamp fields in ten-millisecond multiples.

The valid range of values for these fields shall be:

    At least 0, which represents 0 milliseconds

    At most 199, which represents 1990 milliseconds


since access_time has no corresponding 10msIncrement field, my understanding was that it could only have a 2s granularity.

Happy to have the patch dropped or corrected if I read this wrong, though.

-Eric

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Eric Sandeen]

On 4/18/20 11:04 AM, Eric Sandeen wrote:
> since access_time has no corresponding 10msIncrement field, my understanding was that it could only have a 2s granularity.

Maybe your concern is whether the other _time fields should also be
truncated to 2s even though they have the _ms field?  I don't think so; the
s_time_gran already limits in-core timestamp resolution to 10ms, which will
be properly translated when the inode is written to disk.

atime has a different granularity though, so s_time_gran doens't help and we
must manually change it to 2s whenever we call something like current_time(), which
only enforces the 10ms granularity.

So for cases like this:

 	generic_fillattr(inode, stat);
+	exfat_truncate_atime(&stat->atime);

or this:

 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		EXFAT_I(inode)->i_crtime = current_time(inode);
+	exfat_truncate_atime(&inode->i_atime);

I think it's clearly the right thing to do; anything finer than 2s will be thrown
away when the vfs inode atime is translated to the disk format, so we should never
hold finer granularity in the in-memory vfs inode.

However, in exfat_get_entry_time() maybe all we need to do is set ts->tv_nsec to 0;
that might be clearer.

-Eric

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Eric Sandeen]

On 4/18/20 11:40 AM, Eric Sandeen wrote:
> On 4/18/20 11:04 AM, Eric Sandeen wrote:
>> since access_time has no corresponding 10msIncrement field, my understanding was that it could only have a 2s granularity.
> 
> Maybe your concern is whether the other _time fields should also be
> truncated to 2s even though they have the _ms field?  I don't think so; the
> s_time_gran already limits in-core timestamp resolution to 10ms, which will
> be properly translated when the inode is written to disk.
> 
> atime has a different granularity though, so s_time_gran doens't help and we
> must manually change it to 2s whenever we call something like current_time(), which
> only enforces the 10ms granularity.
> 
> So for cases like this:
> 
>  	generic_fillattr(inode, stat);
> +	exfat_truncate_atime(&stat->atime);
> 
> or this:
> 
>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>  		EXFAT_I(inode)->i_crtime = current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
> 
> I think it's clearly the right thing to do; anything finer than 2s will be thrown
> away when the vfs inode atime is translated to the disk format, so we should never
> hold finer granularity in the in-memory vfs inode.
> 
> However, in exfat_get_entry_time() maybe all we need to do is set ts->tv_nsec to 0;
> that might be clearer.

so maybe this is better - 

diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
index c8b33278d474..2c5629b4e7e6 100644
--- a/fs/exfat/misc.c
+++ b/fs/exfat/misc.c
@@ -89,7 +89,7 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 		ts->tv_sec += time_ms / 100;
 		ts->tv_nsec = (time_ms % 100) * 10 * NSEC_PER_MSEC;
 	} else
-		exfat_truncate_atime(ts);
+		ts->tv_nsec = 0;
 
 	if (tz & EXFAT_TZ_VALID)
 		/* Adjust timezone to UTC0. */


because the conversion should already limit tv_sec to a 2s granularity.

-Eric

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Namjae Jeon]

2020-04-19 2:06 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
>
>
> On 4/18/20 11:40 AM, Eric Sandeen wrote:
>> On 4/18/20 11:04 AM, Eric Sandeen wrote:
>>> since access_time has no corresponding 10msIncrement field, my
>>> understanding was that it could only have a 2s granularity.
>>
>> Maybe your concern is whether the other _time fields should also be
>> truncated to 2s even though they have the _ms field?  I don't think so;
>> the
>> s_time_gran already limits in-core timestamp resolution to 10ms, which
>> will
>> be properly translated when the inode is written to disk.
>>
>> atime has a different granularity though, so s_time_gran doens't help and
>> we
>> must manually change it to 2s whenever we call something like
>> current_time(), which
>> only enforces the 10ms granularity.
>>
>> So for cases like this:
>>
>>  	generic_fillattr(inode, stat);
>> +	exfat_truncate_atime(&stat->atime);
>>
>> or this:
>>
>>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>>  		EXFAT_I(inode)->i_crtime = current_time(inode);
>> +	exfat_truncate_atime(&inode->i_atime);
>>
>> I think it's clearly the right thing to do; anything finer than 2s will be
>> thrown
>> away when the vfs inode atime is translated to the disk format, so we
>> should never
>> hold finer granularity in the in-memory vfs inode.
>>
>> However, in exfat_get_entry_time() maybe all we need to do is set
>> ts->tv_nsec to 0;
>> that might be clearer.
>
> so maybe this is better -
>
> diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
> index c8b33278d474..2c5629b4e7e6 100644
> --- a/fs/exfat/misc.c
> +++ b/fs/exfat/misc.c
> @@ -89,7 +89,7 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
>  		ts->tv_sec += time_ms / 100;
>  		ts->tv_nsec = (time_ms % 100) * 10 * NSEC_PER_MSEC;
>  	} else
> -		exfat_truncate_atime(ts);
> +		ts->tv_nsec = 0;
>
>  	if (tz & EXFAT_TZ_VALID)
>  		/* Adjust timezone to UTC0. */
>
>
> because the conversion should already limit tv_sec to a 2s granularity.
Right. I will update it and replace it with old one.
Thanks!
>
> -Eric
>

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Namjae Jeon]

2020-04-19 1:40 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
> On 4/18/20 11:04 AM, Eric Sandeen wrote:
>> since access_time has no corresponding 10msIncrement field, my
>> understanding was that it could only have a 2s granularity.
>
> Maybe your concern is whether the other _time fields should also be
> truncated to 2s even though they have the _ms field?  I don't think so; the
> s_time_gran already limits in-core timestamp resolution to 10ms, which will
> be properly translated when the inode is written to disk.
>
> atime has a different granularity though, so s_time_gran doens't help and
> we
> must manually change it to 2s whenever we call something like
> current_time(), which
> only enforces the 10ms granularity.
>
> So for cases like this:
>
>  	generic_fillattr(inode, stat);
> +	exfat_truncate_atime(&stat->atime);
>
> or this:
>
>  	inode->i_mtime = inode->i_atime = inode->i_ctime =
>  		EXFAT_I(inode)->i_crtime = current_time(inode);
> +	exfat_truncate_atime(&inode->i_atime);
>
> I think it's clearly the right thing to do; anything finer than 2s will be
> thrown
> away when the vfs inode atime is translated to the disk format, so we should
> never
> hold finer granularity in the in-memory vfs inode.
>
> However, in exfat_get_entry_time() maybe all we need to do is set
> ts->tv_nsec to 0;
> that might be clearer.
Right.
Thanks!
>
> -Eric
>

Re: [PATCH 2/2 V2] exfat: truncate atimes to 2s granularity

[Namjae Jeon]

2020-04-19 1:04 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
> On 4/18/20 8:03 AM, Namjae Jeon wrote:
>> 2020-04-16 13:11 GMT+09:00, Eric Sandeen <sandeen@sandeen.net>:
>> Hi Eric,
>>
>>> exfat atimes are restricted to only 2s granularity so after
>>> we set an atime, round it down to the nearest 2s and set the
>>> sub-second component of the timestamp to 0.
>> Is there any reason why only atime is truncated with 2s granularity ?
>
> You're the expert, so I might be wrong! :)
>
> My reading of the spec & code is that every timestamp has 2s granularity, in
> the
> main time field, and some timestamps also have another field with 10ms
> granularity,
> which can range from 0 to 199, i.e. 0 to 1990 ms, i.e. 0 to 1.99 seconds in
> the _ms
> field.  i.e. the _ms fields can add more than 1 second to the timestamp,
> right?
Right.
>
>                 struct {
>                         __u8 num_ext;
>                         __le16 checksum;
>                         __le16 attr;
>                         __le16 reserved1;
>                         __le16 create_time;
>                         __le16 create_date;
>                         __le16 modify_time;
>                         __le16 modify_date;
>                         __le16 access_time;
>                         __le16 access_date;
>                         __u8 create_time_ms;
>                         __u8 modify_time_ms;
>                         __u8 create_tz;
>                         __u8 modify_tz;
>                         __u8 access_tz;
>                         __u8 reserved2[7];
>                 } __packed file; /* file directory entry */
>
> and per the publiished spec,
>
> 7.4.8.1 DoubleSeconds Field
>
> The DoubleSeconds field shall describe the seconds portion of the Timestamp
> field, in two-second multiples.
>
> The valid range of values for this field shall be:
>
>     0, which represents 0 seconds
>
>     29, which represents 58 seconds
>
> 7.4.9 10msIncrement Fields
>
> 10msIncrement fields shall provide additional time resolution to their
> corresponding Timestamp fields in ten-millisecond multiples.
>
> The valid range of values for these fields shall be:
>
>     At least 0, which represents 0 milliseconds
>
>     At most 199, which represents 1990 milliseconds
>
>
> since access_time has no corresponding 10msIncrement field, my understanding
> was that it could only have a 2s granularity.
Right.
>
> Happy to have the patch dropped or corrected if I read this wrong, though.
Thanks for your detail explanation:)

I just wanted to update the patch's descirption and function's
comments to make it easier for others to understand this patch and
code.
>
> -Eric
>

RE: [PATCH 0/2] exfat: timestamp fixes

[Namjae Jeon]

> I've seen 2 issues w/ exfat timestamps - discovered by xfstests
> generic/003
> 
> 1) time granularity is set wrong, so subsecond timestamps change when
>    an inode cycles out of memory and is reread from disk
> 
> 2) the disk format doesn't seem to support subsecond atime, so a similar
>    problem exists there.e
> 
> The last xfstests generic/003 issue is re: change time but exfat doesn't
> really support change times (?) so that'll be a test workaround.
Applied.
Thanks!