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

[PATCH 1/2] 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 2/2] fat: introduce creation time

[Chung-Chiang Cheng]

In the old days, FAT supports creation time, but there's no corresponding
timestamp in VFS. The implementation mixed the meaning of change time and
creation time into a single ctime field.

This patch introduces a new field for creation time, and reports it in
statx. The original ctime doesn't stand for create time any more. All
the behaviors of ctime (change time) follow mtime, as exfat does.

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/fat/fat.h   |  1 +
 fs/fat/file.c  |  6 ++++++
 fs/fat/inode.c | 15 ++++++++++-----
 fs/fat/misc.c  |  6 +++---
 4 files changed, 20 insertions(+), 8 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 13855ba49cd9..184fa0375152 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -405,6 +405,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 a6f1c6d426d1..41b85b95ee9d 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -566,12 +566,15 @@ 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);
+	inode->i_ctime = inode->i_mtime;
 	if (sbi->options.isvfat) {
-		fat_time_fat2unix(sbi, &inode->i_ctime, de->ctime,
-				  de->cdate, de->ctime_cs);
 		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
-	} else
-		fat_truncate_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
+		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 +759,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,7 +892,7 @@ 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,
+		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);
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index c87df64f8b2b..36b6da6461cc 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -341,10 +341,10 @@ 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)
+	if (flags & (S_CTIME | S_MTIME)) {
 		fat_truncate_mtime(sbi, now, &inode->i_mtime);
+		inode->i_ctime = inode->i_mtime;
+	}
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 2/2] fat: introduce creation time

[OGAWA Hirofumi]

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

> In the old days, FAT supports creation time, but there's no corresponding
> timestamp in VFS. The implementation mixed the meaning of change time and
> creation time into a single ctime field.
>
> This patch introduces a new field for creation time, and reports it in
> statx. The original ctime doesn't stand for create time any more. All
> the behaviors of ctime (change time) follow mtime, as exfat does.

Yes, ctime is issue (include compatibility issue when changing) from
original author of this driver. And there is no perfect solution and
subtle issue I think.

I'm not against about this change though, this behavior makes utimes(2)
behavior strange, e.g. user can change ctime, but FAT forget it anytime,
because FAT can't save it.

Did you consider about those behavior and choose this?

Thanks.

> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
> ---
>  fs/fat/fat.h   |  1 +
>  fs/fat/file.c  |  6 ++++++
>  fs/fat/inode.c | 15 ++++++++++-----
>  fs/fat/misc.c  |  6 +++---
>  4 files changed, 20 insertions(+), 8 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 13855ba49cd9..184fa0375152 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -405,6 +405,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 a6f1c6d426d1..41b85b95ee9d 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -566,12 +566,15 @@ 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);
> +	inode->i_ctime = inode->i_mtime;
>  	if (sbi->options.isvfat) {
> -		fat_time_fat2unix(sbi, &inode->i_ctime, de->ctime,
> -				  de->cdate, de->ctime_cs);
>  		fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
> -	} else
> -		fat_truncate_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
> +		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 +759,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,7 +892,7 @@ 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,
> +		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);
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index c87df64f8b2b..36b6da6461cc 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -341,10 +341,10 @@ 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)
> +	if (flags & (S_CTIME | S_MTIME)) {
>  		fat_truncate_mtime(sbi, now, &inode->i_mtime);
> +		inode->i_ctime = inode->i_mtime;
> +	}
>  
>  	return 0;
>  }

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

Re: [PATCH 2/2] fat: introduce creation time

[Chung-Chiang Cheng]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> 於 2022年3月22日 週二 下午3:33寫道：
>
> Chung-Chiang Cheng <cccheng@synology.com> writes:
>
> > In the old days, FAT supports creation time, but there's no corresponding
> > timestamp in VFS. The implementation mixed the meaning of change time and
> > creation time into a single ctime field.
> >
> > This patch introduces a new field for creation time, and reports it in
> > statx. The original ctime doesn't stand for create time any more. All
> > the behaviors of ctime (change time) follow mtime, as exfat does.
>
> Yes, ctime is issue (include compatibility issue when changing) from
> original author of this driver. And there is no perfect solution and
> subtle issue I think.
>
> I'm not against about this change though, this behavior makes utimes(2)
> behavior strange, e.g. user can change ctime, but FAT forget it anytime,
> because FAT can't save it.
>
> Did you consider about those behavior and choose this?

Yes. I think it's not perfect but a better choice to distinguish between
change-time and creation-time. While change-time is no longer saved to
disk, the new behavior maintains the semantic of "creation" and is more
compatible with non-linux systems.

Thanks.

> Thanks.
>
> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
> > ---
> >  fs/fat/fat.h   |  1 +
> >  fs/fat/file.c  |  6 ++++++
> >  fs/fat/inode.c | 15 ++++++++++-----
> >  fs/fat/misc.c  |  6 +++---
> >  4 files changed, 20 insertions(+), 8 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 13855ba49cd9..184fa0375152 100644
> > --- a/fs/fat/file.c
> > +++ b/fs/fat/file.c
> > @@ -405,6 +405,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 a6f1c6d426d1..41b85b95ee9d 100644
> > --- a/fs/fat/inode.c
> > +++ b/fs/fat/inode.c
> > @@ -566,12 +566,15 @@ 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);
> > +     inode->i_ctime = inode->i_mtime;
> >       if (sbi->options.isvfat) {
> > -             fat_time_fat2unix(sbi, &inode->i_ctime, de->ctime,
> > -                               de->cdate, de->ctime_cs);
> >               fat_time_fat2unix(sbi, &inode->i_atime, 0, de->adate, 0);
> > -     } else
> > -             fat_truncate_time(inode, &inode->i_mtime, S_ATIME|S_CTIME);
> > +             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 +759,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,7 +892,7 @@ 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,
> > +             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);
> > diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> > index c87df64f8b2b..36b6da6461cc 100644
> > --- a/fs/fat/misc.c
> > +++ b/fs/fat/misc.c
> > @@ -341,10 +341,10 @@ 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)
> > +     if (flags & (S_CTIME | S_MTIME)) {
> >               fat_truncate_mtime(sbi, now, &inode->i_mtime);
> > +             inode->i_ctime = inode->i_mtime;
> > +     }
> >
> >       return 0;
> >  }
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/2] fat: introduce creation time

