[PATCH V2 1/2] fat: implement fat_update_time for inode timestamps

[PATCH V2 1/2] fat: implement fat_update_time for inode timestamps

[Frank Sorenson]

Author: Frank Sorenson <sorenson@redhat.com>
Date:   2018-04-03 20:09:35 -0500

    fat: implement fat_update_time for inode timestamps
    
    Add a fat_update_time function to update and truncate
    the inode's timestamps as needed for msdos/vfat filesystems.
    
    If called with the timespec pointer set to NULL, current time
    is used.
    
    Signed-off-by: Frank Sorenson <sorenson@redhat.com>

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 8fc1093da47d..54d1b08b8ecf 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -410,6 +410,7 @@ extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
 			      __le16 __time, __le16 __date, u8 time_cs);
 extern void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
 			      __le16 *time, __le16 *date, u8 *time_cs);
+extern int fat_update_time(struct inode *inode, struct timespec *now, int flags);
 extern int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs);
 
 int fat_cache_init(void);
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index f9bdc1e01c98..8df9409bcf66 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -262,6 +262,54 @@ void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
 }
 EXPORT_SYMBOL_GPL(fat_time_unix2fat);
 
+int fat_update_time(struct inode *inode, struct timespec *now, int flags)
+{
+	int iflags = I_DIRTY_TIME;
+	struct timespec ts;
+
+	if (inode->i_ino == MSDOS_ROOT_INO) {
+		ts = (struct timespec){0, 0};
+		if (flags & S_ATIME)
+			inode->i_atime = ts;
+		if (flags & S_MTIME)
+			inode->i_mtime = ts;
+		if (flags & S_CTIME)
+			inode->i_ctime = ts;
+	} else {
+		struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
+		__le16 time;
+		__le16 date;
+		u8 ctime_cs;
+
+		if (now == NULL) {
+			now = &ts;
+			ts = current_time(inode);
+		}
+
+		if (flags & S_ATIME) {
+			fat_time_unix2fat(sbi, now, &time, &date, NULL);
+			fat_time_fat2unix(sbi, &inode->i_atime, 0, date, 0);
+		}
+		if (flags & S_MTIME) {
+			fat_time_unix2fat(sbi, now, &time, &date, NULL);
+			fat_time_fat2unix(sbi, &inode->i_mtime, time, date, 0);
+		}
+		if (flags & S_CTIME) {
+			fat_time_unix2fat(sbi, now, &time, &date,
+				sbi->options.isvfat ? &ctime_cs :  NULL);
+			fat_time_fat2unix(sbi, &inode->i_ctime, time, date,
+				sbi->options.isvfat ? ctime_cs : 0);
+		}
+	}
+	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
+	    !(inode->i_sb->s_flags & SB_LAZYTIME))
+		iflags |= I_DIRTY_SYNC;
+
+	__mark_inode_dirty(inode, iflags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fat_update_time);
+
 int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
 {
 	int i, err = 0;
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 4724cc9ad650..63ec4a5bde77 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -519,4 +519,5 @@ EXPORT_SYMBOL_GPL(fat_setattr);
 const struct inode_operations fat_file_inode_operations = {
 	.setattr	= fat_setattr,
 	.getattr	= fat_getattr,
+	.update_time	= fat_update_time,
 };
diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index 582ca731a6c9..f157df4c1e51 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -641,6 +641,7 @@ static const struct inode_operations msdos_dir_inode_operations = {
 	.rename		= msdos_rename,
 	.setattr	= fat_setattr,
 	.getattr	= fat_getattr,
+	.update_time	= fat_update_time,
 };
 
 static void setup(struct super_block *sb)
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 2649759c478a..3d24b44cb93d 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -1043,6 +1043,7 @@ static const struct inode_operations vfat_dir_inode_operations = {
 	.rename		= vfat_rename,
 	.setattr	= fat_setattr,
 	.getattr	= fat_getattr,
+	.update_time	= fat_update_time,
 };
 
 static void setup(struct super_block *sb)

Re: [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps

[OGAWA Hirofumi]

Frank Sorenson <sorenson@redhat.com> writes:

> +int fat_update_time(struct inode *inode, struct timespec *now, int flags)
> +{
> +	int iflags = I_DIRTY_TIME;
> +	struct timespec ts;
> +
> +	if (inode->i_ino == MSDOS_ROOT_INO) {
> +		ts = (struct timespec){0, 0};
> +		if (flags & S_ATIME)
> +			inode->i_atime = ts;
> +		if (flags & S_MTIME)
> +			inode->i_mtime = ts;
> +		if (flags & S_CTIME)
> +			inode->i_ctime = ts;

Why do we set epoch here? If rootdir is initialized by epoch, we should
be able to keep epoch for rootdir. I.e. just ignore (and S_NOATIME |
S_CMTIME may be better to skip some)?

> +	} else {
> +		struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> +		__le16 time;
> +		__le16 date;
> +		u8 ctime_cs;
> +
> +		if (now == NULL) {
> +			now = &ts;
> +			ts = current_time(inode);
> +		}
> +
> +		if (flags & S_ATIME) {
> +			fat_time_unix2fat(sbi, now, &time, &date, NULL);
> +			fat_time_fat2unix(sbi, &inode->i_atime, 0, date, 0);
> +		}
> +		if (flags & S_MTIME) {
> +			fat_time_unix2fat(sbi, now, &time, &date, NULL);
> +			fat_time_fat2unix(sbi, &inode->i_mtime, time, date, 0);
> +		}
> +		if (flags & S_CTIME) {
> +			fat_time_unix2fat(sbi, now, &time, &date,
> +				sbi->options.isvfat ? &ctime_cs :  NULL);
> +			fat_time_fat2unix(sbi, &inode->i_ctime, time, date,
> +				sbi->options.isvfat ? ctime_cs : 0);
> +		}

Can't we use timespec_trunc() here (have to check limit of timestamp
though)? Convert twice is inefficient.

> +	}
> +	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
> +	    !(inode->i_sb->s_flags & SB_LAZYTIME))
> +		iflags |= I_DIRTY_SYNC;
> +
> +	__mark_inode_dirty(inode, iflags);

We should make this i_[acm]time update to function, and use everywhere,
or such. So we can skip needless mark_inode_dirty() on metadata update.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fat_update_time);

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

