* [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps
@ 2018-04-04 2:37 Frank Sorenson
2018-04-08 0:11 ` OGAWA Hirofumi
0 siblings, 1 reply; 4+ messages in thread
From: Frank Sorenson @ 2018-04-04 2:37 UTC (permalink / raw)
To: OGAWA Hirofumi, linux-fsdevel
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)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps
2018-04-04 2:37 [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps Frank Sorenson
@ 2018-04-08 0:11 ` OGAWA Hirofumi
2018-04-09 16:27 ` Frank Sorenson
0 siblings, 1 reply; 4+ messages in thread
From: OGAWA Hirofumi @ 2018-04-08 0:11 UTC (permalink / raw)
To: Frank Sorenson; +Cc: linux-fsdevel
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps
2018-04-08 0:11 ` OGAWA Hirofumi
@ 2018-04-09 16:27 ` Frank Sorenson
2018-04-09 22:25 ` OGAWA Hirofumi
0 siblings, 1 reply; 4+ messages in thread
From: Frank Sorenson @ 2018-04-09 16:27 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: linux-fsdevel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps
2018-04-09 16:27 ` Frank Sorenson
@ 2018-04-09 22:25 ` OGAWA Hirofumi
0 siblings, 0 replies; 4+ messages in thread
From: OGAWA Hirofumi @ 2018-04-09 22:25 UTC (permalink / raw)
To: Frank Sorenson; +Cc: linux-fsdevel
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-09 22:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-04 2:37 [PATCH V2 1/2] fat: implement fat_update_time for inode timestamps Frank Sorenson
2018-04-08 0:11 ` OGAWA Hirofumi
2018-04-09 16:27 ` Frank Sorenson
2018-04-09 22:25 ` OGAWA Hirofumi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).