Re: [PATCH v7 05/13] fat: make fat_update_time get its own timestamp

[Jeff Layton]

On Thu, 2023-08-10 at 00:17 +0900, OGAWA Hirofumi wrote:
> Jan Kara <jack@xxxxxxx> 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@xxxxxxxxxx>