[PATCH v2 1/3] fat: split fat_truncate_time() into separate functions

[PATCH v2 1/3] fat: split fat_truncate_time() into separate functions

[Chung-Chiang Cheng]

Separate fat_truncate_time() to each timestamps for later creation time
work.

This patch does not introduce any functional changes, it's merely
refactoring change.

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/fat/fat.h  |  6 +++++
 fs/fat/misc.c | 72 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 02d4d4234956..508b4f2a1ffb 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -446,6 +446,12 @@ 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,
 			      __le16 *time, __le16 *date, u8 *time_cs);
+extern void fat_truncate_atime(struct msdos_sb_info *sbi, struct timespec64 *ts,
+			       struct timespec64 *atime);
+extern void fat_truncate_crtime(struct msdos_sb_info *sbi, struct timespec64 *ts,
+				struct timespec64 *crtime);
+extern void fat_truncate_mtime(struct msdos_sb_info *sbi, struct timespec64 *ts,
+			       struct timespec64 *mtime);
 extern int fat_truncate_time(struct inode *inode, struct timespec64 *now,
 			     int flags);
 extern int fat_update_time(struct inode *inode, struct timespec64 *now,
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index 91ca3c304211..c87df64f8b2b 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -282,16 +282,49 @@ static inline struct timespec64 fat_timespec64_trunc_10ms(struct timespec64 ts)
 	return ts;
 }
 
+/*
+ * truncate atime to 24 hour granularity (00:00:00 in local timezone)
+ */
+void fat_truncate_atime(struct msdos_sb_info *sbi, struct timespec64 *ts,
+			struct timespec64 *atime)
+{
+	/* to localtime */
+	time64_t seconds = ts->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;
+
+	*atime = (struct timespec64){ seconds, 0 };
+}
+
+/*
+ * truncate creation time with appropriate granularity:
+ *   msdos - 2 seconds
+ *   vfat  - 10 milliseconds
+ */
+void fat_truncate_crtime(struct msdos_sb_info *sbi, struct timespec64 *ts,
+			 struct timespec64 *crtime)
+{
+	if (sbi->options.isvfat)
+		*crtime = fat_timespec64_trunc_10ms(*ts);
+	else
+		*crtime = fat_timespec64_trunc_2secs(*ts);
+}
+
+/*
+ * truncate mtime to 2 second granularity
+ */
+void fat_truncate_mtime(struct msdos_sb_info *sbi, struct timespec64 *ts,
+			struct timespec64 *mtime)
+{
+	*mtime = fat_timespec64_trunc_2secs(*ts);
+}
+
 /*
  * truncate the various times with appropriate granularity:
- *   root inode:
- *     all times always 0
- *   all other inodes:
- *     mtime - 2 seconds
- *     ctime
- *       msdos - 2 seconds
- *       vfat  - 10 milliseconds
- *     atime - 24 hours (00:00:00 in local timezone)
+ *   all times in root node are always 0
  */
 int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
 {
@@ -306,25 +339,12 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
 		ts = current_time(inode);
 	}
 
