| Message ID | 8cad6edb-95bd-1c02-1d7d-a23850b329c3@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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)
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>