| Message ID | 20230807-mgctime-v7-5-d1dec143a704@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | fs: implement multigrain timestamps | expand |
On Mon 07-08-23 15:38:36, Jeff Layton wrote: > In later patches, we're going to drop the "now" parameter from the > update_time operation. Fix fat_update_time to fetch its own timestamp. > It turns out that this is easily done by just passing a NULL timestamp > pointer to fat_update_time. ^^^ fat_truncate_time() > Also, it may be that things have changed by the time we get to calling > fat_update_time after checking inode_needs_update_time. Ensure that we > attempt the i_version bump if any of the S_* flags besides S_ATIME are > set. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/fat/misc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fat/misc.c b/fs/fat/misc.c > index 67006ea08db6..8cab87145d63 100644 > --- a/fs/fat/misc.c > +++ b/fs/fat/misc.c > @@ -347,14 +347,14 @@ int fat_update_time(struct inode *inode, struct timespec64 *now, int flags) > return 0; > > if (flags & (S_ATIME | S_CTIME | S_MTIME)) { > - fat_truncate_time(inode, now, flags); > + fat_truncate_time(inode, NULL, flags); > if (inode->i_sb->s_flags & SB_LAZYTIME) > dirty_flags |= I_DIRTY_TIME; > else > dirty_flags |= I_DIRTY_SYNC; > } > > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > dirty_flags |= I_DIRTY_SYNC; > > __mark_inode_dirty(inode, dirty_flags); > > -- > 2.41.0 >
On Tue, Aug 08, 2023 at 11:32:26AM +0200, Jan Kara wrote: > On Mon 07-08-23 15:38:36, Jeff Layton wrote: > > In later patches, we're going to drop the "now" parameter from the > > update_time operation. Fix fat_update_time to fetch its own timestamp. > > It turns out that this is easily done by just passing a NULL timestamp > > pointer to fat_update_time. > ^^^ fat_truncate_time() Fixed in-tree, thanks!
Jeff Layton <jlayton@kernel.org> writes: > Also, it may be that things have changed by the time we get to calling > fat_update_time after checking inode_needs_update_time. Ensure that we > attempt the i_version bump if any of the S_* flags besides S_ATIME are > set. I'm not sure what it meaning though, this is from generic_update_time(). Are you going to change generic_update_time() too? If so, it doesn't break lazytime feature? Thanks. > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/fat/misc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fat/misc.c b/fs/fat/misc.c > index 67006ea08db6..8cab87145d63 100644 > --- a/fs/fat/misc.c > +++ b/fs/fat/misc.c > @@ -347,14 +347,14 @@ int fat_update_time(struct inode *inode, struct timespec64 *now, int flags) > return 0; > > if (flags & (S_ATIME | S_CTIME | S_MTIME)) { > - fat_truncate_time(inode, now, flags); > + fat_truncate_time(inode, NULL, flags); > if (inode->i_sb->s_flags & SB_LAZYTIME) > dirty_flags |= I_DIRTY_TIME; > else > dirty_flags |= I_DIRTY_SYNC; > } > > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > dirty_flags |= I_DIRTY_SYNC; > > __mark_inode_dirty(inode, dirty_flags);
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes: > Jeff Layton <jlayton@kernel.org> writes: > >> Also, it may be that things have changed by the time we get to calling >> fat_update_time after checking inode_needs_update_time. Ensure that we >> attempt the i_version bump if any of the S_* flags besides S_ATIME are >> set. > > I'm not sure what it meaning though, this is from > generic_update_time(). Are you going to change generic_update_time() > too? If so, it doesn't break lazytime feature? > > Thanks. BTW, fat is not implementing lazytime now, but it is for future.
On Wed, 2023-08-09 at 17:37 +0900, OGAWA Hirofumi wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > Also, it may be that things have changed by the time we get to calling > > fat_update_time after checking inode_needs_update_time. Ensure that we > > attempt the i_version bump if any of the S_* flags besides S_ATIME are > > set. > > I'm not sure what it meaning though, this is from > generic_update_time(). Are you going to change generic_update_time() > too? If so, it doesn't break lazytime feature? > Yes. generic_update_time is also being changed in a similar fashion. This shouldn't break the lazytime feature: lazytime is all about how and when timestamps get written to disk. This work is all about which clocksource the timestamps originally come from. > Thanks. > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/fat/misc.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fat/misc.c b/fs/fat/misc.c > > index 67006ea08db6..8cab87145d63 100644 > > --- a/fs/fat/misc.c > > +++ b/fs/fat/misc.c > > @@ -347,14 +347,14 @@ int fat_update_time(struct inode *inode, struct timespec64 *now, int flags) > > return 0; > > > > if (flags & (S_ATIME | S_CTIME | S_MTIME)) { > > - fat_truncate_time(inode, now, flags); > > + fat_truncate_time(inode, NULL, flags); > > if (inode->i_sb->s_flags & SB_LAZYTIME) > > dirty_flags |= I_DIRTY_TIME; > > else > > dirty_flags |= I_DIRTY_SYNC; > > } > > > > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > > dirty_flags |= I_DIRTY_SYNC; > > > > __mark_inode_dirty(inode, dirty_flags); >
Jeff Layton <jlayton@kernel.org> writes: > On Wed, 2023-08-09 at 17:37 +0900, OGAWA Hirofumi wrote: >> Jeff Layton <jlayton@kernel.org> writes: >> >> > Also, it may be that things have changed by the time we get to calling >> > fat_update_time after checking inode_needs_update_time. Ensure that we >> > attempt the i_version bump if any of the S_* flags besides S_ATIME are >> > set. >> >> I'm not sure what it meaning though, this is from >> generic_update_time(). Are you going to change generic_update_time() >> too? If so, it doesn't break lazytime feature? >> > > Yes. generic_update_time is also being changed in a similar fashion. > This shouldn't break the lazytime feature: lazytime is all about how and > when timestamps get written to disk. This work is all about which > clocksource the timestamps originally come from. I can only find the following update in this series, another series updates generic_update_time()? The patch updates only if S_VERSION is set. Your fat patch sets I_DIRTY_SYNC always instead of I_DIRTY_TIME. When I last time checked lazytime, and it was depending on I_DIRTY_TIME. Are you sure it doesn't break lazytime? I'm totally confusing, and really similar with generic_update_time()? Thanks. +/** + * generic_update_time - update the timestamps on the inode + * @inode: inode to be updated + * @flags: S_* flags that needed to be updated + * + * The update_time function is called when an inode's timestamps need to be + * updated for a read or write operation. In the case where any of S_MTIME, S_CTIME, + * or S_VERSION need to be updated we attempt to update all three of them. S_ATIME + * updates can be handled done independently of the rest. + * + * Returns a S_* mask indicating which fields were updated. + */ +int generic_update_time(struct inode *inode, int flags) +{ + int updated = inode_update_timestamps(inode, flags); + int dirty_flags = 0; + if (updated & (S_ATIME|S_MTIME|S_CTIME)) + dirty_flags = inode->i_sb->s_flags & SB_LAZYTIME ? I_DIRTY_TIME : I_DIRTY_SYNC; + if (updated & S_VERSION) + dirty_flags |= I_DIRTY_SYNC; __mark_inode_dirty(inode, dirty_flags); - return 0; + return updated; } >> > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) >> > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) >> > dirty_flags |= I_DIRTY_SYNC; >> > >> > __mark_inode_dirty(inode, dirty_flags); >>
On Wed, 2023-08-09 at 22:36 +0900, OGAWA Hirofumi wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > On Wed, 2023-08-09 at 17:37 +0900, OGAWA Hirofumi wrote: > > > Jeff Layton <jlayton@kernel.org> writes: > > > > > > > Also, it may be that things have changed by the time we get to calling > > > > fat_update_time after checking inode_needs_update_time. Ensure that we > > > > attempt the i_version bump if any of the S_* flags besides S_ATIME are > > > > set. > > > > > > I'm not sure what it meaning though, this is from > > > generic_update_time(). Are you going to change generic_update_time() > > > too? If so, it doesn't break lazytime feature? > > > > > > > Yes. generic_update_time is also being changed in a similar fashion. > > This shouldn't break the lazytime feature: lazytime is all about how and > > when timestamps get written to disk. This work is all about which > > clocksource the timestamps originally come from. > > I can only find the following update in this series, another series > updates generic_update_time()? The patch updates only if S_VERSION is > set. > > Your fat patch sets I_DIRTY_SYNC always instead of I_DIRTY_TIME. When I > last time checked lazytime, and it was depending on I_DIRTY_TIME. > > Are you sure it doesn't break lazytime? I'm totally confusing, and > really similar with generic_update_time()? > I'm a little confused too. Why do you believe this will break -o relatime handling? This patch changes two things: 1/ it has fat_update_time fetch its own timestamp (and ignore the "now" parameter). This is in line with the changes in patch #3 of this series, which explains the rationale for this in more detail. 2/ it changes fat_update_time to also update the i_version if any of S_CTIME|S_MTIME|S_VERSION are set. relatime is all about the S_ATIME, and it is specifically excluded from that set. The rationale for the second change is is also in patch #3, but basically, we can't guarantee that current_time hasn't changed since we last checked for inode_needs_update_time, so if any of S_CTIME/S_MTIME/S_VERSION have changed, then we need to assume that any of them may need to be changed and attempt to update all 3. That said, I think the logic in fat_update_time isn't quite right. I think want something like this on top of this patch to ensure that the S_CTIME and S_MTIME get updated, even if the flags only have S_VERSION set. Thoughts? ---------------------8<----------------------- diff --git a/fs/fat/misc.c b/fs/fat/misc.c index 080a5035483f..313eef02f45c 100644 --- a/fs/fat/misc.c +++ b/fs/fat/misc.c @@ -346,15 +346,21 @@ int fat_update_time(struct inode *inode, int flags) if (inode->i_ino == MSDOS_ROOT_INO) return 0; - if (flags & (S_ATIME | S_CTIME | S_MTIME)) { - fat_truncate_time(inode, NULL, flags); - if (inode->i_sb->s_flags & SB_LAZYTIME) - dirty_flags |= I_DIRTY_TIME; - else - dirty_flags |= I_DIRTY_SYNC; - } + /* + * If any of the flags indicate an expicit change to the file, then we + * need to ensure that we attempt to update all of 3. We do not do + * this in the case of an S_ATIME-only update. + */ + if (flags & (S_CTIME | S_MTIME | S_VERSION)) + flags |= S_CTIME | S_MTIME | S_VERSION; + + fat_truncate_time(inode, NULL, flags); + if (inode->i_sb->s_flags & SB_LAZYTIME) + dirty_flags |= I_DIRTY_TIME; + else + dirty_flags |= I_DIRTY_SYNC; - if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) + if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) dirty_flags |= I_DIRTY_SYNC;
Jeff Layton <jlayton@kernel.org> writes: > I'm a little confused too. Why do you believe this will break > -o relatime handling? This patch changes two things: > > 1/ it has fat_update_time fetch its own timestamp (and ignore the "now" > parameter). This is in line with the changes in patch #3 of this series, > which explains the rationale for this in more detail. > > 2/ it changes fat_update_time to also update the i_version if any of > S_CTIME|S_MTIME|S_VERSION are set. relatime is all about the S_ATIME, > and it is specifically excluded from that set. > > The rationale for the second change is is also in patch #3, but > basically, we can't guarantee that current_time hasn't changed since we > last checked for inode_needs_update_time, so if any of > S_CTIME/S_MTIME/S_VERSION have changed, then we need to assume that any > of them may need to be changed and attempt to update all 3. > > That said, I think the logic in fat_update_time isn't quite right. I > think want something like this on top of this patch to ensure that the > S_CTIME and S_MTIME get updated, even if the flags only have S_VERSION > set. > > Thoughts? I'm not talking about "relatime" at all, I'm always talking about "lazytime". I_DIRTY_TIME delays to update on disk inode only if changed timestamp. But you changed it to I_DIRTY_SYNC, i.e. always update on disk inode by timestamp. Thanks. > ---------------------8<----------------------- > > diff --git a/fs/fat/misc.c b/fs/fat/misc.c > index 080a5035483f..313eef02f45c 100644 > --- a/fs/fat/misc.c > +++ b/fs/fat/misc.c > @@ -346,15 +346,21 @@ int fat_update_time(struct inode *inode, int flags) > if (inode->i_ino == MSDOS_ROOT_INO) > return 0; > > - if (flags & (S_ATIME | S_CTIME | S_MTIME)) { > - fat_truncate_time(inode, NULL, flags); > - if (inode->i_sb->s_flags & SB_LAZYTIME) > - dirty_flags |= I_DIRTY_TIME; > - else > - dirty_flags |= I_DIRTY_SYNC; > - } > + /* > + * If any of the flags indicate an expicit change to the file, then we > + * need to ensure that we attempt to update all of 3. We do not do > + * this in the case of an S_ATIME-only update. > + */ > + if (flags & (S_CTIME | S_MTIME | S_VERSION)) > + flags |= S_CTIME | S_MTIME | S_VERSION; > + > + fat_truncate_time(inode, NULL, flags); > + if (inode->i_sb->s_flags & SB_LAZYTIME) > + dirty_flags |= I_DIRTY_TIME; > + else > + dirty_flags |= I_DIRTY_SYNC; > > - if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > + if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > dirty_flags |= I_DIRTY_SYNC; >
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes: > Jeff Layton <jlayton@kernel.org> writes: > >> I'm a little confused too. Why do you believe this will break >> -o relatime handling? This patch changes two things: >> >> 1/ it has fat_update_time fetch its own timestamp (and ignore the "now" >> parameter). This is in line with the changes in patch #3 of this series, >> which explains the rationale for this in more detail. >> >> 2/ it changes fat_update_time to also update the i_version if any of >> S_CTIME|S_MTIME|S_VERSION are set. relatime is all about the S_ATIME, >> and it is specifically excluded from that set. >> >> The rationale for the second change is is also in patch #3, but >> basically, we can't guarantee that current_time hasn't changed since we >> last checked for inode_needs_update_time, so if any of >> S_CTIME/S_MTIME/S_VERSION have changed, then we need to assume that any >> of them may need to be changed and attempt to update all 3. >> >> That said, I think the logic in fat_update_time isn't quite right. I >> think want something like this on top of this patch to ensure that the >> S_CTIME and S_MTIME get updated, even if the flags only have S_VERSION >> set. >> >> Thoughts? > > I'm not talking about "relatime" at all, I'm always talking about > "lazytime". > > I_DIRTY_TIME delays to update on disk inode only if changed > timestamp. But you changed it to I_DIRTY_SYNC, i.e. always update on > disk inode by timestamp. And if change like it, why doesn't same change goes to generic_update_time()? It looks like generic_update_time() in this series doesn't work like you said.
On Wed 09-08-23 22:36:29, OGAWA Hirofumi wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > On Wed, 2023-08-09 at 17:37 +0900, OGAWA Hirofumi wrote: > >> Jeff Layton <jlayton@kernel.org> writes: > >> > >> > Also, it may be that things have changed by the time we get to calling > >> > fat_update_time after checking inode_needs_update_time. Ensure that we > >> > attempt the i_version bump if any of the S_* flags besides S_ATIME are > >> > set. > >> > >> I'm not sure what it meaning though, this is from > >> generic_update_time(). Are you going to change generic_update_time() > >> too? If so, it doesn't break lazytime feature? > >> > > > > Yes. generic_update_time is also being changed in a similar fashion. > > This shouldn't break the lazytime feature: lazytime is all about how and > > when timestamps get written to disk. This work is all about which > > clocksource the timestamps originally come from. > > I can only find the following update in this series, another series > updates generic_update_time()? The patch updates only if S_VERSION is > set. > > Your fat patch sets I_DIRTY_SYNC always instead of I_DIRTY_TIME. When I > last time checked lazytime, and it was depending on I_DIRTY_TIME. > > Are you sure it doesn't break lazytime? I'm totally confusing, and > really similar with generic_update_time()? Since you are talking past one another with Jeff let me chime in here :). I think you are worried about this hunk: - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) dirty_flags |= I_DIRTY_SYNC; which makes the 'flags' test pass even if we just modified ctime or mtime. But do note the second part of the if - inode_maybe_inc_iversion() - so we are going to mark the inode dirty with I_DIRTY_SYNC only if someone queried iversion since the last time we have incremented it. So this hunk is not really changing how inode is marked dirty, it only changes how often we check whether iversion needs increment and that should be fine (and desirable). Hence lazytime isn't really broken by this in any way. Honza
Jan Kara <jack@suse.cz> writes: > Since you are talking past one another with Jeff let me chime in here :). I > think you are worried about this hunk: Right. > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > dirty_flags |= I_DIRTY_SYNC; > > which makes the 'flags' test pass even if we just modified ctime or mtime. > But do note the second part of the if - inode_maybe_inc_iversion() - so we > are going to mark the inode dirty with I_DIRTY_SYNC only if someone queried > iversion since the last time we have incremented it. > > So this hunk is not really changing how inode is marked dirty, it only > changes how often we check whether iversion needs increment and that should > be fine (and desirable). Hence lazytime isn't really broken by this in any > way. OK. However, then it doesn't explain what I asked. This is not same with generic_update_time(), only FAT does. If thinks it is right thing, why generic_update_time() doesn't? I said first reply, this was from generic_update_time(). (Or I'm misreading updated generic_update_time()?) Thanks.
On Thu, 2023-08-10 at 00:17 +0900, OGAWA Hirofumi wrote: > Jan Kara <jack@suse.cz> writes: > > > Since you are talking past one another with Jeff let me chime in here :). I > > think you are worried about this hunk: > > Right. > > > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > > dirty_flags |= I_DIRTY_SYNC; > > > > which makes the 'flags' test pass even if we just modified ctime or mtime. > > But do note the second part of the if - inode_maybe_inc_iversion() - so we > > are going to mark the inode dirty with I_DIRTY_SYNC only if someone queried > > iversion since the last time we have incremented it. > > > > So this hunk is not really changing how inode is marked dirty, it only > > changes how often we check whether iversion needs increment and that should > > be fine (and desirable). Hence lazytime isn't really broken by this in any > > way. > > OK. However, then it doesn't explain what I asked. This is not same with > generic_update_time(), only FAT does. > > If thinks it is right thing, why generic_update_time() doesn't? I said > first reply, this was from generic_update_time(). (Or I'm misreading > updated generic_update_time()?) > My mistake re: lazytime vs. relatime, but Jan is correct that this shouldn't break anything there. The logic in the revised generic_update_time is different because FAT is is a bit strange. fat_update_time does extra truncation on the timestamp that it is handed beyond what timestamp_truncate() does. fat_truncate_time is called in many different places too, so I don't feel comfortable making big changes to how that works. In the case of generic_update_time, it calls inode_update_timestamps which returns a mask that shows which timestamps got updated. It then marks the dirty_flags appropriately for what was actually changed. generic_update_time is used across many filesystems so we need to ensure that it's OK to use even when multigrain timestamps are enabled. Those haven't been enabled in FAT though, so I didn't bother, and left it to dirtying the inode in the same way it was before, even though it now fetches its own timestamps from the clock. Given the way that the mtime and ctime are smooshed together in FAT, that seemed reasonable. Is there a particular case or flag combination you're concerned about here?
Jeff Layton <jlayton@kernel.org> writes: > On Thu, 2023-08-10 at 00:17 +0900, OGAWA Hirofumi wrote: >> Jan Kara <jack@suse.cz> writes: [...] > My mistake re: lazytime vs. relatime, but Jan is correct that this > shouldn't break anything there. Actually breaks ("break" means not corrupt fs, means it breaks lazytime optimization). It is just not always, but it should be always for some userspaces. > The logic in the revised generic_update_time is different because FAT is > is a bit strange. fat_update_time does extra truncation on the timestamp > that it is handed beyond what timestamp_truncate() does. > fat_truncate_time is called in many different places too, so I don't > feel comfortable making big changes to how that works. > > In the case of generic_update_time, it calls inode_update_timestamps > which returns a mask that shows which timestamps got updated. It then > marks the dirty_flags appropriately for what was actually changed. > > generic_update_time is used across many filesystems so we need to ensure > that it's OK to use even when multigrain timestamps are enabled. Those > haven't been enabled in FAT though, so I didn't bother, and left it to > dirtying the inode in the same way it was before, even though it now > fetches its own timestamps from the clock. Given the way that the mtime > and ctime are smooshed together in FAT, that seemed reasonable. > > Is there a particular case or flag combination you're concerned about > here? Yes. Because FAT has strange timestamps that different granularity on disk . This is why generic time truncation doesn't work for FAT. Well anyway, my concern is the only following part. In generic_update_time(), S_[CM]TIME are not the cause of I_DIRTY_SYNC if lazytime mode. - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) dirty_flags |= I_DIRTY_SYNC; If reverted this part to check only S_VERSION, I'm fine. Thanks.
On Thu, 2023-08-10 at 02:44 +0900, OGAWA Hirofumi wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > On Thu, 2023-08-10 at 00:17 +0900, OGAWA Hirofumi wrote: > > > Jan Kara <jack@suse.cz> writes: > > [...] > > > My mistake re: lazytime vs. relatime, but Jan is correct that this > > shouldn't break anything there. > > Actually breaks ("break" means not corrupt fs, means it breaks lazytime > optimization). It is just not always, but it should be always for some > userspaces. > > > The logic in the revised generic_update_time is different because FAT is > > is a bit strange. fat_update_time does extra truncation on the timestamp > > that it is handed beyond what timestamp_truncate() does. > > fat_truncate_time is called in many different places too, so I don't > > feel comfortable making big changes to how that works. > > > > In the case of generic_update_time, it calls inode_update_timestamps > > which returns a mask that shows which timestamps got updated. It then > > marks the dirty_flags appropriately for what was actually changed. > > > > generic_update_time is used across many filesystems so we need to ensure > > that it's OK to use even when multigrain timestamps are enabled. Those > > haven't been enabled in FAT though, so I didn't bother, and left it to > > dirtying the inode in the same way it was before, even though it now > > fetches its own timestamps from the clock. Given the way that the mtime > > and ctime are smooshed together in FAT, that seemed reasonable. > > > > Is there a particular case or flag combination you're concerned about > > here? > > Yes. Because FAT has strange timestamps that different granularity on > disk . This is why generic time truncation doesn't work for FAT. > > Well anyway, my concern is the only following part. In > generic_update_time(), S_[CM]TIME are not the cause of I_DIRTY_SYNC if > lazytime mode. > > - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) > + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) > dirty_flags |= I_DIRTY_SYNC; > That would be wrong. The problem is that we're changing how update_time works: Previously, update_time was given a timestamp and a set of S_* flags to indicate which fields should be updated. Now, update_time is not given a timestamp. It needs to fetch it itself, but that subtly changes the meaning of the flags field. It now means "these fields needed to be updated when I last checked". The timestamp and i_version may now be different from when the flags field was set. This means that if any of S_CTIME/S_MTIME/S_VERSION were set that we need to attempt to update all 3 of them. They may now be different from the timestamp or version that we ultimately end up with. The above may look to you like it would always cause I_DIRTY_SYNC to be set on any ctime or mtime update, but inode_maybe_inc_iversion only returns true if it actually updated i_version, and it only does that if someone issued a ->getattr against the file since the last time it was updated. So, this shouldn't generate any more DIRTY_SYNC updates than it did before.
Jeff Layton <jlayton@kernel.org> writes: > On Thu, 2023-08-10 at 02:44 +0900, OGAWA Hirofumi wrote: >> Jeff Layton <jlayton@kernel.org> writes: >> > That would be wrong. The problem is that we're changing how update_time > works: > > Previously, update_time was given a timestamp and a set of S_* flags to > indicate which fields should be updated. Now, update_time is not given a > timestamp. It needs to fetch it itself, but that subtly changes the > meaning of the flags field. > > It now means "these fields needed to be updated when I last checked". > The timestamp and i_version may now be different from when the flags > field was set. This means that if any of S_CTIME/S_MTIME/S_VERSION were > set that we need to attempt to update all 3 of them. They may now be > different from the timestamp or version that we ultimately end up with. > > The above may look to you like it would always cause I_DIRTY_SYNC to be > set on any ctime or mtime update, but inode_maybe_inc_iversion only > returns true if it actually updated i_version, and it only does that if > someone issued a ->getattr against the file since the last time it was > updated. > > So, this shouldn't generate any more DIRTY_SYNC updates than it did > before. Again, if you claim so, why generic_update_time() doesn't work same? Why only FAT does? Or I'm misreading generic_update_time() patch? Thanks.
On Thu, 2023-08-10 at 03:31 +0900, OGAWA Hirofumi wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > On Thu, 2023-08-10 at 02:44 +0900, OGAWA Hirofumi wrote: > > > Jeff Layton <jlayton@kernel.org> writes: > > > > > That would be wrong. The problem is that we're changing how update_time > > works: > > > > Previously, update_time was given a timestamp and a set of S_* flags to > > indicate which fields should be updated. Now, update_time is not given a > > timestamp. It needs to fetch it itself, but that subtly changes the > > meaning of the flags field. > > > > It now means "these fields needed to be updated when I last checked". > > The timestamp and i_version may now be different from when the flags > > field was set. This means that if any of S_CTIME/S_MTIME/S_VERSION were > > set that we need to attempt to update all 3 of them. They may now be > > different from the timestamp or version that we ultimately end up with. > > > > The above may look to you like it would always cause I_DIRTY_SYNC to be > > set on any ctime or mtime update, but inode_maybe_inc_iversion only > > returns true if it actually updated i_version, and it only does that if > > someone issued a ->getattr against the file since the last time it was > > updated. > > > > So, this shouldn't generate any more DIRTY_SYNC updates than it did > > before. > > Again, if you claim so, why generic_update_time() doesn't work same? Why > only FAT does? > > Or I'm misreading generic_update_time() patch? > When you say it "doesn't work the same", what do you mean, specifically? I had to make some allowances for the fact that FAT is substantially different in its timestamp handling, and I tried to preserve existing behavior as best I could.
Jeff Layton <jlayton@kernel.org> writes: > When you say it "doesn't work the same", what do you mean, specifically? > I had to make some allowances for the fact that FAT is substantially > different in its timestamp handling, and I tried to preserve existing > behavior as best I could. Ah, ok. I was misreading some. inode_update_timestamps() checks IS_I_VERSION() now, not S_VERSION. So, if adding the check of IS_I_VERSION() and (S_MTIME|S_CTIME|S_VERSION) to FAT? With it, IS_I_VERSION() would be false on FAT, and I'm fine. I.e. something like if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && IS_I_VERSION(inode) && inode_maybe_inc_iversion(inode, false)) dirty_flags |= I_DIRTY_SYNC; Thanks.
On Thu, 2023-08-10 at 05:14 +0900, OGAWA Hirofumi wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > When you say it "doesn't work the same", what do you mean, specifically? > > I had to make some allowances for the fact that FAT is substantially > > different in its timestamp handling, and I tried to preserve existing > > behavior as best I could. > > Ah, ok. I was misreading some. > > inode_update_timestamps() checks IS_I_VERSION() now, not S_VERSION. So, > if adding the check of IS_I_VERSION() and (S_MTIME|S_CTIME|S_VERSION) to > FAT? > > With it, IS_I_VERSION() would be false on FAT, and I'm fine. > > I.e. something like > > if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && IS_I_VERSION(inode) > && inode_maybe_inc_iversion(inode, false)) > dirty_flags |= I_DIRTY_SYNC; > > Thanks. If you do that then the i_version counter would never be incremented. But...I think I see what you're getting at. Most filesystems that support the i_version counter have an on-disk field for it. FAT obviously has no such thing. I suspect the i_version bits in fat_update_time were added by mistake. FAT doesn't set SB_I_VERSION so there's no need to do anything to the i_version field at all. Also, given that the mtime and ctime are always kept in sync on FAT, we're probably fine to have it look something like this: --------------------8<------------------ int fat_update_time(struct inode *inode, int flags) { int dirty_flags = 0; if (inode->i_ino == MSDOS_ROOT_INO) return 0; fat_truncate_time(inode, NULL, flags); if (inode->i_sb->s_flags & SB_LAZYTIME) dirty_flags |= I_DIRTY_TIME; else dirty_flags |= I_DIRTY_SYNC; __mark_inode_dirty(inode, dirty_flags); return 0; } --------------------8<------------------ ...and we should probably do that in a separate patch in advance of the update_time rework, since it's really a different change. If you're in agreement, then I'll plan to respin the series with this fixed and resend. Thanks for being patient!
Jeff Layton <jlayton@kernel.org> writes: > If you do that then the i_version counter would never be incremented. > But...I think I see what you're getting at. > > Most filesystems that support the i_version counter have an on-disk > field for it. FAT obviously has no such thing. I suspect the i_version > bits in fat_update_time were added by mistake. FAT doesn't set > SB_I_VERSION so there's no need to do anything to the i_version field at > all. > > Also, given that the mtime and ctime are always kept in sync on FAT, > we're probably fine to have it look something like this: Yes. IIRC, when I wrote, I decided to make it keep similar with generic function, instead of heavily customize for FAT (for maintenance reason). It is why. There would be other places with same reason. E.g. LAZYTIME check is same reason too. (current FAT doesn't support it) So I personally I would prefer to leave it. But if you want to remove it, it would be ok too. Thanks. > --------------------8<------------------ > int fat_update_time(struct inode *inode, int flags) > { > int dirty_flags = 0; > > if (inode->i_ino == MSDOS_ROOT_INO) > return 0; > > fat_truncate_time(inode, NULL, flags); > if (inode->i_sb->s_flags & SB_LAZYTIME) > dirty_flags |= I_DIRTY_TIME; > else > dirty_flags |= I_DIRTY_SYNC; > > __mark_inode_dirty(inode, dirty_flags); > return 0; > } > --------------------8<------------------ > > ...and we should probably do that in a separate patch in advance of the > update_time rework, since it's really a different change. > > If you're in agreement, then I'll plan to respin the series with this > fixed and resend. > > Thanks for being patient!
diff --git a/fs/fat/misc.c b/fs/fat/misc.c index 67006ea08db6..8cab87145d63 100644 --- a/fs/fat/misc.c +++ b/fs/fat/misc.c @@ -347,14 +347,14 @@ int fat_update_time(struct inode *inode, struct timespec64 *now, int flags) return 0; if (flags & (S_ATIME | S_CTIME | S_MTIME)) { - fat_truncate_time(inode, now, flags); + fat_truncate_time(inode, NULL, flags); if (inode->i_sb->s_flags & SB_LAZYTIME) dirty_flags |= I_DIRTY_TIME; else dirty_flags |= I_DIRTY_SYNC; } - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)) + if ((flags & (S_VERSION|S_CTIME|S_MTIME)) && inode_maybe_inc_iversion(inode, false)) dirty_flags |= I_DIRTY_SYNC; __mark_inode_dirty(inode, dirty_flags);
In later patches, we're going to drop the "now" parameter from the update_time operation. Fix fat_update_time to fetch its own timestamp. It turns out that this is easily done by just passing a NULL timestamp pointer to fat_update_time. Also, it may be that things have changed by the time we get to calling fat_update_time after checking inode_needs_update_time. Ensure that we attempt the i_version bump if any of the S_* flags besides S_ATIME are set. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/fat/misc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)