-	if (flags & S_ATIME) {
-		/* 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 = fat_timespec64_trunc_10ms(*now);
-		else
-			inode->i_ctime = fat_timespec64_trunc_2secs(*now);
-	}
+	if (flags & S_ATIME)
+		fat_truncate_atime(sbi, now, &inode->i_atime);
+	if (flags & S_CTIME)
+		fat_truncate_crtime(sbi, now, &inode->i_ctime);
 	if (flags & S_MTIME)
-		inode->i_mtime = fat_timespec64_trunc_2secs(*now);
+		fat_truncate_mtime(sbi, now, &inode->i_mtime);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v2 2/3] fat: make ctime and mtime identical explicitly

[Chung-Chiang Cheng]

FAT supports creation time but not change time, and there was no
corresponding timestamp for creation time in previous VFS. The
original implementation took the compromise of saving the in-memory
change time into the on-disk creation time field, but this would lead
to compatibility issues with non-linux systems.

In address this issue, this patch changes the behavior of ctime. ctime
will no longer be loaded and stored from the creation time on disk.
Instead of that, it'll be consistent with the in-memory mtime and
share the same on-disk field.

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/fat/dir.c         |  2 +-
 fs/fat/file.c        |  4 ++++
 fs/fat/inode.c       | 11 ++++-------
 fs/fat/misc.c        | 11 ++++++++---
 fs/fat/namei_msdos.c |  6 +++---
 fs/fat/namei_vfat.c  |  6 +++---
 6 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 249825017da7..0ae0dfe278fb 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -1068,7 +1068,7 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
 		}
 	}
 
-	fat_truncate_time(dir, NULL, S_ATIME|S_MTIME);
+	fat_truncate_time(dir, NULL, S_ATIME|S_CTIME|S_MTIME);
 	if (IS_DIRSYNC(dir))
 		(void)fat_sync_inode(dir);
 	else
diff --git a/fs/fat/file.c b/fs/fat/file.c
index a5a309fcc7fa..178c1dde3488 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -544,6 +544,10 @@ int fat_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	/*
 	 * setattr_copy can't truncate these appropriately, so we'll
 	 * copy them ourselves
+	 *
+	 * fat_truncate_time() keeps ctime and mtime the same. if both
+	 * ctime and mtime are need to update here, mtime will overwrite
+	 * ctime
 	 */
 	if (attr->ia_valid & ATTR_ATIME)
 		fat_truncate_time(inode, &attr->ia_atime, S_ATIME);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index bf6051bdf1d1..f2ac55cd4ea4 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -567,12 +567,11 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 			   & ~((loff_t)sbi->cluster_size - 1)) >> 9;
 
 	fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0);
-	if (sbi->options.isvfat) {
-		fat_time_fat2unix(sbi, &inode->i_ctime, de->ctime,
-				  de->cdate, de->ctime_cs);
+	inode->i_ctime = inode->i_mtime;
+	if (sbi->options.isvfat)
 		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
-	} else
-		fat_truncate_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
+	else
+		fat_truncate_atime(sbi, &inode->i_mtime, &inode->i_atime);
 
 	return 0;
 }
@@ -888,8 +887,6 @@ static int __fat_write_inode(struct inode *inode, int wait)
 			  &raw_entry->date, NULL);
 	if (sbi->options.isvfat) {
 		__le16 atime;
-		fat_time_unix2fat(sbi, &inode->i_ctime, &raw_entry->ctime,
-				  &raw_entry->cdate, &raw_entry->ctime_cs);
 		fat_time_unix2fat(sbi, &inode->i_atime, &atime,
 				  &raw_entry->adate, NULL);
 	}
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index c87df64f8b2b..71e6dadf12a2 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -341,10 +341,15 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
 
 	if (flags & S_ATIME)
 		fat_truncate_atime(sbi, now, &inode->i_atime);
-	if (flags & S_CTIME)
-		fat_truncate_crtime(sbi, now, &inode->i_ctime);
-	if (flags & S_MTIME)
+
+	/*
+	 * ctime and mtime share the same on-disk field, and should be
+	 * identical in memory.
+	 */
+	if (flags & (S_CTIME|S_MTIME)) {
 		fat_truncate_mtime(sbi, now, &inode->i_mtime);
+		inode->i_ctime = inode->i_mtime;
+	}
 
 	return 0;
 }
diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index efba301d68ae..b2760a716707 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -328,7 +328,7 @@ static int msdos_rmdir(struct inode *dir, struct dentry *dentry)
 	drop_nlink(dir);
 
 	clear_nlink(inode);
-	fat_truncate_time(inode, NULL, S_CTIME);
+	fat_truncate_time(inode, NULL, S_CTIME|S_MTIME);
 	fat_detach(inode);
 out:
 	mutex_unlock(&MSDOS_SB(sb)->s_lock);
@@ -415,7 +415,7 @@ static int msdos_unlink(struct inode *dir, struct dentry *dentry)
 	if (err)
 		goto out;
 	clear_nlink(inode);
-	fat_truncate_time(inode, NULL, S_CTIME);
+	fat_truncate_time(inode, NULL, S_CTIME|S_MTIME);
 	fat_detach(inode);
 out:
 	mutex_unlock(&MSDOS_SB(sb)->s_lock);
