On 04/07/2018 07:11 PM, OGAWA Hirofumi wrote:
> Frank Sorenson <sorenson@xxxxxxxxxx> 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@xxxxxxxxxx
Senior Software Maintenance Engineer
Global Support Services - filesystems
Red Hat
