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 <[email protected]>
---
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 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 <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed 09-08-23 22:36:29, OGAWA Hirofumi wrote:
> Jeff Layton <[email protected]> writes:
>
> > On Wed, 2023-08-09 at 17:37 +0900, OGAWA Hirofumi wrote:
> >> Jeff Layton <[email protected]> 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 <[email protected]>
SUSE Labs, CR
On Thu, 2023-08-10 at 00:17 +0900, OGAWA Hirofumi wrote:
> Jan Kara <[email protected]> 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 <[email protected]>
Jeff Layton <[email protected]> writes:
> On Thu, 2023-08-10 at 02:44 +0900, OGAWA Hirofumi wrote:
>> Jeff Layton <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>
Jeff Layton <[email protected]> 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!
--
OGAWA Hirofumi <[email protected]>