@@ -550,7 +550,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 		drop_nlink(new_inode);
 		if (is_dir)
 			drop_nlink(new_inode);
-		fat_truncate_time(new_inode, &ts, S_CTIME);
+		fat_truncate_time(new_inode, &ts, S_CTIME|S_MTIME);
 	}
 out:
 	brelse(sinfo.bh);
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 5369d82e0bfb..b8deb859b2b5 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -811,7 +811,7 @@ static int vfat_rmdir(struct inode *dir, struct dentry *dentry)
 	drop_nlink(dir);
 
 	clear_nlink(inode);
-	fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
+	fat_truncate_time(inode, NULL, S_ATIME|S_CTIME|S_MTIME);
 	fat_detach(inode);
 	vfat_d_version_set(dentry, inode_query_iversion(dir));
 out:
@@ -837,7 +837,7 @@ static int vfat_unlink(struct inode *dir, struct dentry *dentry)
 	if (err)
 		goto out;
 	clear_nlink(inode);
-	fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
+	fat_truncate_time(inode, NULL, S_ATIME|S_CTIME|S_MTIME);
 	fat_detach(inode);
 	vfat_d_version_set(dentry, inode_query_iversion(dir));
 out:
@@ -981,7 +981,7 @@ static int vfat_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 		drop_nlink(new_inode);
 		if (is_dir)
 			drop_nlink(new_inode);
-		fat_truncate_time(new_inode, &ts, S_CTIME);
+		fat_truncate_time(new_inode, &ts, S_CTIME|S_MTIME);
 	}
 out:
 	brelse(sinfo.bh);
-- 
2.34.1

[PATCH v2 3/3] fat: report creation time in statx

[Chung-Chiang Cheng]

creation time is no longer mixed with change time. Add a in-memory
field for it, and report it in statx.

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/fat/fat.h   |  1 +
 fs/fat/file.c  |  6 ++++++
 fs/fat/inode.c | 12 ++++++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 508b4f2a1ffb..e4409ee82ea9 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -126,6 +126,7 @@ struct msdos_inode_info {
 	struct hlist_node i_fat_hash;	/* hash by i_location */
 	struct hlist_node i_dir_hash;	/* hash by i_logstart */
 	struct rw_semaphore truncate_lock; /* protect bmap against truncate */
+	struct timespec64 i_crtime;	/* File creation (birth) time */
 	struct inode vfs_inode;
 };
 
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 178c1dde3488..b56f64cc1c9c 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -406,6 +406,12 @@ int fat_getattr(struct user_namespace *mnt_userns, const struct path *path,
 		/* Use i_pos for ino. This is used as fileid of nfs. */
 		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
 	}
+
+	if (request_mask & STATX_BTIME) {
+		stat->result_mask |= STATX_BTIME;
+		stat->btime = MSDOS_I(inode)->i_crtime;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fat_getattr);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index f2ac55cd4ea4..23fac3181fa7 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -568,10 +568,14 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 
 	fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0);
 	inode->i_ctime = inode->i_mtime;
-	if (sbi->options.isvfat)
+	if (sbi->options.isvfat) {
 		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
-	else
+		fat_time_fat2unix(sbi, &MSDOS_I(inode)->i_crtime, de->ctime,
+				  de->cdate, de->ctime_cs);
+	} else {
 		fat_truncate_atime(sbi, &inode->i_mtime, &inode->i_atime);
+		fat_truncate_crtime(sbi, &inode->i_mtime, &MSDOS_I(inode)->i_crtime);
+	}
 
 	return 0;
 }
@@ -756,6 +760,8 @@ static struct inode *fat_alloc_inode(struct super_block *sb)
 	ei->i_logstart = 0;
 	ei->i_attrs = 0;
 	ei->i_pos = 0;
+	ei->i_crtime.tv_sec = 0;
+	ei->i_crtime.tv_nsec = 0;
 
 	return &ei->vfs_inode;
 }
