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

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

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>