Re: [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps

[Frank Sorenson]

On 04/07/2018 07:11 PM, OGAWA Hirofumi wrote:
> Frank Sorenson <sorenson@redhat.com> writes:
> 
>> +int fat_update_time(struct inode *inode, struct timespec *now, int flags)
>> +{
>> +	int iflags = I_DIRTY_TIME;
>> +	struct timespec ts;
>> +
>> +	if (inode->i_ino == MSDOS_ROOT_INO) {
>> +		ts = (struct timespec){0, 0};
>> +		if (flags & S_ATIME)
>> +			inode->i_atime = ts;
>> +		if (flags & S_MTIME)
>> +			inode->i_mtime = ts;
>> +		if (flags & S_CTIME)
>> +			inode->i_ctime = ts;
> 
> Why do we set epoch here? If rootdir is initialized by epoch, we should
> be able to keep epoch for rootdir. I.e. just ignore (and S_NOATIME |
> S_CMTIME may be better to skip some)?

I'll do more testing to verify it's not getting updated anywhere else.

>> +	} else {
>> +		struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>> +		__le16 time;
>> +		__le16 date;
>> +		u8 ctime_cs;
>> +
>> +		if (now == NULL) {
>> +			now = &ts;
>> +			ts = current_time(inode);
>> +		}
>> +
>> +		if (flags & S_ATIME) {
>> +			fat_time_unix2fat(sbi, now, &time, &date, NULL);
>> +			fat_time_fat2unix(sbi, &inode->i_atime, 0, date, 0);
>> +		}
>> +		if (flags & S_MTIME) {
>> +			fat_time_unix2fat(sbi, now, &time, &date, NULL);
>> +			fat_time_fat2unix(sbi, &inode->i_mtime, time, date, 0);
>> +		}
>> +		if (flags & S_CTIME) {
>> +			fat_time_unix2fat(sbi, now, &time, &date,
>> +				sbi->options.isvfat ? &ctime_cs :  NULL);
>> +			fat_time_fat2unix(sbi, &inode->i_ctime, time, date,
>> +				sbi->options.isvfat ? ctime_cs : 0);
>> +		}
> 
> Can't we use timespec_trunc() here (have to check limit of timestamp
> though)? Convert twice is inefficient.

Completely agreed on the inefficiency...

timespec_trunc() doesn't work well for fat, mostly because it will only
truncate down to 1 second, so some additional logic would be needed for
2-second mtime or 24-hour atime.  It is also called from elsewhere in
the fs code using the single time granularity available in the
super_block, so that time granularity is used for all of i_[acm]time


As far as efficiency, I will look into extracting just the truncate
behavior of fat_time_unix2fat to use that.  The patch was aiming for reuse.

Is the result of the fat_time_unix2fat truncate of 2-second mtime in
'struct tm' equivalent to (i_mtime & ~0x1) ?  If we can bypass the need
to convert to 'struct tm' for the truncate, and then back, that's
clearly better.


>> +	}
>> +	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
>> +	    !(inode->i_sb->s_flags & SB_LAZYTIME))
>> +		iflags |= I_DIRTY_SYNC;
>> +
>> +	__mark_inode_dirty(inode, iflags);
> 
> We should make this i_[acm]time update to function, and use everywhere,
> or such. So we can skip needless mark_inode_dirty() on metadata update.