@@ -887,6 +893,8 @@ static int __fat_write_inode(struct inode *inode, int wait)
 			  &raw_entry->date, NULL);
 	if (sbi->options.isvfat) {
 		__le16 atime;
+		fat_time_unix2fat(sbi, &MSDOS_I(inode)->i_crtime, &raw_entry->ctime,
+				  &raw_entry->cdate, &raw_entry->ctime_cs);
 		fat_time_unix2fat(sbi, &inode->i_atime, &atime,
 				  &raw_entry->adate, NULL);
 	}
-- 
2.34.1

Re: [PATCH v2 2/3] fat: make ctime and mtime identical explicitly

[OGAWA Hirofumi]

Chung-Chiang Cheng <cccheng@synology.com> writes:

> FAT supports creation time but not change time, and there was no
> corresponding timestamp for creation time in previous VFS. The
> original implementation took the compromise of saving the in-memory
> change time into the on-disk creation time field, but this would lead
> to compatibility issues with non-linux systems.
>
> In address this issue, this patch changes the behavior of ctime. ctime
> will no longer be loaded and stored from the creation time on disk.
> Instead of that, it'll be consistent with the in-memory mtime and
> share the same on-disk field.

Hm, this changes mtime includes ctime update. So, the question is, this
behavior is compatible with Windows's fatfs behavior? E.g. Windows
updates mtime on rename?

If not same behavior with Windows, new behavior is new incompatible
behavior, and looks break fundamental purpose of this.

