Frank Sorenson <sorenson@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