I can remove; there's at least one location in current code where
metadata is not getting updated to disk without.  I'm still working to
locate it.


Thanks,

Frank
--
Frank Sorenson
sorenson@redhat.com
Senior Software Maintenance Engineer
Global Support Services - filesystems
Red Hat

Re: [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps

[OGAWA Hirofumi]

Frank Sorenson <sorenson@redhat.com> writes:

>>> +	} else {
>>> +		struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>>> +		__le16 time;
>>> +		__le16 date;
>>> +		u8 ctime_cs;
>>> +
>>> +		if (now == NULL) {
>>> +			now = &ts;
>>> +			ts = current_time(inode);
>>> +		}
>>> +
>>> +		if (flags & S_ATIME) {
>>> +			fat_time_unix2fat(sbi, now, &time, &date, NULL);
>>> +			fat_time_fat2unix(sbi, &inode->i_atime, 0, date, 0);
>>> +		}
>>> +		if (flags & S_MTIME) {
>>> +			fat_time_unix2fat(sbi, now, &time, &date, NULL);
>>> +			fat_time_fat2unix(sbi, &inode->i_mtime, time, date, 0);
>>> +		}
>>> +		if (flags & S_CTIME) {
>>> +			fat_time_unix2fat(sbi, now, &time, &date,
>>> +				sbi->options.isvfat ? &ctime_cs :  NULL);
>>> +			fat_time_fat2unix(sbi, &inode->i_ctime, time, date,
>>> +				sbi->options.isvfat ? ctime_cs : 0);
>>> +		}
>> 
>> Can't we use timespec_trunc() here (have to check limit of timestamp
>> though)? Convert twice is inefficient.
>
> Completely agreed on the inefficiency...
>
> timespec_trunc() doesn't work well for fat, mostly because it will only
> truncate down to 1 second, so some additional logic would be needed for
> 2-second mtime or 24-hour atime.  It is also called from elsewhere in
> the fs code using the single time granularity available in the
> super_block, so that time granularity is used for all of i_[acm]time
>
> As far as efficiency, I will look into extracting just the truncate
> behavior of fat_time_unix2fat to use that.  The patch was aiming for reuse.
>
> Is the result of the fat_time_unix2fat truncate of 2-second mtime in
> 'struct tm' equivalent to (i_mtime & ~0x1) ?  If we can bypass the need
> to convert to 'struct tm' for the truncate, and then back, that's
> clearly better.

Ah, right. Can we make fat_timespec_trunc()? I think we can truncate
time what we want without limitation. AFAIK, fat_update_time() and
setattr_copy() is enough to add to fat_timespec_trunc().

And ->s_time_gran doesn't work to skip timespec_equal() check as you
said. So, fat_update_time() may be better to have timespec_equal() check
after fat_timespec_trunc().

>>> +	}
>>> +	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
>>> +	    !(inode->i_sb->s_flags & SB_LAZYTIME))
>>> +		iflags |= I_DIRTY_SYNC;
>>> +
>>> +	__mark_inode_dirty(inode, iflags);
>> 
>> We should make this i_[acm]time update to function, and use everywhere,
>> or such. So we can skip needless mark_inode_dirty() on metadata update.
>
> I can remove; there's at least one location in current code where
> metadata is not getting updated to disk without.  I'm still working to
> locate it.

We need __mark_inode_dirty() for ->update_time() callback though. I
meant, __mark_inode_dirty() should not be required for direct call of
fat_update_time() in patch that replaced i_xxxx = current_time();

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