I was thinking, we ignores ctime update (because fatfs doesn't have) and
always same with mtime. What behavior was actually compatible with
Windows?

Thanks.

> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
> ---
>  fs/fat/dir.c         |  2 +-
>  fs/fat/file.c        |  4 ++++
>  fs/fat/inode.c       | 11 ++++-------
>  fs/fat/misc.c        | 11 ++++++++---
>  fs/fat/namei_msdos.c |  6 +++---
>  fs/fat/namei_vfat.c  |  6 +++---
>  6 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 249825017da7..0ae0dfe278fb 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -1068,7 +1068,7 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
>  		}
>  	}
>  
> -	fat_truncate_time(dir, NULL, S_ATIME|S_MTIME);
> +	fat_truncate_time(dir, NULL, S_ATIME|S_CTIME|S_MTIME);
>  	if (IS_DIRSYNC(dir))
>  		(void)fat_sync_inode(dir);
>  	else
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index a5a309fcc7fa..178c1dde3488 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -544,6 +544,10 @@ int fat_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	/*
>  	 * setattr_copy can't truncate these appropriately, so we'll
>  	 * copy them ourselves
> +	 *
> +	 * fat_truncate_time() keeps ctime and mtime the same. if both
> +	 * ctime and mtime are need to update here, mtime will overwrite
> +	 * ctime
>  	 */
>  	if (attr->ia_valid & ATTR_ATIME)
>  		fat_truncate_time(inode, &attr->ia_atime, S_ATIME);
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index bf6051bdf1d1..f2ac55cd4ea4 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -567,12 +567,11 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
>  			   & ~((loff_t)sbi->cluster_size - 1)) >> 9;
>  
>  	fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0);
> -	if (sbi->options.isvfat) {
> -		fat_time_fat2unix(sbi, &inode->i_ctime, de->ctime,
> -				  de->cdate, de->ctime_cs);
> +	inode->i_ctime = inode->i_mtime;
> +	if (sbi->options.isvfat)
>  		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
> -	} else
> -		fat_truncate_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
> +	else
> +		fat_truncate_atime(sbi, &inode->i_mtime, &inode->i_atime);
>  
>  	return 0;
>  }
> @@ -888,8 +887,6 @@ static int __fat_write_inode(struct inode *inode, int wait)
>  			  &raw_entry->date, NULL);
>  	if (sbi->options.isvfat) {
>  		__le16 atime;
> -		fat_time_unix2fat(sbi, &inode->i_ctime, &raw_entry->ctime,
> -				  &raw_entry->cdate, &raw_entry->ctime_cs);
>  		fat_time_unix2fat(sbi, &inode->i_atime, &atime,
>  				  &raw_entry->adate, NULL);
>  	}
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index c87df64f8b2b..71e6dadf12a2 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -341,10 +341,15 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
>  
>  	if (flags & S_ATIME)
>  		fat_truncate_atime(sbi, now, &inode->i_atime);
> -	if (flags & S_CTIME)
> -		fat_truncate_crtime(sbi, now, &inode->i_ctime);
> -	if (flags & S_MTIME)
> +
> +	/*
> +	 * ctime and mtime share the same on-disk field, and should be
> +	 * identical in memory.
> +	 */
> +	if (flags & (S_CTIME|S_MTIME)) {
>  		fat_truncate_mtime(sbi, now, &inode->i_mtime);
> +		inode->i_ctime = inode->i_mtime;
> +	}
>  
>  	return 0;
>  }
> diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
> index efba301d68ae..b2760a716707 100644
> --- a/fs/fat/namei_msdos.c
> +++ b/fs/fat/namei_msdos.c
> @@ -328,7 +328,7 @@ static int msdos_rmdir(struct inode *dir, struct dentry *dentry)
>  	drop_nlink(dir);
>  
>  	clear_nlink(inode);
> -	fat_truncate_time(inode, NULL, S_CTIME);
> +	fat_truncate_time(inode, NULL, S_CTIME|S_MTIME);
>  	fat_detach(inode);
>  out:
>  	mutex_unlock(&MSDOS_SB(sb)->s_lock);
> @@ -415,7 +415,7 @@ static int msdos_unlink(struct inode *dir, struct dentry *dentry)
>  	if (err)
>  		goto out;
>  	clear_nlink(inode);
> -	fat_truncate_time(inode, NULL, S_CTIME);
> +	fat_truncate_time(inode, NULL, S_CTIME|S_MTIME);
>  	fat_detach(inode);
>  out:
>  	mutex_unlock(&MSDOS_SB(sb)->s_lock);
> @@ -550,7 +550,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
>  		drop_nlink(new_inode);
>  		if (is_dir)
>  			drop_nlink(new_inode);
> -		fat_truncate_time(new_inode, &ts, S_CTIME);
> +		fat_truncate_time(new_inode, &ts, S_CTIME|S_MTIME);
>  	}
>  out:
>  	brelse(sinfo.bh);
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index 5369d82e0bfb..b8deb859b2b5 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -811,7 +811,7 @@ static int vfat_rmdir(struct inode *dir, struct dentry *dentry)
>  	drop_nlink(dir);
>  
>  	clear_nlink(inode);
> -	fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
> +	fat_truncate_time(inode, NULL, S_ATIME|S_CTIME|S_MTIME);
>  	fat_detach(inode);
>  	vfat_d_version_set(dentry, inode_query_iversion(dir));
>  out:
> @@ -837,7 +837,7 @@ static int vfat_unlink(struct inode *dir, struct dentry *dentry)
>  	if (err)
>  		goto out;
>  	clear_nlink(inode);
> -	fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
> +	fat_truncate_time(inode, NULL, S_ATIME|S_CTIME|S_MTIME);
>  	fat_detach(inode);
>  	vfat_d_version_set(dentry, inode_query_iversion(dir));
>  out:
> @@ -981,7 +981,7 @@ static int vfat_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
>  		drop_nlink(new_inode);
>  		if (is_dir)
>  			drop_nlink(new_inode);
> -		fat_truncate_time(new_inode, &ts, S_CTIME);
> +		fat_truncate_time(new_inode, &ts, S_CTIME|S_MTIME);
>  	}
>  out:
>  	brelse(sinfo.bh);

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

Re: [PATCH v2 3/3] fat: report creation time in statx

[OGAWA Hirofumi]

Chung-Chiang Cheng <cccheng@synology.com> writes:

> creation time is no longer mixed with change time. Add a in-memory
> field for it, and report it in statx.
>

[...]

> +
> +	if (request_mask & STATX_BTIME) {
> +		stat->result_mask |= STATX_BTIME;
> +		stat->btime = MSDOS_I(inode)->i_crtime;
> +	}
> +

[...]