[OGAWA Hirofumi]

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

>> Yes, ctime is issue (include compatibility issue when changing) from
>> original author of this driver. And there is no perfect solution and
>> subtle issue I think.
>>
>> I'm not against about this change though, this behavior makes utimes(2)
>> behavior strange, e.g. user can change ctime, but FAT forget it anytime,
>> because FAT can't save it.
>>
>> Did you consider about those behavior and choose this?
>
> Yes. I think it's not perfect but a better choice to distinguish between
> change-time and creation-time. While change-time is no longer saved to
> disk, the new behavior maintains the semantic of "creation" and is more
> compatible with non-linux systems.

Ok, right, creation time is good. But what I'm saying is about new ctime
behavior.

Now, you allow to change ctime as old behavior, but it is not saved. Why
this behavior was preferred?

Just for example, I think we can ignore ctime change, and define new
behavior is as ctime==mtime always. This will prevent time wrap/backward
etc.

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

Re: [PATCH 2/2] fat: introduce creation time

[Chung-Chiang Cheng]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> 於 2022年3月23日 週三 下午2:57寫道：
>
> Chung-Chiang Cheng <shepjeng@gmail.com> writes:
>
> >
> > Yes. I think it's not perfect but a better choice to distinguish between
> > change-time and creation-time. While change-time is no longer saved to
> > disk, the new behavior maintains the semantic of "creation" and is more
> > compatible with non-linux systems.
>
> Ok, right, creation time is good. But what I'm saying is about new ctime
> behavior.
>
> Now, you allow to change ctime as old behavior, but it is not saved. Why
> this behavior was preferred?
>
> Just for example, I think we can ignore ctime change, and define new
> behavior is as ctime==mtime always. This will prevent time wrap/backward
> etc.

I got your point. Correctly speaking ctime is not really dropped but mixed
with mtime after my patch. They share the same field on disk. Before that
ctime is mixed with crtime.

I choose this new behavior because ctime and mtime are similar concepts.
ctime is the update time for file attributes, and mtime is the one for file
contents. They are updated together most of the time with few exceptions
(rename, rmdir, unlink) in the current FAT implementation.

Thanks.

>
> Thanks.

Re: [PATCH 2/2] fat: introduce creation time

[OGAWA Hirofumi]

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

> I got your point. Correctly speaking ctime is not really dropped but mixed
> with mtime after my patch. They share the same field on disk. Before that
> ctime is mixed with crtime.
>
> I choose this new behavior because ctime and mtime are similar concepts.
> ctime is the update time for file attributes, and mtime is the one for file
> contents. They are updated together most of the time with few exceptions
> (rename, rmdir, unlink) in the current FAT implementation.

No, a user can change the ctime to arbitrary time, and after the your
patch, the changed ctime only hold on a memory inode. So a user sees
ctime jump backward and forward when a memory inode is expired. (Of
course, this happens just by "cp -a" in real world use case.)

I'm pointing about this introduced new behavior by your patch.

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

Re: [PATCH 2/2] fat: introduce creation time

[Chung-Chiang Cheng]

On Wed, Mar 23, 2022 at 6:57 PM OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
>
> No, a user can change the ctime to arbitrary time, and after the your
> patch, the changed ctime only hold on a memory inode. So a user sees
> ctime jump backward and forward when a memory inode is expired. (Of
> course, this happens just by "cp -a" in real world use case.)
>
> I'm pointing about this introduced new behavior by your patch.
>

As you mentioned, there are still some cases to consider that ctime
isn't identical to mtime. If so, ctime won't be consistent after
inode is expired because it will be filled with the value of on-disk
mtime, which is weird and confusing.

To solve the issue, I propose to keep ctime and mtime always the same
in memory. If you agree with this approach, I'll send a v2 patch for
it.

Thanks.

Re: [PATCH 2/2] fat: introduce creation time

[OGAWA Hirofumi]

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

> On Wed, Mar 23, 2022 at 6:57 PM OGAWA Hirofumi
> <hirofumi@mail.parknet.co.jp> wrote:
>>
>> No, a user can change the ctime to arbitrary time, and after the your
>> patch, the changed ctime only hold on a memory inode. So a user sees
>> ctime jump backward and forward when a memory inode is expired. (Of
>> course, this happens just by "cp -a" in real world use case.)
>>
>> I'm pointing about this introduced new behavior by your patch.
>>
>
> As you mentioned, there are still some cases to consider that ctime
> isn't identical to mtime. If so, ctime won't be consistent after
> inode is expired because it will be filled with the value of on-disk
> mtime, which is weird and confusing.
>
> To solve the issue, I propose to keep ctime and mtime always the same
> in memory. If you agree with this approach, I'll send a v2 patch for
> it.

Yes, exactly.

Although I think it is better, in real world userspace, it may got
actual compatibility issue and reported, then we may have to revert even
if I personally think new is better.

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