> -	if (sbi->options.isvfat)
> +	if (sbi->options.isvfat) {
>  		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
> -	else
> +		fat_time_fat2unix(sbi, &MSDOS_I(inode)->i_crtime, de->ctime,
> +				  de->cdate, de->ctime_cs);
> +	} else {
>  		fat_truncate_atime(sbi, &inode->i_mtime, &inode->i_atime);
> +		fat_truncate_crtime(sbi, &inode->i_mtime, &MSDOS_I(inode)->i_crtime);
> +	}

This looks strange. MSDOS doesn't have crtime, but set the fake time
from mtime and returns to userspace. Why don't we disable STATX_BTIME
for MSDOS?

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

Re: [PATCH v2 2/3] fat: make ctime and mtime identical explicitly

[Chung-Chiang Cheng]

On Sun, Apr 10, 2022 at 11:43 PM OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
>
> Hm, this changes mtime includes ctime update. So, the question is, this
> behavior is compatible with Windows's fatfs behavior? E.g. Windows
> updates mtime on rename?
>
> If not same behavior with Windows, new behavior is new incompatible
> behavior, and looks break fundamental purpose of this.
>
> I was thinking, we ignores ctime update (because fatfs doesn't have) and
> always same with mtime. What behavior was actually compatible with
> Windows?
>

If possible, to ignore ctime update may be a better choice that doesn't
affect mtime. But we need an initial value for ctime when the inode is
loaded.

One possible option is to use mtime. Although ctime won't be updated
anymore, when mtime is changed, ctime needs to take effect. Otherwise
the next time the inode is loaded, ctime will be inconsistent. That is,
ctime is still updated indirectly by mtime. It seems impossible to avoid
updating ctime, or do we just show ctime a non-sense value?

Thanks.

Re: [PATCH v2 2/3] fat: make ctime and mtime identical explicitly

[OGAWA Hirofumi]

Chung-Chiang Cheng <shepjeng@gmail.com> writes:

> On Sun, Apr 10, 2022 at 11:43 PM OGAWA Hirofumi
> <hirofumi@mail.parknet.co.jp> wrote:
>>
>> Hm, this changes mtime includes ctime update. So, the question is, this
>> behavior is compatible with Windows's fatfs behavior? E.g. Windows
>> updates mtime on rename?
>>
>> If not same behavior with Windows, new behavior is new incompatible
>> behavior, and looks break fundamental purpose of this.
>>
>> I was thinking, we ignores ctime update (because fatfs doesn't have) and
>> always same with mtime. What behavior was actually compatible with
>> Windows?
>>
>
> If possible, to ignore ctime update may be a better choice that doesn't
> affect mtime. But we need an initial value for ctime when the inode is
> loaded.
>
> One possible option is to use mtime. Although ctime won't be updated
> anymore, when mtime is changed, ctime needs to take effect. Otherwise
> the next time the inode is loaded, ctime will be inconsistent. That is,
> ctime is still updated indirectly by mtime. It seems impossible to avoid
> updating ctime, or do we just show ctime a non-sense value?

I mean I also think the behavior what you said sounds like reasonable.
mtime change affect to ctime, but ctime change doesn't affect to mtime.

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

Re: [PATCH v2 3/3] fat: report creation time in statx

[Chung-Chiang Cheng]

> > +
> > +     if (request_mask & STATX_BTIME) {
> > +             stat->result_mask |= STATX_BTIME;
> > +             stat->btime = MSDOS_I(inode)->i_crtime;
> > +     }
> > +
>
> [...]
>
> > -     if (sbi->options.isvfat)
> > +     if (sbi->options.isvfat) {
> >               fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
> > -     else
> > +             fat_time_fat2unix(sbi, &MSDOS_I(inode)->i_crtime, de->ctime,
> > +                               de->cdate, de->ctime_cs);
> > +     } else {
> >               fat_truncate_atime(sbi, &inode->i_mtime, &inode->i_atime);
> > +             fat_truncate_crtime(sbi, &inode->i_mtime, &MSDOS_I(inode)->i_crtime);
> > +     }
>
> This looks strange. MSDOS doesn't have crtime, but set the fake time
> from mtime and returns to userspace. Why don't we disable STATX_BTIME
> for MSDOS?

Agree. It's not necessary to report in statx.

Thanks.